Last Comment Bug 687086 - CSP report only mode doesn't work for inline-script and eval-script
: CSP report only mode doesn't work for inline-script and eval-script
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Security (show other bugs)
: Trunk
: x86 Windows XP
: -- normal (vote)
: mozilla22
Assigned To: Sid Stamm [:geekboy or :sstamm]
:
Mentors:
Depends on: 552523
Blocks: CSP 801783 805929
  Show dependency treegraph
 
Reported: 2011-09-16 09:38 PDT by Sunil
Modified: 2013-03-30 17:59 PDT (History)
9 users (show)
mozbugs: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
wip (25.40 KB, patch)
2012-10-15 16:47 PDT, Sid Stamm [:geekboy or :sstamm]
bzbarsky: feedback+
Details | Diff | Splinter Review
proposed patch (30.71 KB, patch)
2012-10-18 14:40 PDT, Sid Stamm [:geekboy or :sstamm]
bzbarsky: review+
Details | Diff | Splinter Review
fix (35.65 KB, patch)
2012-10-26 12:56 PDT, Sid Stamm [:geekboy or :sstamm]
mozbugs: review+
Details | Diff | Splinter Review
fix (35.95 KB, patch)
2013-03-28 11:59 PDT, Sid Stamm [:geekboy or :sstamm]
mozbugs: review+
Details | Diff | Splinter Review

Description Sunil 2011-09-16 09:38:07 PDT
User Agent: Mozilla/5.0 (Windows NT 5.1) AppleWebKit/535.1 (KHTML, like Gecko) Chrome/13.0.782.220 Safari/535.1

Steps to reproduce:

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.16) Gecko/20110302 Iceweasel/3.5.16 (like Firefox/3.5.16)
Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0.1) Gecko/20100101 Firefox/4.0.1

Hi.

If I use the following (apache) configuration
Header set X-Content-Security-Policy-Report-Only \
  "allow 'none'; \
  report-uri /perl/csp-report.pl"

for a document that has inline scripts, no violation reports are generated for them.

Sincerely,
Sunil


Reproducible: Always



Expected results:

I was expecting a violation report to be generated and sent to the server, however that didn't happen. 

However, if I do operate with CSP header 'X-Content-Security-Policy' it does correctly prevent the script from execution and also send a violation report.
Comment 1 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-09-16 11:06:08 PDT
Does this happen with Firefox 9.0a1?
Comment 2 Sunil 2011-09-16 14:38:47 PDT
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #1)
> Does this happen with Firefox 9.0a1?

Yes, same behavior is observed in Firefox 6 and Firefox 9.0a1.
Comment 3 Boris Zbarsky [:bz] 2012-10-12 14:30:12 PDT
Confirming.  Specifically, at least in the inline script case we don't log correctly.
Comment 4 Sid Stamm [:geekboy or :sstamm] 2012-10-12 14:36:25 PDT
pointing to the CSP tracking bug
Comment 5 Sid Stamm [:geekboy or :sstamm] 2012-10-12 14:42:16 PDT
this is probably because the inline script compiler is asking CSP if it's okay to run the script, and CSP is saying "don't block it" but not telling the caller that a violation report needs to go out.  We probably need an outparam in GetAllowsInline (and GetAllowsEval) that returns whether or not it's a violation for when CSP is report-only and not blocking stuff.
Comment 6 Sid Stamm [:geekboy or :sstamm] 2012-10-15 16:47:12 PDT
Created attachment 671654 [details] [diff] [review]
wip

bz: I couldn't get this out of my head all morning, so I started on a patch even though I told you I had no time. 

I'm new at the outparams thing with XPCOM, is this on the right track?  I promise to add tests (and confirm it doesn't break CSP or other related unit tests).
Comment 7 Boris Zbarsky [:bz] 2012-10-15 20:29:59 PDT
Comment on attachment 671654 [details] [diff] [review]
wip

The outparams bit looks fine, though it may be a good idea to keep the "should I allow this?" as a return value... Note that if you do, I think that will change the order of the two bool* args in C++.

Do you still want the JS_ReportError in the setTimeout code?

I don't see any callers of SetReportCSPViolations on workers. Should there be some?
Comment 8 Sid Stamm [:geekboy or :sstamm] 2012-10-18 12:14:37 PDT
(In reply to Boris Zbarsky (:bz) from comment #7)
> Do you still want the JS_ReportError in the setTimeout code?

Good catch, I don't.  Spec says: "When called with a first argument that is non-callable (e.g., not a function), the setTimeout function must return zero without creating a timer."

> I don't see any callers of SetReportCSPViolations on workers. Should there
> be some?

Probably not.  I just added it as a reflex (copied the pattern around mEvalAllowed).  I'll drop that function.

Thanks for the quick feedback.
Comment 9 Sid Stamm [:geekboy or :sstamm] 2012-10-18 14:40:48 PDT
Created attachment 672965 [details] [diff] [review]
proposed patch

Addressed all the feedback comments, added tests to test_cspreports.js.

It's stewing on try:
https://tbpl.mozilla.org/?tree=Try&rev=93501265c67d
Comment 10 Boris Zbarsky [:bz] 2012-10-18 14:46:33 PDT
Comment on attachment 672965 [details] [diff] [review]
proposed patch

In dom and content code, this:

  if (foo) return something;

should be written like so:

  if (foo) {
    return something;
  }

Please fix the instances of that in this patch, and r=me
Comment 11 Sid Stamm [:geekboy or :sstamm] 2012-10-18 16:39:35 PDT
So removing the JS_ReportError call destroyed the eval-blocking mochitests.
see: 
http://mxr.mozilla.org/mozilla-central/source/content/base/test/file_CSP_evalscript_main.js

I can re-write the tests to check for blocked eval, but without making a racy test I am not sure how to check whether setTimeout executed or not.
Comment 12 Boris Zbarsky [:bz] 2012-10-18 17:33:52 PDT
Well, a setTimeout with a function argument would still run, right?  So if you have:

  function bar() {}
  setTimeout("foo", n);
  setTimeout(bar, n);

then you're guaranteed that the string "foo" will NOT be eval'ed after function bar is called.
Comment 13 Sid Stamm [:geekboy or :sstamm] 2012-10-26 12:56:52 PDT
Created attachment 675659 [details] [diff] [review]
fix

Updated patch with "eval" tests that should work based on the pattern suggested by bz.  For some reason I remember being told that order of execution was not guaranteed for two timeouts with the same interval.  Seems to work though, we'll see if try likes it.

https://tbpl.mozilla.org/?tree=Try&rev=f162b743fd2e

I only found one instance of "if (foo) bar;", but fixed it as requested in comment 10.

Carrying over r=bz.
Comment 14 Boris Zbarsky [:bz] 2012-10-26 12:58:26 PDT
>+        if (!allowsInline) return NS_ERROR_DOM_RETVAL_UNDEFINED;

in nsJSThunk::EvaluateScript.
Comment 15 Sid Stamm [:geekboy or :sstamm] 2012-10-26 12:59:17 PDT
This is no longer blocked by bug 552523 (works without it).
Comment 16 Sid Stamm [:geekboy or :sstamm] 2012-10-26 13:03:09 PDT
(In reply to Boris Zbarsky (:bz) from comment #14)
> >+        if (!allowsInline) return NS_ERROR_DOM_RETVAL_UNDEFINED;
> 
> in nsJSThunk::EvaluateScript.

*sigh*, thanks bz.  My copy of Eyeball Grep is apparently not working today.  I'll fix that and reattach after try gets back to me with a thumbs up or down.
Comment 17 Sid Stamm [:geekboy or :sstamm] 2013-01-30 15:38:07 PST
Need to unbitrot and make sure it doesn't break new stuff.
Comment 18 Sid Stamm [:geekboy or :sstamm] 2013-03-28 11:59:02 PDT
Created attachment 730797 [details] [diff] [review]
fix

Unbitrotted and carrying over r=bz.  Also addressed the failure of my eyegrep.

Try: https://tbpl.mozilla.org/?tree=Try&rev=bdfa3da43011
Comment 19 Sid Stamm [:geekboy or :sstamm] 2013-03-28 17:06:40 PDT
Inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/c29279c95238
Comment 20 Ryan VanderMeulen [:RyanVM] 2013-03-30 17:59:38 PDT
https://hg.mozilla.org/mozilla-central/rev/c29279c95238

Note You need to log in before you can comment on or make changes to this bug.