553448 - nsScriptSecurityManager::ContentSecurityPolicyPermitsJSAction should return JS_TRUE when no subjectPrincipal exists

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect)

Description

[Ben Newman (:bnewman) (:benjamn)]

In the xpcshell we don't have any notion of subject principals (in fact, we don't even have a document).  Failing to find a subjectPrincipal in such an environment should not cause CSP to forbid access.

Comment 1

[Ben Newman (:bnewman) (:benjamn)]

Attachment: Patch emending CSP behavior.

Added an NS_ASSERTION to be sure that no principals could be found at all when subjectPrincipal is null.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 433451 [details] [diff] [review]
Patch emending CSP behavior.

Add a comment near the assertion about why we are asserting that there's no findObjectPrincipals hook and r=me.

Comment 3

[Ben Newman (:bnewman) (:benjamn)]

Comment on attachment 433451 [details] [diff] [review]
Patch emending CSP behavior.

>-    if (callbacks) {
>-        return callbacks->contentSecurityPolicyAllows &&
>-               callbacks->contentSecurityPolicyAllows(cx);
>-    }
>+    if (callbacks && callbacks->contentSecurityPolicyAllows)
>+        return callbacks->contentSecurityPolicyAllows(cx);
> 
>     return JS_TRUE;

For posterity's sake, this change deserves explanation.  Simply, we should allow clients to set callbacks without setting a contendSecurityPolicyAllows callback.

In the situation that inspired this bug, the callbacks->contentSecurityPolicyAllows function pointer was none other than nsScriptSecurityManager::ContentSecurityPolicyPermitsJSAction, by the way.

Comment 4

[Ben Newman (:bnewman) (:benjamn)]

(In reply to comment #2)
> (From update of attachment 433451 [details] [diff] [review])
> Add a comment near the assertion about why we are asserting that there's no
> findObjectPrincipals hook and r=me.

Added a comment pointing to this bug.

Comment 5

[Daniel Veditz [:dveditz]]

Comment on attachment 433451 [details] [diff] [review]
Patch emending CSP behavior.

sr=dveditz

Comment 6

[Ben Newman (:bnewman) (:benjamn)]

Thanks for your help tracking this down, dveditz and mrbkap.

Pushed to projects/electrolysis:
http://hg.mozilla.org/projects/electrolysis/rev/b45221bb1371

Should I also land on mozilla-central, or just wait for e10s to be merged with m-c?

Comment 7

[Ben Newman (:bnewman) (:benjamn)]

Pushed to mozilla-central as well:
http://hg.mozilla.org/mozilla-central/rev/31c428317c68