Return more specific error messages when reasonable in insanity::pkix

RESOLVED FIXED in Firefox 29

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: briansmith, Assigned: briansmith)

Tracking

Trunk
mozilla30
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox29 fixed, firefox30 fixed)

Details

Attachments

(2 attachments)

This is required in order to get the name constraint tests to pass without modification. Without this patch, the result is SEC_ERROR_UNKNOWN_ISSUER, which is reasonable, but not as specific as the SEC_ERROR_CERT_NOT_IN_NAME_SPACE error that the tests are expecting.

This patch will result in friendlier & more helpful error messages for end-users, most of the time.

This bug is NOT intended to solve the problem, in general, of deciding that certain path building problems like "active distrust" are worse than others or anything like that. We can discuss pros/cons of that in a separate bug, if we want to go down that route. It's out of scope for this bug.
Attachment #8377695 - Flags: review?(dkeeler)
Attachment #8377695 - Flags: review?(cviecco)
https://tbpl.mozilla.org/?tree=Try&rev=4db11332bac4

Note that this patch is also required in order for cert overrides to work correctly. In particular, the part that changes the handling of SEC_ERROR_EXPIRED_CERTIFICATE is important for that.
Comment on attachment 8377695 [details] [diff] [review]
better-error-messages.patch

Review of attachment 8377695 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed.

::: security/insanity/lib/pkixbuild.cpp
@@ +279,5 @@
> +    }
> +    if (errorToReturn == 0) {
> +      errorToReturn = currentError;
> +    } else if (errorToReturn != currentError) {
> +      errorToReturn = SEC_ERROR_UNKNOWN_ISSUER;

Just to see if I'm understanding this correctly: if we try one path that results in SEC_ERROR_UNTRUSTED_ISSUER and one path that results in SEC_ERROR_EXPIRED_ISSUER_CERTIFICATE, this would downgrade errorToReturn to SEC_ERROR_UNKNOWN_ISSUER (which is ranked as less serious than either), right?
From comment 0, my impression is this is just to get insanity working like classic verification, and we can improve the ranking later. If that's the case, r=me. I would just like a confirmation on this.

::: security/insanity/lib/pkixcheck.cpp
@@ +235,5 @@
> +        // XXX: It seems like CERT_CheckNameSpace doesn't always call
> +        // PR_SetError when it fails. We set the error code here, though this
> +        // may be papering over some fatal errors. NSS's
> +        // cert_VerifyCertChainOld does something similar.
> +        PR_SetError(SEC_ERROR_CERT_NOT_IN_NAME_SPACE, 0);

Well, what if we PR_SetError(0, 0) and then only PR_SetError(SEC_ERROR_CERT_NOT_IN_NAME_SPACE, 0) if CERT_CheckNameSpace didn't set an error?
Attachment #8377695 - Flags: review?(dkeeler) → review+
Comment on attachment 8377695 [details] [diff] [review]
better-error-messages.patch

Review of attachment 8377695 [details] [diff] [review]:
-----------------------------------------------------------------

While this works for now I think we can combine it with what I have done before. r+ (see the other patch)
Attachment #8377695 - Flags: review?(cviecco) → review+
Comment on attachment 8377695 [details] [diff] [review]
better-error-messages.patch

Review of attachment 8377695 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/insanity/lib/pkixbuild.cpp
@@ +279,5 @@
> +    }
> +    if (errorToReturn == 0) {
> +      errorToReturn = currentError;
> +    } else if (errorToReturn != currentError) {
> +      errorToReturn = SEC_ERROR_UNKNOWN_ISSUER;

Classic NSS verification will only try one path, so it doesn't run into this issue. Note that libpkix seems to completely punt on this as well. I will add a note in the "design docs" bug about this, so we can revisit it.

::: security/insanity/lib/pkixcheck.cpp
@@ +235,5 @@
> +        // XXX: It seems like CERT_CheckNameSpace doesn't always call
> +        // PR_SetError when it fails. We set the error code here, though this
> +        // may be papering over some fatal errors. NSS's
> +        // cert_VerifyCertChainOld does something similar.
> +        PR_SetError(SEC_ERROR_CERT_NOT_IN_NAME_SPACE, 0);

That's a reasonable suggestion. But, we'll eventually rewrite CERT_CheckNameSpace so it won't matter. And, already it doesn't matter enough. If we're OOM them we're screwed anyway (probably crashed already) and it is a lot of effort just to figure out what error codes CERT_CheckNameSpace may end up returning.
Comment on attachment 8377728 [details] [diff] [review]
keep-last-error-new

Camilo, I suggest you put this in a separate bug so we don't forget about it. Next time we see each other let's discuss this in more detail.
https://hg.mozilla.org/mozilla-central/rev/b8fd613d94d5
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Comment on attachment 8377695 [details] [diff] [review]
better-error-messages.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): This patch enables more tests to be uplifted to Firefox Aurora 29. We'd like to uplift as many of the tests for insanity::pkix (bug 878932) as possible to Aurora 29 so that we're more even more confident in the stability and correctness of the code we uplifted for bug 878932 and for bug 896620.

User impact if declined: Less testing, less assurance of stability and correctness for Aurora 29.

Testing completed (on m-c, etc.): This has been in Nightly for almost a week. This code is exercised heavily in automated tests.

Risk to taking this patch (and alternatives if risky): Low. insanity::pkix is disabled by default for TLS certificate verification and enabled by default for app signature verification in Aurora 29. There are a lot of automated tests that use this code path and no problems have been reported. Also, there is no memory management being done in this patch so the risk of memory errors due to this patch is very low.

String or IDL/UUID changes made by this patch: None.
Attachment #8377695 - Flags: approval-mozilla-aurora?
Attachment #8377695 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.