Bug 233760

Summary: Move content filtering to Networking process
Product: WebKit Reporter: Per Arne Vollan <pvollan>
Component: WebKit Misc.Assignee: Per Arne Vollan <pvollan>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, benjamin, bfulgham, cdumez, cmarcelo, ews-watchlist, gavin.p, japhet, mazander, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Patch
none
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
bfulgham: review+
Patch
none
Patch none

Description Per Arne Vollan 2021-12-02 07:45:01 PST 
Moving content filtering to the Networking process will enable us to further strengthen the WebContent process' sandbox.
Comment 1 Per Arne Vollan 2021-12-02 08:04:47 PST 
Created attachment 445721 [details]
Patch
Comment 2 Per Arne Vollan 2021-12-02 08:05:23 PST 
WIP patch.
Comment 3 Per Arne Vollan 2021-12-02 08:58:53 PST 
Created attachment 445723 [details]
Patch
Comment 4 Per Arne Vollan 2021-12-02 14:56:38 PST 
Created attachment 445775 [details]
Patch
Comment 5 Per Arne Vollan 2021-12-02 15:55:57 PST 
Created attachment 445784 [details]
Patch
Comment 6 Alex Christensen 2021-12-02 16:23:15 PST 
Comment on attachment 445784 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=445784&action=review

> Source/WTF/wtf/PlatformEnableCocoa.h:168
> +#define ENABLE_CONTENT_FILTERING_IN_NETWORKING_PROCESS ENABLE_CONTENT_FILTERING

Is there a reason we can't just do this everywhere?
Comment 7 Per Arne Vollan 2021-12-03 12:23:00 PST 
(In reply to Alex Christensen from comment #6)
> Comment on attachment 445784 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=445784&action=review
> 
> > Source/WTF/wtf/PlatformEnableCocoa.h:168
> > +#define ENABLE_CONTENT_FILTERING_IN_NETWORKING_PROCESS ENABLE_CONTENT_FILTERING
> 
> Is there a reason we can't just do this everywhere?

That's a good point. I will enable for all different platforms.

Thanks for reviewing!
Comment 8 Per Arne Vollan 2021-12-05 23:26:45 PST 
Created attachment 446005 [details]
Patch
Comment 9 Per Arne Vollan 2021-12-06 02:03:44 PST 
Created attachment 446012 [details]
Patch
Comment 10 Per Arne Vollan 2021-12-07 02:15:55 PST 
Created attachment 446141 [details]
Patch
Comment 11 Radar WebKit Bug Importer 2021-12-07 04:39:26 PST 
<rdar://problem/86150702>
Comment 12 Per Arne Vollan 2021-12-07 04:42:32 PST 
Created attachment 446160 [details]
Patch
Comment 13 Per Arne Vollan 2021-12-07 04:51:11 PST 
Created attachment 446164 [details]
Patch
Comment 14 Per Arne Vollan 2021-12-07 09:12:54 PST 
Created attachment 446184 [details]
Patch
Comment 15 Per Arne Vollan 2021-12-07 09:31:07 PST 
Created attachment 446188 [details]
Patch
Comment 16 Per Arne Vollan 2021-12-07 09:34:41 PST 
Created attachment 446190 [details]
Patch
Comment 17 Per Arne Vollan 2021-12-07 10:46:27 PST 
Created attachment 446202 [details]
Patch
Comment 18 Per Arne Vollan 2021-12-09 08:25:25 PST 
Created attachment 446551 [details]
Patch
Comment 19 Per Arne Vollan 2021-12-09 11:34:20 PST 
Created attachment 446577 [details]
Patch
Comment 20 Per Arne Vollan 2021-12-09 12:42:33 PST 
Created attachment 446590 [details]
Patch
Comment 21 Per Arne Vollan 2021-12-09 13:30:58 PST 
<rdar://83201124>
Comment 22 Per Arne Vollan 2021-12-09 13:32:52 PST 
<rdar://56634240>
Comment 23 Per Arne Vollan 2022-02-17 09:24:24 PST 
Created attachment 452378 [details]
Patch
Comment 24 Per Arne Vollan 2022-02-17 11:22:12 PST 
Created attachment 452394 [details]
Patch
Comment 25 Per Arne Vollan 2022-02-17 13:30:20 PST 
Created attachment 452424 [details]
Patch
Comment 26 Alex Christensen 2022-02-17 13:47:24 PST 
Comment on attachment 452424 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=452424&action=review

> Source/WTF/wtf/PlatformEnableCocoa.h:169
> +#define ENABLE_CONTENT_FILTERING_IN_NETWORKING_PROCESS ENABLE_CONTENT_FILTERING

I'm still not convinced we need this.  I think it ought to be the same everywhere.  I don't see any need to have it different on newer OSes.

> Source/WebCore/loader/ContentFilter.cpp:279
> +#if ENABLE(CONTENT_FILTERING_IN_NETWORKING_PROCESS)
> +void ContentFilter::deliverResourceData(const SharedBuffer& buffer, size_t encodedDataLength)
> +#else
>  void ContentFilter::deliverResourceData(CachedResource& resource)
> +#endif

I think it would be better to keep the same signature in both cases.

> Source/WebCore/loader/SubstituteData.h:64
> +        template<class Decoder> static WARN_UNUSED_RETURN bool decode(Decoder&, SubstituteData&);

Could you use the modern decoder that returns a std::optional<SubstituteData> instead?

> Source/WebKit/WebProcess/Network/WebResourceLoader.h:37
> +#include <WebCore/SubstituteData.h>

I think this can be a declaration instead of including the whole header.

> Source/WebKit/WebProcess/Network/WebResourceLoader.messages.in:42
> +    ContentFilterDidBlockLoad(WebCore::ContentFilterUnblockHandler unblockHandler, String unblockRequestDeniedScript)

I don't understand how a ContentFilterUnblockHandler can be serialized when it contains a std::function.  Do we just lose that when going to a different process?  Is that ok?

> WebKit.xcworkspace/xcshareddata/xcschemes/All Source.xcscheme:160
> +      buildConfiguration = "Release"

This change should probably not be included.

> WebKit.xcworkspace/xcshareddata/xcschemes/All Tools.xcscheme:99
> +      buildConfiguration = "Release"

ditto
Comment 27 Per Arne Vollan 2022-02-18 13:12:22 PST 
Created attachment 452569 [details]
Patch
Comment 28 Per Arne Vollan 2022-02-18 13:41:37 PST 
Created attachment 452576 [details]
Patch
Comment 29 Per Arne Vollan 2022-02-18 13:55:44 PST 
(In reply to Alex Christensen from comment #26)
> Comment on attachment 452424 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=452424&action=review
> 
> > Source/WTF/wtf/PlatformEnableCocoa.h:169
> > +#define ENABLE_CONTENT_FILTERING_IN_NETWORKING_PROCESS ENABLE_CONTENT_FILTERING
> 
> I'm still not convinced we need this.  I think it ought to be the same
> everywhere.  I don't see any need to have it different on newer OSes.
> 

The reasoning behind the feature flag is to land with the feature flag off by default, and then enable when we believe there are no regressions. This patch does not enable the feature. After enabling, I expect that the feature flag and obsolete code will be deleted quickly.

> > Source/WebCore/loader/ContentFilter.cpp:279
> > +#if ENABLE(CONTENT_FILTERING_IN_NETWORKING_PROCESS)
> > +void ContentFilter::deliverResourceData(const SharedBuffer& buffer, size_t encodedDataLength)
> > +#else
> >  void ContentFilter::deliverResourceData(CachedResource& resource)
> > +#endif
> 
> I think it would be better to keep the same signature in both cases.
> 

Fixed!

> > Source/WebCore/loader/SubstituteData.h:64
> > +        template<class Decoder> static WARN_UNUSED_RETURN bool decode(Decoder&, SubstituteData&);
> 
> Could you use the modern decoder that returns a
> std::optional<SubstituteData> instead?
> 

Done.

> > Source/WebKit/WebProcess/Network/WebResourceLoader.h:37
> > +#include <WebCore/SubstituteData.h>
> 
> I think this can be a declaration instead of including the whole header.
> 

Fixed.

> > Source/WebKit/WebProcess/Network/WebResourceLoader.messages.in:42
> > +    ContentFilterDidBlockLoad(WebCore::ContentFilterUnblockHandler unblockHandler, String unblockRequestDeniedScript)
> 
> I don't understand how a ContentFilterUnblockHandler can be serialized when
> it contains a std::function.  Do we just lose that when going to a different
> process?  Is that ok?
> 

Ah, that is a good catch! I have addressed this in the latest patch by calling the function requestUnblockAsync in the Networking process instead.

> > WebKit.xcworkspace/xcshareddata/xcschemes/All Source.xcscheme:160
> > +      buildConfiguration = "Release"
> 
> This change should probably not be included.
> 
> > WebKit.xcworkspace/xcshareddata/xcschemes/All Tools.xcscheme:99
> > +      buildConfiguration = "Release"
> 
> ditto

Done.

Thanks for reviewing!
Comment 30 Brent Fulgham 2022-02-18 16:12:16 PST 
Comment on attachment 452576 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=452576&action=review

r=me

> Source/WebCore/loader/ContentFilter.cpp:122
> +    LOG(ContentFiltering, "ContentFilter will start filtering main resource at <%s>.\n", url.string().ascii().data());

This is privacy-impacting. I think you need to use the %{sensitive}s logging declaration here.

> Source/WebCore/loader/ContentFilter.cpp:176
> +        LOG(ContentFiltering, "ContentFilter received %zu bytes of data from <%s>.\n", data.size(), url().string().ascii().data());

Ditto (even though the old code added logging with the secret information).

> Source/WebCore/loader/ContentFilter.cpp:216
> +        LOG(ContentFiltering, "ContentFilter will finish filtering main resource at <%s>.\n", url().string().ascii().data());

Ditto.

> Source/WebCore/loader/ContentFilter.cpp:271
> +    LOG(ContentFiltering, "ContentFilter decided load should be %s for main resource at <%s>.\n", state == State::Allowed ? "allowed" : "blocked", url().string().ascii().data());

Ditto.

> Source/WebCore/loader/ContentFilter.cpp:355
> +    for (unsigned i = 0; i < m_buffers.size(); i++)

It seems like this could be a modern C++ loop:

for (auto& buffer in m_buffers)
    deliverResourceData(*buffer.buffer, buffer.encodedDataLength);

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:1296
> +    shouldFilter = m_contentFilter && !m_contentFilter->continueAfterDataReceived(sharedBuffer, m_bufferedDataEncodedDataLength);

Why not:
bool shouldFilter = m_contentFilter .... <<ETC>>

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:614
> +    if (!page)

I'd just do:

if (auto* page = ...
    page->reload....
Comment 31 Per Arne Vollan 2022-02-18 16:39:35 PST 
Created attachment 452606 [details]
Patch
Comment 32 Per Arne Vollan 2022-02-18 16:41:31 PST 
(In reply to Brent Fulgham from comment #30)
> Comment on attachment 452576 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=452576&action=review
> 
> r=me
> 
> > Source/WebCore/loader/ContentFilter.cpp:122
> > +    LOG(ContentFiltering, "ContentFilter will start filtering main resource at <%s>.\n", url.string().ascii().data());
> 
> This is privacy-impacting. I think you need to use the %{sensitive}s logging
> declaration here.
> 
> > Source/WebCore/loader/ContentFilter.cpp:176
> > +        LOG(ContentFiltering, "ContentFilter received %zu bytes of data from <%s>.\n", data.size(), url().string().ascii().data());
> 
> Ditto (even though the old code added logging with the secret information).
> 
> > Source/WebCore/loader/ContentFilter.cpp:216
> > +        LOG(ContentFiltering, "ContentFilter will finish filtering main resource at <%s>.\n", url().string().ascii().data());
> 
> Ditto.
> 
> > Source/WebCore/loader/ContentFilter.cpp:271
> > +    LOG(ContentFiltering, "ContentFilter decided load should be %s for main resource at <%s>.\n", state == State::Allowed ? "allowed" : "blocked", url().string().ascii().data());
> 
> Ditto.
> 
> > Source/WebCore/loader/ContentFilter.cpp:355
> > +    for (unsigned i = 0; i < m_buffers.size(); i++)
> 
> It seems like this could be a modern C++ loop:
> 
> for (auto& buffer in m_buffers)
>     deliverResourceData(*buffer.buffer, buffer.encodedDataLength);
> 
> > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:1296
> > +    shouldFilter = m_contentFilter && !m_contentFilter->continueAfterDataReceived(sharedBuffer, m_bufferedDataEncodedDataLength);
> 
> Why not:
> bool shouldFilter = m_contentFilter .... <<ETC>>
> 
> > Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:614
> > +    if (!page)
> 
> I'd just do:
> 
> if (auto* page = ...
>     page->reload....

I have addressed these issues in the latest patch. Thanks for reviewing!
Comment 33 Per Arne Vollan 2022-02-18 16:57:48 PST 
Created attachment 452610 [details]
Patch
Comment 34 Per Arne Vollan 2022-02-18 17:02:34 PST 
Committed r290189 (247517@trunk): <https://commits.webkit.org/247517@trunk>