193960 - Enhance PSM's certificate categorization

Status: RESOLVED WONTFIX

(Core Graveyard :: Security: UI, defect)

Description

[Kai Engert [:KaiE:]]

Nelson suggests the code in getCertType needs improvement. Let's discuss here
what should be done.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

This change eliminates "orphaned" CA certs, I believe.

   if (nick) {
     if (trust.HasAnyUser())
       return nsIX509Cert::USER_CERT;
-    if (trust.HasAnyCA() || CERT_IsCACert(cert,NULL))
-      return nsIX509Cert::CA_CERT;
     if (trust.HasPeer(PR_TRUE, PR_FALSE, PR_FALSE))
       return nsIX509Cert::SERVER_CERT;
   }
+  if (trust.HasAnyCA() || CERT_IsCACert(cert,NULL))
+    return nsIX509Cert::CA_CERT;
   if (email && trust.HasPeer(PR_FALSE, PR_TRUE, PR_FALSE))
     return nsIX509Cert::EMAIL_CERT;
   return nsIX509Cert::UNKNOWN_CERT;

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Here's another possibility:

   if (nick) {
     if (trust.HasAnyUser())
       return nsIX509Cert::USER_CERT;
     if (trust.HasAnyCA() || CERT_IsCACert(cert,NULL))
       return nsIX509Cert::CA_CERT;
     if (trust.HasPeer(PR_TRUE, PR_FALSE, PR_FALSE))
       return nsIX509Cert::SERVER_CERT;
   }
+  if (trust.HasAnyCA() || CERT_IsCACert(cert,NULL))
+    return nsIX509Cert::CA_CERT;
-  if (email && trust.HasPeer(PR_FALSE, PR_TRUE, PR_FALSE))
+  if (email)
     return nsIX509Cert::EMAIL_CERT;
   return nsIX509Cert::UNKNOWN_CERT; 

Comment 3

[John G. Myers]

Nelson, could you provide some background here?  What is an "orphaned" CA cert?
 From the first proposed change, it appears to be a CA cert without a nickname.

I can see why we'd want a nickname before recognizing as a user cert, but why
would we want to check that for a server cert?  Shouldn't the nickname check
only apply to user certs?  A cert that is trusted as both a CA and a peer should
identify as a CA, should it not?

I agree with removing trust checks for identifying email certs.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

It's been a year since I wrote those comments.  As I recall, the issue was/is 
this:  PSM's cert manager has a set of tabs, and all the certs accessible 
through NSS are intended to be categorized into and displayed by ONE of those
tabs.  Each tab has code that decides "Is this cert in my category?"  
It should be the case that every cert known to NSS appears in ONE of those 
categories, but in fact, there are certs that do no appear in ANY of those
categories, certs that exist in the cert DB or on some token, but do not 
appear in any tab.  Kai called these certs "orphans".  There is no way to 
see, edit, delete or otherwise manipulate the orphans with PSM UI.

Kaie produced his own version of mozilla 1.3.x in which he added numerous
enhncements that he could not get through driver approval.  One of them was 
that he added another tab to cert manager that displayed all the orphans. 
I remember the first time I tried it with my own mozilla profile.  I was 
amazed at all the certs it showed, including one belonging to my boss!

I suggested to Kai that the right fix for mozilla was to eliminate orphans
by changing the code so that every cert would definitely fall into one 
category.  The patches I placed above were attempts to solve the problems
for (some of) the certs that Kai and I were seeing among the orphans.

Comment 5

[John G. Myers]

In general, I dislike using trust bits to determine the category of the cert and
prefer using data in the cert itself, such as the key usage bits, presence of
such things as email addreesses.  A user cert should be any cert for which we
have the associated key.

As a good approximation, I suggest:

 if (nick && trust.HasAnyUser())
     return nsIX509Cert::USER_CERT;
 if (trust.HasAnyCA() || CERT_IsCACert(cert,NULL))
     return nsIX509Cert::CA_CERT;
 if (email)
     return nsIX509Cert::EMAIL_CERT;
 return nsIX509Cert::SERVER_CERT;

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

I agree with the idea of relying on the cert content, but it doesn't work
very well for root CA certs.  Lots of Root CA certs now in use have VERY 
little of that sort of useful stuff.  Trust may help in those cases.

Comment 7

[John G. Myers]

All of the built-in CA's have CERT_IsCACert() return true.  I believe, however,
that we need to check trust bits in order to permit users to remove
inapproproate trust bits--if they've marked a non-CA cert with CA trust bits,
they need it to show up in the CA tab for those trust bits to be removable.

Is there a way to query a cert to see if it has a domain name in it?  I have
seen server certs with email addresses, so believe that, absent trust bit info,
we should consider certs with both domain names and email addresses as server certs.

Also, is there a better test than trust.HasAnyUser() to see if we have a cert's
associated private key?

Here's what I have so far:

PRUint32 
getCertType(CERTCertificate *cert)
{
  nsNSSCertTrust trust(cert->trust);
  if (cert->nickname && trust.HasAnyUser())
    return nsIX509Cert::USER_CERT;
  if (trust.HasAnyCA() || CERT_IsCACert(cert,NULL))
    return nsIX509Cert::CA_CERT;
  if (trust.HasPeer(PR_TRUE, PR_FALSE, PR_FALSE))
    return nsIX509Cert::SERVER_CERT;
  if (cert->emailAddr)
    return nsIX509Cert::EMAIL_CERT;
  return nsIX509Cert::SERVER_CERT;
}

Comment 8

[John G. Myers]

Attachment: Interim fix

It looks like it's not possible to get better than this patch without adding a
new exported function to NSS for testing whether a cert has any DNS names. 
This patch can miscategorize an untrusted server cert as an email cert, but is
no worse than the current code.

Comment 9

[Stephane Saux]

In other apps, we've used a "Others" tabs for certs that we can't categorize,
just to ensure that certs can't creep in and become undiscoverable in the
current UI. I recommend that moz move in that direction. As far as this patch, I
would prefer that an NSS person also review it. Otherwise r=ssaux.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

John asked:
> Is there a way to query a cert to see if it has a domain name in it?  

Is that really the question you want to ask?  Or do you really want to 
ask if the cert has any name in it that identifies it as an SSL server?
SSL server certs can also be identified by IP addresses in the CN or 
subject alt name.

> I have seen server certs with email addresses, so believe that, absent 
> trust bit info, we should consider certs with both domain names and 
> email addresses as server certs.

Or perhaps we should allow those certs to appear in BOTH tabs.  Maybe
this function should take an argument, asking "is the cert valid for 
this tab?" and should answer with a boolean that could be true for more
than one tab.

> is there a better test than trust.HasAnyUser() to see if we have a cert's
> associated private key?

No, that is the right test for a private key.  The user trust bit is set
dynamically when the private key is present.  (It's not really a trust bit,
per se').

It seems to me that PSM's categorization is only for user convenience.  
NSS doesn't rely on it for any functions it performs.

In comment 7 above, I understood you to be suggesting that CA certs 
need to appear in the CA tab so that the CA trust can be edited, 
implying that CA trust cannot be edited elsewhere (in other tabs).
I'd like to suggest that all the trust bits should be editable 
regardless of the tab in which the cert appears.  It may be useful to 
be able to set trusted email peer, or trusted SSL peer on a random cert.

(I wonder if I've argued against that at some point in the past )

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

The "interim fix" patch would certainly be better than what we have now.

Comment 12

[John G. Myers]

IP addresses are sufficient, as I've mentioned in the new, dependent bug I've filed.

Nelson, are you granting that review that Stephane asked for?  If so, could you
set the review+ flag on the patch?

PSM's categorization determines which tab in the cert manager the cert shows up
in, if any.  The code currently doesn't permit a cert to show up in multiple
tabs, though that could be changed.

I would say there is no point in allowing the user to set CA trust bits on certs
for which CERT_IsCACert() doesn't return true.  Allowing the same cert to show
up under multiple tabs would be the most that makes sense.

Comment 13

[Brendan Eich [:brendan]]

I'm waiting for r+ before sr'ing -- someone ownerly please do that and set
blocking1.7b? and we'll get to this for mozilla1.7b.

/be

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 143351 [details] [diff] [review]
Interim fix

In bug 236887 and elsewhere, John and I are discussing a solution that 
allows a cert to appear in multiple tabs, if it appears to be suitable
for multiple uses such as email and SSL, or email and CA.  

I think that's a much preferable solution, whether bug 236887 is fixed
or not.  The patch above is better than what mozilla has now.  But we
could be SO much better than that.  

I'm reluctant to give r+ to this patch if it will reduce the liklihood
that a better solution will be developed.  OTOH, if no better solution
is forthcoming, this patch is better than what we have now.

So, I'll give reluctant r= to this patch.  But hope John will
continue to enhance the cert manager more than this.

Comment 15

[John G. Myers]

This should get some beta testing if it's to be included in 1.7.

Comment 16

[Brendan Eich [:brendan]]

Comment on attachment 143351 [details] [diff] [review]
Interim fix

I'll let another driver approve for 1.7b.  Don't worry about the freeze, this
can go in if approved after tonight.

/be

Comment 17

[John G. Myers]

Got in under the wire.

Comment 18

[Kai Engert [:KaiE:]]

Being not a native english speaker, what is the meaning of John's previous comment?
  "Got in under the wire."

Does it mean it was checked in?
It if was, this bug should be marked fixed.

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

Kai, the patch above was checked in.  "Under the wire" is an English 
idiom which means "at the very last moment of opportunity".  
If there are no more "orphan" certs, then this bug can be marked closed.  
I do not know what value is appropriate for the "target milestron".

Comment 20

[John G. Myers]

I do not consider the previous fix to be sufficient.  This bug is now to change
getCertType to return a bitfield, so that the cert viewer displays a
multi-purpose cert under each of the tabs that are relevant.

Currently PSM has inadequate information to implement this--it cannot reasonably
tell if a cert is appropriate for use as a server cert.  Currently, it makes
this decision by exclusion: if a cert is not good for anything else, it is a
server cert.  Thus, this bug depends on NSS being extended to inform it whether
a cert is appropriate for use as a server cert, which is bug 236887

Comment 21

[Kai Engert [:KaiE:]]

I would like to make this change:

 PRUint32
 getCertType(CERTCertificate *cert)
 {
   nsNSSCertTrust trust(cert->trust);
   if (cert->nickname && trust.HasAnyUser())
     return nsIX509Cert::USER_CERT;
   if (trust.HasAnyCA())
     return nsIX509Cert::CA_CERT;
   if (trust.HasPeer(PR_TRUE, PR_FALSE, PR_FALSE))
     return nsIX509Cert::SERVER_CERT;
   if (trust.HasPeer(PR_FALSE, PR_TRUE, PR_FALSE) && cert->emailAddr)
     return nsIX509Cert::EMAIL_CERT;
   if (CERT_IsCACert(cert,NULL))
     return nsIX509Cert::CA_CERT;
-  if (cert->emailAddr)
-    return nsIX509Cert::EMAIL_CERT;
   return nsIX509Cert::UNKNOWN_CERT;
 }

In other words, as of today, when a cert has no trust flugs, no valid peer flags, but an email address, we will treat it as an email cert.

I would like to propose such certs get treated as unknown certs.
IMHO this is acceptable, because meanwhile we have that "extra" tab in cert manager that displays the unknown/orphan certs.


I have two profiles with email certs. One SeaMonkey profile that I use for personal stuff, it's really old, I migrated it a lot of times, and it contains a lot of email certs. In my testing, with the above change, all email certs were still listed in the other-people tab, no certs were listed in the extra tab.

I also have a thunderbird profile that I use for work purposes, it also has a nice amount of other-people certs. I made the same test, same results, no certs listed in extra-tab.


Why am I proposing this change?

I'm currently working on bug 327181.
I am storing SSL cert overrides (outside of certdb) that are bound to a specific hostname and port number and kind of error (mismatch/untrusted/expired).
But in addition, I'm storing the cert in the certbd, but without any trust assigned.

As of today, such certs would show up in the extra-tab.

With my new code from bug 327181, without the above change, I am experiencing web site sites, that contain an email address, and would therefore be shown in the other-people tab.

I would like to avoid that incorrect sorting into the other-people-tab.
I would like to avoid the trouble to find other means of identifying it's really a server cert.

IMHO the proposed change in this comment seems reasonable.

I plan to include that change in my next patch for bug 327181.


If you have objections to this change, please speak up.
Thanks!

Comment 22

[Kai Engert [:KaiE:]]

Adding rrelyea, please see my previous comment in this bug.

Comment 23

[Nelson Bolyard (seldom reads bugmail)]

Kai, with this proposed change, only email certs on which explicit peer
trust has been set will appear as email certs.  (right?) 
That seems plainly unacceptable. I would give this patch an r-, if it 
was an attached patch.

It MAY be the case that a large number of the email certs in your cert 
DB have had the trusted peer override flag set on them.  That may explain
why most of your correspondents' email certs continued to show up as email
certs when you made this change.  There was a bug for a long time that 
caused the trusted peer bit to be set of most certs that were imported.  
That was a P1 bug, and hopefully has now been fixed.  Newly imported email
certs should no longer have trust override flags being set on them routinely.

Comment 24

[Kai Engert [:KaiE:]]

(In reply to comment #23)
> Kai, with this proposed change, only email certs on which explicit peer
> trust has been set will appear as email certs.  (right?) 

No! By "explicit peer trust" you probably mean CERTDB_TRUSTED.

The PSM code listed above
  trust.HasPeer
does NOT mean trusted peer, it tests for "valid peer", CERTDB_VALID_PEER only.


> That seems plainly unacceptable. I would give this patch an r-, if it 
> was an attached patch.
> 
> It MAY be the case that a large number of the email certs in your cert 
> DB have had the trusted peer override flag set on them.


No! 
# certutil -d . -L |grep -w P

None of my correspondents email certs have uppercase P listed!

I only have this set for a single CA cert (which I explicitly trusted).

What I have is a lowercase p for email certs.


> That may explain
> why most of your correspondents' email certs continued to show up as email
> certs when you made this change.  

I believe they still show up as email certs because they are valid peers.

Comment 25

[Nelson Bolyard (seldom reads bugmail)]

Kai, I mean explicit trust flags, including "VALID". 
I'll bet $$$ that your DB has "p" or "c" set on all those certs that 
still show up in your email tab.  This is an effect of the long standing
bug that routinely set those override flags in error, which hopefully 
is fixed now.  

lower case p is a peer OVERRIDE.  It means "treat this cert as a valid 
email (or whatever) cert EVEN THOUGH ON ITS FACE IT IS NOT A VALID CERT
FOR THAT PURPOSE".  PSM was setting that flag routinely on all email 
certs.   That was an error.  Hopefully it is fixed now.  But your proposed
patch adds an additional dependency on the erroneous behavior.

Comment 26

[Nelson Bolyard (seldom reads bugmail)]

There has obviously been a long-lived misunderstanding of the purpose of
the valid CA and valid peer flags.  The misunderstanding is the belief that
all valid CA certs and all valid peer certs should have those flags set, 
and that those flags can be used to ask "is this a valid CA or peer"?  

But that is NOT the purpose of those flags.  Those flags are overrides.
They override NSS's error detection.  They tell NSS to treat the cert as 
valid, even if on its face it is not valid.  NSS is quite capable of 
determining if a cert is valid, on its face, for some usage (such as email)
without the help of any of those flags.  That is why it is so wrong to 
habitually set those override flags on all certs being imported.  It defeats
NSS's validity checking.  

We want the vast majority of certs to be used because they are valid ON 
THEIR FACE, not because the user has chosen to override errors in them, 
and CERTAINLY not because PSM has set the override flags without the user 
intending to do so.