141355 - Importing Certs which already exist in the database may cause a crash.

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Robert Relyea]

When importing certs either with JSS or with pkcs-12 we will crash if the cert
already exist.

These functions use CERT_DERDecode cert to get the certificate structure. The
certificate structure returned has an NSSCertificate created to handle the
reference count. When we go to import the cert, we discover it is already in the
cache, so we destroy the NSSCertificate we were passed and create a new one. The
originaly CERT_Certificate doesn't know about this, however, so we crash when we
try to destroy the certificate.

I've fixed several cases already, where we know we're importing into the certdb,
by replacing the Decode calls with CERT_NewTempCert() and the PK11_Import calls
with CERT_AddTempCertToPerm(), however the remaining cases need to import the
certificate into arbitrary tokens (either selected by the private keys, or
selected by the slot which is passed in), so CERT_AddTempCertToPerm().

The solution appears to be to 1) add code to the PK11_Import calls to remove a
temp cert from the cert store on successful import, then 2) replace the
remaining CERT_DecodeCertificates with CERT_NewTempCert().

Before we do this though, I'd like Ian to review the idea and possible make
alternative suggestions.

bob

Comment 1

[Robert Relyea]

Adding wan-teh to CC since he's the overall owner of all NSS bugs.

Ian, once you've read this, if you agree on this course of action you can kick
the bug back to me to complete if you want. This is more a 'I want to make sure
there are no more gotcha's with this plan'

bob

Comment 2

[Ian McGreer]

What's interesting is that CERT_DecodeDERCertificate actually *does not* create
an NSSCertificate below the CERTCertificate to handle the reference counting.

CERT_DecodeDERCertificate is exported, but as a super-private function.  If we
are nonetheless worried that applications may still be calling it, I suggest we
replace the implementation of CERT_DecodeDERCertificate with a call to
CERT_NewTempCertificate.  My worry is that allowing CERTCertificate's created by
CERT_DecodeDERCertificate to run around will wreak havoc.

If we are only worried about NSS calling CERT_DecodeDERCertificate, then I agree
with your proposal.  We should replace all of those calls with calls to
CERT_NewTempCertificate, and add code to PK11_ImportCert to handle temp imports.

Bob, I have some cycles to spare right now.  I will look into this.

Comment 3

[Robert Relyea]

Yes, I'm only worried about NSS uses right now.

I've already converted a number of the calls, I just remember that one case I
had the problem where we imported the certificate using the PK11_ call and
didn't remove it from the certstore. I think that it seems reasonable to add
code to the PK11_ calls to do this if the cert is in the cert store.

bob

Comment 4

[Ian McGreer]

Bob, can you post the patch you have for these changes?  I'm finding that a mix
of solutions is required.  Sometimes, the call to DecodeDER is ok, because the
cert is looked at and destroyed immediately, so there is no need for the
overhead of reference counting / importing as temp.  Other times, using NewTemp
makes sense.  And in pk11wrap, some different magic was needed (the certs there
*are* perm certs, so they need to be constructed properly).

Anyway, I just want to avoid stomping over your changes.

Comment 5

[Ian McGreer]

Attachment: remove any CERT_DecodeDERCertificate calls that may cause problems

This patch adds code to PK11_ImportCert to remove temp certs from the store. 
It also addresses all calls to CERT_DecodeDERCertificate in the following
fashion:

1.  If the returned cert was looked at in a benign manner and immediately
destroyed, the call was kept.

2.  If the returned cert left the scope of the calling function, or was passed
to NSS functions that may use it, the call was replaced with
CERT_NewTempCertificate.

The implementation of pk11_fastCert was rewritten to create a valid
NSSCertificate from the object handle.

CERT_DestroyCertificate was fixed to handle certs created by
CERT_DecodeDERCertificate.

Comment 6

[Robert Relyea]

I actually haven't made any changes yet, I was just formulating how exactly to
fix the problem. The areas I was planning on attacking where those places where
we have

    cert = CERT_DecodeDERCert
    PK11_ImportCertXXXXX();
    CERT_DestroyCertificate()

The two places in particular I saw where in pkcs12 and pk11wrap (importing a
cert and key from a p12 file in p12d.c and importing a DERCertForKey in pk11cert.c).

bob

Comment 7

[Robert Relyea]

Comment on attachment 81934 [details] [diff] [review]
remove any CERT_DecodeDERCertificate calls that may cause problems

In general, looks even more thorough than I was expecting. May cause some OCSP
issues to go away as well.

Comments:
Make sure the proper includes are available for respcmn.c and ans1cmn.c to map
CERT_NewTempCertificate to
__CERT_NewTempCertificate.

(nssrenam.h)
We won't notice this is wron until we go and build PSM.
(It looks like asn1cmn.c doesn't have it).

------------

pk11fastcert.c

The if statement should be 
if (nickptr)
not 
if (*nickptr)

---------------------

Your missing the call in pkcs12/p12d.c (line 2422). That's the line that
actually triggered this bug;).

bob

Comment 8

[Ian McGreer]

Attachment: revised patch

Addresses following issues:

1.  Fixes pk11_fastCert as Bob described;
2.  CERT_NewTempCertificate was not correctly handling the copyDER flag;
3.  Two more instances in libpkcs12 where CERT_DecodeDERCertificate needed
replacement (one Bob mentioned, another that is an API function that returns
certs)
4.  The two CRMF files both already included nssrenam.h

Comment 9

[Robert Relyea]

Comment on attachment 82049 [details] [diff] [review]
revised patch

The first DecodeDERCertificate in p12d.c should probably remain (I think it's
safe), but it's not a bug to convert it to CERT_NewTempCert.

The riskiest parts of this change are the changes to pk11_fastCert and to crmf,
the former because it's a complete rewrite and the latter because only PSM
tests it. I think it's time to bring this into the NSS tip and test it. (good
work ian).

r=relyea

Comment 10

[Ian McGreer]

Bob,

The first change in p12d.c is the one you said was missed in comment #7 :)  The
cert is being imported, so it needs to be temp first.

Checking patch in to the tip.

Comment 11

[Ian McGreer]

Perhaps I misunderstood, you meant the first *remaining*
CERT_DecodeDERCertificate in p12d.c.  This usage is identical to all of the
remaining usages in libpkcs12.  In all cases, CERT_DecodeDERCertificate is
simply used to get the decoding of a cert in order to do a search by it, in this
case, by cert->derSubject.  The cert is not used for any other purpose, so
there's no need for the overhead of creating a temp cert.  The real defect is
that PK11_TraverseCertsForSubjectInSlot takes a CERTCertificate as the search
query instead of a SECItem of a derSubject.

Comment 12

[Robert Relyea]

No, my mistake. I remember the function having two calls to DecodeDERCert, one
which gets the cert to do some verification, and the other which imports it. My
memory was faulty, however, and whatever function this was, it's not the case
we're dealing with now (I think it's in high cert somewhere).

So my caveat on your patch is incorrect and your patch is fine as it stands.

bob

Comment 13

[Ian McGreer]

Marking fixed in 3.5.

Comment 14

[Jaime Rodriguez, Jr.]

adt1.0.1+ (on ADT's behalf) for checkin to the 1.0 branch. Pls check this in
asap. thanks!