1473587 - CSP Violation events should have the correct sample for inline contexts

Status: RESOLVED FIXED

(Core :: DOM: Security, enhancement)

Description

[Andrea Marchesini [:baku]]

Currently samples are fixed strings such as "call to eval() or related function blocked by CSP".

Comment 1

[Andrea Marchesini [:baku]]

Attachment: csp_sample_js.patch

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8990013 [details] [diff] [review]
csp_sample_js.patch

Review of attachment 8990013 [details] [diff] [review]:
-----------------------------------------------------------------

Overall this looks pretty good to me. The only thing I don't fully get is. Do you truncate the sample somewhere? What if it's extra long? Other than that I guess that is fine by me. Please update or answer my question and flag me again - thanks!

Comment 3

[Andrea Marchesini [:baku]]

Attachment: csp_sample_js.patch

This part was not implemented before. Let's do it now.

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8990990 [details] [diff] [review]
csp_sample_js.patch

Review of attachment 8990990 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Baku, that improves things. r=me

Probably worth letting Harald know about this one, since he is interested in getting reports updated.

::: testing/web-platform/tests/content-security-policy/securitypolicyviolation/targeting.html
@@ +34,5 @@
>                  return watcher.wait_for('securitypolicyviolation');
>              }))
>              .then(t.step_func(e => {
>                  assert_equals(e.blockedURI, "inline");
> +                assert_equals(e.lineNumber, 111);

As discussed on IRC, I leave that here as a comment/reference:

https://searchfox.org/mozilla-central/source/testing/web-platform/tests/content-security-policy/securitypolicyviolation/targeting.html#111

https://searchfox.org/mozilla-central/source/testing/web-platform/tests/content-security-policy/securitypolicyviolation/targeting.html#135

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 8990990 [details] [diff] [review]
csp_sample_js.patch

Review of attachment 8990990 [details] [diff] [review]:
-----------------------------------------------------------------

I reviewed the parts under js/ only.

r- for now, but this seems straightforward enough once the caching question is answered. Sorry for the slow review! Ping me again once a new one is up.

::: js/public/Principals.h
@@ +67,5 @@
>   * Used to check if a CSP instance wants to disable eval() and friends.
>   * See js_CheckCSPPermitsJSAction() in jsobj.
>   */
>  typedef bool
> +(* JSCSPEvalChecker)(JSContext* cx, JS::HandleValue value);

Please update the comment. Neither js_CheckCSPPermitsJSAction() nor jsobj.h exists anymore. The function was renamed to GlobalObject::isRuntimeCodeGenEnabled and moved to vm/GlobalObject.cpp.

::: js/src/vm/GlobalObject.cpp
@@ +691,5 @@
>          JSCSPEvalChecker allows = cx->runtime()->securityCallbacks->contentSecurityPolicyAllows;
> +        Value boolValue = BooleanValue(!allows || allows(cx, value));
> +        if (boolValue.isFalse()) {
> +            return false;
> +        }

I would write it like this:

    bool isAllowed = !allows || allows(cx, code);
    if (!isAllowed)
        return false;

    ...
    v.set(..., ..., ..., TrueValue());

@@ +695,5 @@
> +        }
> +
> +        // Let's cache the result only if CSP allows code gen. In this way,
> +        // contentSecurityPolicyAllows callback is executed each time, with the current HandleValue
> +        // value.

Hmm. This comment (or else the code) seems wrong. It looks like the first time the callback returns true, regardless of `value`, we'll stop checking. Is that intended? I think the result can't be cached anymore, unless `allows` is null. Right?

::: js/src/vm/GlobalObject.h
@@ +738,5 @@
>      RegExpStatics* getAlreadyCreatedRegExpStatics() const;
>  
>      static JSObject* getOrCreateThrowTypeError(JSContext* cx, Handle<GlobalObject*> global);
>  
> +    static bool isRuntimeCodeGenEnabled(JSContext* cx, HandleValue value,

Rename the parameter `value` to give some clue what the value is.

Comment 6

[Andrea Marchesini [:baku]]

Attachment: csp_sample_js.patch

Comments applied.

Comment 7

[Jason Orendorff [:jorendorff]]

Comment on attachment 8991807 [details] [diff] [review]
csp_sample_js.patch

Review of attachment 8991807 [details] [diff] [review]:
-----------------------------------------------------------------

OK, cool! r=me for all the changes in js/ .

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca44c68906d0
CSP Violation events should have the correct sample for inline contexts, r=jorendorff, r=ckerschb

Comment 9

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12012 for changes under testing/web-platform/tests

Comment 10

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/ca44c68906d0

Comment 11

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged