Bug 1568271 - certutil import changes nick of existing cert/key pair (only SQL)
Summary: certutil import changes nick of existing cert/key pair (only SQL)
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: nss
Version: 28
Hardware: Unspecified
OS: Unspecified
unspecified
urgent
Target Milestone: ---
Assignee: Bob Relyea
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2018-04-17 06:05 UTC by Christian Heimes
Modified: 2018-05-30 18:19 UTC (History)
7 users (show)

Fixed In Version: nss-3.36.1-1.1.fc28
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2018-05-09 21:24:24 UTC


Attachments (Terms of Use)
test files, SQL + DBM NSSDB (12.25 KB, application/x-gzip)
2018-04-17 06:05 UTC, Christian Heimes
no flags Details
Patch to keep existing nickname in SQL layer (2.25 KB, patch)
2018-04-29 15:31 UTC, Christian Heimes
no flags Details | Diff
Patch to keep existing nickname in SQL layer (2) (2.34 KB, patch)
2018-04-29 19:55 UTC, Christian Heimes
rrelyea: review-
Details | Diff
Option 3 implemented (1.09 KB, patch)
2018-04-30 22:35 UTC, Bob Relyea
no flags Details | Diff
Option 1 implemented (1.07 KB, patch)
2018-04-30 22:46 UTC, Bob Relyea
no flags Details | Diff
Option to remove the '-n' requirement from AddCert (-A) (1.25 KB, patch)
2018-04-30 22:51 UTC, Bob Relyea
no flags Details | Diff


Links
System ID Priority Status Summary Last Updated
Fedora Pagure freeipa issue 7498 None None None 2018-04-17 06:05:44 UTC

Description Christian Heimes 2018-04-17 06:05:44 UTC
Created attachment 1422911 [details]
test files, SQL + DBM NSSDB

Description of problem:
Import into a DBM and SQL NSS DBs behave differently. With DBM, certutil -A doesn't change the nick of an existing certificate. With SQL format, an existing cert/key pair is renamed when the cert is imported a second time. FreeIPA's CA replica installer relies on the fact that the nick isn't changed when a CA cert is imported a second time.

Version-Release number of selected component (if applicable):
nss-3.36.0-1.0.fc28.x86_64
nss-3.36.0-1.0.fc27.x86_64

How reproducible:
always

Steps to Reproduce:
1. create SQL NSSDB
2. import p12 file with pk12util
3. import same public cert with certutil

Actual results:
The nickname of the cert/key pair is changed

Expected results:
Second import with certutil shouldn't change the nick

Additional info:

$ mkdir dbm
$ certutil -d dbm:dbm -f pwdfile.txt -N
$ pk12util -i cert.p12 -k pwdfile.txt -w pwdfile.txt -d dbm:dbm
pk12util: PKCS12 IMPORT SUCCESSFUL
$ certutil -d dbm:dbm -f pwdfile.txt -L

Certificate Nickname                                         Trust Attributes
                                                             SSL,S/MIME,JAR/XPI

my-ca-cert                                                   u,u,u
$ certutil -d dbm:dbm -f pwdfile.txt -K
certutil: Checking token "NSS Certificate DB" in slot "NSS User Private Key and Certificate Services"
< 0> rsa      acdf483f38fd167fe29565ab767b7a912d93aeae   my-ca-cert
$ certutil -d dbm:dbm -f pwdfile.txt -A -n ca-pem -t ,, -i cert.pem
$ certutil -d dbm:dbm -f pwdfile.txt -L

Certificate Nickname                                         Trust Attributes
                                                             SSL,S/MIME,JAR/XPI

my-ca-cert                                                   u,u,u
^^^^^^^^^^ NICK STAYS THE SAME

$ mkdir sql
$ certutil -d sql:sql -f pwdfile.txt -N
$ pk12util -i cert.p12 -k pwdfile.txt -w pwdfile.txt -d sql:sql
pk12util: PKCS12 IMPORT SUCCESSFUL
$ certutil -d sql:sql -f pwdfile.txt -L

Certificate Nickname                                         Trust Attributes
                                                             SSL,S/MIME,JAR/XPI

my-ca-cert                                                   u,u,u
$ certutil -d sql:sql -f pwdfile.txt -K
certutil: Checking token "NSS Certificate DB" in slot "NSS User Private Key and Certificate Services"
< 0> rsa      acdf483f38fd167fe29565ab767b7a912d93aeae   my-ca-cert

$ certutil -d sql:sql -f pwdfile.txt -A -n ca-pem -t ,, -i cert.pem
$ certutil -d sql:sql -f pwdfile.txt -L

Certificate Nickname                                         Trust Attributes
                                                             SSL,S/MIME,JAR/XPI

ca-pem                                                       u,u,u
^^^^^^^^^^ NICK IS CHANGED

Comment 1 Christian Heimes 2018-04-25 16:13:48 UTC
New FreeIPA upstream issue looks related to this bug, https://pagure.io/freeipa/issue/7516

Comment 2 Christian Heimes 2018-04-29 15:29:15 UTC
I spent some time in gdb. Eventually I figured out that 
sdb_SetAttributeValue treats CKA_LABEL (0x3) differently for CKO_CERTIFICATE (0x1) than lg_SetAttributeValue. The sdb_SetAttributeValue() function of sqlite storage simply issues an "UPDATE nssPublic SET a3=$label WHERE id=$id;".

The implementation for the legacy database is much more complicated. The call ends up in nsslowcert_AddPermNickname(). The function only changes the nickname if entry->nickname == NULL. In case the name name is already set, it calls AddNicknameToPermCert(dbhandle, cert, entry->nickname). Please note that the call does not use the nickname parameter with the new nick, but re-uses entry->nickname.

Call stack:

#0  nsslowcert_AddPermNickname (dbhandle=0x675730, cert=0x68e5d0, nickname=0x67e510 "ca-pem") at ../../lib/softoken/legacydb/pcertdb.c:3236
#1  0x00007ffff5cd5719 in lg_SetCertAttribute (obj=0x688e10, type=3, value=0x633450, len=6) at ../../lib/softoken/legacydb/lgattr.c:1512
#2  0x00007ffff5cd5ca5 in lg_SetSingleAttribute (obj=0x688e10, attr=0x7fffffffcc50, writePrivate=0x7fffffffcb1c)
    at ../../lib/softoken/legacydb/lgattr.c:1719
#3  0x00007ffff5cd5dcc in lg_SetAttributeValue (sdb=0x676e80, handle=1044714102, templ=0x7fffffffcc50, count=1)
    at ../../lib/softoken/legacydb/lgattr.c:1758
#4  0x00007ffff61ebc1d in sftkdb_setAttributeValue (arena=0x67e610, handle=0x63a9c0, db=0x676e80, objectID=1044714102, template=0x7fffffffcc50, count=1)
    at ../../lib/softoken/sftkdb.c:1090
#5  0x00007ffff61ec463 in sftkdb_SetAttributeValue (handle=0x63a9c0, object=0x691a10, template=0x7fffffffcc50, count=1) at ../../lib/softoken/sftkdb.c:1393
#6  0x00007ffff61e3d41 in sftk_forceTokenAttribute (object=0x691a10, type=3, value=0x633450, len=6) at ../../lib/softoken/pkcs11u.c:521
#7  0x00007ffff61e3e36 in sftk_forceAttribute (object=0x691a10, type=3, value=0x633450, len=6) at ../../lib/softoken/pkcs11u.c:546
#8  0x00007ffff61cf9ee in NSC_SetAttributeValue (hSession=16777219, hObject=3192197750, pTemplate=0x7fffffffcde0, ulCount=2)
    at ../../lib/softoken/pkcs11.c:4620
#9  0x00007ffff7baa66f in nssCKObject_SetAttributes (object=3192197750, obj_template=0x7fffffffcde0, count=2, session=0x688cb0, slot=0x684800)
    at ../../lib/dev/ckhelper.c:219
#10 0x00007ffff7bad15a in nssToken_ImportCertificate (tok=0x683de0, sessionOpt=0x0, certType=NSSCertificateType_PKIX, id=0x68f3d8, 
    nickname=0x633450 "ca-pem", encoding=0x68f3e8, issuer=0x68f3f8, subject=0x68f408, serial=0x68f418, email=0x0, asTokenObject=1)
    at ../../lib/dev/devtoken.c:534
#11 0x00007ffff7b5c5e9 in PK11_ImportCert (slot=0x67f5b0, cert=0x68eac0, key=0, nickname=0x633450 "ca-pem", includeTrust=0)
    at ../../lib/pk11wrap/pk11cert.c:1089
#12 0x000000000040a7c4 in AddCert (slot=0x67f5b0, handle=0x680d60, name=0x633450 "ca-pem", trusts=0x7fffffffd90e ",,", certDER=0x7fffffffd0e0, 
    emailcert=0, pwdata=0x7fffffffd0d0) at ../../cmd/certutil/certutil.c:129
#13 0x0000000000413f16 in certutil_main (argc=12, argv=0x7fffffffd3f8, initialize=1) at ../../cmd/certutil/certutil.c:3693
#14 0x0000000000414701 in main (argc=12, argv=0x7fffffffd3f8) at ../../cmd/certutil/certutil.c:3880

Comment 3 Christian Heimes 2018-04-29 15:31:59 UTC
Created attachment 1428429 [details]
Patch to keep existing nickname in SQL layer

The attached patch fixes the issue for me.

(Note, sqlite3_mprintf() does not implement positional printf arguments like %1$x. I had to hard-code a3.)

Comment 4 Christian Heimes 2018-04-29 19:55:08 UTC
Created attachment 1428556 [details]
Patch to keep existing nickname in SQL layer (2)

The attached patch fixes the issue for me.

Second version only pins nick names of certs. This fixes a problem with private key assignment to certs.

(Note, sqlite3_mprintf() does not implement positional printf arguments like %1$x. I had to hard-code a3.)

Comment 5 Bob Relyea 2018-04-30 16:23:47 UTC
Hmm, the sql-lite behavior is correct. DBM does not handle nicknames well. When a new nickname is written, dbm ignores it. All other PKCS #11 modules accept the new nickname.

The question is what code is depending on the broken behavior?

Comment 6 Bob Relyea 2018-04-30 16:32:43 UTC
Thinking about this a bit, is IPA using the actual pk12util program, or is the pk12util/certutil combination something that is simulating something that IPA is doing programmatically? If it's the former, I think we can add an option to pk12util that tells it to ignore the nickname supplied by the .p12 file.

Comment 7 Bob Relyea 2018-04-30 16:33:49 UTC
Comment on attachment 1428556 [details]
Patch to keep existing nickname in SQL layer (2)

We don't want to change the sql-lite db behavior because that's the correct behavior. We'll need to fix this issue at a different level.

Comment 8 Bob Relyea 2018-04-30 18:17:25 UTC
So at the very least, there's a bug in PK11_ImportCertificate (well actually in nssToken_ImportCertificate()), where we are explicitly setting the label to NULL if we are importing a new certificate with a NULL nickname (and explicitly changing the nickname to NULL if the cert already exists). 

I'll include a patch for that. Also certutil requires -n to be supplied when -A is specified. -E, however, functions just like -A except it doesn't require a nickname to be specified (-E is AddEmailCert, though it doesn't actually require an email certificate).

Option 1:
1) At the very least nssToken_ImportCertificate needs to not set CKA_LABEL if the nickname is NULL.
2) Potentially certutil could drop the -n requirement.
3) We we just do this IPA would have to either change the certutil scripts to drop the nickname on import and if we don't do '2', then they would also have to change their '-A' to '-E'.

NOTE: this means if the cert isn't already in the database, it will be imported without a nickname at all. We really need to understand the actual IPA use case to see whether this is OK or not.

Option 2:
1) We change nssToken_ImportCertificate to not change the nickname when we import a certificate. NOTE: we also try to change the CKA_ID as well. This code is both very deliberate, and would have no effect on the old DBM database. It would change the semantic when importing certificates into a token.

Option 3:
Same as option 2 except we allow CKA_ID and nickname to be changed they are NULL entries.

Comment 9 Bob Relyea 2018-04-30 18:24:47 UTC
Forgot to put the question for Kai.

Which option do you prefer.
Option 1: requires some work for the CS team. Their use case is they are importing a root cert (which doesn't need a nickname), so there shouldn't be any issues with the import without a nickname. If we choose option one, we need to decide if we wan't to remove the -n option from certutil -A or just have IPA use the certutil -E option.

Option3: requires no work from the CS team. There may be some corner cases around tokens which will change their behavior. We can still recover from certs with no nickname (we can still use certutil -A to supply them nicknames), but we can't use certutil -A to change their nicknames. certutil --rename can handle the nickname rename case.

In all options tokens and sql-lite DB will have the same semantics.

I'll attach patches to both option 1 and option 3.

Comment 10 Bob Relyea 2018-04-30 22:35:42 UTC
Created attachment 1428962 [details]
Option 3 implemented

This passes all.sh. Changing anything with CKA_ID causes OCSP to fail.
Option 1 will look very much like this patch.

With this patch:
$ rm sql/*
$ certutil -d sql:sql -f pwdfile.txt -N
$ pk12util -i cert.p12 -k pwdfile.txt -w pwdfile.txt -d sql:sql
pk12util: PKCS12 IMPORT SUCCESSFUL
$ certutil -d sql:sql -f pwdfile.txt -L

Certificate Nickname                                         Trust Attributes
                                                             SSL,S/MIME,JAR/XPI

my-ca-cert                                                   u,u,u
$  certutil -d sql:sql -f pwdfile.txt -K
certutil: Checking token "NSS Certificate DB" in slot "NSS User Private Key and Certificate Services"
< 0> rsa      acdf483f38fd167fe29565ab767b7a912d93aeae   my-ca-cert
$ certutil -d sql:sql -f pwdfile.txt -A -n ca-pem -t ,, -i cert.pem
$ certutil -d sql:sql -f pwdfile.txt -L

Certificate Nickname                                         Trust Attributes
                                                             SSL,S/MIME,JAR/XPI

my-ca-cert                                                   u,u,u

But also:
$ rm sql/*
$ certutil -d sql:sql -f pwdfile.txt -N
$ certutil -d sql:sql -f pwdfile.txt -E -t ,, -i cert.pem
$ certutil -L -d sql:sql

Certificate Nickname                                         Trust Attributes
                                                             SSL,S/MIME,JAR/XPI

(NULL)                                                       ,,   
$ pk12util -i cert.p12 -k pwdfile.txt -w pwdfile.txt -d sql:sql
$ certutil -L -d sql:sql

Certificate Nickname                                         Trust Attributes
                                                             SSL,S/MIME,JAR/XPI

my-ca-cert                                                   u,u,u


So we are able to recover from a NULL nickname, and...
$ certutil --rename -n my-ca-cert --new-n your-ca-cert -d sql:sql
$ certutil -L -d sql:sql

Certificate Nickname                                         Trust Attributes
                                                             SSL,S/MIME,JAR/XPI

your-ca-cert                                                 u,u,u

... rename still works correctly for sql-lite databases. (NOTE: rename still does not work for dbm databases, but that's a limitation of the old dbm database).
$ certutil --rename -n my-ca-cert --new-n your-ca-cert -d dbm:dbm
[rrelyea@rrelyea-laptop dup_reproducer]$ certutil -L -d dbm:dbm

Certificate Nickname                                         Trust Attributes
                                                             SSL,S/MIME,JAR/XPI

my-ca-cert                                                   u,u,u

Comment 11 Bob Relyea 2018-04-30 22:46:48 UTC
Created attachment 1428963 [details]
Option 1 implemented

The simularity of this patch to option 3 is pretty clear. The only difference is we unconditionally set the nickname if the nickname is supplied, so:

$ certutil -L -d sql:sql

Certificate Nickname                                         Trust Attributes
                                                             SSL,S/MIME,JAR/XPI

your-ca-cert                                                 u,u,u
$ certutil -d sql:sql -f pwdfile.txt -E -t ,, -i cert.pem
$  certutil -L -d sql:sql

Certificate Nickname                                         Trust Attributes
                                                             SSL,S/MIME,JAR/XPI

your-ca-cert                                                 u,u,u

...but....

$ certutil -d sql:sql -f pwdfile.txt -A -n ca-pem -t ,, -i cert.pem
$ certutil -L -d sql:sql

Certificate Nickname                                         Trust Attributes
                                                             SSL,S/MIME,JAR/XPI

ca-pem                                                       u,u,u

With this option, the ipa scripts would need to change '-A -n {nickname}' to '-E
'.

Comment 12 Bob Relyea 2018-04-30 22:51:52 UTC
Created attachment 1428964 [details]
Option to remove the '-n' requirement from AddCert (-A)

This patch could be added if we select option 1. If this patch is added, only the -n {nickname} in the ipa scripts would have to change.

Since we have a way of doing this '-E', it's not strictly necessary, but there is a clarity argument that says specifying 'add email cert' isn't the obvious way of importing a certificate without a nickname (though that's exactly what it is).

On the other hand, certs in a hand managed database is difficult to deal with if they don't have nicknames, so removing the requirement for -n may not be desirable.

Comment 13 Bob Relyea 2018-04-30 22:52:27 UTC
My personal recommendation is option 3.

Comment 14 Bob Relyea 2018-04-30 22:53:25 UTC
Comment on attachment 1428962 [details]
Option 3 implemented

Kai if you prefer option 3, r+ this patch.

Comment 15 Bob Relyea 2018-04-30 22:54:16 UTC
Comment on attachment 1428963 [details]
Option 1 implemented

Kai, if you prefer option 1, r+ this patch.

Comment 16 Bob Relyea 2018-04-30 22:56:32 UTC
Comment on attachment 1428964 [details]
Option to remove the '-n' requirement from AddCert (-A)

Kai, If you select option 1, and also want certutil to be updated so that you don't need -n on the -A option, r+ this patch.

NOTE: I currently don't recommend this patch.

Comment 17 Christian Heimes 2018-05-01 12:08:15 UTC
I'm in favor of option (3), too.

FreeIPA's test suite is passing with NSS build nss-3.36.1-1.0.0.option3 without additional modification.

Comment 18 Bob Relyea 2018-05-01 15:03:52 UTC
Thanks Christian,

Does nss-3.36.1-1.0.0.option1 also work if you change the -A -n cert_pem to -E in the certutil import?

bob

Comment 19 Christian Heimes 2018-05-02 08:37:28 UTC
I did not test option (1). It requires several changes to IPA and Dogtag. I simply didn't have time and resources to track down all places and change code to use -E.
Option (3) makes certutil with SQL database behave certutil with DBM database. Since we are considering the behavior of certutil with shared as a regression, option (3) also resolves the regression.

Comment 20 Kai Engert (:kaie) (inactive account) 2018-05-02 10:59:42 UTC
Bob and Christian, thanks a lot for working on this issue.

It seems you have already agreed that Option 3 is the best fix.

My preference is a fix that doesn't require changes to applications. Because this isn't just about Dogtag, but other applications could run into the same issue. So a fix inside NSS, without requiring application level changes, would be the best fix IMHO.

If option 3 doesn't cause any new regressions, that's fine with me.

(In reply to Bob Relyea from comment #9)
> Option3: requires no work from the CS team. There may be some corner cases
> around tokens which will change their behavior. We can still recover from
> certs with no nickname (we can still use certutil -A to supply them
> nicknames), but we can't use certutil -A to change their nicknames. certutil
> --rename can handle the nickname rename case.
> 
> In all options tokens and sql-lite DB will have the same semantics.

Renaming with -A seems unnecessary anyway, because we have --rename.

I cannot judge the risks that Bob implies with his comment "There may be some corner cases around tokens which will change their behavior.". It sounds like you're willing to accept that risk. 

I'll file an upstream bug report, and Christian suggested to also add an upstream test, I'll look into that.

I've started two runs for the NSS upstream test suite, and the Firefox test suite, to ensure there are no obvious regressions.

https://treeherder.mozilla.org/#/jobs?repo=nss-try&revision=3360eed0cc12b17fffac49cda58687167c85a959

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6053ae67cf3b659793abd7b3169c72c03d4cc741

If things look good, it might make sense to start with a Fedora 28 update that includes this as a downstream patch. This would allow us to test this change more widely, prior to including it in the upstream release or more stable releases.

Comment 21 Christian Heimes 2018-05-02 11:06:12 UTC
Thanks Bob and Kai!

Your proposal sounds like a good approach. I'm fine with a Fedora 28 update with downstream patch.

Comment 22 Kai Engert (:kaie) (inactive account) 2018-05-02 12:14:30 UTC
(In reply to Bob Relyea from comment #10)
> Created attachment 1428962 [details]
> Option 3 implemented
> 
> This passes all.sh. Changing anything with CKA_ID causes OCSP to fail.

It's not clear what this comment means. Reading this first, it sounds like you're saying that your option 3 patch introduces a patch for OCSP.

Comment 23 Kai Engert (:kaie) (inactive account) 2018-05-02 12:15:04 UTC
(In reply to Kai Engert (:kaie) from comment #22)
> (In reply to Bob Relyea from comment #10)
> > Created attachment 1428962 [details]
> > Option 3 implemented
> > 
> > This passes all.sh. Changing anything with CKA_ID causes OCSP to fail.
> 
> It's not clear what this comment means. Reading this first, it sounds like
> you're saying that your option 3 patch introduces a patch for OCSP.

"introduces a regression".

Does option 3 introduce a regression for OCSP ?

Comment 24 Fedora Update System 2018-05-03 07:45:27 UTC
nss-3.36.1-1.1.fc28 has been submitted as an update to Fedora 28. https://bodhi.fedoraproject.org/updates/FEDORA-2018-8cf042000b

Comment 25 Fedora Update System 2018-05-03 20:23:40 UTC
nss-3.36.1-1.1.fc28 has been pushed to the Fedora 28 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2018-8cf042000b

Comment 26 Fedora Update System 2018-05-09 21:24:24 UTC
nss-3.36.1-1.1.fc28 has been pushed to the Fedora 28 stable repository. If problems still persist, please make note of it in this bug report.

Comment 27 Bob Relyea 2018-05-30 18:19:28 UTC
> It's not clear what this comment means. Reading this first, it sounds like
> you're saying that your option 3 patch introduces a patch for OCSP.

Sorry, no. It means If I carry the same semantic for CKA_ID as I did for CKA_LABEL, then we have failurs in the OCSP tests, so I only made the change for CKA_LABEL in my patch, to reduce regressions.


Note You need to log in before you can comment on or make changes to this bug.