cmsutil coredumps due to uninitialized auto variables in smime lib.

RESOLVED FIXED in 3.3

Status

P1
major
RESOLVED FIXED
18 years ago
18 years ago

People

(Reporter: mbar, Assigned: wtc)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

18 years ago
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
(Assignee)

Updated

18 years ago
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → 3.2.2
(Assignee)

Comment 1

18 years ago
Created attachment 39438 [details] [diff] [review]
Proposed patch.
(Assignee)

Comment 2

18 years ago
Kirk, could you please review my patch?  Thank you.

Comment 3

18 years ago
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!
(Assignee)

Comment 4

18 years ago
Created attachment 39563 [details] [diff] [review]
Proposed patch v2, which also fixes the uninitialized variable that Kirk found.
(Assignee)

Comment 5

18 years ago
I checked in the revised patch on the trunk of NSS.
This fix will be in NSS 3.3.
Status: ASSIGNED → RESOLVED
Last Resolved: 18 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.