595861 - When CERT_AddTempCertToPerm fails with TOKEN_NOT_LOGGED_IN, it *has* added the cert with RANDOM trust bits

Status: ASSIGNED

(NSS :: Libraries, defect, P2)

Description

[David Woodhouse]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-GB; rv:1.9.2.7) Gecko/20100803 Fedora/3.6.7-1.fc13 Firefox/3.6.7
Build Identifier: nss-3.12.6-12.fc13.x86_64

We seem to be required to log in, just to deal with trust on CA certificates.

Discovered in Evolution, where the certificate manager UI started failing unexpectedly. It's even nastier there -- a call to CERT_AddTempCertToPerm() fails, but has actually *succeeded* in adding the cert to the database, just with the wrong permissions. 

See Evolution bug: https://bugzilla.gnome.org/show_bug.cgi?id=626066

Reproducible: Always

Actual Results:  
 (with SQL database)


[dwmw2@i7 tmp]$ certutil -d sql:`pwd` -N
Enter a password which will be used to encrypt your keys.
The password should be at least 8 characters long,
and should contain at least one non-alphabetic character.

Enter new password: 
Re-enter password: 
[dwmw2@i7 tmp]$ certutil -d sql:`pwd` -A -i ../class3.crt -n class3 -t TC,C,C
Enter Password or Pin for "NSS Certificate DB":


Expected Results:  
 (with DBM database)

[dwmw2@i7 tmp]$ certutil -d `pwd` -N
Enter a password which will be used to encrypt your keys.
The password should be at least 8 characters long,
and should contain at least one non-alphabetic character.

Enter new password: 
Re-enter password: 
[dwmw2@i7 tmp]$ certutil -d `pwd` -A -i ../class3.crt -n class3 -t TC,C,C
[dwmw2@i7 tmp]$ certutil -d `pwd` -L

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

class3                                                       CT,C,C

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

That's correct.  Editing the trust in the sqk DB requires a login, because
the trust info is authenticated. 

This bug report says the cert was added with the "wrong" permissions.
I think you mean the "wrong" trust flags.  Without authentication, 
there should be NO new trust flags.  Is that what you saw? 
If so, then this is all working as designed and intended, not a bug.
If not, please tell us what trust flags you saw.

Comment 2

[David Woodhouse]

The trust flags are random, I think.

See the patch at https://bugzilla.gnome.org/show_bug.cgi?id=626066#c18

When we call CERT_AddTempCertToPerm() and it fails with SEC_ERROR_TOKEN_NOT_LOGGED_IN, it *has* added the CA with random trust bits. Authenticating and then calling CERT_AddTempCertToPerm() fails, because the cert already exists. We have to just call CERT_ChangeCertTrust for the newly-added cert.

I've just retested this with the above patches applied. After asking Evolution to import a cert, I get asked for the password to log in to the token. I run certutil at that point, before entering the password, and the cert has *already*
been added. In the latest test it was added as follows, but I've seen various different bits set:
CAcert Class 3 Root - Root CA                                CT,c,C

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

OK, well, I think adding a cert with NO trust bits while unauthenticated
is thought to be acceptable, but adding a cert with RANDOM trust bits 
should never be acceptable, authenticated or otherwise.  

Also, at one time, the act of adding a cert that was already present was
not deemed to be a failure.  The reasoning was "you wanted this cert to 
be in the DB/token, and now it is, so you've succeeded".  If that is no 
longer true, then something has regressed, IMO.

Comment 4

[David Woodhouse]

Indeed. In the meantime, given that I need to fix things for an Evolution release today, I think the best option is this; please advise:

		srv = CERT_AddTempCertToPerm (tmpCert,
					     nickname,
					     &trust);

		/* If we aren't logged into the token, then what *should*
		   happen is the above call should fail, and we should
		   authenticate and then try again. But see NSS bug #595861.
		   With NSS 3.12.6 at least, the above call will fail, but
		   it *will* have added the cert to the database, with
		   random trust bits. We have to authenticate and then set
		   the trust bits correctly. And calling
		   CERT_AddTempCertToPerm() again doesn't work either -- it'll
		   fail even though it arguably ought to succeed (which is
		   probably another NSS bug).
		   So if we get SEC_ERROR_TOKEN_NOT_LOGGED_IN, we first try
		   CERT_ChangeCertTrust(), and if that doesn't work we hope
		   we're on a fixed version of NSS and we try calling
		   CERT_AddTempCertToPerm() again instead.
		*/
		if (srv != SECSuccess &&
		    PORT_GetError() == SEC_ERROR_TOKEN_NOT_LOGGED_IN &&
		    e_cert_db_login_to_slot (NULL, PK11_GetInternalKeySlot())) {
			srv = CERT_ChangeCertTrust (CERT_GetDefaultCertDB (),
						    tmpCert, &trust);
			if (srv != SECSuccess)
				srv = CERT_AddTempCertToPerm (tmpCert,
							      nickname,
							      &trust);
		}
		if (srv != SECSuccess) {
			set_nss_error (error);
			return FALSE;
		}

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

David, 
At present, I can't extemporaneously advise you on the "best" workaround.
Thinking aloud, if you delete the cert and then re-add it, that might work 
with broken and unbroken versions of NSS.

Comment 6

[David Woodhouse]

Is there any particular reason to prefer that approach over "fix the permissions, and if that fails then someone must have fixed NSS so it wasn't added in the first place, so try adding it again", as in comment 4?

Comment 7

[David Woodhouse]

s/permissions/trust bits/ of course; sorry.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Not really.  Whatever does the job with the widest variety of versions of NSS
and types of NSS DB is probably best.

Comment 9

[Robert Relyea]

re comment 4: 

yes, that's the best way to handle the problem.It will not only work with sql databases, but with other tokens that require auth from the get go.

(I believe this is the code we have in certutil).

bob