943115 - return early from CreateCertErrorRunnable for verification errors other than the ones we allow overrides for

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Dana Keeler (she/her) [:keeler]]

(Brian Smith (:briansmith, was :bsmith; Please NEEDINFO? me if you want a response) from bug 929617 comment 10)
> Comment on attachment 8337993 [details] [diff] [review]
> patch v2
> 
> Review of attachment 8337993 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please make the blacklist/whitelist change I suggest below, unless that
> breaks something.
> 
> ::: security/manager/ssl/src/SSLServerCertVerification.cpp
> @@ +456,5 @@
> > +      defaultErrorCodeToReport == SEC_ERROR_OCSP_TRY_SERVER_LATER ||
> > +      defaultErrorCodeToReport == SEC_ERROR_OCSP_REQUEST_NEEDS_SIG ||
> > +      defaultErrorCodeToReport == SEC_ERROR_OCSP_UNAUTHORIZED_REQUEST ||
> > +      defaultErrorCodeToReport == SEC_ERROR_OCSP_UNKNOWN_CERT ||
> > +      defaultErrorCodeToReport == SEC_ERROR_OCSP_MALFORMED_RESPONSE) {
> 
> Instead of having this be a blacklist, should we make it a whitelist of the
> error codes that we will allow a cert error override for?:
> 
>       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:
>       case SEC_ERROR_INADEQUATE_KEY_USAGE:
>       case SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED:
>       case SSL_ERROR_BAD_CERT_DOMAIN:
>       case SEC_ERROR_EXPIRED_CERTIFICATE:
> 
> It seems like the original call to CertVerifier::VerifyCert must have
> returned one of these error codes in order for us to be able to successfully
> allow the cert override. (And, if we wouldn't allow the cert override in the
> first place, there's no reason to call CertVerifier::VerifyCert again.) I
> think this would resolve your issue with SEC_ERROR_BAD_DATABASE more
> generally.
> 
> To answer your question more directly though, SEC_ERROR_BAD_DATABASE isn't
> an overridable error, so we can short-circuit in that case, as you have done.
> 
> Nit: If it is not too much trouble, this change should be done in its own
> bug or at least its own patch. It doesn't need its own tests.

(In reply to Brian Smith (:briansmith, was :bsmith; Please NEEDINFO? me if you want a response) from comment #10)
> Comment on attachment 8337993 [details] [diff] [review]
> patch v2
> 
> Review of attachment 8337993 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please make the blacklist/whitelist change I suggest below, unless that
> breaks something.
> 
> ::: security/manager/ssl/src/SSLServerCertVerification.cpp
> @@ +456,5 @@
> > +      defaultErrorCodeToReport == SEC_ERROR_OCSP_TRY_SERVER_LATER ||
> > +      defaultErrorCodeToReport == SEC_ERROR_OCSP_REQUEST_NEEDS_SIG ||
> > +      defaultErrorCodeToReport == SEC_ERROR_OCSP_UNAUTHORIZED_REQUEST ||
> > +      defaultErrorCodeToReport == SEC_ERROR_OCSP_UNKNOWN_CERT ||
> > +      defaultErrorCodeToReport == SEC_ERROR_OCSP_MALFORMED_RESPONSE) {
> 
> Instead of having this be a blacklist, should we make it a whitelist of the
> error codes that we will allow a cert error override for?:
> 
>       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:
>       case SEC_ERROR_INADEQUATE_KEY_USAGE:
>       case SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED:
>       case SSL_ERROR_BAD_CERT_DOMAIN:
>       case SEC_ERROR_EXPIRED_CERTIFICATE:
> 
> It seems like the original call to CertVerifier::VerifyCert must have
> returned one of these error codes in order for us to be able to successfully
> allow the cert override. (And, if we wouldn't allow the cert override in the
> first place, there's no reason to call CertVerifier::VerifyCert again.) I
> think this would resolve your issue with SEC_ERROR_BAD_DATABASE more
> generally.
> 
> To answer your question more directly though, SEC_ERROR_BAD_DATABASE isn't
> an overridable error, so we can short-circuit in that case, as you have done.
> 
> Nit: If it is not too much trouble, this change should be done in its own
> bug or at least its own patch. It doesn't need its own tests.

Comment 1

[Dana Keeler (she/her) [:keeler]]

Attachment: patch

Just making sure this is what you meant and that the formatting/method of implementing the whitelist is good.

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8338108 [details] [diff] [review]
patch

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

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +446,5 @@
>    MOZ_ASSERT(infoObject);
>    MOZ_ASSERT(cert);
>    
> +  // We only allow overrides for certain errors. Return early if the error
> +  // is not one of them.

Add a comment here, and in the switch statement below, saying that these two switch statements need to be kept in sync. Or, factor them into a common function somehow (if you do the latter, then re-request review).

Comment 3

[Dana Keeler (she/her) [:keeler]]

Attachment: patch v2

Good call - keeping two lists in sync is error-prone. I took a shot at refactoring this - let me know what you think.

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8338670 [details] [diff] [review]
patch v2

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

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +438,5 @@
> +//   nsICertOverrideService::ERROR_MISMATCH,
> +//   nsICertOverrideService::ERROR_TIME
> +// if the given error code is an overridable error.
> +// If it is not, then 0 is returned.
> +uint32_t

If this is not in the unnamed namespace, then make it static.

@@ +439,5 @@
> +//   nsICertOverrideService::ERROR_TIME
> +// if the given error code is an overridable error.
> +// If it is not, then 0 is returned.
> +uint32_t
> +PRErrorCodeToOverrideType(PRErrorCode aErrorCode)

Nit: In the new code in PSM, we (I) haven't been using the a prefix on parameters. IMO, it is better to avoid it.

@@ +475,5 @@
>    MOZ_ASSERT(infoObject);
>    MOZ_ASSERT(cert);
>    
> +  // We only allow overrides for certain errors. Return early if the error
> +  // is not one of them.

I would add a comment specifically saying that this is necessary in order to avoid doing revocation fetching in the case of OCSP stapling and probably for other reasons.

@@ +555,5 @@
> +      return nullptr;
> +    }
> +    collected_errors |= overrideType;
> +    if (overrideType == nsICertOverrideService::ERROR_UNTRUSTED &&
> +        errorCodeTrust == SECSuccess) {

This was wrong in the original code. You should not be comparing to SECSuccess because SECSuccess is of type SECStatus. You should be comparing to zero because zero is a PRErrorCode.

(3x here.)

@@ +558,5 @@
> +    if (overrideType == nsICertOverrideService::ERROR_UNTRUSTED &&
> +        errorCodeTrust == SECSuccess) {
> +      errorCodeTrust = i_node->error;
> +    }
> +    if (overrideType == nsICertOverrideService::ERROR_MISMATCH &&

if you make these "else if" then you can add a:

} else {
  MOZ_CRASH("unexpected return value from PRErrorCodeToOverrideType");
}

Comment 5

[Dana Keeler (she/her) [:keeler]]

Attachment: patch v2.1

Addressed comments (in particular, it turns out that function is in the unnamed namespace); carrying over r+.

Comment 6

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8338828 [details] [diff] [review]
patch v2.1

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

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +556,5 @@
> +      return nullptr;
> +    }
> +    collected_errors |= overrideType;
> +    if (overrideType == nsICertOverrideService::ERROR_UNTRUSTED) {
> +      errorCodeTrust = (errorCodeTrust == 0 ? i_node->error : errorCodeTrust);

To get the desired behavior, I had to move the setting of errorCode{Trust,Mismatch,Expired} inside the bodies of their respective conditionals - let me know if this is too clever or unclear.

Comment 7

[Dana Keeler (she/her) [:keeler]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e781364ed4f8
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc7b76d9da1e

(pushed with the patch for bug 929617)

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e781364ed4f8

Comment 9

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8338828 [details] [diff] [review]
patch v2.1

Turns out, this also needs to be uplifted if OCSP stapling telemetry is going to be uplifted to beta.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): needed to uplift bug 887321 (OCSP stapling telemetry) to beta
User impact if declined: no OCSP stapling telemetry on beta
Testing completed (on m-c, etc.): landed in 28, no problems since then
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Comment 10

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to David Keeler (:keeler) from comment #9)
> Turns out, this also needs to be uplifted if OCSP stapling telemetry is
> going to be uplifted to beta.

I agree it is a good idea to uplift this. However, is it really necessary for uplifting the OCSP stapling telemetry? I think there must be a simple way to merge the OCSP telemetry in without this patch.

> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): needed to uplift bug 887321 (OCSP
> stapling telemetry) to beta

More importantly, this is a prerequisite for bug 929617.

> User impact if declined: no OCSP stapling telemetry on beta

And, more importantly, we can't uplift bug 929617 to beta.

> Testing completed (on m-c, etc.): landed in 28, no problems since then

We also have quite a few automated tests for the related functionality, to give us reasonable confidence that this patch doesn't cause new regressions or new bugs.

> Risk to taking this patch (and alternatives if risky): low
> String or IDL/UUID changes made by this patch: none

I agree.

Comment 11

[Dana Keeler (she/her) [:keeler]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1786928ad7b2
(landed with bug 887321, bug 929617, bug 938805, bug 932519, bug 934327, and bug 930209)