376737 - CERT_ImportCerts routinely sets VALID_PEER or VALID_CA OVERRIDE trust flags

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Nelson Bolyard (seldom reads bugmail)]

Revision 1.23 of certdb/certdb.c claims that it was committed to fix 
bug 122720 and bug 121487.  See 
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/security/nss/lib/certdb&command=DIFF_FRAMESET&file=certdb.c&rev2=1.23&rev1=1.22

That change had the effect that any certs imported by CERT_ImportCerts have
the VALID_PEER or VALID_CA trust flags set.  

Those trust flags are OVERRIDES.  They cause a lot of cert validity checking 
to be skipped during cert verification.  One consequence of setting the 
VALID_CA trust bit is that it turns any intermediate CA into a valid object 
signing CA, completely defeating the logic that checks for an EKU extension 
to enable object signing.

These OVERRIDE trust bits should NEVER be set routinely.  They should ONLY 
be set when necessary to make a badly constructed cert work. 

So, this "fix" actually introduced a serious bug into NSS, turning all
imported CA certs into object signing CA certs.  

The original problem of PSM not displaying some CA certs should have been
fixed without necessitating that the VALID_CA override be set in all CA
certs.  Perhaps PSM remains dependent on this incorrect behavior.
It may be necessary to fix PSM when this NSS bug is fixed.

This NSS flaw needs to be fixed.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

It appears to me that the function CERT_ImportCerts was originally intended 
to be used to permanently import ONLY trusted and/or known valid certs.  

It appears that, when the function was written, the unstated assumption was 
that, when a cert is being imported "permanently" ("keepcerts" is true), 
the cert is being imported explicitly by the user, specifically to make it 
usable where it otherwise was not usable, such as 
- when importing a user's own certs (and their chain) from a p12 file, or 
- when importing an SSL server cert for the purpose of marking it trusted, or
- when importing a CA cert (e.g. a missing intermediate CA cert, or a root
CA cert (which were often X.509.v1 certs with no extensions).  

Under those circumstances, the user was implicitly expressing his faith in
the validity of the certs he was importing.  Under those circumstances, it 
may have seemed reasonable to force the VALID_{CA,PEER} bits to be set. 

But that assumption was unstated, and since then, the function has become
used in many other places as a general purpose cert importing function.

It is now used in places where the import is "automatic" (done without 
any express user action to trigger the import), and where the certs may 
not be known to be valid or trustworthy when it is called.  For example, 
it is now used to import any certs and chains found in any signed CMS 
(PKCS#7) messages.  Such places are now vulnerable, because when they 
import invalid certs, those cert become valid by virtue of these overrides.

Consider the difference in intent between (a) the user deliberately 
importing an email cert chain for a recipient with whom he wishes to 
correspond, say from a DER file or from an LDAP server, and (b) the 
automatic import of encryption cert chain found in a signed SMIME email.

Perhaps it can be argued in the first of those cases that the purpose
of the import clearly presumes the validity of the certs involved, but
IMO that cannot be said of the second (automatic) case.  

I devised a patch that would cease to set VALID_CA/VALID_PEER flags on
any imported certs, EXCEPT in the case of imported "user" certs.  But 
I am concerned that this may break existing products that depend (perhaps 
unwittingly) on that behavior.  

So I think the two choices of direction for fixing this bug are:

a) change CERT_SaveImportedCert (and hence CERT_ImportCerts) to no 
longer set the valid CA flag except for user certs, (and perhaps X.509.v1
root CA certs).  or 

b)  Create a new variant of CERT_ImportCerts that does not set those 
trust bits (does not call CERT_SaveImportedCert) and, change the places 
that do "automatic" imports of certs (such as in PKCS#7 and CMS) to 
call the new function that does not set any VALID trust flags, instead
of calling the old one that does set those bits.  

Thoughts?  Any other suggestions?

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

This MUST get fixed, in NSS *AND* in PSM for 3.12.  
We simply MUST NOT continue to have the valid override flag being set in
all imported certs, as it defeats many of the cert validity checks.  

According to bug 118612, PSM hides any cert whose trust flags are ",,"
and does not display it at all, making it appear to have been deleted.
The bug says this is done to make "deleted" built-in roots appear to be
deleted.  But (if true) that behavior is seriously flawed.  It causes 
all "normal" certs (which inherit trust from their root) to not be 
displayed.  Someone chose to "fix" that by making NSS/PSM set one of the
trust flags to non-zero on every imported cert.  They set the "valid peer"
or "valid ca" bit in the S/MIME trust flag on every imported cert, to 
make those certs appear in PSM.  WHAT A MISTAKE!

This MUST be fixed for NSS 3.12.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

I propose to back out revision 1.23 of certdb/certdb.c and to
remove function CERT_SaveImportedCert from certdb.c and from cert.h.

This may reintroduce the PSM bug that causes it to not display ordinary
imported certs that have no trust flags.  But that problem should be 
fixed by some other means than by overriding cert security for all certs.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Attachment: remove function CERT_SaveImportedCert and its declaration - v1

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

I observe that the VALID_CA trust flag was not consistently used as an
override until NSS 3.9.  That probably explains how this came to be.

Comment 6

[Kai Engert [:KaiE:]]

Nelson, does this block the landing of a NSS 3.12 alpha into Mozilla trunk?
(I added this bug to the list of PSM TODO items related to NSS 3.12.)

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Kai, I think this bug, and PSM bug 350567, are both absolute Must Fix 
blockers for any release that claims to do EV.  That includes any Alpha
or Beta release that claims to show EV.  

Comment 8

[Robert Relyea]

Comment on attachment 266705 [details] [diff] [review]
remove function CERT_SaveImportedCert and its declaration - v1

r+

Comment 9

[Kai Engert [:KaiE:]]

Nelson, do you plan to check this patch in?

Is this bug fixed once you do?
(sorry, didn't read the full bug yet, just asking for a yes or no)

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Back out revision 1.23 of certdb/certdb.c, the "fix" for bug 121487 that 
started setting the valid override flags routinely on all imported certs.
Bug 376737.  r=rrelyea

Checking in cert.h;   new revision: 1.59; previous revision: 1.58
Checking in certdb.c; new revision: 1.80; previous revision: 1.79

I anticipate that there may be some perceived PSM cert manager regressions
due to this change.  I hope we can find and fix them quickly.