Bug 233760 – Move content filtering to Networking process

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Per Arne Vollan]

Moving content filtering to the Networking process will enable us to further strengthen the WebContent process' sandbox.

[Per Arne Vollan]

Created attachment 445721 [details]
Patch

[Per Arne Vollan]

WIP patch.

[Per Arne Vollan]

Created attachment 445723 [details]
Patch

[Per Arne Vollan]

Created attachment 445775 [details]
Patch

[Per Arne Vollan]

Created attachment 445784 [details]
Patch

[Alex Christensen]

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?

[Per Arne Vollan]

(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!

[Per Arne Vollan]

Created attachment 446005 [details]
Patch

[Per Arne Vollan]

Created attachment 446012 [details]
Patch

[Per Arne Vollan]

Created attachment 446141 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/86150702>

[Per Arne Vollan]

Created attachment 446160 [details]
Patch

[Per Arne Vollan]

Created attachment 446164 [details]
Patch

[Per Arne Vollan]

Created attachment 446184 [details]
Patch

[Per Arne Vollan]

Created attachment 446188 [details]
Patch

[Per Arne Vollan]

Created attachment 446190 [details]
Patch

[Per Arne Vollan]

Created attachment 446202 [details]
Patch

[Per Arne Vollan]

Created attachment 446551 [details]
Patch

[Per Arne Vollan]

Created attachment 446577 [details]
Patch

[Per Arne Vollan]

Created attachment 446590 [details]
Patch

[Per Arne Vollan]

<rdar://83201124>

[Per Arne Vollan]

<rdar://56634240>

[Per Arne Vollan]

Created attachment 452378 [details]
Patch

[Per Arne Vollan]

Created attachment 452394 [details]
Patch

[Per Arne Vollan]

Created attachment 452424 [details]
Patch

[Alex Christensen]

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

[Per Arne Vollan]

Created attachment 452569 [details]
Patch

[Per Arne Vollan]

Created attachment 452576 [details]
Patch

[Per Arne Vollan]

(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!

[Brent Fulgham]

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....

[Per Arne Vollan]

Created attachment 452606 [details]
Patch

[Per Arne Vollan]

(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!

[Per Arne Vollan]

Created attachment 452610 [details]
Patch

[Per Arne Vollan]

Committed r290189 (247517@trunk): <https://commits.webkit.org/247517@trunk>