The Content Security Policy sandbox directive should only be honored when enforcing a policy defined via a HTTP header as per section sandbox of the Content Security Policy 2.0 spec., <https://www.w3.org/TR/CSP2/#directive-sandbox> (21 July 2015): [[ The sandbox directive will be ignored when monitoring a policy, and when contained in a policy defined via a meta element. Moreover, this directive has no effect when monitored, and has no reporting requirements. ]]
<rdar://problem/24680433>
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>.