awnuk Reporter
	Description • 23 years ago

I'm using  2001-10-01-05-0.9.4. 
crypto.generateCRMFRequest used according to 
http://docs.iplanet.com/docs/manuals/psm/12/cmcjavascriptapi.html
to generate DSA request is crashing 6.2.

	John Unruh
	Comment 1 • 23 years ago

Worksforme. There are some DSA tests at http://junruh.mcom.com/tests.html on the
right side under "CMS 4.2 Testing."

	awnuk Reporter
	Comment 2 • 23 years ago

This only happens when you pass PQG via "keyParamsN".

	Stephane Saux
	Comment 3 • 23 years ago

->javi
cc lakshmi.
crash keyword.

	John Unruh
	Comment 4 • 23 years ago

awnuk, can you zip up the html file and email it to me so that I can add it to
my CMS server for testing?

	John Unruh
	Comment 5 • 23 years ago

Changing the test URL to http://pki.mcom.com/tests.html

	Kai Engert (:KaiE:) Assignee
	Comment 6 • 22 years ago

Is this still happening? How can I reproduce the crash?

	Brant Gurganus
	Comment 7 • 22 years ago

By the definitions on <http://bugzilla.mozilla.org/bug_status.html#severity> and
<http://bugzilla.mozilla.org/enter_bug.cgi?format=guided>, crashing and dataloss
bugs are of critical or possibly higher severity.  Only changing open bugs to
minimize unnecessary spam.  Keywords to trigger this would be crash, topcrash,
topcrash+, zt4newcrash, dataloss.

	Nelson Bolyard (seldom reads bugmail)
	Comment 8 • 21 years ago

Mass reassign Javi's old PSM bugs to nobody

	Nelson Bolyard (seldom reads bugmail)
	Comment 9 • 19 years ago

Here's a 4 year old P1 critical bug.  :(

The URL cited above is now long gone.  It was
http://docs.iplanet.com/docs/manuals/psm/12/cmcjavascriptapi.html

The only docs I could find related to crypto.generateCRMFRequest
on docs.sun.com were
http://docs.sun.com/source/816-5548-10/kra.htm
http://docs.sun.com/source/816-5541-10/kra.htm
http://docs.sun.com/source/816-5531-10/kycrt_ee.htm

One of the arguments to crypto.generateCRMFRequest is a string intended
to be used to pass the PQG parameters needed when generating a DSA cert.
If the string is absent (null), crypto.generateCRMFRequest makes up a 
new PQGparams on the spot, which may be ok for playing, but isn't very
useful with a real DSA-based CA.  

Sadly, it appears that crypto.generateCRMFRequest ONLY works for DSA if
the string is null.  If the string is non-null, there is no code to handle
that case.  See function nsConvertToActualKeyGenParams near 
http://lxr.mozilla.org/security/source/security/manager/ssl/src/nsCrypto.cpp#486

I suspect that is the root cause of this ~4 year old bug

	Nelson Bolyard (seldom reads bugmail)
	Comment 10 • 19 years ago

Here is some additional documentation on crypto.generateCRMFRequest, based 
partly on a posting by Steve Parkinson

      crmfObject = crypto.generateCRMFRequest(
             subject.value,          // 'CN=Fred'
             "regToken",             // string defined by CRMF, may be null
             "authenticator",        // string defined by CRMF, may be null
// if present, the above two strings are placed into the CRMF request as
// UTF8 encoded strings.
             null,                   // string containing RA's cert for key
                                     // escrow (base 64 encoded), or null
// note, if the cert string is not NULL, the user will be shown a dialog 
// informing him that key escrow will be done.  
             "setCRMFRequest();",    // required callback invocation
// the next 3 arguments are taken as a triplet.  
// there may be as many triplets as there are desired key pairs to be 
// generated. The 3 argument values in each triplet are: 
//  - key size in bits, 
//  - base 64 encoded string containing PQG or DH params, or null 
//    for RSA use null
//  - a string identifying the type of key pair to gen.  See below
             1024, null, keyGenAlg); // key parameters

    keyGenAlg can be 
       "rsa-sign"     (signature key) or 
       "rsa-ex"       (encryption) or 
       "rsa-dual-use" (both)

Other defined values include

       "rsa-sign-nonrepudiation"
       "rsa-nonrepudiation"
       "dsa-sign-nonrepudiation"
       "dsa-sign"
       "dsa-nonrepudiation"
       "dh-ex"

    the cert request (in CRMF format) is retrieved by accessing:
    crmfObject.request; 

I suspect that much of this is untested.  NSS contains a test page (html)
and test cgi script for it, but it tests only a tiny subset of the 
combinations of key types and sizes provided.

	Steve Parkinson
	Comment 11 • 19 years ago

I found an old PSM distribution with the missing doc. Hope it's useful.

	Nelson Bolyard (seldom reads bugmail)
	Comment 12 • 19 years ago

That's a great doc, Wan-Teh.   Thanks.

The CRMF code in PSM and NSS is an orphan.  It is demonstrably broken and
incorrect in several ways.  (See the NSS CRMF bugs, some of which are assigned 
to me since I worked at AOL.)  It needs someone who wants it to own it. 
Might that be RedHat's CMS team?

	Kai Engert (:KaiE:) Assignee
	Comment 13 • 14 years ago

I'm able to reproduce the crash.

PSM function nsConvertToActualKeyGenParams says:

  case CKM_DSA_KEY_PAIR_GEN:
  {
    // For DSA, we don't support passing in key generation arguments from
    // the JS code just yet.


It contains a check that tries to simply abort when giving parameters

    if (params)
      return nsnull;


but apparently this doesn't work. The code will continue, it will call into PK11_GenerateKeyPair using NULL parameters, which will forward the call elsewhere, and finally crash, when the code attempts to dereference a null pointer.

#0  0x01b52200 in PK11_GenerateKeyPairWithOpFlags (slot=0xa7819c00, type=16, param=0x0, pubKey=0xa473ed50, attrFlags=69, opFlags=0, opFlagsMask=0, wincx=0xa499e140) at pk11akey.c:1263
#1  0x01b52bab in PK11_GenerateKeyPairWithFlags (slot=0xa7819c00, type=16, param=0x0, pubKey=0xa473ed50, attrFlags=69, wincx=0xa499e140) at pk11akey.c:1507
#2  0x01b52c1b in PK11_GenerateKeyPair (slot=0xa7819c00, type=16, param=0x0, pubKey=0xa473ed50, token=1, sensitive=1, wincx=0xa499e140) at pk11akey.c:1531


1260        case CKM_DSA_KEY_PAIR_GEN:
1261            dsaParams = (SECKEYPQGParams *)param;
1262            attrs = dsaPubTemplate;
1263            PK11_SETATTRS(attrs, CKA_PRIME, dsaParams->prime.data,
1264                                    dsaParams->prime.len); attrs++;


If you would like to reproduce the crash, go to:
http://kuix.de/misc/test3/crmf-dsa-with-dummy-param.php

	Kai Engert (:KaiE:) Assignee
	Comment 14 • 14 years ago

The crash is not limited to "dsa-sign", but can be reproduced with "rsa-dual-use", and probably other combinations, too.

The following links looks similar, but is different. dsa vs. rsa.

http://kuix.de/misc/test3/crmf-rsa-with-dummy-param.php

	Kai Engert (:KaiE:) Assignee
	Comment 15 • 14 years ago

The patch in bug 572809 is one way to fix this bug.

However, given that Mozilla application users might continue to use older NSS versions for a while, I propose to use a patch at the PSM level, too.

Unfortunately, the patch at the PSM level has the slight risk that we might introduce a regression in the interaction with consumers of the related JavaScript function. I've contacted the maintainers of Red Hat Certificate System, which are an important consumer, and have asked them to help testing.

	Kai Engert (:KaiE:) Assignee
	Comment 16 • 14 years ago

Is it true that former Sun = Oracle has a Certificate System that might be a consumer of this function, too?

Should we involve them for testing and QA resources?

	Daniel Veditz [:dveditz]
	Comment 17 • 14 years ago

Crashes that are always a null deref (with no way to generate an arbitrary offset from null) generally do not need to be security-sensitive. They are not exploitable beyond the denial of service.  [DoS bugs in NSS server code might be another matter; the NSS team should make that call.]

	Robert Relyea
	Comment 18 • 14 years ago

Comment on attachment 452038 [details] [diff] [review]
Patch v1

r+ I don't know why it took so long for me to get to this simple patch.

bob

	Kai Engert (:KaiE:) Assignee
	Comment 19 • 14 years ago

Comment on attachment 452038 [details] [diff] [review]
Patch v1

Requesting approval for this trivial null-check crasher fix.

	Daniel Veditz [:dveditz]
	Comment 20 • 14 years ago

Comment on attachment 452038 [details] [diff] [review]
Patch v1

Do we need or want this on 1.9.1 branch as well? Approved for 1.9.2.13, a=dveditz for release-drivers

	Kai Engert (:KaiE:) Assignee
	Comment 21 • 14 years ago

Comment on attachment 452038 [details] [diff] [review]
Patch v1

ok, should land on 1.9.1.x too

I'll wait with any landing until I have trunk approval.

	Daniel Veditz [:dveditz]
	Comment 22 • 14 years ago

Comment on attachment 452038 [details] [diff] [review]
Patch v1

Approved for 1.9.1.16, a=dveditz for release-drivers

	Reed Loden [:reed]
	Comment 23 • 14 years ago

http://hg.mozilla.org/mozilla-central/rev/ab4cdb935ccf
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/8f9df3735c4d
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c9f85e155229