625675 - trust flags are not being deleted when we delete the associated certificate

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[u238590]

User-Agent:       Mozilla/5.0 (X11; U; SunOS sun4u; en-US; rv:1.9.2.12) Gecko/20101027 Firefox/3.6.12
Build Identifier: 3.12.8

Kishore from our team found this bug. 

When cert-key pair is imported in the DBs (attached), the certificate gets imported with "CTu,u,u" flags. But when a new cert9 DB is created and the same certificate key pair is imported, it shows trust flag as "u,u"u,"

This is happening only when we are using cert9.db with cert8.db this problem didn't exist. Can somebody confirm if its a cert9* ONLY bug? 

Reproducible: Always

Steps to Reproduce:
1. Copy ert9.db etc. NSS DBs (attached in this bug)
2. $certutil -L -d "sql:."
3. $pk12util -i keyandcert.p12 -h internal -d sql:. -W changeit -K 88888888
4. $certutil -L -d "sql:."
----------- AND ---------------
1. $rm *.db pkcs11.txt
2. certutil -N -d "sql:."
3. $pk12util -i keyandcert.p12 -h internal -d "sql:." -W changeit
4. $certutil -L -d "sql:."
Actual Results:  
When using the NSS DBs attached in this bug :

$certutil -L -d "sql:."
Certificate Nickname                               Trust Attributes
                                                   SSL,S/MIME,JAR/XPI
keytypeTest002                                     u,u,u
keytypeTest005                                     u,u,u
keytypeTest006                                     u,u,u
keytypeTest007                                     u,u,u
keytypeTest008                                     u,u,u
keytypeTest009                                     u,u,u
keytypeTest010                                     u,u,u
keytypeTest015                                     u,u,u
keytypeTest016                                     u,u,u
keytypeTest018                                     u,u,u
keytypeTest019                                     u,u,u
keytypeTest020                                     u,u,u

$pk12util  -i keyandcert.p12 -h internal -d sql:. -W changeit -K 88888888
pk12util: PKCS12 IMPORT SUCCESSFUL

$certutil -L -d "sql:." 
Certificate Nickname                               Trust Attributes
                                                   SSL,S/MIME,JAR/XPI
keytypeTest002                                     u,u,u
keytypeTest005                                     u,u,u
keytypeTest006                                     u,u,u
keytypeTest007                                     u,u,u
keytypeTest008                                     u,u,u
keytypeTest009                                     u,u,u
keytypeTest010                                     u,u,u
keytypeTest015                                     u,u,u
keytypeTest016                                     u,u,u
keytypeTest018                                     u,u,u
keytypeTest019                                     u,u,u
keytypeTest020                                     u,u,u
CN=Server,OU=JWS,O=SUN,ST=Some-State,C=AU          CTu,u,u

The last certificate got imported with "CTu,u,u" flags.
---------------------------------------------------------
But when new cert9.db etc. NSS DB are created and the same certificate key pair is imported, trust flags are "u,u"u,"

$rm *.db pkcs11.txt
$certutil -N -d "sql:."
$pk12util  -i keyandcert.p12 -h internal -d sql:. -W changeit
$certutil -L -d "sql:." 
Certificate Nickname                               Trust Attributes
                                                   SSL,S/MIME,JAR/XPI

CN=Server,OU=JWS,O=SUN,ST=Some-State,C=AU          u,u,u

Expected Results:  
Trust flags in both the cases should be the same.

I debugged whatever I can :

SCENARIO : When I used attached cert9.db :
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
http://mxr.mozilla.org/security/source/security/nss/lib/pki/trustdomain.c#1096

In the function  nssTrustDomain_FindTrustForCertificate, it executes line#1116 and returns "to"  NON NULL.  It goes to line #1124 and creates pkio object. then it goes to line #1137 and calls "nssTrust_create" function.


t@1 (l@1) stopped in nssTrustDomain_FindTrustForCertificate at line 1146 in file "trustdomain.c"
 1146       return rvt;
(dbx) p *rvt
*rvt = {
...
    serverAuth      = nssTrustLevel_TrustedDelegator
    clientAuth      = nssTrustLevel_TrustedDelegator
    emailProtection = nssTrustLevel_NotTrusted
    codeSigning     = nssTrustLevel_NotTrusted
    stepUpApproved  = 0
}
...
(dbx) n
t@1 (l@1) stopped in nssTrust_GetCERTCertTrustForCert at line 610 in file "pki3hack.c"
  610           rvTrust = cert_trust_from_stan_trust(t, cc->arena);
(dbx) n
t@1 (l@1) stopped in nssTrust_GetCERTCertTrustForCert at line 611 in file "pki3hack.c"
  611           if (!rvTrust) {
(dbx) p *rvTrust
*rvTrust = {
    sslFlags           = 152U
    emailFlags         = 0
    objectSigningFlags = 0
}
...
t@1 (l@1) stopped in nssTrust_GetCERTCertTrustForCert at line 623 in file "pki3hack.c"
  623       if (NSSCertificate_IsPrivateKeyAvailable(c, NULL, NULL)) {
(dbx) n
t@1 (l@1) stopped in nssTrust_GetCERTCertTrustForCert at line 624 in file "pki3hack.c"
  624           rvTrust->sslFlags |= CERTDB_USER;
(dbx) n
t@1 (l@1) stopped in nssTrust_GetCERTCertTrustForCert at line 625 in file "pki3hack.c"
  625           rvTrust->emailFlags |= CERTDB_USER;
(dbx) n
t@1 (l@1) stopped in nssTrust_GetCERTCertTrustForCert at line 626 in file "pki3hack.c"
  626           rvTrust->objectSigningFlags |= CERTDB_USER;
(dbx) n
(dbx) p *rvTrust
*rvTrust = {
    sslFlags           = 216U
    emailFlags         = 64U
    objectSigningFlags = 64U
}


SCENARIO : Newly created cert9.db :
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

In the function  nssTrustDomain_FindTrustForCertificate, it executes line#1116 and returns "to"  is NULL.  So pkio is always NULL, so it never goes to nssTrustCreate() at line#1137. this function returns rvt=NULL.


(dbx) n
t@1 (l@1) stopped in nssTrustDomain_FindTrustForCertificate at line 1146 in file "trustdomain.c"
 1146       return rvt;
(dbx) p rvt
rvt = (nil)
...
t@1 (l@1) stopped in nssTrust_GetCERTCertTrustForCert at line 609 in file "pki3hack.c"
  609       if (t) {
(dbx) p t
t = (nil)
(dbx) n
t@1 (l@1) stopped in nssTrust_GetCERTCertTrustForCert at line 617 in file "pki3hack.c"
  617           rvTrust = PORT_ArenaAlloc(cc->arena, sizeof(CERTCertTrust));
(dbx) n
t@1 (l@1) stopped in nssTrust_GetCERTCertTrustForCert at line 618 in file "pki3hack.c"
  618           if (!rvTrust) {
(dbx) n
t@1 (l@1) stopped in nssTrust_GetCERTCertTrustForCert at line 621 in file "pki3hack.c"
  621           memset(rvTrust, 0, sizeof(*rvTrust));
(dbx) p  *rvTrust
*rvTrust = {
    sslFlags           = 0
    emailFlags         = 0
    objectSigningFlags = 0
}
(dbx) n
t@1 (l@1) stopped in nssTrust_GetCERTCertTrustForCert at line 623 in file "pki3hack.c"
  623       if (NSSCertificate_IsPrivateKeyAvailable(c, NULL, NULL)) {
(dbx) n
t@1 (l@1) stopped in nssTrust_GetCERTCertTrustForCert at line 624 in file "pki3hack.c"
  624           rvTrust->sslFlags |= CERTDB_USER;
(dbx) n
t@1 (l@1) stopped in nssTrust_GetCERTCertTrustForCert at line 625 in file "pki3hack.c"
  625           rvTrust->emailFlags |= CERTDB_USER;
(dbx) n
t@1 (l@1) stopped in nssTrust_GetCERTCertTrustForCert at line 626 in file "pki3hack.c"
  626           rvTrust->objectSigningFlags |= CERTDB_USER;
(dbx) n
(dbx) p *rvTrust
*rvTrust = {
    sslFlags           = 64U
    emailFlags         = 64U
    objectSigningFlags = 64U
}

Comment 1

[u238590]

Attachment: This tar file contains the NSS DBs(cert9.db, key4.db, pkcs11.txt), and keycertpair to be imported keyandcert.p12

Comment 2

[u238590]

Also certutil -K -d . returned no keys on the DBs I have attached. Probably because certutil -T was run on this DB before this.

But still trust flags of these certs was "u,u,u" in the certutil -L output. This is happening only in cert9.db. I think this is one more bug.

In cert8.db I can see ",," as expected :
  $certutil -L -d ...
   Certificate Nickname            Trust Attributes SSL,S/MIME,JAR/XPI
   keytypeTest002                                 ,,
   keytypeTest005                                 ,,
   keytypeTest007                                 ,,
   keytypeTest009                                 ,,
   keytypeTest010                                 ,,
   keytypeTest018                                 ,,
   keytypeTest020                                 ,,
   keytypeTest006                                 ,,
   keytypeTest008                                 ,,
   keytypeTest015                                 ,,
   keytypeTest016                                 ,,
   keytypeTest019                                 ,,

Comment 3

[Alexei Volkov]

I also see that certutil -T command ruins cert and/or key database as I was not able to open it after running this command:

$ echo $NSS_DEFAULT_DB_TYPE 
sql
$ certutil -d nss -L

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

TestCA                                                       u,u,u
$ certutil -d nss -T
$ certutil -d nss -L
certutil: function failed: security library: bad database.

Comment 4

[u238590]

Looking at Alexei's comments above, I tried to find out if the trust flag problem is related to DB corruption by certutil -T or not. The answer is "no".

When I removed all "certutil -T" test cases, in cert9.db, the certificate with nickname "CN=Server,OU=JWS,O=SUN,ST=Some-State,C=AU" gets added with "CTu,u,u" trust flags. certutil -T wasn't executed and hence keys were not deleted. so for the rest of the certs "u,u,u" flag is ok in this case.

$certutil -L -d sql:after

Certificate Nickname                                         Trust Attributes
                                                             SSL,S/MIME,JAR/XPI
keytypeTest002                                               u,u,u
keytypeTest005                                               u,u,u
keytypeTest006                                               u,u,u
keytypeTest007                                               u,u,u
keytypeTest008                                               u,u,u
keytypeTest009                                               u,u,u
keytypeTest010                                               u,u,u
keytypeTest015                                               u,u,u
keytypeTest016                                               u,u,u
keytypeTest018                                               u,u,u
keytypeTest019                                               u,u,u
keytypeTest020                                               u,u,u
CN=Server,OU=JWS,O=SUN,ST=Some-State,C=AU                    CTu,u,u

Comment 5

[u238590]

correction : I meant to say that the problem in Description is not due to DB corruption by "certutil -T". But the problem in comment#2 could be due to that I am not sure.

Comment 6

[u238590]

Test case to reproduce the problem
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

$rm cert9.db key4.db pkcs11.txt
$certutil -N -d "sql:."
$certutil -A -d "sql:." -n s1as  -i s1as.cert -t "CT,,"

At this point certutil output was:
Certificate Nickname                                         Trust Attributes
                                                             SSL,S/MIME,JAR/XPI

s1as                                                         CT,,

Now delete the cert 
$certutil -D -d "sql:." -n s1as
$pk12util -i test.p12 -d sql:. -W changeit

At this point certutil output is :
Certificate Nickname                                         Trust Attributes
                                                             SSL,S/MIME,JAR/XPI

CN=Server,OU=JWS,O=SUN,ST=Some-State,C=AU                    CTu,u,u


This server inherited "CT" flag when it was installed before (and deleted).

Comment 7

[u238590]

Attachment: file test.p12

Comment 8

[u238590]

Attachment: s1as.cert

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Comment 7 demonstrates that the trust object is not destroyed when the cert
to which it applies is deleted, and hence is inherited by a subsequently installed cert.  That's VERY BAD.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Confirmed also on Windows.  Bob, please have a look.

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Bob, I think this problem can occur potentially with any cert import. 
certutil masks it by always demanding an explicit trust argument for cert 
import, and always setting the trust on the imported cert explicitly. 
But other programs, and other NSS import functions, may well import certs
without setting the trust, and those certs will "inherit" any left-over 
trust objects from before.  

The questions are:
- is it a bug to leave trust after deleting a trusted cert?
- is it a bug to import a cert and not set trust?

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Meena, Thanks for tracking this down and writing simple steps to reproduce!!

Comment 13

[Robert Relyea]

Sounds exactly like a problem I have Elio looking into right now. Assigning to him.

Comment 14

[Robert Relyea]

The underlying problem: When we delete a certificate, we should delete the trust record associated with that certificate in that token. In the old databases trust records could only be stored with the certificate, so they were automatically deleted even if we didn't explicitly delete them.

bob

Comment 15

[Robert Relyea]

> - is it a bug to leave trust after deleting a trusted cert?

Yes. We should do this automatically if the trust is in the same token as the certificate we are deleting.

> - is it a bug to import a cert and not set trust?

No, It's permissible to import a cert that we already trust (A root CA certificate sent as part of the SSL connection, or as part of a pk12 package that we already trust through another slot, like the built-in roots).

What you didn't ask is '- is it a bug that we don't list free floating trust records?'

I think the answer is yes.

bob

Comment 16

[Elio Maldonado]

(In reply to comment #13)
> Sounds exactly like a problem I have Elio looking into right now. 
In the course o investigating Bug 595988 Bob found out the trust records records remained after a certificated had been deleted. See verification steps at https://bug595988.bugzilla.mozilla.org/attachment.cgi?id=506417

Comment 17

[Alexei Volkov]

Oracle needs this bug fixed in 3.12.10. Web server depends on it to be fixed to ship with cert9 db enable by default.

Comment 18

[Elio Maldonado]

Attachment: Delete cert trust on cert deletion

Comment 19

[Elio Maldonado]

Comment on attachment 513787 [details] [diff] [review]
Delete cert trust on cert deletion

This naive patch does delete trust in the cases provided in the bug report but it isn't quite ready. Among other things, what if nssPKIObject_DeleteStoredObject returns PR_FAILURE?

Comment 20

[Robert Relyea]

Comment on attachment 513787 [details] [diff] [review]
Delete cert trust on cert deletion

r-

The idea looks good, but we need to destroy the trust record only if it lives in the same slots as the certificate itself.

(Also, I'm pretty sure a cert can have multiple trust objects associated with it, this code seems to delete a random one).

bob

Comment 21

[Elio Maldonado]

Attachment: V2 Delete cert trust on cert deletion if slot matches

Comment 22

[Elio Maldonado]

Comment on attachment 517051 [details] [diff] [review]
V2 Delete cert trust on cert deletion if slot matches

will do some failure testing next.

Comment 23

[Elio Maldonado]

Attachment: V3 Delete trust on cert deletion on matching slot

Comment 24

[Robert Relyea]

Comment on attachment 517151 [details] [diff] [review]
V3 Delete trust on cert deletion on matching slot

r- You need to use the stan cert's instance list to determine which trust records to delete. I believe NSSCertificate_DeleteStoredObject will delete all the cert instances stored in the stan instance structure.

We need to filter the trust through all those instances when choosing which trust objects to delete.

Comment 25

[Elio Maldonado]

Attachment: V4 Delete trust on cert deletion on matching slot

Comment 26

[Elio Maldonado]

This is still a work in progress. I have tested this patch on the trunk and the stable branch and also with RHEL 6 and Fedora 14 builds using Fedora's build sytem. I saw a core dump with a scratch debug build for i386 on a Fedora 14 build system machine on the ssl test test suite. Repeated same build done locally with the same tools and configurations, using mock, but I haven't been able to reproduce yet.

Comment 27

[Elio Maldonado]

Now the tests keep passing everywhere, I can't reproduce any core dump in the official build machines either.

Comment 28

[Elio Maldonado]

Comment on attachment 518745 [details] [diff] [review]
V4 Delete trust on cert deletion on matching slot

Since nssCryptokiObject_Destroy(tInstance); doesn't return an error, the if (nssrv != PR_SUCCESS) could be dropped. Refactoring out the inner loop can make the code a bit easier to read.

Comment 29

[Elio Maldonado]

Attachment: V5 delete trust flags on cert deletion

Comment 30

[Robert Relyea]

Comment on attachment 518895 [details] [diff] [review]
V5 delete trust flags on cert deletion

r-

Bug:

In DeleteCertTrustMatchingSlot, PK11_IsReadOnly should be !PK11_IsReadOnly

Style:

STAN_DeleteCertTrustMatchingSlot takes both an NSSCertificate and a CERTCertificate, but never uses the CERTCertificate. You can dispense with that argument.

Comment 31

[Robert Relyea]

Comment on attachment 518895 [details] [diff] [review]
V5 delete trust flags on cert deletion

oops gave the review, but forgot to mark it.

Comment 32

[Elio Maldonado]

(In reply to comment #30)
I made the changes requested in Comment 30. When I reran the test from Comment 6 it failed. It was passint with earlier faulty versions of the patch. If instead of calling the nssCryptokiObject_Destroy(instance) I call nssToken_DeleteStoredObject(instance); the trust flags then get deleted.

Comment 33

[Elio Maldonado]

Attachment: V6: Address review comments and use different call to delete instances

Comment 34

[Elio Maldonado]

Attachment: certutil:don't ignore result from SEC_DeletePermCertificate

Comment 35

[Elio Maldonado]

(In reply to comment #32)
The naming was giving me the clue and upon closer inspection I see that nssToken_DeleteStoredObject calls CKAPI(epv)->C_DestroyObject, as we need, whereas nssCryptokiObject_Destroy doesn't.

Comment 36

[Elio Maldonado]

Attachment: certutil - don't ignore result from SEC_DeletePermCertificate

CERT_DestroyCertificate doesn't return a value but SEC_DeletePermCertificate does.

Comment 37

[Elio Maldonado]

One step forward, two steps back. Bob pointed out some flaws and this revision doesn't meet our requirements. Failure to delete from one device is not sufficienet reason to bail out and not delete the certificate. We must go on. In stanpcertdb.c we must delete the certificate even if one acn't delete trust from some devices. For pki3hack.c, in the DeleteCertTrustMatchingSlot, when a failure is reported by nssToken_DeleteStoredObject(instance) the code breaks out the loop and it shoudln't. It must continue trying to delete from subsequent ones. This function and it's loop could be modelled after nssPKIObject_DeleteStoredObject wehere a notDeleted counter is kept and the list is modified to keep the survivors in the front. We do have nested loops that might complicate things a bit. New revision coming.

Comment 38

[Elio Maldonado]

Attachment: Patch V7: Delete all objects matching slot even if one fails

Comment 39

[Robert Relyea]

Comment on attachment 519928 [details] [diff] [review]
Patch V7: Delete all objects matching slot even if one fails

r+  rrelyea

But I'd like you to clean up the error handling as follows...

Currently you set the status of nssrv in each of your loops to the last result of nsstokenDeleteStoredObject. Instead you should either use a local variable here, and set nssrv to fail if you ever get an unsuccessful delete.

bob

Comment 40

[Robert Relyea]

Comment on attachment 519928 [details] [diff] [review]
Patch V7: Delete all objects matching slot even if one fails

r- Sorry, There's still a problem.

You need to treat the R/O case and the case where slots aren't equal the same as failure to delete. (with the exception of the error code.)

bob

Comment 41

[Elio Maldonado]

Yes, the nssrv error gets lost if the next device that tries to delete succeeds. Indeed, R/O and slot didn't match are the same and distinct from true failures. An alternative to another status local variable would be to could get rid of the existing one altogether. Add another counter for the true failures and increment  it evry time nssToken_DeleteStoredObject(instance) fails. At the end, return failure if that counter isn't zero. The STAN errors are there on the stack and the outer caller, SEC_DeletePermCertificate, maps the last one placed there.

Comment 42

[Elio Maldonado]

Attachment: V8: Fix error handling

... and hopefully made code intentions clearer

Comment 43

[Elio Maldonado]

Attachment: V8Plus: Fix the outer loop also

Similar medicine for the outer loop. Thank you Bob for the reminder.

Comment 44

[Robert Relyea]

Comment on attachment 522008 [details] [diff] [review]
V8Plus: Fix the outer loop also

r+ but make one change please:

change
	    	nssrv = PR_FALSE;
to

	    	nssrv = PR_FAILURE;

Thanks

bob

Comment 45

[Elio Maldonado]

Patch applied to TRUNK

Checking in certdb/stanpcertdb.c;
/cvsroot/mozilla/security/nss/lib/certdb/stanpcertdb.c,v  <--  stanpcertdb.c
new revision: 1.84.4.2; previous revision: 1.84.4.1
done
Checking in pki/pki3hack.c;
/cvsroot/mozilla/security/nss/lib/pki/pki3hack.c,v  <--  pki3hack.c
new revision: 1.100.2.1; previous revision: 1.100
done
Checking in pki/pki3hack.h;
/cvsroot/mozilla/security/nss/lib/pki/pki3hack.h,v  <--  pki3hack.h
new revision: 1.19.192.1; previous revision: 1.19
done

Comment 46

[Elio Maldonado]

Attachment: The patch as applied

Comment 47

[Elio Maldonado]

(In reply to comment #45)
> Patch applied to TRUNK
That was actually the patch applied to NSS_3_12_BRANCH. 
The patch applied to TRUNK is:
certdb/stanpcertdb.c pki/pki3hack.c pki/pki3hack.h
Checking in certdb/stanpcertdb.c;
/cvsroot/mozilla/security/nss/lib/certdb/stanpcertdb.c,v  <--  stanpcertdb.c
new revision: 1.86; previous revision: 1.85
done
Checking in pki/pki3hack.c;
/cvsroot/mozilla/security/nss/lib/pki/pki3hack.c,v  <--  pki3hack.c
new revision: 1.101; previous revision: 1.100
done
Checking in pki/pki3hack.h;
/cvsroot/mozilla/security/nss/lib/pki/pki3hack.h,v  <--  pki3hack.h
new revision: 1.20; previous revision: 1.19
done