399389 - SSL error page: be more specific in the "untrusted" scenario

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Kai Engert [:KaiE:]]

Currently when a server's certificate is rejected, we group several errors into a single category and use the same string for all of them.

The NSS error codes are:
        case SEC_ERROR_UNKNOWN_ISSUER:
        case SEC_ERROR_CA_CERT_INVALID:
        case SEC_ERROR_UNTRUSTED_ISSUER:
        case SEC_ERROR_EXPIRED_ISSUER_CERTIFICATE:
        case SEC_ERROR_UNTRUSTED_CERT:

This bug proposes we be more specific and report what error it is.

In addition, while NSS does not report it as a separate error code, the reason can be a "self signed cert", which can easily be detected by comparing the issuer and subject names of a cert.

So the proposal is:
- if one the above error code is seen, let's check if the cert is self signed
- if the cert is self signed, report that
- if the cert is not self signed, use a matching phrase for each of above codes


Finally, one more change. The error page reports a single error code at the end of the message. Right now, that error code could be one of the codes listed above, or SSL_ERROR_BAD_CERT_DOMAIN or SEC_ERROR_EXPIRED_CERTIFICATE.

The proposal is to report the error with the highest priority.
So, we should make a decision about the order of the following categories:

U : an error code from the group of 5 listed above
M : SSL_ERROR_BAD_CERT_DOMAIN
T : SEC_ERROR_EXPIRED_CERTIFICATE

I propose U to have highest priority, then M, then T with lowest priority.

Comment 1

[Kai Engert [:KaiE:]]

Attachment: Patch v1

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

> a "self signed cert" [...] can easily be detected by comparing the
> issuer and subject names of a cert.

Actually, the matching issuer and subject names are a necessary but not
complete condition for determining that a cert is self-signed.  It is 
possible to have a cert in which the subject and issuer names are the same, 
and yet the cert is not self signed. Such a cert can be an intermediate CA 
cert, or even an EE cert. 

The full test for a self-signed cert involves checking for the presence 
and values of several certificate extensions.  NSS has one or more 
functions that do that.  NSS sets a flag in the CERTCertificate structure 
to signify certs that are self-issued, so that the full test doesn't need 
to be repeated often.  It's cert->isRoot.  The value of that flag is 
computed by NSS's static function cert_IsRootCert(), which you can study
to see the full test.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 284399 [details] [diff] [review]
Patch v1

The test employed by this patch to determine if a cert is self-signed, or not, is insufficient.  
Please see the preceding comment.

Comment 4

[Robert Relyea]

I suggest using cert->isRoot cert since cert_IsRootCert() is private, but the CERTCertificate structure isn't (an unfortunate oversight, but there it is).

I just reviewed the cert_IsRootCert function, and it is not unique to CA's (that is an EE cert that is self-signed will trigger this function).

The difference between subject == issuer and a self-signed cert is if the key's match (the true definition of self-signed). This is determined by the authKeyID extentions (which were initially created to allow CA's to re-key, but can be used to form cert chains all with the same subject).

Comment 5

[Kai Engert [:KaiE:]]

If I understand correctly, the following is a sufficient test to check that a certificate is self signed?

+  *aIsSelfSigned = (0 == strcmp(mCert->subjectName, mCert->issuerName)
+                    && mCert->isRoot);


Because it requires the use of NSS private data, I'm going to make the self-signed property available as a function to the rest of mozilla code.

Comment 6

[Kai Engert [:KaiE:]]

Attachment: Patch v2

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Kai, mcert->isRoot is necessary and sufficient.  The strcmp is unnecessary.

Comment 8

[Kai Engert [:KaiE:]]

Attachment: Patch v3

removed the unnecessary strcmp

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 284960 [details] [diff] [review]
Patch v3

This patch appears to correctly optimize the test for self signed-ness.
r+ for this patch's version of function nsNSSCertificate::GetIsSelfSigned

Since you asked me to review the whole patch, I looked at parts of it I 
had previously ignored, and found another concern.
This patch adds a bunch of new strings that all say:
   The certificate is not trusted because ...
one of which replaces an existing string with similar wording.

I have a problem with using the word "trusted" there.  We use the word 
"trusted" to mean "explicitly marked as trustworthy by the user."  If we use 
the same word to mean both "explicitly marked as trustworthy by the user" 
and also to mean "found valid by virtue of having been issued (perhaps 
indirectly) by an issuer who was explicitly marked trustworthy by the user",
I think that confuses the user about the meaning of the word "trusted".

There's a big difference between a cert being "explicitly marked trusted", 
and a cert being validated for a purpose.  Most certs are validated for a 
purpose without being trusted.  We honor them, treat them as valid, because 
they were issued by someone we trust.  We also honor certs that have been 
directly trusted (so called "trusted peer" certs), but they are exceptions.  

So, I think we need a different word than "trusted" in those error strings.
Ideas include: honored, believed, validated, accepted, authenticated, 
authorized, relied upon (ick).

One might argue that "trusted" is the natural word to use for this, and that
we really ought to find another word than "trusted" to describe certs that 
have been explicitly marked as being trustworthy regardless of source.  
Certs so marked are formally known as "trust anchors".  Perhaps we could 
call them "anchored" rather than trusted.  But I think that change would be
a huge shift for the whole PKI community.  So, I'd rather that we find a 
different term to describe the state of being "honored by virtue of having 
been issued by an issuer who was explicitly marked trustworthy by the user".

Comment 10

[Kai Engert [:KaiE:]]

In reply to comment 9.

> We use the word 
> "trusted" to mean "explicitly marked as trustworthy by the user."

Nelson, I think your point is, the definition of the term trust has a very clearly defined meaning at the code / implementation level in NSS.

I think you're saying, "Only use the term trust, when it's explicit trust".

But should end users really worry about that difference?
Should we avoid the term "trust" when describing issues to users, only because the term trust has a very specific meaning at the code implementation level?

I think we should not.


In addition, do they have a choice?
Why is it bad to talk about trust, even when it's implicit/indirect/inherited trust?

I find it misleading that you call the root CA certs that we ship with Mozilla as "explicitly trusted by the user". Did we ask the users? No, they didn't have a choice. They were looking for a software that works, and Mozilla.org made a decision for them. The software trusts the root, the software trusts issued valid leaf certs, when used in the right context and not expired and not revoked. It obviously trusts them in an allowed context, because it accepts them without complaining.

Why is it wrong to say "This certificate is not trusted..."?
The wording gives a reason why we can't trust it in the current context.

Please forgive me when I jump to a general discussion about logic.

Maybe I'll tell you:
  "I don't like this car because it's not blue".

From my statement, can you conclude, if the car were blue, I would like it?
I think you can't.
When you show me a blue car, I'll continue looking and say
  "Oh, I also don't like this car, because it has a manual transmission."


My point is, just because we say "this is not trusted because A", nobody can conclude "this is explicitly trusted because not A". 

Comment 11

[Nelson Bolyard (seldom reads bugmail)]

Years ago, some browser UI person decided that it was too "confusing" for 
users if the UI used both the term "certificate" and "private key".  So he
unilaterally decided that he would substitute the word "certificate" for the
term "private key" everywhere in the UI.  He thought this would dumb it down
enough for the average user, reducing the number of new terms a user might 
have to learn. 

Of course, it created nothing but confusion.  People couldn't understand how
a certificate could be both public and also something that empowered them 
(alone) to sign things, but did not also empower others to sign things.  
The NSS team objected, but the UI folks insisted, and won.  

Ever since that day, people have been confused.  I've answered countless 
emails from people who were completely confused how their certificate could 
be useful for them (alone) to make signatures if everyone has a copy.  

The ultimate example of this confusion was the publication of a book by mozdev.org and O'Reilly that contained a complete chapter, explaining how 
some certs are public and some certs are private, and how a CA creates some 
certs for code signing that are private, and others that are public.  That 
whole chapter had to be repudiated by the author.  See 

http://www.amazon.com/gp/product/0596000529
http://books.mozdev.org/chapters/ch12.html#77079 and finally
http://certs.mozdev.org/   (the repudiation)

We still get questions in the crypto newsgroup even now, years later, from 
people confused by that book.  

The lesson is: Be careful to use terms in UI PRECISELY and CONSISTENTLY.

Comment 12

[Kai Engert [:KaiE:]]

A private key is not a certificate. Sure, that's wrong.

But trust can be implicit trust or explicit trust.

A valid cert requires trust.
Either explicit or inherited from a trusted anchor.

There can be other reasons why a cert is invalid, but these error messages in particular are about scenarios where the trust is missing.

It's not about anything else related to the cert that is wrong.

We require trust, as required by PKI, but we didn't find any trust, and that's what the error messages are trying to say.

I don't see why that's wrong.

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 284960 [details] [diff] [review]
Patch v3

This patch contains UI parts and non-UI parts.
I am confident to give review in the non-UI parts.  

While I feel strongly about the UI parts, I do not wish to withhold review 
of the non-UI parts because of them.  So this review comments gives r+ to 
all parts of this patch except the patch to file 
mozilla/security/manager/locales/en-US/chrome/pipnss/pipnss.properties

I hope that's OK.

Comment 14

[Robert Relyea]

Comment on attachment 284960 [details] [diff] [review]
Patch v3

this patch looks good as long as the error strings issue gets resolved.

Comment 15

[Kai Engert [:KaiE:]]

I'm suffering from "merging hell".

Besides general context changes around this patch, there are conflicts with the work going on in bug 398718.

I would like to move this patch over to bug 398718 and provide a merged patch there. I might attach two separate patches to indicate the portions from this one that are "stable and already reviewed" (but can not land without the rest).

Comment 16

[Kai Engert [:KaiE:]]

Attachment: stable subset of reviewed patch

These are portions of reviewed patch v3 that shall remain stable when working on bug 398718. It's not really a patch, I manually erased portions that change.

Only for demonstration purposes.

Comment 17

[Kai Engert [:KaiE:]]

resolving fixed as part of bug 398718