Closed Bug 86981 Opened 23 years ago Closed 23 years ago

cmsutil coredumps due to uninitialized auto variables in smime lib.

Categories

(NSS :: Libraries, defect, P1)

3.2.1

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mbar, Assigned: wtc)

Details

Attachments

(2 files)

cmsutil is dumping core in smime tests due to uninitialized auto variables in cmsrecinfo.c. In the function NSS_CMSRecipientInfo_Create(), none of the auto variables are initialized. The variable rv is used to indicate success or failure at one point in the code, but the variable is only "explicitly set" in the case of failure. Because of the way the code is written, a successful code path can be taken, but the variable, not being initialized, can take on a value indicating failure to the later code. Please note that this is clearly a cross-platform bug. All of the auto-storage variables, especially those used for branching decisions should be initialized to a sensible value. Otherwise, the code is very liable to intermittent errors. Please refer to the C standard's rules for auto-storage variables and their initialized value. Below you'll find code snippets from the pertinent sections of certdb.c, cmsrecinfo.c, smime.sh (test driver), and the debugging output from the test run. I have put in quite a few statements for debugging output in the C modules. I've also included the pertinent section of the output from the test run so the order in which the debugging statements are executed can be seen. Please forward any questions. Cheers, Matthew ============================================================================ mozilla/security/nss/lib/smime/cmsrecinfo.c ============================================================================ NSSCMSRecipientInfo * NSS_CMSRecipientInfo_Create(NSSCMSMessage *cmsg, CERTCertificate *cert) { NSSCMSRecipientInfo *ri; void *mark; SECOidTag certalgtag; SECStatus rv; NSSCMSRecipientEncryptedKey *rek; NSSCMSOriginatorIdentifierOrKey *oiok; unsigned long version; SECItem *dummy; PLArenaPool *poolp; #if 1 fprintf( stderr, "NSS_CMSRecipientInfo_Create(NSSCMSMessage *cmsg=%p, CERTCertificate *cert=%p)\n", cmsg, cert ); #endif poolp = cmsg->poolp; mark = PORT_ArenaMark(poolp); ri = (NSSCMSRecipientInfo *)PORT_ArenaZAlloc(poolp, sizeof(NSSCMSRecipientInfo)); #if 1 fprintf( stderr, "1>> poolp=%p, ri=%p\n", poolp, ri ); #endif if (ri == NULL) goto loser; ri->cmsg = cmsg; ri->cert = CERT_DupCertificate(cert); #if 1 fprintf( stderr, "2>> ri=%p, ri->cert=%p, ri->cmsg=%p\n", ri, ri->cert, ri->cmsg ); #endif if (ri->cert == NULL) goto loser; certalgtag = SECOID_GetAlgorithmTag(&(cert->subjectPublicKeyInfo.algorithm)); switch (certalgtag) { case SEC_OID_PKCS1_RSA_ENCRYPTION: #if 1 fprintf( stderr, "2.1>> case SEC_OID_PKCS1_RSA_ENCRYPTION:\n" ); #endif ri->recipientInfoType = NSSCMSRecipientInfoID_KeyTrans; /* hardcoded issuerSN choice for now */ ri->ri.keyTransRecipientInfo.recipientIdentifier.identifierType = NSSCMSRecipientID_IssuerSN; #if 1 fprintf( stderr, "2.2>> poolp=%p, cert=%p\n", poolp, cert ); #endif ri->ri.keyTransRecipientInfo.recipientIdentifier.id.issuerAndSN = CERT_GetCertIssuerAndSN(poolp, cert); #if 1 fprintf( stderr, "2.3>> ri->ri.keyTransRecipientInfo.recipientIdentifier.id.issuerAndSN=%p\n", ri->ri.keyTransRecipientInfo.recipientIdentifier.id.issuerAndSN ); #endif if (ri->ri.keyTransRecipientInfo.recipientIdentifier.id.issuerAndSN == NULL) { #if 1 fprintf( stderr, "2.4>> Going to failure\n" ); #endif rv = SECFailure; break; } #if 1 fprintf( stderr, "2.5.0>> SECFailure == %ld\n", SECFailure ); fprintf( stderr, "2.5.1>> Taking successful branch.\n" ); #endif break; case SEC_OID_MISSI_KEA_DSS_OLD: #if 1 fprintf( stderr, "2.1>> case SEC_OID_MISSI_KEA_DSS_OLD:\n" ); #endif case SEC_OID_MISSI_KEA_DSS: #if 1 fprintf( stderr, "2.1>> case SEC_OID_MISSI_KEA_DSS:\n" ); #endif case SEC_OID_MISSI_KEA: #if 1 fprintf( stderr, "2.1>> case SEC_OID_MISSI_KEA:\n" ); #endif /* backward compatibility - this is not really a keytrans operation */ ri->recipientInfoType = NSSCMSRecipientInfoID_KeyTrans; /* hardcoded issuerSN choice for now */ ri->ri.keyTransRecipientInfo.recipientIdentifier.identifierType = NSSCMSRecipientID_IssuerSN; ri->ri.keyTransRecipientInfo.recipientIdentifier.id.issuerAndSN = CERT_GetCertIssuerAndSN(poolp, cert); if (ri->ri.keyTransRecipientInfo.recipientIdentifier.id.issuerAndSN == NULL) { rv = SECFailure; break; } break; case SEC_OID_X942_DIFFIE_HELMAN_KEY: /* dh-public-number */ #if 1 fprintf( stderr, "2.1>> case SEC_OID_X942_DIFFIE_HELMAN_KEY: /* dh-public-number */\n" ); #endif /* a key agreement op */ ri->recipientInfoType = NSSCMSRecipientInfoID_KeyAgree; if (ri->ri.keyTransRecipientInfo.recipientIdentifier.id.issuerAndSN == NULL) { rv = SECFailure; break; } /* we do not support the case where multiple recipients * share the same KeyAgreeRecipientInfo and have multiple RecipientEncryptedKeys * in this case, we would need to walk all the recipientInfos, take the * ones that do KeyAgreement algorithms and join them, algorithm by algorithm * Then, we'd generate ONE ukm and OriginatorIdentifierOrKey */ /* only epheremal-static Diffie-Hellman is supported for now * this is the only form of key agreement that provides potential anonymity * of the sender, plus we do not have to include certs in the message */ /* force single recipientEncryptedKey for now */ if ((rek = NSS_CMSRecipientEncryptedKey_Create(poolp)) == NULL) { rv = SECFailure; break; } /* hardcoded IssuerSN choice for now */ rek->recipientIdentifier.identifierType = NSSCMSKeyAgreeRecipientID_IssuerSN; if ((rek->recipientIdentifier.id.issuerAndSN = CERT_GetCertIssuerAndSN(poolp, cert)) == NULL) { rv = SECFailure; break; } oiok = &(ri->ri.keyAgreeRecipientInfo.originatorIdentifierOrKey); /* see RFC2630 12.3.1.1 */ oiok->identifierType = NSSCMSOriginatorIDOrKey_OriginatorPublicKey; rv = NSS_CMSArray_Add(poolp, (void ***)&ri->ri.keyAgreeRecipientInfo.recipientEncryptedKeys, (void *)rek); break; default: #if 1 fprintf( stderr, "2.1>> default:\n" ); #endif /* other algorithms not supported yet */ /* NOTE that we do not support any KEK algorithm */ PORT_SetError(SEC_ERROR_INVALID_ALGORITHM); rv = SECFailure; break; } if (rv == SECFailure) { #if 1 fprintf( stderr, "3>> rv==SECFailure\n" ); #endif goto loser; } . . . ============================================================================ ============================================================================ mozilla/security/nss/lib/certdb/certdb.c - Starting Line 1498 ============================================================================ CERTIssuerAndSN * CERT_GetCertIssuerAndSN(PRArenaPool *arena, CERTCertificate *cert) { CERTIssuerAndSN *result; SECStatus rv; #if 1 fprintf( stderr, "1.0.0>> CERT_GetCertIssuerAndSN(PRArenaPool *arena=%p, CERTCertificate *cert=%p), cert->arena=%p\n", arena , cert, cert->arena ); #endif if ( arena == NULL ) { arena = cert->arena; } result = (CERTIssuerAndSN*)PORT_ArenaZAlloc(arena, sizeof(*result)); #if 1 fprintf( stderr, "1.1.0>> result = PORT_ArenaZAlloc() = %p \n", result ); #endif if (result == NULL) { PORT_SetError (SEC_ERROR_NO_MEMORY); return NULL; } #if 1 fprintf( stderr, "1.2.0>> (arena=%p, &result->derIssuer=%p, &cert->derIssuer=%p)\n", arena, &result->derIssuer, &cert->derIssuer ); #endif rv = SECITEM_CopyItem(arena, &result->derIssuer, &cert->derIssuer); #if 1 fprintf( stderr, "1.2.1>> (arena=%p, &result->derIssuer=%p, &cert->derIssuer=%p)\n", arena, &result->derIssuer, &cert->derIssuer ); fprintf( stderr, "1.3.0>> rv=%s \n", (rv==SECSuccess)?"OK":"Not OK" ); #endif if (rv != SECSuccess) return NULL; rv = CERT_CopyName(arena, &result->issuer, &cert->issuer); #if 1 fprintf( stderr, "1.4.0>> CERT_CopyName(arena=%p, &result->issuer=%p, &cert->issuer=%p)\n", arena, &result->derIssuer, &cert->derIssuer ); fprintf( stderr, "1.5.0>> rv=%s \n", (rv==SECSuccess)?"OK":"Not OK" ); #endif if (rv != SECSuccess) return NULL; rv = SECITEM_CopyItem(arena, &result->serialNumber, &cert->serialNumber); #if 1 fprintf( stderr, "1.6.0>> SECITEM_CopyItem(arena=%p, &result->serialNumber=%p, &cert->serialNumber=%p)\n", arena, &result->serialNumber, &cert->serialNumber); fprintf( stderr, "1.7.0>> rv=%s \n", (rv==SECSuccess)?"OK":"Not OK" ); #endif if (rv != SECSuccess) return NULL; #if 1 fprintf( stderr, "1.8.0>> result=%p \n", result ); #endif return result; } ============================================================================ ============================================================================ nss/tests/smime/smime.sh: added two lines at line 109 to stop after coredump. ============================================================================ . . . 107 cmsutil -E -r bob@bogus.com -i alice.txt -d ${R_ALICEDIR} -p nss -o alice.env 108 html_msg $? 0 "Create Enveloped Data Alice" "." 109+ # MDB vvv 110+ exit . . . ============================================================================ Output from test run of nss/tests/smime/smime.sh ============================================================================ . . . imported certificate 1.0.0>> CERT_GetCertIssuerAndSN(PRArenaPool *arena=1009b078, CERTCertificate *cert=10098940), cert->arena=100988c0 1.1.0>> result = PORT_ArenaZAlloc() = 10097238 1.2.0>> (arena=1009b078, &result->derIssuer=10097238, &cert->derIssuer=10098988) 1.2.1>> (arena=1009b078, &result->derIssuer=10097238, &cert->derIssuer=10098988) 1.3.0>> rv=OK 1.4.0>> CERT_CopyName(arena=1009b078, &result->issuer=10097238, &cert->issuer=10098988) 1.5.0>> rv=OK 1.6.0>> SECITEM_CopyItem(arena=1009b078, &result->serialNumber=1009724c, &cert->serialNumber=100989c4) 1.7.0>> rv=OK 1.8.0>> result=10097238 created signed-date message cmsg [1009ac68] arena [10097068] password [nss] input len [158] 44 61 74 65 3a 20 57 65 64 2c 20 32 30 20 53 65 70 20 32 30 30 30 20 30 30 3a 30 30 3a 30 31 20 2d 30 37 30 30 20 28 50 44 54 29 a 46 72 6f 6d 3a 20 61 6c 69 63 65 40 62 6f 67 75 73 2e 63 6f 6d a 53 75 62 6a 65 63 74 3a 20 6d 65 73 73 61 67 65 20 41 6c 69 63 65 20 2d 2d 3e 20 42 6f 62 a 54 6f 3a 20 62 6f 62 40 62 6f 67 75 73 2e 63 6f 6d a a 54 68 69 73 20 69 73 20 61 20 74 65 73 74 20 6d 65 73 73 61 67 65 20 66 72 6f 6d 20 41 6c 69 63 65 20 74 6f 20 42 6f 62 2e a encoding passed wrote to file smime.sh: Create Signature Alice . PASSED cmsutil -D -i alice.sig -d ../bobdir -o alice.data1 starting program parsed command line received commands NSS has been initialized. Got default certdb DECODE smime.sh: Decode Alice's Signature . PASSED diff alice.txt alice.data1 smime.sh: Compare Decoded Signature and Original . PASSED smime.sh: Enveloped Data Tests ------------------------------ cmsutil -E -r bob@bogus.com -i alice.txt -d ../alicedir -p nss \ -o alice.env starting program parsed command line received commands NSS has been initialized. Got default certdb NSS_CMSRecipientInfo_Create(NSSCMSMessage *cmsg=100a4eb0, CERTCertificate *cert=100a4690) 1>> poolp=1009b078, ri=100a4ff0 2>> ri=100a4ff0, ri->cert=100a4690, ri->cmsg=100a4eb0 2.1>> case SEC_OID_PKCS1_RSA_ENCRYPTION: 2.2>> poolp=1009b078, cert=100a4690 1.0.0>> CERT_GetCertIssuerAndSN(PRArenaPool *arena=1009b078, CERTCertificate *cert=100a4690), cert->arena=1009b008 1.1.0>> result = PORT_ArenaZAlloc() = 100a5050 1.2.0>> (arena=1009b078, &result->derIssuer=100a5050, &cert->derIssuer=100a46d8) 1.2.1>> (arena=1009b078, &result->derIssuer=100a5050, &cert->derIssuer=100a46d8) 1.3.0>> rv=OK 1.4.0>> CERT_CopyName(arena=1009b078, &result->issuer=100a5050, &cert->issuer=100a46d8) 1.5.0>> rv=OK 1.6.0>> SECITEM_CopyItem(arena=1009b078, &result->serialNumber=100a5064, &cert->serialNumber=100a4714) 1.7.0>> rv=OK 1.8.0>> result=100a5050 2.3>> ri->ri.keyTransRecipientInfo.recipientIdentifier.id.issuerAndSN=100a5050 2.5.0>> SECFailure == -1 2.5.1>> Taking successful branch. 3>> rv==SECFailure ERROR: cannot create CMS recipientInfo object. cmsutil: problem enveloping: Certificate extension not found. cmsg [0] arena [10097030] password [nss] smime_main[19]: 279131 Memory fault(coredump) smime.sh: Create Enveloped Data Alice . FAILED
Status: UNCONFIRMED → NEW
Ever confirmed: true
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → 3.2.2
Attached patch Proposed patch. (deleted) — Splinter Review
Kirk, could you please review my patch? Thank you.
SECStatus NSS_CMSRecipientInfo_WrapBulkKey(NSSCMSRecipientInfo *ri, PK11SymKey *bulkkey, SECOidTag bulkalgtag) { CERTCertificate *cert; SECOidTag certalgtag; SECStatus rv; <--- line 277 I believe this spot in the same file needs it too. For example the first case (SEC_OID_PKCS1_RSA_ENCRYPTION) might not set rv. Also the SEC_OID_X942_DIFFIE_HELMAN_KEY case. Otherwise, your change gets my approval. Good work Matthew and Wan-Teh!
I checked in the revised patch on the trunk of NSS. This fix will be in NSS 3.3.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Component: Tools → Libraries
Resolution: --- → FIXED
Target Milestone: 3.2.2 → 3.3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: