Bug 153748 – CSP: Support checking content security policy without a script execution context

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Daniel Bates]

We should support instantiating a ContentSecurityPolicy object for checking a content security policy without a script execution context. This is a step towards checking the CSP of a web worker for redirected script and XHR loads.

[Radar WebKit Bug Importer]

<rdar://problem/24439149>

[Daniel Bates]

Created attachment 270395 [details]
Patch

[Daniel Bates]

Created attachment 270396 [details]
Patch

[Darin Adler]

Comment on attachment 270395 [details]
Patch

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

So all of this refactoring for workers results in no behavior change? I have a few comments.

> Source/WebCore/page/csp/ContentSecurityPolicy.cpp:242
> +            return m_policy->protocolMatchesSelf(url);

What guarantees m_policy is non-null?

> Source/WebCore/page/csp/ContentSecurityPolicy.cpp:1328
> +    SecurityOrigin* securityOrigin = scriptExecutionContext.securityOrigin();
> +    ASSERT(securityOrigin);

Why does ScriptExecutionContext::securityOrigin return a pointer instead of a reference? Can it ever be null? Why can’t it be null here?

I would like the local variable to be a reference. One way to do that is:

    ASSERT(scriptExecutionContext.securityOrigin());
    auto& securityOrigin = *scriptExecutionContext.securityOrigin();

> Source/WebCore/page/csp/ContentSecurityPolicy.cpp:1623
> +    if (!m_scriptExecutionContext || !is<Document>(*m_scriptExecutionContext))

Can write this more simply:

    if (!is<Document>(m_scriptExecutionContext))

Also, once we know it’s a Document it seems the rest of the function should use document, not m_scriptExecutionContext.

> Source/WebCore/page/csp/ContentSecurityPolicy.cpp:1680
> +        PingLoader::sendViolationReport(*frame, m_scriptExecutionContext->completeURL(url), report.copyRef());

Should be document.completeURL, right?

> Source/WebCore/page/csp/ContentSecurityPolicy.h:32
> +#include "SecurityContext.h"

Not good to require even more includes. Can we do something to keep this to a minimum?

> Source/WebCore/page/csp/ContentSecurityPolicy.h:34
>  #include <wtf/RefCounted.h>

Don’t need this. Also don’t need the WTFString.h include if we are including URL.h.

[Daniel Bates]

(In reply to comment #4)
> Comment on attachment 270395 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=270395&action=review
> 
> So all of this refactoring for workers results in no behavior change? 

Correct. I'll add a remark in the ChangeLog to state that there is no behavior change.

> 
> > Source/WebCore/page/csp/ContentSecurityPolicy.cpp:242
> > +            return m_policy->protocolMatchesSelf(url);
> 
> What guarantees m_policy is non-null?
>

Notice that m_policy is a pointer to a ContentSecurityPolicy object. A CSPSource is always initialized with a non-null pointer to the ContentSecurityPolicy object that created it and only a ContentSecurityPolicy object can create a CSPSource because the  declaration/definition of class CSPSource is in ContentSecurityPolicy.cpp.

I plan to change the classes CSPSource, CSPSourceList, CSPDirective, MediaListDirective, CSPDirectiveList from taking a pointer to a ContentSecurityPolicy object to taking a reference to a ContentSecurityPolicy to make it clear that their member field m_policy is never null. Unless you feel that we should incorporate such a change in this patch, I suggest that we defer such a this change to a subsequent patch to keep the patch focused on the refactoring necessary to separate out the script execution context.
 
> > Source/WebCore/page/csp/ContentSecurityPolicy.cpp:1328
> > +    SecurityOrigin* securityOrigin = scriptExecutionContext.securityOrigin();
> > +    ASSERT(securityOrigin);
> 
> Why does ScriptExecutionContext::securityOrigin return a pointer instead of
> a reference?

Notice that ScriptExecutionContext inherits from SecurityContext, which defines member function securityOrigin(). We should look to make SecurityContext::securityOrigin() return a reference as it is a programmer error/correctness issue that a SecurityContext does not have a SecurityOrigin. From my understanding the design of having SecurityContext::securityOrigin() return a nullptr falls out from the concept of an "empty" SecurityOrigin (see bug #72250 that helped move us farther away from this concept) and the current design of SecurityContext not taking a SecurityOriginPolicy/SecurityOrigin on instantiation. A caller must explicitly call SecurityContext::setSecurityOriginPolicy() to pass a SecurityOriginPolicy that owns a SecurityOrigin.

> Can it ever be null?

In theory, it can be null and its nullity represents a programmer error/correctness issue of failing to initialize it. In practice it is not null and its nullity would manifest itself in the crashing of many layout tests.

> Why can’t it be null here?

It can be null if a ContentSecurityPolicy is constructed before a SecurityOrigin is set on the passed ScriptExecutionContext (say, lines 74 and 75 of WorkerGlobalScope.cpp are swapped in attachment #270396 [details]). Maybe we should prevent this as opposed to using a run-time assertion?

I chose a run-time assertion since failing to initialize a SecurityOrigin would lead to many layout tests crashing, including CSP tests, because we access SecurityContext::securityOrigin() without checking if it is null throughout the codebase.

Additional remarks:

One way to fix this without changing the design of SecurityContext is to have ContentSecurityPolicy constructor take both a reference to a ScriptExecutionContext and a reference to a SecurityOrigin. Another way, is to have the a single constructor that takes a reference to a SecurityOrigin and expose a member functions that takes a ScriptExecutionContext or a new interface, say ContentSecurityPolicyDelegate, that only exposes the functionality of ScriptExecutionContext that is needed by ContentSecurityPolicy: logging, disabling of JavaScript eval() and operator eval, URL completion, and applying of Document sandbox properties.

> I would like the local variable to be a reference. One way to do that is:
> 
>     ASSERT(scriptExecutionContext.securityOrigin());
>     auto& securityOrigin = *scriptExecutionContext.securityOrigin();
> 

Will fix.

> > Source/WebCore/page/csp/ContentSecurityPolicy.cpp:1623
> > +    if (!m_scriptExecutionContext || !is<Document>(*m_scriptExecutionContext))
> 
> Can write this more simply:
> 
>     if (!is<Document>(m_scriptExecutionContext))
> 
> Also, once we know it’s a Document it seems the rest of the function should
> use document, not m_scriptExecutionContext.
> 

Will fix.

> > Source/WebCore/page/csp/ContentSecurityPolicy.cpp:1680
> > +        PingLoader::sendViolationReport(*frame, m_scriptExecutionContext->completeURL(url), report.copyRef());
> 
> Should be document.completeURL, right?
> 

Yes. Will fix.

> > Source/WebCore/page/csp/ContentSecurityPolicy.h:32
> > +#include "SecurityContext.h"
> 
> Not good to require even more includes. Can we do something to keep this to
> a minimum?
> 

Notice that I include this header to get the definition of the enum SandboxFlags and the enumeration value SandboxNone.

One way to avoid including this header here is to revert the removal of the SandboxFlags typedef from this file and default initialize m_sandboxFlags to SandboxNone in the constructor.

> > Source/WebCore/page/csp/ContentSecurityPolicy.h:34
> >  #include <wtf/RefCounted.h>
> 
> Don’t need this. Also don’t need the WTFString.h include if we are including
> URL.h.

Will remove RefCounted.h and WTFString.h.

[Daniel Bates]

Created attachment 270466 [details]
Patch

Updated patch based on Darin Adler's feedback. I changed the added overloaded ContentSecurityPolicy constructor to take a SecurityOrigin as opposed to a URL since this will better align with its future use in DocumentThreadableLoader. I also chose to incorporate many pointer-to-reference conversions in this patch as opposed to doing such conversions in another patch. Let me know if it is preferred to do such changes in a separate bug/patch.

[Daniel Bates]

Created attachment 270467 [details]
Patch

[Darin Adler]

Comment on attachment 270467 [details]
Patch

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

Looks like iOS EWS is broken. We should get someone to fix it.

> Source/WebCore/page/csp/ContentSecurityPolicy.cpp:1204
>          if (urlBegin < position) {
>              String url = String(urlBegin, position - urlBegin);
> -            m_reportURIs.append(m_policy->completeURL(url));
> +            m_reportURIs.append(url);
>          }

I think this would read better without the local variable. In fact, I think it should say:

    m_reportURIs.append(value.substring(urlBegin, position - urlBegin));

That will optimize the case where the entire string is a single URL to not copy the string at all.

> Source/WebCore/page/csp/ContentSecurityPolicy.cpp:1402
> +    ASSERT(m_scriptExecutionContext);
> +    if (!m_lastPolicyEvalDisabledErrorMessage.isNull())
> +        m_scriptExecutionContext->disableEval(m_lastPolicyEvalDisabledErrorMessage);
> +
> +    if (m_sandboxFlags != SandboxNone && is<Document>(m_scriptExecutionContext))
> +        m_scriptExecutionContext->enforceSandboxFlags(m_sandboxFlags);

Paragraphing here is slightly peculiar. I would suggest adding or removing a blank line so this is either 3 paragraphs or 1 paragraph, rather than 2.

> Source/WebCore/page/csp/ContentSecurityPolicy.h:107
> +    // The following functions are used by internal data structures to callback into this object when parsing, validating,

The verb would be "call back"; "callback" is a noun.

> Source/WebCore/workers/WorkerGlobalScope.cpp:75
> +    setContentSecurityPolicy(std::make_unique<ContentSecurityPolicy>(*static_cast<ScriptExecutionContext*>(this)));

I don’t understand why the static_cast is needed here. Wouldn’t *this work?

[Daniel Bates]

(In reply to comment #8)
> > Source/WebCore/page/csp/ContentSecurityPolicy.cpp:1204
> >          if (urlBegin < position) {
> >              String url = String(urlBegin, position - urlBegin);
> > -            m_reportURIs.append(m_policy->completeURL(url));
> > +            m_reportURIs.append(url);
> >          }
> 
> I think this would read better without the local variable. In fact, I think
> it should say:
> 
>     m_reportURIs.append(value.substring(urlBegin, position - urlBegin));
> 
> That will optimize the case where the entire string is a single URL to not
> copy the string at all.
> 

I will write this as:

m_reportURIs.append(value.substring(urlBegin - characters, position - urlBegin));

because urlBegin - characters represents an index position (since urlBegin is a pointer).

> > Source/WebCore/page/csp/ContentSecurityPolicy.cpp:1402
> > +    ASSERT(m_scriptExecutionContext);
> > +    if (!m_lastPolicyEvalDisabledErrorMessage.isNull())
> > +        m_scriptExecutionContext->disableEval(m_lastPolicyEvalDisabledErrorMessage);
> > +
> > +    if (m_sandboxFlags != SandboxNone && is<Document>(m_scriptExecutionContext))
> > +        m_scriptExecutionContext->enforceSandboxFlags(m_sandboxFlags);
> 
> Paragraphing here is slightly peculiar. I would suggest adding or removing a
> blank line so this is either 3 paragraphs or 1 paragraph, rather than 2.
> 

Will remove the blank line between paragraph 1 and paragraph 2 so that there is exactly 1 paragraph.

> > Source/WebCore/page/csp/ContentSecurityPolicy.h:107
> > +    // The following functions are used by internal data structures to callback into this object when parsing, validating,
> 
> The verb would be "call back"; "callback" is a noun.
> 

Will substitute "call back" for "callback".

> > Source/WebCore/workers/WorkerGlobalScope.cpp:75
> > +    setContentSecurityPolicy(std::make_unique<ContentSecurityPolicy>(*static_cast<ScriptExecutionContext*>(this)));
> 
> I don’t understand why the static_cast is needed here.

Will remove the static_cast as it is not needed.

> Wouldn’t *this work?

Yes.

[Daniel Bates]

Committed r196012: <http://trac.webkit.org/changeset/196012>