516101 - If PK11_ImportCert fails, it leaves the certificate undiscoverable by CERT_PKIXVerifyCert

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Wan-Teh Chang]

Attachment: Proposed patch

PK11_ImportCert first removes the certificate from the crypto context's
certificate store, and then import the certificate into the token:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/pk11wrap/pk11cert.c&rev=1.170&mark=875-882,910-911#874

If the token import fails, PK11_ImportCert returns without adding the
certificate back to the crypto context's certificate store.  The result
is that the certificate cannot be discovered by CERT_PKIXVerifyCert,
for example, using the find_cert_issuer or CERT_CreateSubjectCertList
function, because the certificate is in neither the crypto context
nor the trust domain now:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/pki/certificate.c&rev=1.66&mark=410,434,441#410

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/certdb/stanpcertdb.c&rev=1.84&mark=674,686,691#674

The attached patch shuffles code around.  It removes the certificate
from the crypto context after token import.  It is the obvious fix,
and seems to work, but I need to review it carefully.

Background: I initialize NSS with NSS_NoDB_Init.  To my surprise,
the softoken still has two slots, with the second slot being the
"NSS User Private Key and Certificate Services".  It is marked as
readOnly=1, and therefore PK11_ImportCert into that slot fails.

Chromium copies this algorithm from PSM:

http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNSSCallbacks.cpp#1013

When NSS is initialized with NSS_NoDB_Init (e.g., as a fallback
after NSS_InitReadWrite fails), PK11_GetInternalKeySlot returns
a read-only DB slot, and PK11_ImportCert fails, leaving the
intermediate CA certs undiscoverable by subsequent CERT_PKIXVerifyCert
calls.

I suspect PSM has the same bug.  I am working around this bug
by testing if the slot is not read-only:
  PK11SlotInfo *slot = PK11_GetInternalKeySlot();
  if (slot) {
    if (!PK11_IsReadOnly(slot)) {
      PK11_ImportCert(slot, node->cert, ...);
    }
    PK11_FreeSlot(slot);
  }

Comment 1

[Robert Relyea]

Comment on attachment 400208 [details] [diff] [review]
Proposed patch

r+ I've convinced myself the patch is correct and safe for the following reasons:

1) In the success case, there is no change in the resulting state from this code.

2) On failure, the following things may have been done to the temp cert in the cert store:
  a) the cert will have it's c.id set. This should be innocuous.
  b) any key associated with that cert will also get it's id and subject attributes set. This too should be innocuous.

The only other possible issue is a potential existing race condition on c->object.cryptoContext.

bob

Comment 2

[Wan-Teh Chang]

I checked in the patch on the NSS trunk (NSS 3.12.5).

Checking in pk11cert.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11cert.c,v  <--  pk11cert.c
new revision: 1.171; previous revision: 1.170
done