Bug 154299 – CSP: sandbox directive should be ignored when contained in a policy defined via a meta element

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Daniel Bates]

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

[Radar WebKit Bug Importer]

<rdar://problem/24680433>

[Daniel Bates]

Created attachment 271465 [details]
Patch and Layout Tests

[Daniel Bates]

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.

[Daniel Bates]

Created attachment 271510 [details]
Patch and Layout Tests

[Brent Fulgham]

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

[Daniel Bates]

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

[Daniel Bates]

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

[Daniel Bates]

Committed r196874: <http://trac.webkit.org/changeset/196874>

[Daniel Bates]

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