407523 - Error page should not allow adding an exception for uncategorized errors

Status: VERIFIED FIXED

(Core :: Security: PSM, defect)

Description

[Daniel Brooks [:db48x]]

I get the sec_error_inadequate_key_usage error while visiting https://sage.math.washington.edu:8103/. Since it gives me the option of adding an exception, I tried it. However, when I clicked on the 'get certificate' button, a dialog popped up giving me the exact same error message. Perhaps it's just not possible to have an exception for this type of error?

See the screenshot in the url field.

The site is very slow, so it takes some time to reproduce. The problem could concievably be that it's simply timing out, and the error messages are misleading. I don't know enough about ssl to make that determination.

Comment 1

[Johnathan Nightingale [:johnath]]

It's certainly an odd certificate.  :)  Self-signed, but with "key usage" bits that suggest it isn't intended for securing SSL/TLS communications, afaict.  Nevertheless, I'd like to hear from Kai, Bob or Nelson as to whether this represents an exception-worthy scenario or not because it's a little odd.

If this were a certificate issued by a normal CA, without having key usage bits which permit SSL, we wouldn't allow it, so it seems odd that taking the CA out of the equation would make it more acceptable.  On the other hand, someone might argue that a site presenting a self-signed certificate of any parseable variety is just as good as any other, since the data in a self-signed cert isn't being attested to by anyone other than the site anyhow.

In either case, it feels like an edge case to me, assuming I'm interpreting it correctly.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Kai, when a cert is imported to become an "exception", do we set the 
VALID_PEER flag on it? or VALID_CA?

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Hmm.  Bug 405399 said it was not possible to make an exception from a cert 
with inadequate key usage.  This bug seems to say it is possible, but has
no effect.  I wonder which is correct.

Daniel, does the cert you added show up in cert manager as an exception?

Comment 5

[Daniel Brooks [:db48x]]

No, you can't get as far as actually adding the exception, because you get the inadequate key usage error again when you click the get cert button.

It either needs to have a less strict test when you're just getting the cert to make an exception, or the option to add an exception needs to be hidden for this particular error.

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

So, then, actually, this bug was a dup of bug 405399, which had the correct
description.

Comment 7

[Kai Engert (:KaiE:)]

(In reply to comment #2)
> Kai, when a cert is imported to become an "exception", do we set the 
> VALID_PEER flag on it? or VALID_CA?

No. PSM uses:

  SECStatus srv = PK11_ImportCert(slot, nsscert, CK_INVALID_HANDLE,
                                  const_cast<char*>(nickname.get()), PR_FALSE);

It was my understanding this will import without any flags added to the trust.

Comment 8

[Kai Engert (:KaiE:)]

I think something is wrong.

There are two different SSL error pages:

(a) protocol error, dead end. No offer to add an exception

(b) rejected bad cert, possible to override, offer to add an exception.

For the given error sec_error_inadequate_key_usage one should get page (a), but it  appears you got (b). That's a bug.

Comment 9

[Kai Engert (:KaiE:)]

NSS delivers a list of multiple errors that were encountered during verification.
This includes both sec_error_inadequate_key_usage and SEC_ERROR_UNKNOWN_ISSUER.

Error SEC_ERROR_UNKNOWN_ISSUER alone would be sufficient to conclude "bad cert" and trigger page (b).

However, because we also have error code sec_error_inadequate_key_usage, it causes PSM to stop the processing, without allowing overrides.


So, why did Firefox display the wrong error page (b) ?

Because the error code reported externally was set to SEC_ERROR_UNKNOWN_ISSUER.
When the global application code asked for a decision, whether to show (a) or (b), it asked PSM
  "what page should be shown for SEC_ERROR_UNKNOWN_ISSUER ?"
and PSM replied: (b)


The fix is to change PSM to externally report the error code that was the stop condition by setting PR_SetError().

Comment 10

[Kai Engert (:KaiE:)]

Attachment: Patch v1

With this patch applied, the error page no longer offers to add an override.

Comment 11

[Kai Engert (:KaiE:)]

Attachment: Dump and ascii encoding of server cert

I'm also attaching the server cert.

I suspect we get the error about key usage, because the extension is critical and does not allow encrytion?

Comment 12

[Kai Engert (:KaiE:)]

I'm changing the summary from the current
 "Can't add an exception for cert with inadequate key usage"

to the new
 "Error page should not allow adding an exception for inadequate key usage"

Comment 13

[Kaspar]

(In reply to comment #11)
> I suspect we get the error about key usage, because the extension is critical
> and does not allow encrytion?

It's the keyCertSign bit which is missing, which NSS requires for all certs used as CA certs - cf. also http://lxr.mozilla.org/mozilla/ident?i=CERT_KeyUsageAndTypeForCertUsage. (Adding the keyCertSign bit turns the error into SEC_ERROR_CA_CERT_INVALID, so the cert can then be added as a "normal" exception.)

The question raised by Nelson in bug 403220 comment 17 probably remains: are there additional SEC_ERRORs/SSL_ERRORs where PSM should allow to add an override? Currently, nsNSSBadCertHandler in nsNSSIOLayer.cpp deals with these seven:

 SEC_ERROR_UNKNOWN_ISSUER
 SEC_ERROR_CA_CERT_INVALID
 SEC_ERROR_UNTRUSTED_ISSUER
 SEC_ERROR_EXPIRED_ISSUER_CERTIFICATE
 SEC_ERROR_UNTRUSTED_CERT
 SSL_ERROR_BAD_CERT_DOMAIN
 SEC_ERROR_EXPIRED_CERTIFICATE

I'm not sure if SEC_ERROR_INADEQUATE_KEY_USAGE is really "worse" than these (and would therefore justify to reject the cert completely vs. allowing to add an override).

Comment 14

[Robert Relyea]

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

r+ rrelyea

Comment 15

[Kai Engert (:KaiE:)]

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

Requesting approval.

This patch is supposed to fix the problem that various scenarios might trigger the wrong error page.

I will file a separate bug to address Kaspar's proposal.

Comment 16

[Kai Engert (:KaiE:)]

Regarding comment 13, I filed bug 412277 where I describe some concerns around that proposal.

Comment 17

[Kai Engert (:KaiE:)]

fixed

Comment 18

[Marcia Knous [:marcia]]

verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b3pre) Gecko/2008011604 Minefield/3.0b3pre. I verified by visiting the site in Comment 0 and confirming that an exception cannot be added.