1429591 - Importing a certificate with CERT_ImportCerts to an NSS SQL db doesn't associate it to the existing private key

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Kai Engert [:KaiE:]]

Given a NSS database using the SQL storage format, that already contains a private key, e.g. after having created a certificate request.

If the application uses CERT_ImportCerts to import the certificate, the resulting SQL database has an incomplete state. NSS doesn't associate the import certificate to the private key. Contrary to the expectation, certutil -L reports the cert without the "u" flag.

It works fine, if the same data is stored in a DBM database,

It works fine, if the application uses SQL but calls PK11_ImportCert.

certutil -A uses PK11_ImportCert.

I'll attach a patch that can be used to reproduce the bug. It adds an alternative implementation for -A, which can be accessed using --alternative-add-cert, and which uses CERT_ImportCerts.

I'll attach example databases and certificates that can be used to reproduce.

Comment 1

[Kai Engert [:KaiE:]]

Attachment: 1429591-reproducer.patch

Comment 2

[Kai Engert [:KaiE:]]

To reproduce:

Apply the attached patch to a local copy of NSS, build, and setup your environemt to use your locally built NSS and tools.

Download the attached .tar.gz archive and extract it.

First, we demonstrate that CERT_ImportCerts works with DBM:

$ cd dbm-without-cert
$ certutil -d dbm:. -f pwdfile.txt -K

certutil: Checking token "NSS Certificate DB" in slot "NSS User Private Key and Certificate Services"
< 0> rsa      0320a8df63765c9e4d135bb2fbd4b32d6e3ec57f   Server-Cert

$ certutil -d dbm:. -f pwdfile.txt -L

IPA.EXAMPLE IPA CA                                           ,,   

$ certutil -d dbm:. -f pwdfile.txt --alternative-add-cert -t ,, -a -i cert.pem -n Server-Cert

Server-Cert                                                  u,u,u
IPA.EXAMPLE IPA CA                                           ,,   

-> it worked, cert imported, listed with 'u'


Now we show that it fails with SQL and CERT_ImportCerts:

$ cd ../sql-without-cert

$ certutil -d sql:. -f pwdfile.txt -L

IPA.EXAMPLE IPA CA                                           CT,C,C

$ certutil -d sql:. -f pwdfile.txt --alternative-add-cert -t ,, -a -i cert.pem -n Server-Cert

IPA.EXAMPLE IPA CA                                           CT,C,C
Server-Cert                                                  ,,   

-> import worked, but is incomplete, NO 'u'


Now we show that it works with SQL and PK11_ImportCerts.

$ certutil -d sql:. -f pwdfile.txt -A -t ,, -a -i cert.pem -n Server-Cert

$ certutil -d sql:. -f pwdfile.txt -L

IPA.EXAMPLE IPA CA                                           CT,C,C
Server-Cert                                                  u,u,u

-> cert was associated with private key, we have 'u'

Comment 3

[Kai Engert [:KaiE:]]

Attachment: 1429591-data.tar.gz

Comment 4

[Robert Relyea]

Comment on attachment 8941608 [details] [diff] [review]
1429591-reproducer.patch

Review of attachment 8941608 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/certdb/cert.h
@@ +1082,5 @@
>   * import a collection of certs into the temporary or permanent cert
>   * database
>   */
>  SECStatus CERT_ImportCerts(CERTCertDBHandle *certdb, SECCertUsage usage,
> +                           unsigned int ncerts, const SECItem **derCerts,

This change causes problems with compiling NSS. The issue is we are trying to make a double pointer a const. The compilier won't let you convert a non-const double pointer to a const double pointer by default. This means all nss application code that uses this function would have to add a cast (or remove an existing cast).

It would have been better to start with this as a const, but that ship has sailed.

::: lib/certdb/certdb.c
@@ +2391,5 @@
>  }
>  
>  SECStatus
>  CERT_ImportCerts(CERTCertDBHandle *certdb, SECCertUsage usage,
> +                 unsigned int ncerts, const SECItem **derCerts,

My change in cert.h would necessitate a change here as well.

Comment 5

[Robert Relyea]

Attachment: sql-import-fix.patch

Fixes the issue. Includes Kai's test code added to certutil.

Comment 6

[Kai Engert [:KaiE:]]

Bob, thanks for the patch.

I've tested it locally with the reproducer and confirm the fix.

I don't want to check in the alternative add code to certutil yet. It's my own code, so I cannot review it.

Therefore I will attach the subset of your patch that you produced, and will check that in.

I've filed a separate bug to get an automated test added, and we can reuse the certutil code for that. I'll ask you for review, when I have the test in the other bug.

Comment 7

[Kai Engert [:KaiE:]]

Attachment: bob-1429591.patch

Comment 8

[Kai Engert [:KaiE:]]

Comment on attachment 8954352 [details] [diff] [review]
bob-1429591.patch

This is Bob's fix.
r=kaie

Comment 9

[Kai Engert [:KaiE:]]

Attachment: only-bob-1429591.patch

(In reply to Kai Engert (:kaie:) from comment #8)
> Comment on attachment 8954352 [details] [diff] [review]
> bob-1429591.patch
> 
> This is Bob's fix.
> r=kaie

No.

This patch included an ABI change, that isn't necessary for Bob's fix.

I had landed it, 
https://hg.mozilla.org/projects/nss/rev/abb0424391ed

but our automatic ABI detection complained about it. Which is a correct complaint, because CERT_NewTempCertificate isn't treating the derCert parameter as const, it may copy it. And the compiler doesn't detect it, because the function calls a macro that casts const away.

I've removed the parts that are unwanted and unnecessary:
https://hg.mozilla.org/projects/nss/rev/152dd9663989

I'm attaching the effective fix that was landed.

Comment 10

[Kai Engert [:KaiE:]]

(In reply to Kai Engert (:kaie:) from comment #9)
> parameter as const, it may copy it. And the compiler doesn't detect it,

s/copy it/absorb it/