Summary: | CSP: sandbox directive should be ignored when contained in a policy defined via a meta element | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Daniel Bates <dbates> | ||||||
Component: | WebCore Misc. | Assignee: | Daniel Bates <dbates> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | aestes, bfulgham, cdumez, commit-queue, esprehn+autocc, kangil.han, mkwst, sam, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar, WebExposed | ||||||
Version: | WebKit Local Build | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 154307, 154523 | ||||||||
Attachments: |
|
Description
Daniel Bates
2016-02-16 11:18:10 PST
Created attachment 271465 [details]
Patch and Layout Tests
Comment on attachment 271465 [details] Patch and Layout Tests Actually, we should teach ContentSecurityPolicyDirectiveList::parse() to ignore the sandbox directive when the policy is from a HTTP equiv meta tag. This will make us more closely match the algorithm defined in <https://www.w3.org/TR/2015/CR-CSP2-20150721/#delivery-html-meta-element> and will make it straightforward to also ignore the directive report-uri (bug #154307) and directive frame-ancestors. Created attachment 271510 [details]
Patch and Layout Tests
Comment on attachment 271510 [details] Patch and Layout Tests View in context: https://bugs.webkit.org/attachment.cgi?id=271510&action=review r=me. I don't like to see 'goto' being used, but this case seems to be well considered. > Source/WebCore/page/csp/ContentSecurityPolicy.cpp:462 > + logToConsole("The Content Security Policy directive '" + directiveName + "' is ignored when delivered via a HTML meta element."); Shouldn't this be "... when delivered via an HTML meta element." ? > Source/WebCore/page/csp/ContentSecurityPolicyDirectiveList.cpp:421 > skipExactly<UChar>(position, end, ';'); You could make this a small method, then call it in the "FIXME" case above, followed by continue. This would avoid the use of a 'goto'. However, if you intend to include more cases above, handling the termination state properly might get complicated, so the 'goto' might be the best choice. > LayoutTests/ChangeLog:14 > + create analogous tests for when the sandbox directive is delivered via a HTTP header. I think this should say, ".... delivered via an HTTP header." (In reply to comment #5) > [...] > > Source/WebCore/page/csp/ContentSecurityPolicy.cpp:462 > > + logToConsole("The Content Security Policy directive '" + directiveName + "' is ignored when delivered via a HTML meta element."); > > Shouldn't this be "... when delivered via an HTML meta element." ? > Will fix. > > Source/WebCore/page/csp/ContentSecurityPolicyDirectiveList.cpp:421 > > skipExactly<UChar>(position, end, ';'); > > You could make this a small method, then call it in the "FIXME" case above, > followed by continue. This would avoid the use of a 'goto'. > > However, if you intend to include more cases above, handling the termination > state properly might get complicated, so the 'goto' might be the best choice. > Another idea is use a switch block that breaks out of case ContentSecurityPolicy::PolicyFrom::HTTPEquivMeta for a directive that is not supported when delivered via a meta element. For all other cases, we would add the directive. This would look like: ... while (position < end) { ... if (parseDirective(directiveBegin, position, name, value)) { ASSERT(!name.isEmpty()); switch (policyFrom) { case ContentSecurityPolicy::PolicyFrom::HTTPEquivMeta: // FIXME: We also need to ignore directive report-uri (https://bugs.webkit.org/show_bug.cgi?id=154307). if (equalLettersIgnoringASCIICase(name, sandbox)) { m_policy.reportInvalidDirectiveInHTTPEquivMeta(name); break; } FALLTHROUGH; default: addDirective(name, value); break; } } ASSERT(position == end || *position == ';'); skipExactly<UChar>(position, end, ';'); } ... What are your thoughts? > > LayoutTests/ChangeLog:14 > > + create analogous tests for when the sandbox directive is delivered via a HTTP header. > > I think this should say, ".... delivered via an HTTP header." Will fix. (In reply to comment #6) > [...] > > > Source/WebCore/page/csp/ContentSecurityPolicyDirectiveList.cpp:421 > > > skipExactly<UChar>(position, end, ';'); > > > > You could make this a small method, then call it in the "FIXME" case above, > > followed by continue. This would avoid the use of a 'goto'. > > > > However, if you intend to include more cases above, handling the termination > > state properly might get complicated, so the 'goto' might be the best choice. > > > > Another idea is use a switch block that breaks out of case > ContentSecurityPolicy::PolicyFrom::HTTPEquivMeta for a directive that is not > supported when delivered via a meta element. For all other cases, we would > add the directive. This would look like: > > ... > while (position < end) { > ... > if (parseDirective(directiveBegin, position, name, value)) { > ASSERT(!name.isEmpty()); > switch (policyFrom) { > case ContentSecurityPolicy::PolicyFrom::HTTPEquivMeta: > // FIXME: We also need to ignore directive report-uri > (https://bugs.webkit.org/show_bug.cgi?id=154307). > if (equalLettersIgnoringASCIICase(name, sandbox)) { > m_policy.reportInvalidDirectiveInHTTPEquivMeta(name); > break; > } > FALLTHROUGH; > default: > addDirective(name, value); > break; > } > } > > ASSERT(position == end || *position == ';'); > skipExactly<UChar>(position, end, ';'); > } > ... > > What are your thoughts? > Brent Fulgham liked the switch-statement approach, preferring it over the use of goto, from talking with him last Friday (02/19). Committed r196874: <http://trac.webkit.org/changeset/196874> Added iOS Simulator-specific expected result for test LayoutTests/http/tests/security/contentSecurityPolicy/sandbox-empty-in-http-header-inherited-by-subframe.html in <http://trac.webkit.org/changeset/196885>. |