Last Comment Bug 824652 - crypto.generateCRMFRequest bypasses CSP (allows script execution from a string, without unsafe-eval)
: crypto.generateCRMFRequest bypasses CSP (allows script execution from a strin...
Status: RESOLVED FIXED
[mentor=imelven lang=c++]
: dev-doc-needed, sec-moderate
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: unspecified
: x86 Mac OS X
: P3 normal (vote)
: mozilla26
Assigned To: David Dahl :ddahl
:
Mentors:
Depends on:
Blocks: csp-w3c-1.0
  Show dependency treegraph
 
Reported: 2012-12-25 17:46 PST by Paul Theriault [:pauljt]
Modified: 2013-08-07 11:58 PDT (History)
17 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
PoC (287 bytes, text/php)
2012-12-25 17:46 PST, Paul Theriault [:pauljt]
no flags Details
Patch 1 (4.17 KB, patch)
2013-07-12 10:17 PDT, David Dahl :ddahl
no flags Details | Diff | Splinter Review
Patch 2 (5.54 KB, patch)
2013-07-12 15:08 PDT, David Dahl :ddahl
no flags Details | Diff | Splinter Review
Patch 3 (8.22 KB, patch)
2013-07-15 11:22 PDT, David Dahl :ddahl
mozbugs: review+
dkeeler: review+
Details | Diff | Splinter Review
Patch 4 (8.60 KB, patch)
2013-07-17 10:40 PDT, David Dahl :ddahl
brian: review+
mozbugs: review+
Details | Diff | Splinter Review
fix-crypto-GenerateCRMFRequest (13.56 KB, patch)
2013-07-18 12:41 PDT, David Dahl :ddahl
no flags Details | Diff | Splinter Review
Patch 5 (14.46 KB, patch)
2013-07-18 12:44 PDT, David Dahl :ddahl
no flags Details | Diff | Splinter Review
Patch 6 (13.23 KB, patch)
2013-07-18 13:06 PDT, David Dahl :ddahl
khuey: review-
Details | Diff | Splinter Review
Patch 7 (12.61 KB, patch)
2013-07-22 11:19 PDT, David Dahl :ddahl
no flags Details | Diff | Splinter Review
Patch 8 (17.41 KB, patch)
2013-07-24 13:05 PDT, David Dahl :ddahl
khuey: review+
Details | Diff | Splinter Review
fix-crypto-GenerateCRMFRequest (17.48 KB, patch)
2013-08-05 14:15 PDT, David Dahl :ddahl
bugzilla: review+
Details | Diff | Splinter Review

Description Paul Theriault [:pauljt] 2012-12-25 17:46:30 PST
Created attachment 695690 [details]
PoC

TLDR: crypto.generateCRMFRequest doesn't abide by a CSP default-src of 'self'.

I was looking at DOM xss execution sinks (http://code.google.com/p/domxsswiki/wiki/ExecutionSinks) and came across crypto.generateCRMFRequest.

I wondered if CSP blocked the script here correctly, turns out it doesn't.

Actually while trying to search for example for how to use this function, I came across this presentation [1], by Yosuke Hasegawa, which has a relevant example. So this is already public, hence why I am not hiding this.


[1] http://view.officeapps.live.com/op/view.aspx?src=http%3A%2F%2Futf-8.jp%2Fpublic%2F20120421%2Fmomiji.pptx
Comment 1 Masahiro YAMADA 2012-12-25 19:53:45 PST
Blocks Bug 493857 ?
Comment 2 Ian Melven :imelven 2012-12-27 10:55:20 PST
Thanks Paul. 

Probably a sec-low - dveditz, does that sound right ? Feel free to correct if not :)

Sid and I looked at the POC - it seems like that script needs to be in a script tag so the csp would need unsafe-inline for the script to execute. Is the POC complete ?

It seems to me like this would make a good mentored bug. We'd need to add a call to nsIContentSecurityPolicy::allowsEval before executing the script passed to crypto.generateCRMFRequest and would need to add tests to content/base/test/test_CSP_evalscript.html to make sure the script is blocked (and allowed if unsafe-eval is part of the CSP). 

Also, if the CSP 1.0 bugs (bug 746978 bug 783049 bug 763879) land before this, the 1.0 spec compliant tests those bugs add will need the test for this bug added as well.
Comment 3 Masatoshi Kimura [:emk] 2012-12-27 15:06:30 PST
(In reply to Ian Melven :imelven from comment #2)
> Also, if the CSP 1.0 bugs (bug 746978 bug 783049 bug 763879) land before
> this, the 1.0 spec compliant tests those bugs add will need the test for
> this bug added as well.

I think the spec itself should say something about browser extensions that can evaluate the JavaScript string. Currently, the spec only enumerate the known cases (eval, Function constructor, setTimeout, and setInterval).
Comment 4 Ian Melven :imelven 2012-12-27 15:08:44 PST
(In reply to Masatoshi Kimura [:emk] from comment #3)
> (In reply to Ian Melven :imelven from comment #2)
> > Also, if the CSP 1.0 bugs (bug 746978 bug 783049 bug 763879) land before
> > this, the 1.0 spec compliant tests those bugs add will need the test for
> > this bug added as well.
> 
> I think the spec itself should say something about browser extensions that
> can evaluate the JavaScript string. Currently, the spec only enumerate the
> known cases (eval, Function constructor, setTimeout, and setInterval).

Makes sense, I'll bring this up on the w3c webappsec list.
Comment 5 Paul Theriault [:pauljt] 2012-12-29 18:09:36 PST
(In reply to Ian Melven :imelven from comment #2)
> Thanks Paul. 
> 
> Probably a sec-low - dveditz, does that sound right ? Feel free to correct
> if not :)
> 
> Sid and I looked at the POC - it seems like that script needs to be in a
> script tag so the csp would need unsafe-inline for the script to execute. Is
> the POC complete ?

The PoC is both a valid HTML & JavaScript file. Note the /*<html><script src=?></script>*/ which loads itself as a script, with the bootstrap HTML code commented. Sorry, its pretty cryptic for something which is supposed to be explanatory (especially at this time of the year!). 

> 
> It seems to me like this would make a good mentored bug. We'd need to add a
> call to nsIContentSecurityPolicy::allowsEval before executing the script
> passed to crypto.generateCRMFRequest and would need to add tests to
> content/base/test/test_CSP_evalscript.html to make sure the script is
> blocked (and allowed if unsafe-eval is part of the CSP). 
> 
> Also, if the CSP 1.0 bugs (bug 746978 bug 783049 bug 763879) land before
> this, the 1.0 spec compliant tests those bugs add will need the test for
> this bug added as well.
Comment 6 Ian Melven :imelven 2012-12-29 19:47:26 PST
(In reply to Paul Theriault [:pauljt] from comment #5)
> (In reply to Ian Melven :imelven from comment #2)
> > Thanks Paul. 
> > 
> > Probably a sec-low - dveditz, does that sound right ? Feel free to correct
> > if not :)
> > 
> > Sid and I looked at the POC - it seems like that script needs to be in a
> > script tag so the csp would need unsafe-inline for the script to execute. Is
> > the POC complete ?
> 
> The PoC is both a valid HTML & JavaScript file. Note the /*<html><script
> src=?></script>*/ which loads itself as a script, with the bootstrap HTML
> code commented. Sorry, its pretty cryptic for something which is supposed to
> be explanatory (especially at this time of the year!). 

thanks for the explanation, Paul, i wasn't aware of src=? !
Comment 7 Ian Melven :imelven 2013-01-30 15:16:23 PST
Upgrading to a sec-moderate per discussion with dveditz
Comment 8 Johnny Stenback (:jst, jst@mozilla.com) 2013-07-11 14:19:14 PDT
ddahl, can you grab this one and figure out what we need to do here. We either need to stop exposing this method period, or we need to fix the implementation to honor CSP etc. There's also an awful lot of JS API calls in this code (some of which is buggy, incorrect format string usage for one at http://hg.mozilla.org/mozilla-central/file/8aca531ff163/security/manager/ssl/src/nsCrypto.cpp#l1866), maybe the new bindings would help?
Comment 9 David Dahl :ddahl 2013-07-12 10:17:51 PDT
Created attachment 774752 [details] [diff] [review]
Patch 1

Ian: would love some feedback here.

This patch seems to build (still compiling, haven't tested).
Comment 10 David Dahl :ddahl 2013-07-12 14:55:47 PDT
Comment on attachment 774752 [details] [diff] [review]
Patch 1

cancelling feedback, this patch does not work yet. close.
Comment 11 David Dahl :ddahl 2013-07-12 15:08:05 PDT
Created attachment 774943 [details] [diff] [review]
Patch 2

This patch works, I added a test. The test has some wonky error: failed | [SimpleTest.finish()] this test already called finish!
Comment 12 David Dahl :ddahl 2013-07-12 15:17:53 PDT
(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #8)
> ddahl, can you grab this one and figure out what we need to do here. We
> either need to stop exposing this method period, or we need to fix the
> implementation to honor CSP etc. 
According to the dev-tech-crypto list it may not be a good idea to remove the method, yet. This should make the short list of code to remove soon however.

Fixing this seems pretty straightforward based on imelven's comments above.

> There's also an awful lot of JS API calls
> in this code (some of which is buggy, incorrect format string usage for one
> at
> http://hg.mozilla.org/mozilla-central/file/8aca531ff163/security/manager/ssl/
> src/nsCrypto.cpp#l1866), maybe the new bindings would help?

Perhaps we should create (a) new bug(s) for these issues so we can get this landed quicker?
Comment 13 Ian Melven :imelven 2013-07-12 15:23:24 PDT
(In reply to David Dahl :ddahl from comment #12)
> (In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #8)
> > ddahl, can you grab this one and figure out what we need to do here. We
> > either need to stop exposing this method period, or we need to fix the
> > implementation to honor CSP etc. 
> According to the dev-tech-crypto list it may not be a good idea to remove
> the method, yet. This should make the short list of code to remove soon
> however.
> 
> Fixing this seems pretty straightforward based on imelven's comments above.
> 
> > There's also an awful lot of JS API calls
> > in this code (some of which is buggy, incorrect format string usage for one
> > at
> > http://hg.mozilla.org/mozilla-central/file/8aca531ff163/security/manager/ssl/
> > src/nsCrypto.cpp#l1866), maybe the new bindings would help?
> 
> Perhaps we should create (a) new bug(s) for these issues so we can get this
> landed quicker?

fwiw, I would suggest taking that approach. I'd file a bug for switching nsIDOMCrypto to the new bindings and cleaning up the JS API calls and another bug for removing this API. Please put the arguments made on dev-tech-crypto for not getting rid of this API now in the latter so that discussion can take place there.
Comment 14 David Dahl :ddahl 2013-07-12 15:30:51 PDT
(In reply to Ian Melven :imelven from comment #13)

> fwiw, I would suggest taking that approach. I'd file a bug for switching
> nsIDOMCrypto to the new bindings 

There is a bug for Web IDL: bug 883741
Comment 15 Ian Melven :imelven 2013-07-12 15:48:13 PDT
(In reply to David Dahl :ddahl from comment #11)
> Created attachment 774943 [details] [diff] [review]
> Patch 2
> 
> This patch works, I added a test. The test has some wonky error: failed |
> [SimpleTest.finish()] this test already called finish!

The main eval test file has counts that need to be changed since you added another test, see http://mxr.mozilla.org/mozilla-central/source/content/base/test/test_CSP_evalscript.html?force=1#43
Comment 16 Ian Melven :imelven 2013-07-12 15:51:03 PDT
Comment on attachment 774943 [details] [diff] [review]
Patch 2

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

If you wanted to be nice, you could clean up the existing whitespace in nsCrypto.cpp ;)

Overall, well on the way, I think.

::: content/base/test/file_CSP_evalscript_main.js
@@ +38,5 @@
> +    console.log(e);
> +    console.log("CRMFRequest failed.");
> +    onevalblocked(false, "eval(script) inside crypto.generateCRMFRequest",
> +                  "eval was blocked during crypto.generateCRMFRequest");
> +  }

this looks fine, you need to modify the test count in test_CSP_evalscript.html as commented previously

I'd remove the logging before committing after you have it working

You probably should add an 'allowed' test as well in content/base/test/file_CSP_evalscript_main_allowed.js

::: security/manager/ssl/src/nsCrypto.cpp
@@ +2088,5 @@
> +    NS_ERROR("CSP: Failed to get CSP from principal.");
> +    return false;
> +  }
> +
> +  csp.forget(aCSP);

Do you need this with the getter_Addrefs ? From looking over the nsCOMPtr docs just now, it looks like using the getter_Addrefs means the nsCOMPtr won't increment the reference count (which is OK since calling GetCSP does the addref for us).
Comment 17 Sid Stamm [:geekboy or :sstamm] 2013-07-12 16:06:37 PDT
Comment on attachment 774943 [details] [diff] [review]
Patch 2

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

I was doing a drive-by and midaired ian.  Piling a few comments on.

::: content/base/test/file_CSP_evalscript_main.js
@@ +38,5 @@
> +    console.log(e);
> +    console.log("CRMFRequest failed.");
> +    onevalblocked(false, "eval(script) inside crypto.generateCRMFRequest",
> +                  "eval was blocked during crypto.generateCRMFRequest");
> +  }

Yes, what Ian said here.  Uptick test count, add a call to onevalexecuted inside the try block.

::: security/manager/ssl/src/nsCrypto.cpp
@@ +1884,5 @@
> +
> +  if (csp && NS_FAILED(csp->GetAllowsEval(&reportEvalViolations, &evalAllowed))) {
> +    NS_ERROR("CSP: failed to get allowsEval");
> +    return NS_ERROR_FAILURE;
> +  }

You should add CSP violation reporting here too if reportEvalViolations is true.  For example:
http://mxr.mozilla.org/mozilla-central/source/caps/src/nsScriptSecurityManager.cpp#457
Comment 18 Ian Melven :imelven 2013-07-12 16:12:16 PDT
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #17)
> Comment on attachment 774943 [details] [diff] [review]
> Patch 2
> 
> Review of attachment 774943 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I was doing a drive-by and midaired ian.  Piling a few comments on.
> 
> ::: content/base/test/file_CSP_evalscript_main.js
> @@ +38,5 @@
> > +    console.log(e);
> > +    console.log("CRMFRequest failed.");
> > +    onevalblocked(false, "eval(script) inside crypto.generateCRMFRequest",
> > +                  "eval was blocked during crypto.generateCRMFRequest");
> > +  }
> 
> Yes, what Ian said here.  Uptick test count, add a call to onevalexecuted
> inside the try block.
> 
> ::: security/manager/ssl/src/nsCrypto.cpp
> @@ +1884,5 @@
> > +
> > +  if (csp && NS_FAILED(csp->GetAllowsEval(&reportEvalViolations, &evalAllowed))) {
> > +    NS_ERROR("CSP: failed to get allowsEval");
> > +    return NS_ERROR_FAILURE;
> > +  }
> 
> You should add CSP violation reporting here too if reportEvalViolations is
> true.  For example:
> http://mxr.mozilla.org/mozilla-central/source/caps/src/
> nsScriptSecurityManager.cpp#457

thanks, I noticed that too but then got lost in trying to work out the nsCOMPtr stuff and forgot to put it in my comments :)
Comment 19 David Dahl :ddahl 2013-07-15 11:19:56 PDT
(In reply to Ian Melven :imelven from comment #16)
> > +  csp.forget(aCSP);
> 
> Do you need this with the getter_Addrefs ? From looking over the nsCOMPtr
> docs just now, it looks like using the getter_Addrefs means the nsCOMPtr
> won't increment the reference count (which is OK since calling GetCSP does
> the addref for us).

Without the csp.forget(aCSP) call we never get a reference to CSP and the block fails. Uploading a new patch momentarily.

Also, evalScriptsTotal has to be  27 to avoid the pass/fail counting error. Will keep peeking at this...
Comment 20 David Dahl :ddahl 2013-07-15 11:21:29 PDT
(In reply to Ian Melven :imelven from comment #16)
> Comment on attachment 774943 [details] [diff] [review]
> Patch 2
> 
> Review of attachment 774943 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> If you wanted to be nice, you could clean up the existing whitespace in
> nsCrypto.cpp ;)

I have always been under the impression that whitespace clean-up like this should be done in another bug as well, no?
Comment 21 David Dahl :ddahl 2013-07-15 11:22:52 PDT
Created attachment 775813 [details] [diff] [review]
Patch 3
Comment 22 David Dahl :ddahl 2013-07-16 11:15:59 PDT
note:

On irc, imelven and khuey discussed the call to csp.forget(aCSP) and decided it is required. Without it, the csp object is always null.
Comment 23 David Keeler [:keeler] (use needinfo?) 2013-07-16 14:50:15 PDT
Comment on attachment 775813 [details] [diff] [review]
Patch 3

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

Looks good. Just a few suggestions.

::: security/manager/ssl/src/nsCrypto.cpp
@@ +1880,1 @@
>    

might as well remove this unnecessary whitespace while you're here

@@ +1881,5 @@
> +  if (!GetContentSecurityPolicy(cx, getter_AddRefs(csp))) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  if (csp && NS_FAILED(csp->GetAllowsEval(&reportEvalViolations, &evalAllowed))) {

How about

  if (!csp || NS_FAILED(csp->GetAllowsEval(&reportEvalViolations, &evalAllowed))) {

(unless GetContentSecurityPolicy will only return true if csp is not null, in which case just remove the 'csp && ' part)

@@ +1886,5 @@
> +    NS_ERROR("CSP: failed to get allowsEval");
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  if (reportEvalViolations) {

I assume reportEvalViolations can never be true if evalAllowed is true?

@@ +2083,5 @@
>  
>    return rv;
>  }
>  
> +bool

maybe return nsresult instead?

@@ +2099,5 @@
> +  nsCOMPtr<nsIPrincipal> subjectPrincipal = ssm->GetCxSubjectPrincipal(aCx);
> +  NS_ASSERTION(subjectPrincipal, "Failed to get subjectPrincipal");
> +
> +  nsCOMPtr<nsIContentSecurityPolicy> csp;
> +  nsresult rv = subjectPrincipal->GetCsp(getter_AddRefs(csp));

Can you not just do

  nsresult rv = subjectPrincipal->GetCsp(aCSP);

?
This would get rid of the csp.forget(aCSP) issue.
Stylistically, I don't know which is better (or, indeed, if my suggestion is entirely safe/correct). It just seems that less is more, in this case.

@@ +2104,5 @@
> +  if (NS_FAILED(rv)) {
> +    NS_ERROR("CSP: Failed to get CSP from principal.");
> +    return false;
> +  }
> +  

unnecessary whitespace
Comment 24 Sid Stamm [:geekboy or :sstamm] 2013-07-16 16:02:09 PDT
Comment on attachment 775813 [details] [diff] [review]
Patch 3

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

::: content/base/test/test_CSP_evalscript.html
@@ +20,5 @@
>  var path = "/tests/content/base/test/";
>  
>  var evalScriptsThatRan = 0;
>  var evalScriptsBlocked = 0;
> +var evalScriptsTotal = 27;

Why is this 27 and not 26?

::: security/manager/ssl/src/nsCrypto.cpp
@@ +1886,5 @@
> +    NS_ERROR("CSP: failed to get allowsEval");
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  if (reportEvalViolations) {

Actually, reportEvalViolations can be true if evalAllowed is true (report-only CSP).
Comment 25 Sid Stamm [:geekboy or :sstamm] 2013-07-16 16:02:49 PDT
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #24)
> > +  if (reportEvalViolations) {
> 
> Actually, reportEvalViolations can be true if evalAllowed is true
> (report-only CSP).

Sorry, this was unclear.  Keeler: this was in response to your question about reportEvalViolations.
Comment 26 David Dahl :ddahl 2013-07-16 16:10:53 PDT
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #24)
> Comment on attachment 775813 [details] [diff] [review]
> Patch 3
> 
> Review of attachment 775813 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/base/test/test_CSP_evalscript.html
> @@ +20,5 @@
> >  var path = "/tests/content/base/test/";
> >  
> >  var evalScriptsThatRan = 0;
> >  var evalScriptsBlocked = 0;
> > +var evalScriptsTotal = 27;
> 
> Why is this 27 and not 26?
> 
I asked imelven about this is in comment 19 - doesn't seem right
Comment 27 David Dahl :ddahl 2013-07-17 10:38:28 PDT
(In reply to David Dahl :ddahl from comment #26)
> (In reply to Sid Stamm [:geekboy or :sstamm] from comment #24)
> > Comment on attachment 775813 [details] [diff] [review]
> > Patch 3
> > 
> > Review of attachment 775813 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: content/base/test/test_CSP_evalscript.html
> > @@ +20,5 @@
> > >  var path = "/tests/content/base/test/";
> > >  
> > >  var evalScriptsThatRan = 0;
> > >  var evalScriptsBlocked = 0;
> > > +var evalScriptsTotal = 27;
> > 
> > Why is this 27 and not 26?
> > 
> I asked imelven about this is in comment 19 - doesn't seem right
When you count the tests that actually run, you do indeed reach 27 tests. I see 3 iframes, 2 of the iframes now load 9 tests each, the third now loads 8 tests.
Comment 28 David Dahl :ddahl 2013-07-17 10:40:40 PDT
Created attachment 777208 [details] [diff] [review]
Patch 4

Review comments addressed. Need a peer or sr review. Will push to try.
Comment 29 David Dahl :ddahl 2013-07-17 15:29:48 PDT
On tryserver, seeing bug 762578 with debug builds only: https://tbpl.mozilla.org/?tree=Try&rev=91ede996d344

 12:25:16 INFO - 3527 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_CSP_evalscript.html | Assertion count 2 is greater than expected range 0-0 assertions.
Comment 30 Sid Stamm [:geekboy or :sstamm] 2013-07-17 15:39:24 PDT
(In reply to David Dahl :ddahl from comment #27)
> > > Why is this 27 and not 26?
> > > 
> > I asked imelven about this is in comment 19 - doesn't seem right
> When you count the tests that actually run, you do indeed reach 27 tests. I
> see 3 iframes, 2 of the iframes now load 9 tests each, the third now loads 8
> tests.

Drat.  That's probably double-calls to shouldLoad (bug 612921).
Comment 31 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2013-07-17 16:45:15 PDT
Comment on attachment 777208 [details] [diff] [review]
Patch 4

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

I only reviewed the changes to nsCrypto.cpp and nsCrypto.h. The CSP people should review the tests.

::: security/manager/ssl/src/nsCrypto.cpp
@@ +1875,5 @@
>    }
> +
> +  nsCOMPtr<nsIContentSecurityPolicy> csp;
> +  bool evalAllowed = true;
> +  bool reportEvalViolations = false;

Nit: move these too boolean variables' declarations to just above the place they are first used.

@@ +1878,5 @@
> +  bool evalAllowed = true;
> +  bool reportEvalViolations = false;
> +
> +  nrv = GetContentSecurityPolicy(cx, getter_AddRefs(csp));
> +  if (NS_FAILED(nrv)) {

NS_ENSURE_SUCCESS(nrv, nrv);

@@ +1889,5 @@
> +  }
> +
> +  if (reportEvalViolations) {
> +    nsAutoString fileName;
> +    unsigned lineNum = 0;

nit: do not initialize this here, since it is initialized by JS_DescribeScriptedCaller.

@@ +1890,5 @@
> +
> +  if (reportEvalViolations) {
> +    nsAutoString fileName;
> +    unsigned lineNum = 0;
> +    NS_NAMED_LITERAL_STRING(scriptSample, "window.crypto.generateCRMFRequest: call to eval() or related function blocked by CSP");

Are these messages supposed to be localized?

@@ +2108,5 @@
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  return rv;
> +}

1. This should not be a member function because it doesn't use any members of the class.

2. This is almost character-for-character a duplicate of WorkerPrivate::GetContentSecurityPolicy. Please move the existing WorkerPrivate::GetContentSecurityPolicy code to nsContentPolicyUtils.h so that it can be used by all modules. khuey already agreed to review this movement.

::: security/manager/ssl/src/nsCrypto.h
@@ +10,5 @@
>  #include "Crypto.h"
>  #include "nsCOMPtr.h"
>  #include "nsIDOMCRMFObject.h"
>  #include "nsIDOMCryptoLegacy.h"
> +#include "nsIContentSecurityPolicy.h"

undo this change after you move the code to nsContentUtils.
Comment 32 Sid Stamm [:geekboy or :sstamm] 2013-07-18 10:16:34 PDT
Comment on attachment 775813 [details] [diff] [review]
Patch 3

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

r=me on the CSP tests with these two things fixed.

::: content/base/test/file_CSP_evalscript_main.js
@@ +31,5 @@
>  // out.
>  addEventListener('load', function() {
> +  try {
> +    var script = 'console.log("dynamic script eval\'d in crypto.generateCRMFRequest should be disallowed")';
> +    crypto.generateCRMFRequest('CN=0', 0, 0, null, script, 384, null, 'rsa-dual-use');

You should trigger onevalexecuted here too (as the fail case).

::: content/base/test/file_CSP_evalscript_main_allowed.js
@@ +33,5 @@
> +      var script =
> +        'console.log("dynamic script passed to crypto.generateCRMFRequest should execute")';
> +      crypto.generateCRMFRequest('CN=0', 0, 0, null, script, 384, null, 'rsa-dual-use');
> +      onevalexecuted(true, "eval(script) inside crypto.generateCRMFRequest",
> +                   "eval executed during crypto.generateCRMFRequest");

nit: indentation is a little off here.
Comment 33 Sid Stamm [:geekboy or :sstamm] 2013-07-18 10:18:33 PDT
Comment on attachment 777208 [details] [diff] [review]
Patch 4

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

Patch 3 should be obsolete, but my comments carry over to this patch 4 as well.  Please fix indentation (to be consistent) and add the negative case mentioned in my review.  Then r+
Comment 34 David Dahl :ddahl 2013-07-18 11:51:43 PDT
(In reply to Brian Smith (:briansmith), was bsmith@mozilla.com (:bsmith) from comment #31)
> Comment on attachment 777208 [details] [diff] [review]
> @@ +1890,5 @@
> > +
> > +  if (reportEvalViolations) {
> > +    nsAutoString fileName;
> > +    unsigned lineNum = 0;
> > +    NS_NAMED_LITERAL_STRING(scriptSample, "window.crypto.generateCRMFRequest: call to eval() or related function blocked by CSP");
> 
> Are these messages supposed to be localized?
> 
I don't think so. I'll have to look around for similar errors to see.

> @@ +2108,5 @@
> > +    return NS_ERROR_FAILURE;
> > +  }
> > +
> > +  return rv;
> > +}
> 
> 1. This should not be a member function because it doesn't use any members
> of the class.
> 
> 2. This is almost character-for-character a duplicate of
> WorkerPrivate::GetContentSecurityPolicy. Please move the existing
> WorkerPrivate::GetContentSecurityPolicy code to nsContentPolicyUtils.h so
> that it can be used by all modules. khuey already agreed to review this
> movement.

Ok!

> 
> ::: security/manager/ssl/src/nsCrypto.h
> @@ +10,5 @@
> >  #include "Crypto.h"
> >  #include "nsCOMPtr.h"
> >  #include "nsIDOMCRMFObject.h"
> >  #include "nsIDOMCryptoLegacy.h"
> > +#include "nsIContentSecurityPolicy.h"
> 
> undo this change after you move the code to nsContentUtils.
We still call csp->LogViolationDetails(nsIContentSecurityPolicy::VIOLATION_TYPE_EVAL... in nsCrypto, won't we need this header still?

Thanks for the fast review.
Comment 35 David Dahl :ddahl 2013-07-18 12:41:54 PDT
Created attachment 778007 [details] [diff] [review]
fix-crypto-GenerateCRMFRequest

Moved nsWorkerPrivate::GetContentSecurityPolicy to nsContentUtils::GetContentSecurityPolicy

All CSP eval tests pass. Which test suite should I run for nsWorkerPrivate's use of GetContentSecurityPolicy?
Comment 36 David Dahl :ddahl 2013-07-18 12:44:51 PDT
Created attachment 778009 [details] [diff] [review]
Patch 5

Whoops, forgot to remove the method declaration in WorkerPrivate.h
Comment 37 Sid Stamm [:geekboy or :sstamm] 2013-07-18 12:51:05 PDT
(In reply to David Dahl :ddahl from comment #35)
> All CSP eval tests pass. Which test suite should I run for nsWorkerPrivate's
> use of GetContentSecurityPolicy?

Probably dom/workers/test/test_csp.html
Comment 38 David Dahl :ddahl 2013-07-18 13:01:37 PDT
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #37)
> (In reply to David Dahl :ddahl from comment #35)
> > All CSP eval tests pass. Which test suite should I run for nsWorkerPrivate's
> > use of GetContentSecurityPolicy?
> 
> Probably dom/workers/test/test_csp.html

Cool, all pass!
Comment 39 David Dahl :ddahl 2013-07-18 13:06:06 PDT
Created attachment 778016 [details] [diff] [review]
Patch 6

Ok, this patch has all of the detritus removed. Moved WorkerPrivate::GetContentSecurityPolicy to nsContentUtils. Tests in dom/worker/ pass.
Comment 40 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2013-07-21 19:33:40 PDT
Comment on attachment 778016 [details] [diff] [review]
Patch 6

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

r- because you broke this function if no CSP is present, which I don't think is the intent.

::: security/manager/ssl/src/nsCrypto.cpp
@@ +1874,5 @@
>      return NS_ERROR_FAILURE;
>    }
> +
> +  nsCOMPtr<nsIContentSecurityPolicy> csp;
> +  

extra whitespace.  Gecko also usually skips the newline between the pointer declaration and the function that gets it.

@@ +1879,5 @@
> +  if (!nsContentUtils::GetContentSecurityPolicy(cx, getter_AddRefs(csp))) {
> +    NS_ERROR("Error: failed to get CSP");
> +    return NS_ERROR_FAILURE;
> +  }
> +  

more whitespace here too

@@ +1882,5 @@
> +  }
> +  
> +  bool evalAllowed = true;
> +  bool reportEvalViolations = false;
> +  if (!csp || NS_FAILED(csp->GetAllowsEval(&reportEvalViolations, &evalAllowed))) {

Surely you mean if (csp && NS_FAILED(...))?  You should add a test proving that GenerateCRMFRequest works in the absence of a CSP.

@@ +1883,5 @@
> +  
> +  bool evalAllowed = true;
> +  bool reportEvalViolations = false;
> +  if (!csp || NS_FAILED(csp->GetAllowsEval(&reportEvalViolations, &evalAllowed))) {
> +    NS_ERROR("CSP: failed to get allowsEval");

NS_ERROR is effectively an assertion.  You want NS_WARNING (or nothing) here.  Any XPCOM call is allowed to fail.

@@ +1893,5 @@
> +    unsigned lineNum;
> +    NS_NAMED_LITERAL_STRING(scriptSample, "window.crypto.generateCRMFRequest: call to eval() or related function blocked by CSP");
> +
> +    JSScript *script;
> +    if (JS_DescribeScriptedCaller(cx, &script, &lineNum)) {

Use nsJSUtils::GetCallingLocation here.

@@ +1905,5 @@
> +                             lineNum);
> +  }
> +
> +  if (!evalAllowed) {
> +    NS_ERROR("eval() not allowed by Content Security Policy");

Same here.  This NS_ERROR can be directly triggered by web code ...

::: security/manager/ssl/src/nsCrypto.h
@@ +10,5 @@
>  #include "Crypto.h"
>  #include "nsCOMPtr.h"
>  #include "nsIDOMCRMFObject.h"
>  #include "nsIDOMCryptoLegacy.h"
> +#include "nsIContentSecurityPolicy.h"

you shouldn't need this include here *and* in nsCrypto.cpp ...
Comment 41 David Dahl :ddahl 2013-07-22 11:17:58 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #40)
> Comment on attachment 778016 [details] [diff] [review]
> Patch 6
>
> Surely you mean if (csp && NS_FAILED(...))?  You should add a test proving
> that GenerateCRMFRequest works in the absence of a CSP.
Ah yes, that is what I meant:) There was a test added for this, which was reviewed already.

Thanks for the review, new patch uploading...
Comment 42 David Dahl :ddahl 2013-07-22 11:19:05 PDT
Created attachment 779296 [details] [diff] [review]
Patch 7

Comments addressed, all relavant tests pass
Comment 43 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2013-07-23 09:57:40 PDT
(In reply to David Dahl :ddahl from comment #41)
> (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #40)
> > Comment on attachment 778016 [details] [diff] [review]
> > Patch 6
> >
> > Surely you mean if (csp && NS_FAILED(...))?  You should add a test proving
> > that GenerateCRMFRequest works in the absence of a CSP.
> Ah yes, that is what I meant:) There was a test added for this, which was
> reviewed already.
> 
> Thanks for the review, new patch uploading...

So you're saying there were tests that patch 6 failed?
Comment 44 David Dahl :ddahl 2013-07-23 13:19:02 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #43)

> So you're saying there were tests that patch 6 failed?

No, the test still ran:
 0:04.94 TEST-PASS | unknown test url | EVAL SCRIPT RAN: eval(script) inside crypto.generateCRMFRequest(eval executed during crypto.generateCRMFRequest)

Wouldn't this code indicate that we always get a csp object and if not something is really wrong:
  nsCOMPtr<nsIContentSecurityPolicy> csp;
  if (!nsContentUtils::GetContentSecurityPolicy(cx, getter_AddRefs(csp))) {
    NS_ERROR("Error: failed to get CSP");
    return NS_ERROR_FAILURE;
  }

In which case do we not even need a check for csp at all in:
  if (csp && NS_FAILED(csp->GetAllowsEval(&reportEvalViolations, &evalAllowed)))
Comment 45 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2013-07-23 13:36:29 PDT
(In reply to David Dahl :ddahl from comment #44)
> (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #43)
> 
> > So you're saying there were tests that patch 6 failed?
> 
> No, the test still ran:
>  0:04.94 TEST-PASS | unknown test url | EVAL SCRIPT RAN: eval(script) inside
> crypto.generateCRMFRequest(eval executed during crypto.generateCRMFRequest)

There should be a test that fails in the presence of the error from comment 41.

> Wouldn't this code indicate that we always get a csp object and if not
> something is really wrong:
>   nsCOMPtr<nsIContentSecurityPolicy> csp;
>   if (!nsContentUtils::GetContentSecurityPolicy(cx, getter_AddRefs(csp))) {
>     NS_ERROR("Error: failed to get CSP");
>     return NS_ERROR_FAILURE;
>   }

No.  Putting null in the outparam is not failing.  e.g. http://hg.mozilla.org/mozilla-central/annotate/b717a7945dfb/caps/src/nsSystemPrincipal.cpp#l128
Comment 46 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2013-07-23 13:36:51 PDT
Comment on attachment 779296 [details] [diff] [review]
Patch 7

Clearing review until the test I want is added.
Comment 47 David Dahl :ddahl 2013-07-23 13:56:24 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #46)
> Comment on attachment 779296 [details] [diff] [review]
> Patch 7
> 
> Clearing review until the test I want is added.

I am unsure how to test that the nsIContentSecurityPolicy object was created and valid from a mochitest or content JS. Can you please explain in some detail how we can force that to happen in a test? 

I would think in this case, generateCRMFRequest() would throw NS_ERROR_FAILURE, but in content I would not know what triggered it as there are a couple of spots where that can happen.
Comment 48 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2013-07-23 14:04:20 PDT
I want you to test that generateCRMFRequest works (and allows eval) if there is no CSP specified.  There was a mistake in patch 6 that turned "if we have a CSP and it says deny, deny" into "if we don't have a CSP or we have a CSP that says deny, deny".  You should add a test that has no CSP so fails in the (erroneous) latter case but passes in the (what-we're-going-to-land) former case.
Comment 49 David Dahl :ddahl 2013-07-24 13:03:31 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #48)
> I want you to test that generateCRMFRequest works (and allows eval) if there
> is no CSP specified.

Ok,  I get it. The ^headers^ should not specify anything related to CSP. I forgot that the "eval allow" tests specify a CSP. This is one hell of a corner case;)
Comment 50 David Dahl :ddahl 2013-07-24 13:05:58 PDT
Created attachment 780561 [details] [diff] [review]
Patch 8
Comment 51 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2013-07-29 12:09:41 PDT
Comment on attachment 780561 [details] [diff] [review]
Patch 8

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

I don't think "no CSP at all" is a corner case, given that that's most of the web ;-)
Comment 52 David Dahl :ddahl 2013-08-05 14:15:12 PDT
Created attachment 785946 [details] [diff] [review]
fix-crypto-GenerateCRMFRequest

Patch to land on inbound, unbitrotten, tests pass
Comment 53 David Dahl :ddahl 2013-08-05 14:18:30 PDT
Pushing to inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4dd0dc4e354
Comment 55 David Dahl :ddahl 2013-08-05 16:32:44 PDT
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #54)
> Backed out for Android mochitest-1 failures.
> https://hg.mozilla.org/integration/mozilla-inbound/rev/82d3b49ce0df
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=26181254&tree=Mozilla-
> Inbound

Gah! these tests (for getCRMFRequest) should never run on Android as there is no getCRMFRequest method on Android! I will have to break out the tests for getCRMFRequest into their own files.
Comment 56 David Dahl :ddahl 2013-08-06 19:47:48 PDT
Broke out tests, pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/8dc0904666fd
Comment 57 David Dahl :ddahl 2013-08-06 20:18:36 PDT
also had to disable the new test in b2g: https://hg.mozilla.org/integration/mozilla-inbound/rev/616e6e640f43

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