524664 - nsCrypto::GenerateCRMFRequest should use nsIScriptContext::EvaluateString

Status: RESOLVED INVALID

(Core :: Security: PSM, defect)

Description

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Possible fix

See bug 520386, comment 15.

Kai, what's the best way to test this patch? I see /security/nss/cmd/crmf-cgi/crmfcgi.html -- but it isn't entirely clear how to use it.

Comment 1

[kashyap chamarthy]

Hi, I can help test on dogtag certificate system (http://pki.fedoraproject.org)
If someone can provide a test build of firefox.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Builds can be found at http://build.mozilla.org/tryserver-builds/[e-mail]-crmfrequest-nsjscontext .

Comment 3

[Kai Engert [:KaiE:]]

kashyap chamarthy, the test builds have a limited lifetime, I guess you didn't test them.

I apologize that testing hasn't happened yet.

Blake, maybe you can use http://kuix.de/misc/test3/crmf-rsa.php for testing.
If you click the button, you should get content inserted into the page.

Paste that content to a file named bla.
Then, Linux, use the following command to pretty print the blob:

cat bla | sed 's# #\n#g' | openssl base64 -d | openssl asn1parse -inform DER

On my system, this produced a result of
    0:d=0  hl=4 l= 415 cons: SEQUENCE          
    4:d=1  hl=4 l= 411 cons: SEQUENCE          
    8:d=2  hl=4 l= 257 cons: SEQUENCE          
   12:d=3  hl=2 l=   4 prim: INTEGER           :3BA0B5EB
   18:d=3  hl=3 l= 208 cons: SEQUENCE          
   21:d=4  hl=2 l=   1 prim: cont [ 0 ]        
   24:d=4  hl=2 l=  23 cons: cont [ 5 ]        
   26:d=5  hl=2 l=  21 cons: SEQUENCE          
   28:d=6  hl=2 l=  19 cons: SET               
   30:d=7  hl=2 l=  17 cons: SEQUENCE          
   32:d=8  hl=2 l=   3 prim: OBJECT            :commonName
   37:d=8  hl=2 l=  10 prim: PRINTABLESTRING   :Kai Engert
   49:d=4  hl=3 l= 159 cons: cont [ 6 ]        
   52:d=5  hl=2 l=  13 cons: SEQUENCE          
   54:d=6  hl=2 l=   9 prim: OBJECT            :rsaEncryption
   65:d=6  hl=2 l=   0 prim: NULL              
   67:d=5  hl=3 l= 141 prim: BIT STRING        
  211:d=4  hl=2 l=  16 cons: cont [ 9 ]        
  213:d=5  hl=2 l=  14 cons: SEQUENCE          
  215:d=6  hl=2 l=   3 prim: OBJECT            :X509v3 Key Usage
  220:d=6  hl=2 l=   1 prim: BOOLEAN           :255
  223:d=6  hl=2 l=   4 prim: OCTET STRING      [HEX DUMP]:030205E0
  229:d=3  hl=2 l=  38 cons: SEQUENCE          
  231:d=4  hl=2 l=  17 cons: SEQUENCE          
  233:d=5  hl=2 l=   9 prim: OBJECT            :id-regCtrl-regToken
  244:d=5  hl=2 l=   4 prim: UTF8STRING        :bla1
  250:d=4  hl=2 l=  17 cons: SEQUENCE          
  252:d=5  hl=2 l=   9 prim: OBJECT            :id-regCtrl-authenticator
  263:d=5  hl=2 l=   4 prim: UTF8STRING        :bla2
  269:d=2  hl=3 l= 147 cons: cont [ 1 ]        
  272:d=3  hl=2 l=  13 cons: SEQUENCE          
  274:d=4  hl=2 l=   9 prim: OBJECT            :sha1WithRSAEncryption
  285:d=4  hl=2 l=   0 prim: NULL              
  287:d=3  hl=3 l= 129 prim: BIT STRING        


If, after your change, you still get content that produce similar output, your patch is fine.

Comment 4

[kashyap chamarthy]

Kai, Apologies. 

I couldn't get to this at the time the builds arrived, as I was dragged into a more higher priority task and haven't had spare cycles. And, after that, this issue slipped my mind.

If it's possible to provide another test build, I'd be happy to test it out on Dogtag certificate system.

PS: FWIW, I tried out the instructions from comment #3 and I got a similar output on my firefox version 3.5.9 (on Fedora12)

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Brian, I think I mentioned this bug to you a while back. It would be nice to get it in (and tested, which was the reason I never managed to check this in) to help reduce the number of places that we manually execute JS code from Gecko. Would you be willing to drive it?

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

This bug isn't blocking any other bug and it isn't clear if/why it is a high priority on its own. The summary proposes a solution without stating the problem, and the problem is likely non-obvious to all PSM people. What is the actual problem?

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

This used to be higher priority. Now, I think it's more "nice to have." At the time that I filed this, it was possible to cause us to navigate while generateCRMFRequest was running, which could have caused us to crash (in particular, we would call JS_ClearScope when navigating a window, causing references to global variables to become reads and writes past the end of an array). At the time, bug 520386 fixed the problem by adding state to nsIScriptContext::EvaluateString that caused us to not JS_ClearScope until we were truly ready for the JS_ClearScope (after the script had stopped running). Therefore, having code that didn't go through the script context could have still crashed.

We've since removed the JS_ClearScope, so the only real gain from this patch is simplifying the code and reducing the number of places in Gecko where we touch the JS engine directly (which, IMO, is always a good thing).

Comment 8

[:Cykesiopka]

Bug 1030963 removed window.crypto functions/properties, which includes nsCrypto::GenerateCRMFRequest():
https://hg.mozilla.org/mozilla-central/diff/68499003df5e/security/manager/ssl/src/nsCrypto.cpp

So this is now "invalid", or whatever status makes most sense.

Comment 9

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

When I bug is made invalid because another bug made it irrelevant, it is good to add a dependency. That way, if that other bug fix gets undone, we know to reopen the dependent bugs.