Closed
Bug 1429591
Opened 6 years ago
Closed 6 years ago
Importing a certificate with CERT_ImportCerts to an NSS SQL db doesn't associate it to the existing private key
Categories
(NSS :: Libraries, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
3.36
People
(Reporter: KaiE, Assigned: rrelyea)
References
(Blocks 1 open bug)
Details
Attachments
(3 files, 2 obsolete files)
12.97 KB,
patch
|
Details | Diff | Splinter Review | |
18.64 KB,
application/octet-stream
|
Details | |
2.58 KB,
patch
|
KaiE
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•6 years ago
|
||
Reporter | ||
Comment 2•6 years ago
|
||
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'
Reporter | ||
Comment 3•6 years ago
|
||
Assignee | ||
Comment 4•6 years ago
|
||
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.
Assignee | ||
Comment 5•6 years ago
|
||
Fixes the issue. Includes Kai's test code added to certutil.
Attachment #8953652 -
Flags: review?(kaie)
Reporter | ||
Comment 6•6 years ago
|
||
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.
Reporter | ||
Comment 7•6 years ago
|
||
Attachment #8953652 -
Attachment is obsolete: true
Attachment #8953652 -
Flags: review?(kaie)
Reporter | ||
Comment 8•6 years ago
|
||
Comment on attachment 8954352 [details] [diff] [review] bob-1429591.patch This is Bob's fix. r=kaie
Attachment #8954352 -
Flags: review+
Reporter | ||
Comment 9•6 years ago
|
||
(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.
Attachment #8954352 -
Attachment is obsolete: true
Attachment #8954372 -
Flags: review+
Reporter | ||
Updated•6 years ago
|
Attachment #8954352 -
Flags: review+
Reporter | ||
Comment 10•6 years ago
|
||
(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/
Reporter | ||
Updated•6 years ago
|
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.36
You need to log in
before you can comment on or make changes to this bug.
Description
•