Closed Bug 1473587 Opened Last year Closed Last year

CSP Violation events should have the correct sample for inline contexts

Categories

(Core :: DOM: Security, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: baku, Assigned: baku)

References

Details

(Whiteboard: [domsecurity-backlog1] [domsecurity-active][wptsync upstream])

Attachments

(1 file, 2 obsolete files)

Currently samples are fixed strings such as "call to eval() or related function blocked by CSP".
Status: NEW → ASSIGNED
Whiteboard: [domsecurity-backlog1] [domsecurity-active]
Attached patch csp_sample_js.patch (obsolete) — Splinter Review
Attachment #8990013 - Flags: review?(jorendorff)
Attachment #8990013 - Flags: review?(ckerschb)
Blocks: 1472661
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!
Attachment #8990013 - Flags: review?(ckerschb)
Attached patch csp_sample_js.patch (obsolete) — Splinter Review
This part was not implemented before. Let's do it now.
Attachment #8990013 - Attachment is obsolete: true
Attachment #8990013 - Flags: review?(jorendorff)
Attachment #8990990 - Flags: review?(jorendorff)
Attachment #8990990 - Flags: review?(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
Attachment #8990990 - Flags: review?(ckerschb) → review+
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.
Attachment #8990990 - Flags: review?(jorendorff) → review-
Comments applied.
Attachment #8990990 - Attachment is obsolete: true
Attachment #8991807 - Flags: review?(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/ .
Attachment #8991807 - Flags: review?(jorendorff) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca44c68906d0
CSP Violation events should have the correct sample for inline contexts, r=jorendorff, r=ckerschb
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/12012 for changes under testing/web-platform/tests
Whiteboard: [domsecurity-backlog1] [domsecurity-active] → [domsecurity-backlog1] [domsecurity-active][wptsync upstream]
https://hg.mozilla.org/mozilla-central/rev/ca44c68906d0
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.