303010 - Certificate upgrade can drop S/MIME certificates.

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Robert Relyea]

When going from cert7 to cert8 or from cert8 to rdb, S/MIME certificates are
sometimes dropped. That is because the upgrade is handled by only reconstructing
the certs and s/mime records. If the S/Mime record is encountered before the
corresponding certificate, then the S/MIME record is lost.

The solution is to loop through the update twice, grabbing the certificates
first, then the S/MIME records.

Comment 1

[Robert Relyea]

Attachment: Delay loading the S/MIME records on upgrade until the cert is loaded.

Comment 2

[Robert Relyea]

Attachment: remove the incorrect spacing change to the 'if' in ReadSubjectEntry()

Comment 3

[Wan-Teh Chang]

Comment on attachment 191278 [details] [diff] [review]
remove the incorrect spacing change to the 'if' in ReadSubjectEntry()

r=wtc.	Please address the issues I raised in
a separate patch.

> static SECStatus
> UpdateV8DB(NSSLOWCERTCertDBHandle *handle, DB *updatedb)
> {
>-    return UpdateV7DB(handle,updatedb);
>+    SECStatus rv;
>+    certDBEntryVersion *versionEntry = NULL;
>+
>+    versionEntry = NewDBVersionEntry(0);
>+    if ( versionEntry == NULL ) {
>+	rv = SECFailure;
>+	goto loser;
>+    }
>+	
>+    rv = WriteDBVersionEntry(handle, versionEntry);
>+
>+    DestroyDBEntry((certDBEntry *)versionEntry);
>+
>+    if ( rv != SECSuccess ) {
>+	goto loser;
>+    }
>+    rv =  UpdateV7DB(handle,updatedb);
>+loser:
>+    return rv;
> }

Can we accomplish the same thing by moving the following code
in openNewCertDB:

    if (appName) {
	updatedb = dbsopen(certdbname, NO_RDONLY, 0600, DB_HASH, 0);
	if (updatedb) {
	    rv = UpdateV8DB(handle, updatedb);
	    db_FinishTransaction(handle->permCertDB,PR_FALSE);
	    db_InitComplete(handle->permCertDB);
	    return(rv);
	}
    }

after the existing code that writes the DB version entry?

>+    do {
>+	unsigned char *dataBuf = (unsigned char *)data.data;
>+	unsigned char *keyBuf = (unsigned char *)key.data;
>+	dbEntry.data = &dataBuf[SEC_DB_ENTRY_HEADER_LEN];
>+	dbEntry.len = data.size - SEC_DB_ENTRY_HEADER_LEN;
>+ 	entryType = (certDBEntryType) keyBuf[0];
>+	if (entryType != certDBEntryTypeSMimeProfile) {
>+	    continue;
>+	}

Nit: perhaps you can do the keyBuf and entryType assignments
first so that you can test entryType as early as possible.

>+        smimeEntry.common.version = (unsigned int)dataBuf[0];
>+	smimeEntry.common.type = entryType;
>+	smimeEntry.common.flags = (unsigned int)dataBuf[2];
>+	smimeEntry.common.arena = PORT_NewArena(DER_DEFAULT_CHUNKSIZE);

We should test the return value of PORT_NewArena.  This is a
pre-existing problem.

>+	/* decode entry */
>+	rv = DecodeDBSMimeEntry(&smimeEntry,&dbEntry,(char *)dbKey.data);
>+	if (rv == SECSuccess) {
> 	    nsslowcert_UpdateSMimeProfile(handle, smimeEntry.emailAddr,
> 		&smimeEntry.subjectName, &smimeEntry.smimeOptions,
> 						 &smimeEntry.optionsDate);
> 	}
>+	PORT_FreeArena(smimeEntry.common.arena, PR_FALSE);
>+	smimeEntry.common.arena = NULL;
>     } while ( (* updatedb->seq)(updatedb, &key, &data, R_NEXT) == 0 );

Are you sure you don't want this function (UpdateV7DB) to fail
if DecodeDBSMimeEntry fails?  Note that the last statement of
this function is return(SECSuccess), not return(rv).

It seems that we should handle DecodeDBCertEntry, DecodeDBCrlEntry,
and DecodeDBSMimeEntry failures the same way.  Right now the
failures of the first two functions will cause do-while loop to
abort, but the failure of the last one won't.  Is the inconsistency
intentional?

Comment 4

[Wan-Teh Chang]

Attachment: How I (Wan-Teh) think the UpdateV8DB change should be done

This patch is meant to make precise how I think the
UpdateV8DB change should be done.  Bob, what do you
think?

Comment 5

[Robert Relyea]

Patch checked in. The checked in version has wtc's version of the UpdateV8DB()
fix, which is cleaner than the original.

/cvsroot/mozilla/security/nss/lib/softoken/pcertdb.c,v  <--  pcertdb.c
new revision: 1.51; previous revision: 1.50
done

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

Adding self to cc list.  
Please, when asking me to review a patch, add me to the cc list.  

This bug has been marked fixed, but the target milestone has not been
set to show the release in which the fix first appears.  
I'm setting it to 3.11.  Please change it if that is incorrect.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 191278 [details] [diff] [review]
remove the incorrect spacing change to the 'if' in ReadSubjectEntry()

It appears to me that another patch (not this one) was checked in.  
So I am cancelling the review request for this patch.

While we're correcting the v8 upgrade code, I think this erroneous
(and sadly misleading) source code comment should be fixed:

>Index: pcertdb.c
>===================================================================
>RCS file: /cvsroot/mozilla/security/nss/lib/softoken/pcertdb.c,v
>retrieving revision 1.50
>diff -u -1 -5 -r1.50 pcertdb.c
>--- pcertdb.c	1 Aug 2005 20:41:30 -0000	1.50
>+++ pcertdb.c	1 Aug 2005 22:52:57 -0000

>@@ -3460,31 +3460,49 @@
> 
> /* forward declaration */
> static SECStatus
> UpdateV7DB(NSSLOWCERTCertDBHandle *handle, DB *updatedb);
> 
> /*
>  * version 8 uses the same schema as version 7. The only differences are
>  * 1) version 8 db uses the blob shim to store data entries > 32k.
>  * 2) version 8 db sets the db block size to 32k.

 + * 3) version 8 db has a new format for the subject name record.
 + *	The new format accomodates multiple email addresses.

>  * both of these are dealt with by the handle.
>  */