Closed Bug 991177 Opened 6 years ago Closed 6 years ago

SEC_ERROR_CA_CERT_INVALID is not overridable with mozilla::pkix, but the UI makes it look like it is

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: keeler, Assigned: mmc)

References

Details

Attachments

(2 files, 2 obsolete files)

See bug 990557. The UI makes it look like the error is overridable, but it is not when using mozilla::pkix (but it appears to be for classic verification). We'll probably have to modify NSSErrorsService::GetErrorClass and related code to take into account which verification library is in use. Or we could just disallow such overrides completely.
(In reply to David Keeler (:keeler) from comment #0)
> Or we could just disallow such overrides completely.

I suggest we just disallow the overrides completely, and mark this bug with an indicator that its patch would need to be backed out if we switch the default cert verifier.
Assignee: nobody → mmc
Attached image no_override.png
Tested manually.
Comment on attachment 8407036 [details] [diff] [review]
Disallow overrides for SEC_ERROR_CA_CERT_INVALID (

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

I thought it was better to be explicit about non-overridable errors instead of falling through to default. Your choice, though.
Attachment #8407036 - Flags: review?(dkeeler)
Comment on attachment 8407036 [details] [diff] [review]
Disallow overrides for SEC_ERROR_CA_CERT_INVALID (

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

Looks good, but I forgot to mention a few other things you'll need to change:
https://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/SSLServerCertVerification.cpp#302
https://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/SSLServerCertVerification.cpp#566
https://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementChildPreload.js#87 (this was implemented with zero input from PSM folks, so yay!)

Also, these will need some modification:
https://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/tests/unit/test_cert_overrides.js#100
https://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/tests/unit/test_cert_overrides.js#130
(do something like https://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/tests/unit/test_cert_overrides.js#135 where for classic verification we don't expect an override to be possible)

::: security/manager/ssl/src/NSSErrorsService.cpp
@@ +105,5 @@
>      case SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED:
>        *aErrorClass = ERROR_CLASS_BAD_CERT;
>        break;
> +    // Non-overridable errors.
> +    case SEC_ERROR_CA_CERT_INVALID:

The thing about this is that every other SEC_ERROR_* or SSL_ERROR_* is non-overridable, so I don't think it makes sense to enumerate them (or even this one) here.
Attachment #8407036 - Flags: review?(dkeeler) → review-
Attachment #8407036 - Attachment is obsolete: true
Comment on attachment 8407139 [details] [diff] [review]
Disallow overrides for SEC_ERROR_CA_CERT_INVALID (

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

Please take another look. Unittests in security/manager/ssl pass locally.

https://tbpl.mozilla.org/?tree=Try&rev=1dfc58de2a47
Attachment #8407139 - Flags: review?(dkeeler)
Comment on attachment 8407139 [details] [diff] [review]
Disallow overrides for SEC_ERROR_CA_CERT_INVALID (

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

::: security/manager/ssl/tests/unit/test_cert_overrides.js
@@ +52,5 @@
>                      .getHistogramById("SSL_CERT_ERROR_OVERRIDES")
>                      .snapshot();
>    do_check_eq(histogram.counts[ 0], 0);
>    do_check_eq(histogram.counts[ 2], 8 + 1); // SEC_ERROR_UNKNOWN_ISSUER
> +  do_check_eq(histogram.counts[ 4], 0 + 5); // SEC_ERROR_UNTRUSTED_ISSUER

this is a duplicated line, will remove
Comment on attachment 8407139 [details] [diff] [review]
Disallow overrides for SEC_ERROR_CA_CERT_INVALID (

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

Awesome - r=me with comments addressed.

::: security/manager/ssl/tests/unit/test_cert_overrides.js
@@ +35,5 @@
>  }
>  
> +function add_non_overridable_test(aHost, aExpectedError) {
> +  add_connection_test(
> +    aHost, getXPCOMStatusFromNSS(aExpectedError), null,

nit: so far, the convention has been to convert the expected NSS error to an xpcom status before passing it to any function, which is not what you're doing here (i.e. when this function gets called). However, this makes the calling code a bit easier to read, so I like it. How about filing a follow-up bug to convert the rest of the tests to do the same thing? (i.e. have add_cert_override_test do the conversion from NSS error to xpcom status).

@@ +52,5 @@
>                      .getHistogramById("SSL_CERT_ERROR_OVERRIDES")
>                      .snapshot();
>    do_check_eq(histogram.counts[ 0], 0);
>    do_check_eq(histogram.counts[ 2], 8 + 1); // SEC_ERROR_UNKNOWN_ISSUER
> +  do_check_eq(histogram.counts[ 4], 0 + 5); // SEC_ERROR_UNTRUSTED_ISSUER

This should be:
do_check_eq(histogram.counts[ 3], 0); // SEC_ERROR_CA_CERT_INVALID
Attachment #8407139 - Flags: review?(dkeeler) → review+
Attachment #8407139 - Attachment is obsolete: true
Comment on attachment 8407160 [details] [diff] [review]
Disallow overrides for SEC_ERROR_CA_CERT_INVALID (

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

Filed https://bugzilla.mozilla.org/show_bug.cgi?id=996872 and carrying over r+ from David. Will wait a bit just in case try turns up anything interesting.
Attachment #8407160 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/eacdbceea76d
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.