Closed Bug 824652 Opened 12 years ago Closed 11 years ago

crypto.generateCRMFRequest bypasses CSP (allows script execution from a string, without unsafe-eval)

Categories

(Core :: DOM: Core & HTML, defect, P3)

Product:
Core
Component:
DOM: Core & HTML
Platform:
x86
macOS
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla26

People

(Reporter: pauljt, Assigned: ddahl)

References

(Blocks 1 open bug)

Details

(Keywords: sec-moderate, Whiteboard: [mentor=imelven lang=c++])

Attachments

(2 files, 9 obsolete files)

PoC
287 bytes, text/php
Details
fix-crypto-GenerateCRMFRequest
17.48 KB, patch
ddahl
: review+
Details | Diff | Splinter Review
Attached file 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
Blocks Bug 493857 ?
Keywords: dev-doc-needed
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.
Keywords: sec-low
Whiteboard: [mentor=imelven lang=c++]
(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).
(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.
(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.
(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=? !
Upgrading to a sec-moderate per discussion with dveditz
Keywords: sec-low → sec-moderate
Priority: -- → P3
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?
Assignee: nobody → ddahl
Attached patch Patch 1 (obsolete) — — Splinter Review 
Ian: would love some feedback here.

This patch seems to build (still compiling, haven't tested).
Attachment #774752 - Flags: feedback?(imelven)
Comment on attachment 774752 [details] [diff] [review]
Patch 1

cancelling feedback, this patch does not work yet. close.
Attachment #774752 - Flags: feedback?(imelven)
Attached patch Patch 2 (obsolete) — — Splinter Review 
This patch works, I added a test. The test has some wonky error: failed | [SimpleTest.finish()] this test already called finish!
Attachment #774752 - Attachment is obsolete: true
Attachment #774943 - Flags: feedback?(imelven)
(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?
(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.
Status: NEW → ASSIGNED
(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
(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 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).
Attachment #774943 - Flags: feedback?(imelven) → feedback+
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
Attachment #774943 - Flags: feedback+
(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 :)
(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...
(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?
Attached patch Patch 3 (obsolete) — — Splinter Review 

        Attachment #774943 -
        Attachment is obsolete: true

        Attachment #775813 -
        Flags: review?(imelven)

    
  

        Attachment #775813 -
        Flags: review?(imelven) → review?(grobinson)

    
    

    
  
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.

    
  

        Attachment #775813 -
        Flags: review?(brian)

    
  

        Attachment #775813 -
        Flags: review?(brian) → review?(dkeeler)

    
    

    
  
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

        Attachment #775813 -
        Flags: review?(dkeeler) → review+

    
    

    
  
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).

    
    

    
  
(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.

    
    

    
  
(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

    
    

    
  
(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.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch 4 (obsolete)
        — 
          — Splinter Review
      

    


  
Review comments addressed. Need a peer or sr review. Will push to try.

        Attachment #777208 -
        Flags: review?(brian)

    
    

    
  
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.

    
    

    
  
(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 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.

        Attachment #777208 -
        Flags: review?(brian) → review+

    
    

    
  
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.

        Attachment #775813 -
        Flags: review?(grobinson) → review+

    
    

    
  
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+

        Attachment #777208 -
        Flags: review+

    
    

    
  
(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.

    
  

        Attachment #775813 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        fix-crypto-GenerateCRMFRequest (obsolete)
        — 
          — Splinter Review
      

    


  
Moved nsWorkerPrivate::GetContentSecurityPolicy to nsContentUtils::GetContentSecurityPolicy

All CSP eval tests pass. Which test suite should I run for nsWorkerPrivate's use of GetContentSecurityPolicy?

        Attachment #777208 -
        Attachment is obsolete: true

        Attachment #778007 -
        Flags: review?(khuey)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch 5 (obsolete)
        — 
          — Splinter Review
      

    


  
Whoops, forgot to remove the method declaration in WorkerPrivate.h

        Attachment #778007 -
        Attachment is obsolete: true

        Attachment #778007 -
        Flags: review?(khuey)

        Attachment #778009 -
        Flags: review?(khuey)

    
  

        Attachment #778009 -
        Attachment is obsolete: true

        Attachment #778009 -
        Flags: review?(khuey)

    
    

    
  
(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

    
    

    
  
(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!

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch 6 (obsolete)
        — 
          — Splinter Review
      

    


  
Ok, this patch has all of the detritus removed. Moved WorkerPrivate::GetContentSecurityPolicy to nsContentUtils. Tests in dom/worker/ pass.

        Attachment #778016 -
        Flags: review?(khuey)

    
    

    
  
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 ...

        Attachment #778016 -
        Flags: review?(khuey) → review-

    
    

    
  
(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...

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch 7 (obsolete)
        — 
          — Splinter Review
      

    


  
Comments addressed, all relavant tests pass

        Attachment #778016 -
        Attachment is obsolete: true

        Attachment #779296 -
        Flags: review?(khuey)

    
    

    
  
(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?

    
    

    
  
(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)))

    
    

    
  
(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 on attachment 779296 [details] [diff] [review]
Patch 7

Clearing review until the test I want is added.

        Attachment #779296 -
        Flags: review?(khuey)

    
    

    
  
(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.

    
    

    
  
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.

    
    

    
  
(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;)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch 8 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #779296 -
        Attachment is obsolete: true

        Attachment #780561 -
        Flags: review?(khuey)

    
    

    
  
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 ;-)

        Attachment #780561 -
        Flags: review?(khuey) → review+

    
    

    
  


  
Patch to land on inbound, unbitrotten, tests pass

        Attachment #780561 -
        Attachment is obsolete: true

        Attachment #785946 -
        Flags: review+

    
    

    
  
(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.

    
    

    
  
Broke out tests, pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/8dc0904666fd

    
    

    
  
also had to disable the new test in b2g: https://hg.mozilla.org/integration/mozilla-inbound/rev/616e6e640f43

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/8dc0904666fd
https://hg.mozilla.org/mozilla-central/rev/616e6e640f43
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla26

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: