Summary: | [Beacon] Do connect-src CSP check on redirects as well | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||
Component: | WebKit2 | Assignee: | 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
Chris Dumez
2017-08-09 16:54:40 PDT
Created attachment 317821 [details]
Patch
Created attachment 317824 [details]
Patch
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 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 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? Created attachment 317837 [details]
Patch
> > 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 Created attachment 317841 [details]
Patch
Comment on attachment 317841 [details] Patch Clearing flags on attachment: 317841 Committed r220549: <http://trac.webkit.org/changeset/220549> All reviewed patches have been landed. Closing bug. |