Bug 175410

Summary: [Beacon] Do connect-src CSP check on redirects as well
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebKit2Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: beidson, buildbot, commit-queue, dbates, ggaren, japhet, mkwst, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
URL: https://w3c.github.io/webappsec-csp/#directive-connect-src
Bug Depends on:    
Bug Blocks: 147885    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Chris Dumez 2017-08-09 16:54:40 PDT
Do connect-src CSP check on sendBeacon redirects as well.
Comment 1 Radar WebKit Bug Importer 2017-08-09 16:55:31 PDT
<rdar://problem/33815470>
Comment 2 Chris Dumez 2017-08-10 10:52:36 PDT
Created attachment 317821 [details]
Patch
Comment 3 Chris Dumez 2017-08-10 10:57:57 PDT
Created attachment 317824 [details]
Patch
Comment 4 youenn fablet 2017-08-10 12:02:08 PDT
Comment on attachment 317824 [details]
Patch

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

> Source/WebCore/loader/cache/CachedResource.cpp:266
> +        auto* contentSecurityPolicy = document && !document->shouldBypassMainWorldContentSecurityPolicy() ? document->contentSecurityPolicy() : nullptr;

I guess this code is fine since beacon is only exposed in document.
But it goes somehow against the idea of CachedResource being something that relates to a loading context, which is not always a document.
Maybe worth a comment.

> Source/WebKit/NetworkProcess/PingLoad.cpp:191
> +            m_contentSecurityPolicy->didReceiveHeaders(*m_parameters.cspResponseHeaders, ContentSecurityPolicy::ReportParsingErrors::No);

Do we always need to create m_contentSecurityPolicy?
If m_parameters.cspResponseHeaders is null, does not it mean we do not need to do CSP checks?

> Source/WebKitLegacy/WebCoreSupport/WebResourceLoadScheduler.h:62
> +    void createPingHandle(WebCore::NetworkingContext*, WebCore::ResourceRequest&, Ref<WebCore::SecurityOrigin>&& sourceOrigin, WebCore::ContentSecurityPolicy*, const WebCore::FetchOptions&) override;

Can we make WebResourceLoadScheduler final and change override to final here?
Comment 5 Chris Dumez 2017-08-10 12:47:32 PDT
Comment on attachment 317824 [details]
Patch

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

>> Source/WebCore/loader/cache/CachedResource.cpp:266
>> +        auto* contentSecurityPolicy = document && !document->shouldBypassMainWorldContentSecurityPolicy() ? document->contentSecurityPolicy() : nullptr;
> 
> I guess this code is fine since beacon is only exposed in document.
> But it goes somehow against the idea of CachedResource being something that relates to a loading context, which is not always a document.
> Maybe worth a comment.

Actually, I can get a contentSecurityPolicy from any ScriptExecutionContext, not just the document. I'll look at making this clearer.

>> Source/WebKit/NetworkProcess/PingLoad.cpp:191
>> +            m_contentSecurityPolicy->didReceiveHeaders(*m_parameters.cspResponseHeaders, ContentSecurityPolicy::ReportParsingErrors::No);
> 
> Do we always need to create m_contentSecurityPolicy?
> If m_parameters.cspResponseHeaders is null, does not it mean we do not need to do CSP checks?

Yes, this is the point. m_parameters.cspResponseHeaders is null when document->shouldBypassMainWorldContentSecurityPolicy() is true.

>> Source/WebKitLegacy/WebCoreSupport/WebResourceLoadScheduler.h:62
>> +    void createPingHandle(WebCore::NetworkingContext*, WebCore::ResourceRequest&, Ref<WebCore::SecurityOrigin>&& sourceOrigin, WebCore::ContentSecurityPolicy*, const WebCore::FetchOptions&) override;
> 
> Can we make WebResourceLoadScheduler final and change override to final here?

Ok.
Comment 6 Chris Dumez 2017-08-10 12:54:43 PDT
Comment on attachment 317824 [details]
Patch

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

>>> Source/WebCore/loader/cache/CachedResource.cpp:266
>>> +        auto* contentSecurityPolicy = document && !document->shouldBypassMainWorldContentSecurityPolicy() ? document->contentSecurityPolicy() : nullptr;
>> 
>> I guess this code is fine since beacon is only exposed in document.
>> But it goes somehow against the idea of CachedResource being something that relates to a loading context, which is not always a document.
>> Maybe worth a comment.
> 
> Actually, I can get a contentSecurityPolicy from any ScriptExecutionContext, not just the document. I'll look at making this clearer.

Hmm... This method takes in a CachedResourceLoader which seems to be tied to a document, isn't it?
Comment 7 Chris Dumez 2017-08-10 12:55:29 PDT
Created attachment 317837 [details]
Patch
Comment 8 youenn fablet 2017-08-10 13:05:01 PDT
> > Do we always need to create m_contentSecurityPolicy?
> > If m_parameters.cspResponseHeaders is null, does not it mean we do not need to do CSP checks?
> 
> Yes, this is the point. m_parameters.cspResponseHeaders is null when
> document->shouldBypassMainWorldContentSecurityPolicy() is true.

When document->shouldBypassMainWorldContentSecurityPolicy() is true, we are currently creating an empty m_contentSecurityPolicy and calling allowConnectToSource on it.

What is done in WebCore is that we are not calling at all allowConnectToSource if document->shouldBypassMainWorldContentSecurityPolicy() is true.

It seems to me we should return a null m_contentSecurityPolicy if m_parameters.cspResponseHeaders is null.
Or probably better add a checkCSP routine that would return true if m_parameters.cspResponseHeaders is null.


> Hmm... This method takes in a CachedResourceLoader which seems to be tied to
> a document, isn't it?

Yes
Comment 9 Chris Dumez 2017-08-10 14:08:21 PDT
Created attachment 317841 [details]
Patch
Comment 10 WebKit Commit Bot 2017-08-10 14:51:19 PDT
Comment on attachment 317841 [details]
Patch

Clearing flags on attachment: 317841

Committed r220549: <http://trac.webkit.org/changeset/220549>
Comment 11 WebKit Commit Bot 2017-08-10 14:51:20 PDT
All reviewed patches have been landed.  Closing bug.