142776 - builtins should correctly implement CKA_SERIAL_NUMBER

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Ian McGreer]

The builtins currently store decoded serial numbers.  The token only handles
searches by decoded serials, and returns decoded serial values.  The latter is
more serious, the former is currented worked around in NSS.

The stored values should be changed to be DER-encoded, following the standard. 
However, to maintain backwards compatibility, it is necessary for the builtins
to allow searches by either DER-encoded or decoded values.

Comment 1

[Jens Jorgensen]

This turns out to be a serious problem for me. I'm using a Dallas Semiconductor 
Java iButton hardware security token and because of this problem I cannot 
decrypt emails that are sent to me that are encrypted with my hardware token. 
The s/mime code tries to lookup the certificate of the recipient by 
issuer/serial number and since the pkcs11 module for this device is implemented 
correctly the cert is not found. 

Ack! This means that for correct implementations of pkcs11 modules a person 
cannot read emails that have been encrypted with keys that are stored on the 
token. Accordingly there are two ways to fix this.

1) Fix the real bug so that the serial number is stored as its DER-encoded 
value. Naturally this can be expected to break other code that relies on this 
bug.

2) Just add in a little fixing scrap of code to 
pk11_FindCertObjectByRecipientNew that would try the search with the correct 
serial number if the search with the incorrect one failed. 

I'm sure purists who don't use hardware crypto tokens will believe that it is 
better to wait until CERTIssuerAndSN is properly fixed everywhere. People like 
me who just want this to work will not like 2) since it introduces some 
additional overhead but would still be much happier if it can be added so we 
can use our tokens and receive encrypted email. In 
security/nss/lib/pk11wrap/pk11cert.c:2026 inside the function 
pk11_FindCertObjectByRecipientNew we can fix by changing:

2053         PK11_SETATTRS(attrs, CKA_CLASS, &certClass,sizeof(certClass)); 
attrs++;
2054         PK11_SETATTRS(attrs, CKA_ISSUER, ri->id.issuerAndSN-
>derIssuer.data, 
2055                                 ri->id.issuerAndSN->derIssuer.len); 
attrs++;
2056         PK11_SETATTRS(attrs, CKA_SERIAL_NUMBER, 
2057           ri->id.issuerAndSN->serialNumber.data,ri->id.issuerAndSN-
>serialNumber.len);
2058 
2059         certHandle = pk11_FindObjectByTemplate(slot,searchTemplate,count);
2060         if (certHandle != CK_INVALID_HANDLE) {
2061             CERTCertificate *cert = pk11_fastCert
(slot,certHandle,NULL,NULL);
2062             if (PK11_IsUserCert(slot,cert,certHandle)) {
2063                 /* we've found a cert handle, now let's see if there is a 
key
2064                  * associated with it... */
2065                 ri->slot = PK11_ReferenceSlot(slot);
2066                 *rlIndex = i;
2067         
2068                 CERT_DestroyCertificate(cert);       
2069                 return certHandle;
2070             }
2071             CERT_DestroyCertificate(cert);       
2072         }

to: 

2053         PK11_SETATTRS(attrs, CKA_CLASS, &certClass,sizeof(certClass)); 
attrs++;
2054         PK11_SETATTRS(attrs, CKA_ISSUER, ri->id.issuerAndSN-
>derIssuer.data, 
2055                                 ri->id.issuerAndSN->derIssuer.len); 
attrs++;
2056         PK11_SETATTRS(attrs, CKA_SERIAL_NUMBER, 
2057           ri->id.issuerAndSN->serialNumber.data,ri->id.issuerAndSN-
>serialNumber.len);
2058 
2059         certHandle = pk11_FindObjectByTemplate(slot,searchTemplate,count);
             if (certHandle == CK_INVALID_HANDLE) {
		  /* PKCS#11 needs to use DER-encoded serial numbers.  Create a
		   * CERTIssuerAndSN that actually has the encoded value and 
pass that
		   * to PKCS#11 (and the crypto context).
		   */
                  /* This is a temporary fix because the CERTIssuerAndSN has 
the serial
                   * *not* DER-encoded but rather with just the bytes of the 
serial number.
                   * This means that people using addition security modules 
that are not broken
                   * like the built-in one cannot receive emails encrypted with 
certificates
                   * that live on their hardware tokens. This fixes the problem 
by trying the
                   * search again if it failes with the correct serial number 
data
                   */
		  SECItem *derSerial;
	      
		  derSerial = SEC_ASN1EncodeItem(NULL, NULL,
						 &ri->id.issuerAndSN-
>serialNumber,
						 SEC_IntegerTemplate);
		  PK11_SETATTRS(attrs, CKA_SERIAL_NUMBER, 
		      derSerial->data,derSerial->len);
		
		  certHandle = pk11_FindObjectByTemplate
(slot,searchTemplate,count);
                  SECITEM_FreeItem(derSerial, PR_TRUE);
             }
2060         if (certHandle != CK_INVALID_HANDLE) {
2061             CERTCertificate *cert = pk11_fastCert
(slot,certHandle,NULL,NULL);
2062             if (PK11_IsUserCert(slot,cert,certHandle)) {
2063                 /* we've found a cert handle, now let's see if there is a 
key
2064                  * associated with it... */
2065                 ri->slot = PK11_ReferenceSlot(slot);
2066                 *rlIndex = i;
2067         
2068                 CERT_DestroyCertificate(cert);       
2069                 return certHandle;
2070             }
2071             CERT_DestroyCertificate(cert);       
2072         }

Note that I have not tested nor even compiled the above since I am not set up 
to be able to build mozilla or any of its parts. However, since I just stole 
code that presumably works from that other function it should be ok I'm 
thinking.

Should the priority of this bug be raised?

Comment 2

[Wan-Teh Chang]

Bob, could you review the suggested workaround

Comment 3

[Robert Relyea]

This bug is not about NSS using correct serial numbers. NSS should already be
using the correct DER encoded serial numbers in the latest releases. If there is
a problem there, that should be a separate bug.

The issue is NSS still supports the old incorrect serial numbers as well (so all
the old tokens you were issued under communicator or older versions of NSS still
work). One token, the Built-in's module, still uses the old serial numbers. In
some sense this is so that the built-in's won't break Netscape 6.x deployments.

Anyway this is only an issue if you have client software which loads nssckbi as
a pkcs #11 module.

bob

Comment 4

[Jens Jorgensen]

Ok, but the problem I'm seeing is that the pkcs11 code passes the 
CKA_SERIAL_NUMBER as the "raw" value, not the DER-encoded value as it should. 
This means that C_FindObjectsInit will never find certs by issuer/serial number 
for correctly implemented pkcs11 modules. Ergo mozilla cannot decrypted emails 
that have been encrypted for a cert on a hardware token. 

I think what you're saying then is that we should relabel the bug description 
then? It seems to me the problem is deeper since in this case the serial number 
is gotten out of the pkcs7 blob in the email so whatever is filling the 
CERTIssuerAndSN is doing it incorrectly since that structure purports to carry 
the DER-encoded value of serial number when it clearly does not.

Comment 5

[Robert Relyea]

No, don't rename this bug, what you are describing is a separate bug.

ARG!! FindCertObjectByRecipient is building it's own Template instead of calling
findCertByIssuer/SN! That's the real bug. This problem has been fixed in
findCertByIssuer/SN already.

Anyway please create a new bug for this problem.

thanks,

bob

Comment 6

[Wan-Teh Chang]

I filed bug 161552.

Comment 7

[Robert Relyea]

The problem with S/MIME grabbing the wrong serial number is bug 161552 , and
still targeted for NSS 3.6. This bug is not that severe and is now targed for 4.0

Comment 8

[Robert Relyea]

The fix for Bug 171331 required this fix. Checked in to 3.7 and 3.6.1

bob