Closed Bug 104103 Opened 19 years ago Closed 10 years ago

crypto.generateCRMFRequest used to generate DSA request: crash

Categories

(Core :: Security: PSM, defect, P1)

x86
Windows NT
defect

Tracking

()

RESOLVED FIXED
mozilla2.0b8
Tracking Status
status1.9.2 --- .13-fixed
status1.9.1 --- .16-fixed

People

(Reporter: awnuk, Assigned: KaiE)

References

()

Details

(Keywords: crash, testcase, Whiteboard: [sg:dos][kerh-brz][psm-fatal][psm-enroll])

Attachments

(2 files)

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.
Priority: -- → P1
Worksforme. There are some DSA tests at http://junruh.mcom.com/tests.html on the
right side under "CMS 4.2 Testing."
Status: UNCONFIRMED → NEW
Ever confirmed: true
This only happens when you pass PQG via "keyParamsN".
Severity: critical → normal
Priority: P1 → P2
->javi
cc lakshmi.
crash keyword.
Assignee: ssaux → javi
Keywords: crash
Priority: P2 → P1
Target Milestone: --- → Future
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?
Is this still happening? How can I reproduce the crash?
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.
Severity: normal → critical
Mass reassign Javi's old PSM bugs to nobody
Assignee: javi → nobody
QA Contact: junruh → nobody
Target Milestone: Future → ---
Product: PSM → Core
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
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.
I found an old PSM distribution with the missing doc. Hope it's useful.
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?
Whiteboard: [kerh-brz]
QA Contact: nobody → ui
Version: psm2.0 → 1.0 Branch
Version: 1.0 Branch → Trunk
Assignee: nobody → kaie
Component: Security: UI → Security: PSM
QA Contact: ui → psm
Group: core-security
Depends on: 572809
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
Summary: crypto.generateCRMFRequest used to generate DSA request is crashing 6.2 → crypto.generateCRMFRequest used to generate DSA request: crash
Whiteboard: [kerh-brz] → [kerh-brz][psm-fatal]
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
Attached patch Patch v1Splinter Review
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.
Attachment #452038 - Flags: review?(rrelyea)
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?
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.]
Keywords: testcase
Whiteboard: [kerh-brz][psm-fatal] → [sg:dos][kerh-brz][psm-fatal]
Whiteboard: [sg:dos][kerh-brz][psm-fatal] → [sg:dos][kerh-brz][psm-fatal][psm-enroll]
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
Attachment #452038 - Flags: review?(rrelyea) → review+
Comment on attachment 452038 [details] [diff] [review]
Patch v1

Requesting approval for this trivial null-check crasher fix.
Attachment #452038 - Flags: approval2.0?
Attachment #452038 - Flags: approval1.9.2.13?
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
Attachment #452038 - Flags: approval1.9.2.13? → approval1.9.2.13+
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.
Attachment #452038 - Flags: approval1.9.1.16?
Comment on attachment 452038 [details] [diff] [review]
Patch v1

Approved for 1.9.1.16, a=dveditz for release-drivers
Attachment #452038 - Flags: approval1.9.1.16? → approval1.9.1.16+
Attachment #452038 - Flags: approval2.0? → approval2.0+
Group: core-security
You need to log in before you can comment on or make changes to this bug.