350567 - PSM sets "valid CA" trust flags on CA certs it imports

Status: RESOLVED WONTFIX

(Core :: Security: PSM, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

bug 321156 comment 1 reports that cert chains valid for code signing can 
be made to validate for object signing, just by importing the intermediate 
CA certs into the cert DB as "perm" certs, without setting any trust flags 
on them, even though they lack EKU extensions for object signing.  

The report claims that the "valid CA" trust gets effectively set on all 
permanent CA certs in the cert DB, even if no other trust flags are set.  
This bypasses the EKU check required for object signing cert chains.

I haven't yet confirmed that report, but if true (and others seem to 
confirm it) that is a error in NSS.  I am reluctant to call it a 
vulnerability because the cert chain still must have a valid object 
signing trust anchor.  The only elevation here is from code signing 
to object signing, a distinction not widely understood.  
See bug 321156 for details.

Still, mozilla's XPI installer requires the stronger object signing certs, 
not the weaker code signing ones, so this is potentially an issue for 
signed mozilla extensions.  

Or, maybe the time has come to drop the distinction between code signing
and object signing and use the weaker code signing standard everywhere 
that we now require object signing?  

Work should be done to confirm or refute this bug before we worry about it.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

OK I have investigated this and reproduced it. 
It is not the fault of NSS.  It appears to be caused by PSM.

I took a cert chain of a code signing cert and tested it with vfycert,
testing that chain for object signing usage.  The command was:

   vfychain -v -u 6 -d DB $DIR/cert1.der $DIR/cert2.der

It reported these results:

  Chain is bad, -8156 = Issuer certificate is invalid.
  PROBLEM WITH THE CERT CHAIN:
  CERT 1. CN=Certum Level I,O=Unizeto Sp. z o.o.,C=PL [Certificate Authority]:
    ERROR -8156: Issuer certificate is invalid.

Those were the expected results.
Adding the intermediate CA cert to the cert DB with the command

   certutil -A -n "nickname" -t ",," -d DB -i $DIR/cert2.der

did not add any unexpected trust flags to the CA cert, and did not 
change the behavior of the above vfychain command.  That is also the
expected result.

But if I go into PSM, into the authorities tag, and add the cert that way,
and don't check any of the 3 trust boxes, then when I'm done, the cert
in the cert DB has trust flags "c,c,c".  And if I use that cert DB with
the above vfychain command, it reports that the chain is good.
That's a false elevation of privileges for that cert chain.

So, it must be that PSM is putting the "valid CA" trust flag on CA certs 
when it imports them.   

I will attach two certs that can be used to reproduce this problem.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Attachment: EE code signing cert

This is the code signing (not object signing cert), cert1.der

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Attachment: Intermediate code signing CA cert

This is the intermediate CA cert, cert2.der, which, when imported
by PSM, gets the "valid CA" trust flag set in all 3 usage categories
(e.g. "c,c,c").

Comment 4

[Kai Engert [:KaiE:]]

Nelson, I confirm that PSM uses code that set the "valid CA" flag on imported CA certs.

This code has already been in PSM when I started to work on it.

The code is currently in file nsNSSCertificateDB.cpp and carries my blame.
However, the code in that file got moved, it was previously contained in file nsNSSCertificate.cpp

Here is the last revision of that file that still contained the "SetValidCA" calls  (in its original place) and has some information who added it, when and with what patches.

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/manager/ssl/src/nsNSSCertificate.cpp&rev=1.92

Do you have a proposal?
Under what circumstances should PSM add the "valid CA" flag?

Thanks

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

I think trust flags should only be set by the user, never automatically.
But I haven't yet thought about that in detail for every trust flag.  
I guess it is OK to set it whenever you set the "Trusted CA" flag to 1.
We should discuss this among the PSM/NSS team.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

This bug may have multiple causes.  It appears that one NSS's numerous 
functions for importing certs sets the VALID trust flags.  
See bug 376737.  I will mark bug 376737 as blocking this one, but I 
really only mean to make them each reference the other, to tie them 
together in some way.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Kai, Please mark this bug with whatever priority and severity and other
settings are necessary to make it a stopper for FireFox 3.

The "VALID CA" flag is an OVERRIDE.  It tells NSS to skip most of the 
cert validity checks, and treat the cert as valid, whether it is or not.
It defeats MUCH of the security that should come from valid cert chains.

PSM MUST NOT set it routinely on any certs.  This must be fixed as a 
prerequisite for EV work for FF 3.

According to bug 118612, bug 121487 and bug 122720, this business of 
always setting the "valid" trust flag was done to solve  a problem with 
PSM's cert display in Cert Manager.  Bug 118612 claims that PSM intentionally
"hides" (does NOT display) any cert in Cert Manager that has a trust of ",," 
that is, that has no trust flags set at all.  The bug says this is how PSM 
tries to make "deleted" built-in root certs appear to have been deleted.  

But it causes all "normal" certs (which inherit their trust from their
issuers) to be hidden, which is undesirable.  So, to work around that 
problem, someone decided to set the "valid" trust flag on all imported
certs, so that PSM would display them.  

But (again) the "valid" trust flag is an OVERRIDE.  It tells NSS to skip 
most of the cert validity checks, and treat the cert as valid, whether it 
is or not.  It defeats MUCH of the security that should come from valid 
cert chains.

PSM MUST NOT set it routinely on any certs.  This must be fixed as a 
prerequisite for EV work for FF 3.

Sorry to repeat this, but this is vital.  

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

I have learned that, at one time, PSM did not display a CA cert in the 
Certificate Manager unless the cert had at least ONE trust flag set.  
A CA cert with NO trust flags set (trust == 0) would not be displayed in 
Certificate Manager.  That was to provide a way to make certs from nssckbi
appear to be deleted when a PSM user clicked the delete button.

So, PSM began setting the valid CA flag on newly imported CA certs, to 
ensure that the new certs showed up in the Cert manager list of CA certs.

I do not know if PSM still tries to hide built-in root certs when they are
"deleted" or not.  But if so, it should STOP using the absence of any trust
flags as the condition to hide the cert, and switch to using the NSS trust
flag that exists for this very purpose.

NSS has a trust flag specifically for the purpose of making a certificate
"invisible" (not showing up in cert manager).  NSS does not use the flag
itself.  It merely stores the flag.  PSM should set this flag on certs to 
be hidden, and should honor that flag in cert manager when making the 
display of CA certs.  

The trust flag is CERTDB_INVISIBLE_CA.  It should be set to 1 in all 3 
categories of trust (SSL, email, object signing), or be set to zero in all 3.

Comment 9

[Kai Engert [:KaiE:]]

Please let's try to write a very short pointed summary about the immediate risks, like a description of the worst case.

What's the risk for EV?
Why does an imported CA cert with VALID_CA attached to it causes negative impact for EV?
EV will only be possibly for certs which are shipped as part of the built in module, and have the EV meta information compiled in.
I think imported CA certs will have no effect on that?

Why is the code signing / object signing elevation a risk for EV?
Firefox 3 will only check EV for usage ssl server, so it seems unrelated.

Furthermore, besides the description of the risks, I'm looking for a constructive proposal on how to fix it.
Should PSM simply stop setting any valid_ca flags?
If not, what's the logic? When should it be set, when not?

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Kai, in answer to your question in comment 9,
PSM should never set the VALID_CA override flag automatically.
It should stop doing so immediately. 
Since there is no UI by which a user can do it, it simply should not be done.

The risks are that we construct invalid chains and find them to be valid.
For EV, the risk is both false positives and false negatives.  
For code signing the risk is false positives.

Comment 11

[Kai Engert [:KaiE:]]

I think fixing this bug must go together with a review of all code that does currently set the CERTDB_VALID_CA flag.

I was not involved in the original process to set these flags, so I have no idea what side effects our change could have.

As a reference I'm pasting some snippets:

trust flags and their meaning (from certutil -H):

  p      valid peer
  P      trusted peer (implies p)
  c      valid CA
  T      trusted CA to issue client certs (implies c)
  C      trusted CA to issue server certs (implies c)
  u      user cert
  w      send warning
  g      make step-up cert


PSM code that sets trust flags (from nsNSSCertTrust.h)

  /* equivalent to "c,c,c" */
  void SetValidCA();
  /* equivalent to "C,C,C" */
  void SetTrustedServerCA();
  /* equivalent to "CT,CT,CT" */
  void SetTrustedCA();
  /* equivalent to "p,," */
  void SetValidServerPeer();
  /* equivalent to "p,p,p" */
  void SetValidPeer();
  /* equivalent to "P,P,P" */
  void SetTrustedPeer();
  /* equivalent to "u,u,u" */
  void SetUser();

  /* general setters */
  /* read: "p, P, c, C, T, u, w" */
  void SetSSLTrust(PRBool peer, PRBool tPeer,
                   PRBool ca,   PRBool tCA, PRBool tClientCA,
                   PRBool user, PRBool warn); 

  void SetEmailTrust(PRBool peer, PRBool tPeer,
                     PRBool ca,   PRBool tCA, PRBool tClientCA,
                     PRBool user, PRBool warn);

  void SetObjSignTrust(PRBool peer, PRBool tPeer,
                       PRBool ca,   PRBool tCA, PRBool tClientCA,
                       PRBool user, PRBool warn);

  /* set c <--> CT */
  void AddCATrust(PRBool ssl, PRBool email, PRBool objSign);
  /* set p <--> P */
  void AddPeerTrust(PRBool ssl, PRBool email, PRBool objSign);

Comment 12

[Kai Engert [:KaiE:]]

CA cert gets downloaded and imported:

nsresult
nsNSSCertificateDB::handleCACertDownload(nsIArray *x509Certs,
                                         nsIInterfaceRequestor *ctx)
{
  ...
  nsNSSCertTrust trust;
  trust.SetValidCA();
  trust.AddCATrust(trustBits & nsIX509CertDB::TRUSTED_SSL,
                   trustBits & nsIX509CertDB::TRUSTED_EMAIL,
                   trustBits & nsIX509CertDB::TRUSTED_OBJSIGN);

  SECStatus srv = CERT_AddTempCertToPerm(tmpCert, 
                                         const_cast<char*>(nickname.get()), 
                                         trust.GetTrust()); 
  ...
}


helper function, only called when user uses cert manager's "edit trust" feature:

NS_IMETHODIMP 
nsNSSCertificateDB::SetCertTrust(nsIX509Cert *cert, 
                                 PRUint32 type,
                                 PRUint32 trusted)
{
  ..
  if (type == nsIX509Cert::CA_CERT) {
    // always start with untrusted and move up
    trust.SetValidCA();
    trust.AddCATrust(trusted & nsIX509CertDB::TRUSTED_SSL,
                     trusted & nsIX509CertDB::TRUSTED_EMAIL,
                     trusted & nsIX509CertDB::TRUSTED_OBJSIGN);
    srv = CERT_ChangeCertTrust(CERT_GetDefaultCertDB(), 
                               nsscert,
                               trust.GetTrust());
  } else if (type == nsIX509Cert::SERVER_CERT) {
    // always start with untrusted and move up
    trust.SetValidPeer();
    trust.AddPeerTrust(trusted & nsIX509CertDB::TRUSTED_SSL, 0, 0);
    srv = CERT_ChangeCertTrust(CERT_GetDefaultCertDB(), 
                               nsscert,
                               trust.GetTrust());
  } else if (type == nsIX509Cert::EMAIL_CERT) {
    // always start with untrusted and move up
    trust.SetValidPeer();
    trust.AddPeerTrust(0, trusted & nsIX509CertDB::TRUSTED_EMAIL, 0);
    srv = CERT_ChangeCertTrust(CERT_GetDefaultCertDB(), 
                               nsscert,
                               trust.GetTrust());
  } else {
    // ignore user certs
  }
  ...
}


In particular, when user edits a CA cert:
  var trustssl = (ssl.checked) ? nsIX509CertDB.TRUSTED_SSL : 0;
  var trustemail = (email.checked) ? nsIX509CertDB.TRUSTED_EMAIL : 0;
  var trustobjsign = (objsign.checked) ? nsIX509CertDB.TRUSTED_OBJSIGN : 0;
  certdb.setCertTrust(cert, nsIX509Cert.CA_CERT, 
                      trustssl | trustemail | trustobjsign);

when editing a server cert (from old style server overrides):
  var trustssl = ssl.selected ? nsIX509CertDB.TRUSTED_SSL : 0;
  certdb.setCertTrust(cert, nsIX509Cert.SERVER_CERT, trustssl);

when editing an email cert:
  var trustemail = email.selected ? nsIX509CertDB.TRUSTED_EMAIL : 0;
  certdb.setCertTrust(cert, nsIX509Cert.EMAIL_CERT, trustemail);

Comment 13

[Kai Engert [:KaiE:]]

There are many helper functions defined here.
In order to simplify review, I looked up which of these helper functions are being called by real application logic, and which are merely provided as helper functions.

The following functions are being called from PSM's application logic, so I consider them public:
  void SetValidCA();
  void SetValidServerPeer();
  void SetValidPeer();
  void AddCATrust(PRBool ssl, PRBool email, PRBool objSign);
  void AddPeerTrust(PRBool ssl, PRBool email, PRBool objSign);


The following functions are simply helper functions, or never get called by PSM's application logic, so I consider them private:
  void SetTrustedServerCA();
  void SetTrustedCA();
  void SetTrustedPeer();
  void SetUser();
  void SetSSLTrust(...); 
  void SetEmailTrust(...);
  void SetObjSignTrust(...);


We should review all scenarios where the application uses one of the public functions to define something as "valid".

While the bug subject explicitly talks about CAs only, I suspect the same complaint can be made about peer trust? Nelson, do you agree with this sentence?

Comment 14

[Kai Engert [:KaiE:]]

Attachment: patch to reduce the trust API

This patch doesn't fix the bug.
This patch marks unused/private functions as such, in order to simplify the review.
This patch compiles successfully, this proves my previous comment where I classify public/private functions.

Comment 15

[Kai Engert [:KaiE:]]

So, looking at the code that imports a CA cert from a download location:

(a) it sets the "CERTDB_VALID_CA" flag for all three usages
    (ssl, email, objsign)

(b) if the user decides to give explicit trust (by clicking the checkbox
    in the UI), the code will add the trust flag CERTDB_TRUSTED_CA.


I understand, this bug requests that we should drop (a).


What should happen when the user explicitly assigns trust to a CA cert? Should the code do:
(c) add flag CERTDB_TRUSTED_CA only
(d) set both flags CERTDB_TRUSTED_CA and CERTDB_VALID_CA

(c) or (d) ?

Comment 16

[Kai Engert [:KaiE:]]

Trying to understand things better:

Is it correct to say, CERTDB_VALID_CA can be used to override the "basic constraint is ca = true" flag?


Is there an equivalent cert attribute that CERTDB_VALID_PEER can override?

Is CERTDB_VALID_PEER equivalent to "basic constraint is ca = false"?

Comment 17

[Kai Engert [:KaiE:]]

It appears, when cert manager and the categorization got originally implemented, flags CERTDB_VALID_CA and CERTDB_VALID_PEER was "abused" to store a hint about the type of the cert. This is still being used. When enumerating certs, PSM still uses these flags for deciding about the right tab...

We should change that.
Bob pointed me to cert_GetCertType() and its results which are stored in cert->nsCertType

This still doesn't help for distinguishing whether a cert is a server cert or an email cert, so we'll have to query common name or subject alt names and check whether they look like server names or email addresses... 

Comment 18

[Kai Engert [:KaiE:]]

Attachment: Patch v2

Bob, Nelson, what are your thoughts about the new implementation of function getCertType() ?

It took me several iterations to get to this new patch.

Once I realized what the purpose of that "trust" code really was (not adding trust, but abusing CERTDB_VALID_CA or CERTDB_VALID_PEER flags to indicate a cer type), and after I had the code to use the alternative mechanism suggested by Bob, I started to remove code step by step. Luckily I was able to remove a lot. I had often seen that code in the past and never dared to touch it...

I changed a comparison in nsNSSCertificate::destructorSafeDestroyNSSReference() because in theory, the cached cert type might not yet be available. And we don't need the full type information, only check for the user-cert scenario, which is the most simple test, therefore I introduced a separate function to test for that, without the need to run the other tests.

I removed all calls to SetValidCA, SetValidPeer, SetValidServerPeer.
Those calls had the effect to initialize all trust flags to null, then set some flags. While the implementation used a lot of explicit bool parameters, it really became unnecessary. As soon as I removed the scenario to set CERTDB_VALID_*, only PR_FALSE values were remaining. As the constructor for the trust object sets all members to null, the calls became completely unnecessary. I verified that no other calls happen on the trust object.

I removed several functions that were never called.

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 289429 [details] [diff] [review]
Patch v2

I apologize in advance for what I am about to write. :-/

I reviewed this patch, and it made me rethink about this whole bug.
Maybe I was wrong to state that PSM must not set the valid flags 
upon import.  Maybe I should not have been shocked to discover that
PSM was routinely setting VALID_PEER and VALID_CA flags on import.

I now suspect that the original authors of this code reasoned that the 
ONLY purpose of MANUALLY importing a cert was indeed to effectively 
force it to be valid.  This idea creates a distinction between MANUAL 
import and AUTOMATIC import (such as PSM now does for CA certs received 
in valid SSL server chains).

Historically, one of the reasons to manually import a cert was to force 
it to be valid for some purpose for which it was not valid on its face, 
e.g. to make a cert work as an SSL server cert, or for an email 
correspondent's cert, even if its extensions said it could not be used 
for that purpose.  Perhaps the original authors thought that was the 
ONLY reason to import, and therefore, setting an override flag on all 
manually imported certs was justified.  

I started to think that when I noticed that this patch causes function
getCertType to completely ignore the trust override flags, and to honor 
only the flags that indicate the purposes for which the cert is valid on 
its face (e.g. as indicated by extensions).  It occurred to me that with 
this patch in place, a cert that is not valid on its face as an SSL server 
cert, but which has a VALID_PEER trust flag in the SSL trust category, 
would not show up in the SSL server's tab.  Then I got to thinking: 
how will that VALID_PEER flag ever get set with this patch?
How would a user force an invalid SSL server cert or SMIME email cert
to be considered valid for that purpose.

Then I got to thinking about the new PSM security exception cert store.
As I understand it, these certs will be imported into the cert DB for
storage (is that right?) but we do NOT want to categorically mark them 
valid for all purposes and host names on their faces. So, does the 
valid cert flag want to get set on those certs?  (I think not)

This also makes me wonder: with this patch in place, will a cert that 
is not valid on its face as an SSL server cert, but has been made into
a PSM security exception, show up in the server cert tab?

Have we communicated to PSM users that any cert they import will be 
effectively forced to be a valid cert?  Have we told them "Don't import
any cert that you don't want to treat as valid."?  (I certainly didn't
understand that to be the case, and I work on this stuff!)

Should PSM abandon the whole VALID_CA and VALID_PEER trust flag scheme,
and use a security exception scheme (like it now has for SSL servers) 
for all exceptions, including SMIME certs, code signing, etc?

Or, should PSM provide UI to explicitly control the VALID flags as well 
as the TRUSTED flags?

Finally, I observe that the present design of gerCertType puts each 
cert into exactly one category.  There's no chance for a cert to BOTH 
a valid email cert AND a valid SSL server cert with this interface.
But it is quite possible for a cert to be both.  When a cert is valid
as both an SSL server and an email correspondent, should it show up in
both the "people" and "servers" tabs of PSM cert manager.

I'm sorry that I have only more questions now, not more answers.  :-/

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

Minor addition:  I wrote:
> It occurred to me that with this patch in place, a cert that is not 
> valid on its face as an SSL server cert, but which has a VALID_PEER trust 
> flag in the SSL trust category, would not show up in the SSL server's tab.

I should have written: "... but which has a VALID_PEER or TRUSTED_PEER trust
flag ..."

Comment 21

[Nelson Bolyard (seldom reads bugmail)]

If we decide that it IS desirable for PSM to set a VALID_ trust flag on 
every cert that is MANUALLY imported, that would be a VERY important 
distinction between PSM and NSS's own utilities, such as certutil.  
We would need to somehow make that distinction VERY clear to users of PSM
and of NSS utilities: Importing with one is not necessarily equivalent 
to importing with the other.

Comment 22

[Kai Engert [:KaiE:]]

Nelson, before we dive into ypir new discussion, could please answer the following questions with either yes, no or "don't know"?

(a) Do you still require that this bug blocks the release of Firefox 3?

(b) Do you believe the latest patch makes things worse?

(c) Do you believe the latest patch makes things better?

Comment 23

[Kai Engert [:KaiE:]]

s/ypir/your/

Comment 24

[Kai Engert [:KaiE:]]

Nelson, I have another questions:

(d) Do you believe the latest patch would introduce new side effects?

Comment 25

[Kai Engert [:KaiE:]]

(In reply to comment #19)
> I reviewed this patch, and it made me rethink about this whole bug.
> Maybe I was wrong to state that PSM must not set the valid flags 
> upon import. Maybe I should not have been shocked to discover that
> PSM was routinely setting VALID_PEER and VALID_CA flags on import.

I am inclined to conlude this bug is no longer a P1,
no longer a blocker.


> I now suspect that the original authors of this code reasoned that the 
> ONLY purpose of MANUALLY importing a cert was indeed to effectively 
> force it to be valid.

I don't know what they were thinking.
We are unable to find out what they were thinking.

Let's stick to the effects of this code and the potential
effects of removing it.



> This idea creates a distinction between MANUAL 
> import and AUTOMATIC import (such as PSM now does for CA certs received 
> in valid SSL server chains).

Please let's keep things simple.
Please let's not introduce a new hideen-behing-the-scenes-distinction
based on the original motiviation to import a cert.
We can't predict all possible code pathes.

From my point of view, we should limit the states to what we can see:
- either stored without trust
- or stored with trust

As I understand it from earlier comments, the _VALID_ flags
indicate some sort of override.

For certs that are simply stored, without any indication override,
I agree we should not use _VALID_ flags.

If _VALID_ flags are required to use a cert for a particular purpose,
which might be different from the purpose expressed "on the cert's face",
then I propose we always use _VALID_ flags and _TRUSTED_ flags
in combination.


> Historically, one of the reasons to manually import a cert was to force 
> it to be valid for some purpose for which it was not valid on its face, 
> e.g. to make a cert work as an SSL server cert, or for an email 
> correspondent's cert, even if its extensions said it could not be used 
> for that purpose.  Perhaps the original authors thought that was the 
> ONLY reason to import, and therefore, setting an override flag on all 
> manually imported certs was justified.  

I don't know if that was their thought.

Fact is, they were using these flags to derive a cert type.
Given the existing code, I would tend to assume 
  "lack of knowledge of a better mechanism"
rather than
  "they knew in full detail what they were doing".

Fact is we may be doing imports that are "not manual".


I have no idea whether people import certs to force them to be valid
for a purpose (without adding explict trust).
I may be lacking information, but this is the first time I heard
about this strategy.

If you believe this is a valid strategy, and if it's likely that some
people depend on this strategy to work, then our patch would
introduce side effects for them.

The question is, do we want to allow this as a valid strategy?
Would it be reasonable for us to say: If anyone requires the _VALID_
flag, it's our policy the user must mark the cert as _TRUSTED, too?


> I started to think that when I noticed that this patch causes function
> getCertType to completely ignore the trust override flags, and to honor 
> only the flags that indicate the purposes for which the cert is valid on 
> its face (e.g. as indicated by extensions).  It occurred to me that with 
> this patch in place, a cert that is not valid on its face as an SSL server 
> cert, but which has a VALID_PEER or TRUSTED_PEER trust flag 
> in the SSL trust category, 
> would not show up in the SSL server's tab.  Then I got to thinking: 
> how will that VALID_PEER flag ever get set with this patch?

I won't get set (with the latest patch).


> How would a user force an invalid SSL server cert or SMIME email cert
> to be considered valid for that purpose.

The user can't set it (with the latest patch).

You don't say whether you consider that to be bad.


> Then I got to thinking about the new PSM security exception cert store.
> As I understand it, these certs will be imported into the cert DB for
> storage (is that right?)

yes, right

> but we do NOT want to categorically mark them 
> valid for all purposes and host names on their faces.

correct

> So, does the 
> valid cert flag want to get set on those certs?  (I think not)

I have no idea.


> This also makes me wonder: with this patch in place, will a cert that 
> is not valid on its face as an SSL server cert, but has been made into
> a PSM security exception, show up in the server cert tab?

A security exception will always be listed in the server cert tab.
It doesn't matter whether a cert is around or not.
Display of entries for exceptions is based on the secondary storage
(profile, not cert db).

If PSM is able to find the associated cert in the db, then PSM will
display its detailed information and give the user the option
to view the cert.


> Have we communicated to PSM users that any cert they import will be 
> effectively forced to be a valid cert?

I have no idea. I'm not aware of such a communication.


> Have we told them "Don't import
> any cert that you don't want to treat as valid."?
> (I certainly didn't
> understand that to be the case, and I work on this stuff!)

I think that's a very philosophical question...
Let's keep things simple. We should be able to assume: 
  "If people import a cert using client UI, they want to be able to use it."


> Should PSM abandon the whole VALID_CA and VALID_PEER trust flag scheme,
> and use a security exception scheme (like it now has for SSL servers) 
> for all exceptions, including SMIME certs, code signing, etc?

Please let's not discuss this here and further complicate the discussion.
If you would like to propose/discuss this, please file a separate bug
(unless you find a security issue).

Please keep in mind this bug is flagged as a blocker and requires 
a stable fix very soon.


> Or, should PSM provide UI to explicitly control the VALID flags as well 
> as the TRUSTED flags?

Seems like overkill?
If we didn't understand the distinction until today, how would anybody else?


> Finally, I observe that the present design of gerCertType puts each 
> cert into exactly one category.  There's no chance for a cert to BOTH 
> a valid email cert AND a valid SSL server cert with this interface.
> But it is quite possible for a cert to be both.

Yes, this is a current limitiation.


> When a cert is valid
> as both an SSL server and an email correspondent, should it show up in
> both the "people" and "servers" tabs of PSM cert manager?

Yes, this would be a nice enhancement.
I refused to implement this short term, because it would require
non-trivial changes to the internal data structures used by the tree
in cert manager (one of the hard to read areas).
This kind of change would require that we track delete events
across tabs, in other words, if a cert gets deleted from one tab,
we no longer can immediately remove it, but need to check for the certs
presence in other tabs. This is not trivial, and I don't want to do that
this late in the release game (and we have many other issues that
are more important than this little enhancement).


(In reply to comment #21)
> If we decide that it IS desirable for PSM to set a VALID_ trust flag on 
> every cert that is MANUALLY imported, 

I'm not asking to do it.
Someone else already decided to do it like that (in the past).
You complained it, and now you've become unsure whether to complain at all.

I have no idea whether it is desirable.

Please let's be pragmatic.

If the idea of PSM to set these flags no longer ring a bell for you,
let's keep the code as is.

This is really your call.

You asked that we stop doing it, so please make a decision whether you
still want us to stop doing it, or close this bug as INVALID.

But maybe you still want us to stop doing it, but you are now concerned
about possible side effects?
Concerned that people might report problems once we stop setting the
_VALID_ flags?
If that is your concern, would my proposal to always combine
the _VALID_ and _TRUSTED_ flags sufficiently protect us from regressions?


> that would be a VERY important 
> distinction between PSM and NSS's own utilities, such as certutil.  
> We would need to somehow make that distinction VERY clear to users of PSM
> and of NSS utilities: Importing with one is not necessarily equivalent 
> to importing with the other.

I'm not sure why you are concerned here.
Most end users will never use the NSS tools to import stuff.
If they ever do, and if they intend to mirror PSM's behavior,
they have the change to test and see the effect of a PSM import,
and mirror the same flags when using the NSS tools, no?


I hope my comments can help you answering my 4 questions a, b, c and d.

Please, let's get a summary and simple answers, before we proceed with the complicated discussion.

Comment 26

[Nelson Bolyard (seldom reads bugmail)]

In answer to questions b & c:
This patch does ~4 things:
1. Eliminates a bunch of dead code:  is better
2. Eliminates all/most places that set the VALID_CA/PEER flags:  Don't know
3. Seems to correct the user cert test:  is better
4. Makes getCertType ignore trust flags: is worse

Answer to question d) yes, side effects.  IIRC, quite a few root CA certs 
don't look like CA certs, and rely on the trust flags to be recognized as 
CA certs at all.  I think with this patch, getCertType will now return 
a different value than it did before, such as nsIX509Cert::UNKNOWN_CERT or
nsIX509Cert::EMAIL_CERT for some of those root CA certs.  I think that 
would cause the certs to be displayed in either a different tab or in no 
tab at all.  (Yes(?)  Either way, that would be bad.  If I was SURE that 
this patch would cause a bunch of root CA certs to appear in the wrong tab
or in no tab, I would give it r- without hesitation.  If you're running with 
this patch, maybe you can tell by examination of the tabs in cert manager.

I gather that you're saying the release blockers are also now Beta Blockers?
I think this bug should not be a Beta Blocker.  (no pun intended)
But I'm not ready to say it's invalid. 

As for the distinction between NSS and PSM semantics for importing certs,
this is not important to most browser users, who (as you say) never use NSS
tools.  But more than a few NSS tool users and sysadmins use the browser as
an alternative way to manage their cert databases (alternative to certutil).
They're the ones who need to know this distinction, IMO.  

I am pondering your suggestion to always set VALID and TRUSTED in tandem.

There's no question (in my mind) about the distinction of TRUSTED and VALID
in terms of their behavior on NSS.  The questions with which I have been 
wrestling are: 
What is the intended/defined semantic of importing a cert manually with PSM?  
and 
If users had no way to set VALID without also setting TRUSTED, would that 
have negative unintended consequences on the security of PSM-based products?
and
If so, which would be worse: the status quo, or VALID==TRUSTED?

I agree that it's too late in the FF3 cycle to begin designing new UI to 
manager VALID trust separately.

Comment 27

[Nelson Bolyard (seldom reads bugmail)]

In today's conference call, we decided that, while we like the idea of 
setting the valid bit only when we set the trusted bit, there isn't time
left in the FF3 cycle to adequates shake out all the impact of that 
change.  

We think this "problem" mostly (perhaps only) affects object signing CAs,
and there is another RFE/bug somewhere to change how those work so that 
object signing CAs have the same test as code signing CAs, which should
eliminate the case that brought this issue to light.  

So, we'll try to fix that bug, and left this one alone, at least for FF3.

Comment 28

[Daniel Veditz [:dveditz]]

Can the security-sensitive flag be removed from this WONTFIX bug now?

Comment 29

[Nelson Bolyard (seldom reads bugmail)]

Removed security sensitive flag.