178692 - Accepting a self-signed cert adds it to Authorities tab.

Status: RESOLVED FIXED

(Core Graveyard :: Security: UI, defect, P3)

Description

[John Unruh]

1.) Visit https://twist.phys.ualberta.ca, a "plesk.com" type cert where the
issuer and issuee are identical, and the serial number is 00.
2.) Accept the cert permanently
3.) Open Cert Manager, web sites tab.
What is expected: That the cert would appear.
What happens: The cert appears in the Authorities tab.

Comment 1

[John Unruh]

Changing summary

Comment 2

[John G. Myers]

Mass reassign ssaux bugs to nobody

Comment 3

[John G. Myers]

This appears to have been caused by the checkin to bug 171224.  The cert->isRoot
check added to CERT_IsCACert() caused it to misidentify all self-signed
certificates as CA certificates.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

cert->isRoot does not include all self-signed certs.  It only includes
self-signed certs that do not have authority key ID extensions that do 
not match their own subject key ID extensions.

Comment 5

[John G. Myers]

A cert with an authority key ID extension that does not match itself would by
definition not be self-signed, right?

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

This bug involves an X.509 v1 (not v3) cert.  
Like all v1 certs, it has NO extensions.
Thus there is nothing in the cert that can definitely show that it is 
NOT a CA cert.  

The appearance of domain names or email addresses in a cert's subject 
name do not disprove that it is a CA cert.  

The cert in question is a "self-issued" cert, meaning that subject and 
issuer names are equal.  A self-issued v1 cert must be, by definition, 
self signed.  

Trusted self-issued self-signed v1 certs are INDISTINGUISHABLE from v1
root CA certs, and are identified as root CA certs.  There are numerous
trusted root CA certs that fit that description exactly.  
Consequently, NSS defines self-issued v1 certs as root CA certs, and has
done so since before I started working on NSS 8 years ago.
Such certs may also be suitable for other non-CA purposes.

cert->isRoot and CERT_IsCACert() correctly identify this cert according
to NSS's long standing rules.  

John wrote: "The cert->isRoot check added to CERT_IsCACert() caused it to
misidentify all self-signed certificates as CA certificates."  That statement
is mistaken in two ways:
a) it is false for v3 certs, whose extensions (including basic constraints
and authority key id) provides lots of extra info in identifying which 
certs are CA certs, and which are root CA certs, and 
b) this cert is not misidentified.

There is no NSS bug here.  There MAY be a PSM bug here.  If PSM does not 
enable the user to manipulate the trust for this cert in such a way to 
mark it as a trusted peer, but not as a trusted CA, then that is a PSM bug.

Comment 7

[John G. Myers]

Attachment: Check trust bits before cert attributes

This will make the cert show up on the Sites tab when it is trusted as a SSL
server.  If the user removes that trust bit, the cert will then start appearing
under the CA tab, where the user won't be able to edit the SSL server trust.

Comment 8

[John G. Myers]

Bug prevents users from removing the existing trust bits for affected certs.

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 144550 [details] [diff] [review]
Check trust bits before cert attributes

>Index: security/manager/ssl/src/nsNSSCertHelper.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/security/manager/ssl/src/nsNSSCertHelper.cpp,v
>retrieving revision 1.8
>diff -u -p -r1.8 nsNSSCertHelper.cpp
>--- security/manager/ssl/src/nsNSSCertHelper.cpp	10 Mar 2004 07:36:50 -0000	1.8
>+++ security/manager/ssl/src/nsNSSCertHelper.cpp	23 Mar 2004 07:27:02 -0000
>@@ -545,10 +545,14 @@ getCertType(CERTCertificate *cert)
>   nsNSSCertTrust trust(cert->trust);
>   if (cert->nickname && trust.HasAnyUser())
>     return nsIX509Cert::USER_CERT;
>-  if (trust.HasAnyCA() || CERT_IsCACert(cert,NULL))
>+  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)

Boolean actual argument values are so opaque -- how about some comments?

rs=me again.

/be

>+    return nsIX509Cert::EMAIL_CERT;
>+  if (CERT_IsCACert(cert,NULL))
>+    return nsIX509Cert::CA_CERT;
>   if (cert->emailAddr)
>     return nsIX509Cert::EMAIL_CERT;
>   return nsIX509Cert::SERVER_CERT;

Comment 10

[Asa Dotzler [:asa]]

Comment on attachment 144550 [details] [diff] [review]
Check trust bits before cert attributes

a=asa (on behalf of drivers) for checkin to 1.7

Comment 11

[Daniel Veditz [:dveditz]]

This should block 1.7, too easy to get users to do the wrong thing.

Comment 12

[John G. Myers]

Fix checked in.

Comment 13

[Daniel Veditz [:dveditz]]

John says this isn't security sensitive, just a bug.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

I spent some time looking at how Windows' cert manager handles categorization
of certs and trust.  The following info is gathered from my limited experiments
with it on Win2k and WinXP.

When windows displays stores certs, it displays them in 4 tabs, representing
4 categories:  
       tab name                               store name
    ======================================   ============
  - Personal                                     My
  - Other people                                 ??
  - Intermediate Certification Authorities       CA
  - Trusted Root Certification Authorities       Root

Windows has separate stores for the separate catgeories.  Additional cert 
stores may be added.  When Windows' cert manager displays the certs in any 
one tab, it is displaying all the certs in the corresponding store(s). 
Cert categorization decisions are made when the cert is imported into a 
cert store, not later when the store's content is listed.  Some cert stores
exist that are not displayed in any tab, such as NetMeeting's cert store.

Trust is bestowed on all certs in the Root store, and ONLY on certs in the 
root store.  If you want to trust an SSL server's cert, or a peer's email 
cert, or an intermediate CA cert, even though there is a problem with the 
chain for that cert, you must store that cert in the Root store.  

The cert import wizard lets the user select whether to let the wizard itself
choose a store, or to have the user choose the store himself.  The wizard
never tells the user which store it would choose.  The only way for the 
user to find out is to try it, and then go find the cert in the stores 
afterwords.  The user may delete the cert and reimport it if desired.

Regardless of which tab/store it is in, a cert with an extended key usage 
extension will only be allowed to be used for the purposes allowed by the
OIDs in that extension.  An empty EKU extension allows no uses.  An absent 
EKU extension allows all uses.  

The cert manager also allows the user to define a set of Extended Key Usages
(OIDs) for each cert.  The user-chosen EKU OIDs are stored along with the 
cert in the store.  The user-chosen list, if present, seems to override the
EKU extension.  The GUI presents the user with a list of checkboxes with 
purpose names, each of which corresponds to an OID.  The user may also add
additional purpose names and OIDs to that list.  

So, a user with a self-signed SSL server cert apparently must put it into 
the trusted root CA tab/store for it to be trusted, and then if the user
wants the cert to be used only as an SSL server cert, the user may limit
the cert to that usage by adding a user-chosen EKU.  

The thing I like about this UI is that, regardless of the tab/store, the
user is presented with the same list of editable choices about the cert.  
I think mozilla's trust editing UI should be similar, allowing ALL the 
known trust flags to be editable, regardless of the tab in which the cert
appears.  That way, the user can set the trust flags appropriately, 
regardless of the tab into which PSM has characterized the cert, and the 
user can fix ALL miscategorization issues that way.

Comment 15

[Daniel Veditz [:dveditz]]

Adding Jon Granrose to CC list to help round up QA resources for verification

Comment 16

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 144550 [details] [diff] [review]
Check trust bits before cert attributes

a=blizzard for 1.4.3

Comment 17

[Christopher Aillon (sabbatical, not receiving bugmail)]

Checked in to the 1.4 branch