975122 - Cert error overrides do not work when insanity::pkix is the chosen CertVerifier implementation

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Comment 1

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

Attachment: cert-overrides.patch

1. I removed the cert overrides mochitest. I hate mochitest, I think that the new xpcshell test now (with this patch) covers the interesting cases of the mochitest, we'll be more efficient if we use xpcshell more and mochitest less, and I didn't feel like figuring out what was necessary to modify to get the mochitest to test the insanity and non-insanity cases.

2. This patch relies on the error processing changes in bug 973268 to function correctly; particularly, it relies on the delayed reporting of expired certificate errors relative to untrusted-issuer certificate errors.

3. This patch makes SEC_ERROR_INADEQUATE_KEY_USAGE (EKU and/or KU mismatch) a non-overridable error. The telemetry that David added seems to suggest that very few cert error overrides are made for this error, so I think the compatibility impact will be minimal. This is also necessary in order to allow the simplified processing for insanity::pkix.

4. This patch is a prerequisite for removing the verifyLog functionality completely. But, I didn't remove any of the verifyLog stuff because the NSS-based verification is still using it, and because I heard some of Camilo's key pinning patches still require it.

Comment 2

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8379778 [details] [diff] [review]
cert-overrides.patch

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

r=me with comments addressed. Mostly I would like to see a few more comments and another test case (see below).

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +336,5 @@
> +    {
> +      collectedErrors = nsICertOverrideService::ERROR_UNTRUSTED;
> +      errorCodeTrust = SEC_ERROR_UNKNOWN_ISSUER;
> +
> +      SECCertTimeValidity validity = CERT_CheckCertValidTimes(cert, now, false);

It seems like we should have a test for an expired certificate with an md5 signature.

@@ -493,5 @@
>      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:

MapCertErrorToProbeValue should also be updated to not handle this case.

@@ +592,5 @@
> +  MOZ_ASSERT(defaultErrorCodeToReport != 0);
> +  MOZ_ASSERT(collectedErrors == 0);
> +  MOZ_ASSERT(errorCodeTrust == 0);
> +  MOZ_ASSERT(errorCodeMismatch == 0);
> +  MOZ_ASSERT(errorCodeExpired == 0);

Should we assert infoObject? We do dereference it. We don't dereference cert, but maybe it should be asserted as well?

@@ +652,5 @@
> +    if (overrideType == nsICertOverrideService::ERROR_UNTRUSTED) {
> +      errorCodeTrust = (errorCodeTrust == 0 ? i_node->error : errorCodeTrust);
> +    } else if (overrideType == nsICertOverrideService::ERROR_MISMATCH) {
> +      errorCodeMismatch = (errorCodeMismatch == 0 ? i_node->error
> +        : errorCodeMismatch);

nit: indentation (is the line too long if this is lined up with the ? on the previous line?)

@@ +655,5 @@
> +      errorCodeMismatch = (errorCodeMismatch == 0 ? i_node->error
> +        : errorCodeMismatch);
> +    } else if (overrideType == nsICertOverrideService::ERROR_TIME) {
> +      errorCodeExpired = (errorCodeExpired == 0 ? i_node->error
> +        : errorCodeExpired);

nit: indentation

@@ +707,5 @@
> +                                               errorCodeMismatch,
> +                                               errorCodeExpired);
> +      break;
> +
> +    default:

Maybe we should assert here? This is in case we unexpectedly have an implementation we don't know about, right?

::: security/manager/ssl/tests/unit/test_cert_overrides.js
@@ +57,5 @@
> +  let fakeOCSPResponder = new HttpServer();
> +  fakeOCSPResponder.registerPrefixHandler("/", function (request, response) {
> +    response.setStatusLine(request.httpVersion, 500, "Internal Server Error");
> +  });
> +  fakeOCSPResponder.start(8080);

Hmmm - is this why this test was taking so long on windows? My assumption was the OCSP code would try to connect to a port that wasn't open and would fail quickly.

@@ +60,5 @@
> +  });
> +  fakeOCSPResponder.start(8080);
> +
> +  add_tests_in_mode(true);
> +  add_tests_in_mode(false);

The ordering here is important, because in the classic case we add active distrust, which would cause the insanity tests to fail with non-overridable certs. (So, a comment would be nice.)

@@ +136,5 @@
> +  add_cert_override_test("mismatch-expired.example.com",
> +                         Ci.nsICertOverrideService.ERROR_MISMATCH |
> +                         Ci.nsICertOverrideService.ERROR_TIME,
> +                         getXPCOMStatusFromNSS(SSL_ERROR_BAD_CERT_DOMAIN));
> +  add_cert_override_test("mismatch-untrusted.example.com",

When we're using insanity to verify, we haven't added active distrust for the CA that issued this certificate, so this doesn't quite test the same thing as in the classic case (indeed, as you indicate above, it can't, since active distrust can't be overridden). Since the name of the server is a bit misleading, a comment explaining this would be great.

Comment 3

[Camilo Viecco (:cviecco)]

Comment on attachment 8379778 [details] [diff] [review]
cert-overrides.patch

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

r- for:
1. You are removing a test without providing one with more coverage.
2. Having a test that can take 30 seconds to run for waiting for a port that is not listening.

and the comment inside SLServerCertVerification.cpp

Also, instead of adding all that logic into SSL erver I would have dome somthing similar to https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=699874&attachment=705474

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +606,5 @@
> +  // case of OCSP stapling and probably for other reasons.
> +  if (PRErrorCodeToOverrideType(defaultErrorCodeToReport) == 0) {
> +    PR_SetError(defaultErrorCodeToReport, 0);
> +    return SECFailure;
> +  }

Move this to the common block (outside the nss call, should be the same for insanity and classic)

::: security/manager/ssl/tests/unit/test_cert_overrides.js
@@ +97,5 @@
>                           Ci.nsICertOverrideService.ERROR_UNTRUSTED,
> +                            getXPCOMStatusFromNSS(
> +                              useInsanity
> +                                ? SEC_ERROR_UNKNOWN_ISSUER
> +                                : SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED));

I consider SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED and SEC_ERROR_UNKNOWN_ISSUER completely different cases. In fact, why do we still permit to overrides the cert signature algorithm?

Comment 4

[Camilo Viecco (:cviecco)]

Comment on attachment 8379778 [details] [diff] [review]
cert-overrides.patch

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

opopsy. reseting r+ from keeler

Comment 5

[Camilo Viecco (:cviecco)]

Comment on attachment 8379778 [details] [diff] [review]
cert-overrides.patch

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

Seems like I cant bugzilla today. removing cvieccos r+ and r- as I cannot reset keeler's flag and having r+ and r- is quite confusing. Keeping the worse r-

Comment 6

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

Comment on attachment 8379778 [details] [diff] [review]
cert-overrides.patch

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

> r- for:
> 1. You are removing a test without providing one with more coverage.

I didn't intend to reduce test coverage. That's why I added the combination cases. Please help me understand better which cases the removed test is testing that the xpcshell test isn't covering. I'd be happy to expand the test to make the test coverage equal or better.

> 2. Having a test that can take 30 seconds to run
>    for waiting for a port that is not listening.

That's bug 973134; I don't think this bug made it worse.

> and the comment inside SLServerCertVerification.cpp

See my response inline in the patch review comments.

> Also, instead of adding all that logic into SSL erver I
> would have dome somthing similar to
> https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=699874&attachment=705474

I still think that CreateCertErrorRunnable is a better place to put cert error override logic, since cert error override logic is only used in CreateCertErrorRunnable, and only for SSL, not for all kinds of certificate verifications. Indeed, the old cert error override classification is in the exact same place already. So, I'd rather keep things the way they are.

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +334,5 @@
> +    case SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED:
> +    case SEC_ERROR_UNKNOWN_ISSUER:
> +    {
> +      collectedErrors = nsICertOverrideService::ERROR_UNTRUSTED;
> +      errorCodeTrust = SEC_ERROR_UNKNOWN_ISSUER;

errorCodeTrust = defaultErrorCodeToReport;

@@ +606,5 @@
> +  // case of OCSP stapling and probably for other reasons.
> +  if (PRErrorCodeToOverrideType(defaultErrorCodeToReport) == 0) {
> +    PR_SetError(defaultErrorCodeToReport, 0);
> +    return SECFailure;
> +  }

I thought about doing what you suggested. However, with my current approach, when we remove the non-insanity code, we will be able to remove PRErrorCodeToOverrideType too. Putting it in the NSS-specific (non-insanity) code path makes that future removal easier. Consequently, I'd like to keep this the way it is.

::: security/manager/ssl/tests/unit/test_cert_overrides.js
@@ +97,5 @@
>                           Ci.nsICertOverrideService.ERROR_UNTRUSTED,
> +                            getXPCOMStatusFromNSS(
> +                              useInsanity
> +                                ? SEC_ERROR_UNKNOWN_ISSUER
> +                                : SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED));

Basically, there are still too many servers that have MD5-based certificates, and we were getting too many complaints when we made it a hard failure. There are a lot of self-signed certificates that used MD5. For a self-signed certificate, it doesn't matter what the algorithm is. And, self-signed certificates will result in SEC_ERROR_UNKNOWN_ISSUER otherwise.

The logic we used to decide that disabled signature algorithms should be in the ERROR_UNTRUSTED category went like this: If the signature can't be trusted, then that means you don't really know who signed the certificate. Consequently, you can't say for sure that a trusted issuer signed the certificate even if it successfully chains to a trusted root. Well, that's very similar to the case where you have a missing intermediate: you just don't know if a trusted CA signed the certificate.

We don't make it a hard failure (preventing override) because an attacker could just use a self-signed certificate, instead of forging a certificate using an MD5-based signature, to get an overridable error. Thus, treating any particular error--and this error in particular--as non-overridable doesn't really add any security against an actual attacker. Note that this was the consensus of the NSS team during discussions in the NSS teleconference when we made this change.

Now, maybe your problem is that the error code in the insanity::pkix case is SEC_ERROR_UNKNOWN_ISSUER instead of SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED? If so, I believe I can fix that with this incremental patch:

-      errorCodeTrust = SEC_ERROR_UNKNOWN_ISSUER;
+      errorCodeTrust = defaultErrorCodeToReport;

I will make this change, which should make the insanity::pkix case and the non-insanity case equivalent.

Note that we're trying to minimize any changes in semantics with this transition to insanity::pkix, so doing the same as what we is OK (good).

Comment 7

[Camilo Viecco (:cviecco)]

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #6)
> Comment on attachment 8379778 [details] [diff] [review]
> cert-overrides.patch
> 
> Review of attachment 8379778 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> > r- for:
> > 1. You are removing a test without providing one with more coverage.
> 
> I didn't intend to reduce test coverage. That's why I added the combination
> cases. Please help me understand better which cases the removed test is
> testing that the xpcshell test isn't covering. I'd be happy to expand the
> test to make the test coverage equal or better.

Notice that the mochitest is doing a double loop over the errors. We are testing that
1. Errors match the exact thing we expect (i=j) (you cover this)
2. When you have overrides set you connect when the flags cover your errors not not otherwise(i!=j)
This seconds set of cases is the ones you are not covering. (that is why we test for 49 and not 7 cases)

> 
> > 2. Having a test that can take 30 seconds to run
> >    for waiting for a port that is not listening.
> 
> That's bug 973134; I don't think this bug made it worse.

ACK.

> > and the comment inside SLServerCertVerification.cpp
> 
> See my response inline in the patch review comments.
> 
> > Also, instead of adding all that logic into SSL erver I
> > would have dome somthing similar to
> > https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=699874&attachment=705474
> 
> I still think that CreateCertErrorRunnable is a better place to put cert
> error override logic, since cert error override logic is only used in
> CreateCertErrorRunnable, and only for SSL, not for all kinds of certificate
> verifications. Indeed, the old cert error override classification is in the
> exact same place already. So, I'd rather keep things the way they are.
> 
> ::: security/manager/ssl/src/SSLServerCertVerification.cpp
> @@ +334,5 @@
> > +    case SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED:
> > +    case SEC_ERROR_UNKNOWN_ISSUER:
> > +    {
> > +      collectedErrors = nsICertOverrideService::ERROR_UNTRUSTED;
> > +      errorCodeTrust = SEC_ERROR_UNKNOWN_ISSUER;
> 
> errorCodeTrust = defaultErrorCodeToReport;
> 
> @@ +606,5 @@
> > +  // case of OCSP stapling and probably for other reasons.
> > +  if (PRErrorCodeToOverrideType(defaultErrorCodeToReport) == 0) {
> > +    PR_SetError(defaultErrorCodeToReport, 0);
> > +    return SECFailure;
> > +  }
> 
> I thought about doing what you suggested. However, with my current approach,
> when we remove the non-insanity code, we will be able to remove
> PRErrorCodeToOverrideType too. Putting it in the NSS-specific (non-insanity)
> code path makes that future removal easier. Consequently, I'd like to keep
> this the way it is.

I think it makes it worse but I can live with this.

> 
> ::: security/manager/ssl/tests/unit/test_cert_overrides.js
> @@ +97,5 @@
> >                           Ci.nsICertOverrideService.ERROR_UNTRUSTED,
> > +                            getXPCOMStatusFromNSS(
> > +                              useInsanity
> > +                                ? SEC_ERROR_UNKNOWN_ISSUER
> > +                                : SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED));
> 
> Basically, there are still too many servers that have MD5-based
> certificates, and we were getting too many complaints when we made it a hard
> failure. There are a lot of self-signed certificates that used MD5. For a
> self-signed certificate, it doesn't matter what the algorithm is. And,
> self-signed certificates will result in SEC_ERROR_UNKNOWN_ISSUER otherwise.
> 
> The logic we used to decide that disabled signature algorithms should be in
> the ERROR_UNTRUSTED category went like this: If the signature can't be
> trusted, then that means you don't really know who signed the certificate.
> Consequently, you can't say for sure that a trusted issuer signed the
> certificate even if it successfully chains to a trusted root. Well, that's
> very similar to the case where you have a missing intermediate: you just
> don't know if a trusted CA signed the certificate.
> 
> We don't make it a hard failure (preventing override) because an attacker
> could just use a self-signed certificate, instead of forging a certificate
> using an MD5-based signature, to get an overridable error. Thus, treating
> any particular error--and this error in particular--as non-overridable
> doesn't really add any security against an actual attacker. Note that this
> was the consensus of the NSS team during discussions in the NSS
> teleconference when we made this change.
> 
> Now, maybe your problem is that the error code in the insanity::pkix case is
> SEC_ERROR_UNKNOWN_ISSUER instead of
> SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED? If so, I believe I can fix that
> with this incremental patch:
> 
> -      errorCodeTrust = SEC_ERROR_UNKNOWN_ISSUER;
> +      errorCodeTrust = defaultErrorCodeToReport;
> 
> I will make this change, which should make the insanity::pkix case and the
> non-insanity case equivalent.
> 
> Note that we're trying to minimize any changes in semantics with this
> transition to insanity::pkix, so doing the same as what we is OK (good).

Agreed

I am resetting the review to r- until the test issue is addressed. (IE, remove it and is automatic r+)

Comment 8

[Camilo Viecco (:cviecco)]

Comment on attachment 8379778 [details] [diff] [review]
cert-overrides.patch

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

r- for the removal of test coverage. (otherwise r+) See response on previous message on what is missing.

Comment 9

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

Comment on attachment 8379778 [details] [diff] [review]
cert-overrides.patch

(In reply to Camilo Viecco (:cviecco) from comment #8)
> r- for the removal of test coverage. (otherwise r+) See response on previous
> message on what is missing.

I'm going to undo the removal of security/manager/ssl/tests/mochitest/bugs/test_certificate_overrides.html. I filed bug 975763 for moving its logic to security/manager/ssl/tests/unit/test_cert_overrides.js.

Comment 10

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

Comment on attachment 8379778 [details] [diff] [review]
cert-overrides.patch

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

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +592,5 @@
> +  MOZ_ASSERT(defaultErrorCodeToReport != 0);
> +  MOZ_ASSERT(collectedErrors == 0);
> +  MOZ_ASSERT(errorCodeTrust == 0);
> +  MOZ_ASSERT(errorCodeMismatch == 0);
> +  MOZ_ASSERT(errorCodeExpired == 0);

I added MOZ_ASSERT(cert) and MOZ_ASSERT(infoObject). In InsanityDetermineCertOverrideErrors I added MOZ_ASSERT(cert) and MOZ_ASSERT(hostName).

@@ +652,5 @@
> +    if (overrideType == nsICertOverrideService::ERROR_UNTRUSTED) {
> +      errorCodeTrust = (errorCodeTrust == 0 ? i_node->error : errorCodeTrust);
> +    } else if (overrideType == nsICertOverrideService::ERROR_MISMATCH) {
> +      errorCodeMismatch = (errorCodeMismatch == 0 ? i_node->error
> +        : errorCodeMismatch);

I rewrote these lines to use this pattern:

      if (errorCodeTrust == 0) {
        errorCodeTrust = i_node->error;
      }

This seems much clearer to me than the existing pattern anyway:
   x = (is x not set) ? newValue : x;

@@ +655,5 @@
> +      errorCodeMismatch = (errorCodeMismatch == 0 ? i_node->error
> +        : errorCodeMismatch);
> +    } else if (overrideType == nsICertOverrideService::ERROR_TIME) {
> +      errorCodeExpired = (errorCodeExpired == 0 ? i_node->error
> +        : errorCodeExpired);

ditto.

@@ +707,5 @@
> +                                               errorCodeMismatch,
> +                                               errorCodeExpired);
> +      break;
> +
> +    default:

I added a MOZ_CRASH.

::: security/manager/ssl/tests/unit/test_cert_overrides.js
@@ +57,5 @@
> +  let fakeOCSPResponder = new HttpServer();
> +  fakeOCSPResponder.registerPrefixHandler("/", function (request, response) {
> +    response.setStatusLine(request.httpVersion, 500, "Internal Server Error");
> +  });
> +  fakeOCSPResponder.start(8080);

I don't know. I don't think that such an assumption is safe, because there might be something else running on localhost:80, at least in theory. Also, localhost works in a special way on Windows.

@@ +60,5 @@
> +  });
> +  fakeOCSPResponder.start(8080);
> +
> +  add_tests_in_mode(true);
> +  add_tests_in_mode(false);

I copied your comment verbatim into here.

@@ +136,5 @@
> +  add_cert_override_test("mismatch-expired.example.com",
> +                         Ci.nsICertOverrideService.ERROR_MISMATCH |
> +                         Ci.nsICertOverrideService.ERROR_TIME,
> +                         getXPCOMStatusFromNSS(SSL_ERROR_BAD_CERT_DOMAIN));
> +  add_cert_override_test("mismatch-untrusted.example.com",

Thanks for pointing this out. I wasn't intending to test any active distrust, but I decided to make the active distrust tests work for both insanity::pkix and classic NSS. Unfortunately, an old bug in PSM (bug 754369) prevents me from getting the insanity::pkix variant of the test working correctly. Please see my updated version of the patch.

Comment 11

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

Attachment: cert-overrides.patch [v2]

Camilo: As I mentioned above, I undid the deletion of the mochitest.

David: I made several changes to the patch based on review comments. Please re-review test_cert_overrides.js and SSLServerCertVerification.cpp.

Comment 12

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8380379 [details] [diff] [review]
cert-overrides.patch [v2]

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

Doesn't look like this is the patch you wanted me to review here.

Comment 13

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

Attachment: cert-overrides.patch [v3]

David, thanks for pointing out that I attached the wrong patch. Here is the right one.

Comment 14

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8380816 [details] [diff] [review]
cert-overrides.patch [v3]

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

r=me with comments addressed.

::: security/manager/ssl/public/nsIX509CertDB.idl
@@ +196,5 @@
>    void setCertTrust(in nsIX509Cert cert,
>                      in unsigned long type,
>                      in unsigned long trust);
>  
> +  void setCertTrustFromString(in nsIX509Cert3 cert, in string trustString);

Maybe add a comment saying "trustString is decoded via CERT_DecodeTrustString".
I think cert only needs to be a nsIX509Cert2 (for GetCert) (of course, I realize they're all the same thing, so I'm not sure it matters).

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +731,5 @@
>      PR_SetError(SEC_ERROR_NO_MEMORY, 0);
>      return nullptr;
>    }
>  
> +  if (!collected_errors) {

From my understanding of the code, it shouldn't be possible to get here with collected_errors == 0, so let's turn this into an invalid state case.

::: security/manager/ssl/src/nsNSSCertificateDB.cpp
@@ +1650,5 @@
> +NS_IMETHODIMP
> +nsNSSCertificateDB::SetCertTrustFromString(nsIX509Cert3* cert,
> +                                           const char* trustString)
> +{
> +

nit: unnecessary empty line

@@ +1664,5 @@
> +  srv = CERT_ChangeCertTrust(CERT_GetDefaultCertDB(), nssCert.get(), &trust);
> +  return MapSECStatus(srv);
> +}
> +
> +

nit: unnecessary empty line

::: security/manager/ssl/tests/unit/test_cert_overrides.js
@@ +46,5 @@
> +  do_check_eq(histogram.counts[ 5], 0 + 1); // SEC_ERROR_EXPIRED_ISSUER_CERTIFICATE
> +  do_check_eq(histogram.counts[ 6], 0 + 1); // SEC_ERROR_UNTRUSTED_CERT
> +  do_check_eq(histogram.counts[ 7], 0);     // SEC_ERROR_INADEQUATE_KEY_USAGE
> +  do_check_eq(histogram.counts[ 8], 2 + 2); // SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED
> +  do_check_eq(histogram.counts[9], 4 + 4); // SSL_ERROR_BAD_CERT_DOMAIN

nit: alignment

@@ +63,5 @@
> +    response.setStatusLine(request.httpVersion, 500, "Internal Server Error");
> +  });
> +  fakeOCSPResponder.start(8080);
> +
> +  // The ordering here is important, because in the classic case we add active

Actually, since you reset the trust at the end of the distrust tests, I think this isn't the case anymore.

@@ -69,4 @@
>    add_cert_override_test("md5signature.example.com",
>                           Ci.nsICertOverrideService.ERROR_UNTRUSTED,
> -                         getXPCOMStatusFromNSS(SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED));
> -  add_cert_override_test("inadequatekeyusage.example.com",

since this isn't used anymore, let's remove it from generate_certs.sh

::: security/manager/ssl/tests/unit/tlsserver/cmd/BadCertServer.cpp
@@ +43,4 @@
>    { nullptr, nullptr }
>  };
>  
> +#include "prthread.h"

nit: I assume you added this for debugging.

@@ +49,5 @@
>  DoSNISocketConfig(PRFileDesc *aFd, const SECItem *aSrvNameArr,
>                    uint32_t aSrvNameArrSize, void *aArg)
>  {
> +//  PR_Sleep(5000);
> +  

nit: looks like some debugging cruft to clean up.

::: security/manager/ssl/tests/unit/tlsserver/generate_certs.sh
@@ +135,5 @@
>  $RUN_MOZILLA $CERTUTIL -d $OUTPUT_DIR -D -n deletedINT
>  make_INT expiredINT 'CN=Expired Test Intermediate' testCA "-w -400"
>  make_EE expiredissuer 'CN=Test End-entity with expired issuer' expiredINT "expiredissuer.example.com"
>  NSS_ALLOW_WEAK_SIGNATURE_ALG=1 make_EE md5signature 'CN=Test End-entity with MD5 signature' testCA "md5signature.example.com" "-Z MD5"
>  make_EE inadequatekeyusage 'CN=Test End-entity with inadequate key usage' testCA "inadequatekeyusage.example.com" "--keyUsage crlSigning"

My understanding is this isn't used anymore, so let's remove it.

@@ +142,5 @@
> +make_EE mismatch-expired 'CN=Mismatch-Expired Test End-entity' testCA "doesntmatch.example.com" "-w -400"
> +make_EE mismatch-untrusted 'CN=Mismatch-Untrusted Test End-entity' otherCA "doesntmatch.example.com"
> +make_EE untrusted-expired 'CN=Untrusted-Expired Test End-entity' otherCA "untrusted-expired.example.com" "-w -400"
> +make_EE mismatch-untrusted-expired 'CN=Mismatch-Untrusted-Expired Test End-entity' otherCA "doesntmatch.example.com" "-w -400"
> +NSS_ALLOW_WEAK_SIGNATURE_ALG=1 make_EE md5signature-expired 'CN=Test MD5Signature-Expired End-entity' testCA "md5signature-expired.example.com" "-Z MD5" "-w -400"

Does this last one work? Since EXTRA_ARGS is just "${5}" in make_EE, I would expect this to result in a certificate that is not expired (i.e. EXTRA_ARGS is just "-Z MD5" and the "-w -400" argument gets ignored). I was expecting this to be "-Z MD5 -w -400". If my reasoning is correct, this is a flaw with make_EE that we can fix in another bug - I think there's a way to say "the rest of the arguments".

Comment 15

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

Comment on attachment 8380816 [details] [diff] [review]
cert-overrides.patch [v3]

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

::: security/manager/ssl/public/nsIX509CertDB.idl
@@ +196,5 @@
>    void setCertTrust(in nsIX509Cert cert,
>                      in unsigned long type,
>                      in unsigned long trust);
>  
> +  void setCertTrustFromString(in nsIX509Cert3 cert, in string trustString);

/**
   * @param cert        The certificate for which to modify trust.
   * @param trustString decoded by CERT_DecodeTrustString. 3 comma separated
   *                    characters, indicating SSL, Email, and Obj signing
   *                    trust.
   */

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +731,5 @@
>      PR_SetError(SEC_ERROR_NO_MEMORY, 0);
>      return nullptr;
>    }
>  
> +  if (!collected_errors) {

I know for a fact that we do get here in the NSSDetermineCertOverrideErrors case (that is, currently, before adding insanity::pkix support for cert error overrides). This happens when CERT_VerifyCert returns an overridable error AND there are ALSO non-overridable errors, for example. (It surprised me as well, when I first found out about this.)

::: security/manager/ssl/tests/unit/test_cert_overrides.js
@@ +63,5 @@
> +    response.setStatusLine(request.httpVersion, 500, "Internal Server Error");
> +  });
> +  fakeOCSPResponder.start(8080);
> +
> +  // The ordering here is important, because in the classic case we add active

Removed the comment.

@@ -69,4 @@
>    add_cert_override_test("md5signature.example.com",
>                           Ci.nsICertOverrideService.ERROR_UNTRUSTED,
> -                         getXPCOMStatusFromNSS(SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED));
> -  add_cert_override_test("inadequatekeyusage.example.com",

Also removed it from BadCertServer.

::: security/manager/ssl/tests/unit/tlsserver/cmd/BadCertServer.cpp
@@ +49,5 @@
>  DoSNISocketConfig(PRFileDesc *aFd, const SECItem *aSrvNameArr,
>                    uint32_t aSrvNameArrSize, void *aArg)
>  {
> +//  PR_Sleep(5000);
> +  

Removed.

::: security/manager/ssl/tests/unit/tlsserver/generate_certs.sh
@@ +142,5 @@
> +make_EE mismatch-expired 'CN=Mismatch-Expired Test End-entity' testCA "doesntmatch.example.com" "-w -400"
> +make_EE mismatch-untrusted 'CN=Mismatch-Untrusted Test End-entity' otherCA "doesntmatch.example.com"
> +make_EE untrusted-expired 'CN=Untrusted-Expired Test End-entity' otherCA "untrusted-expired.example.com" "-w -400"
> +make_EE mismatch-untrusted-expired 'CN=Mismatch-Untrusted-Expired Test End-entity' otherCA "doesntmatch.example.com" "-w -400"
> +NSS_ALLOW_WEAK_SIGNATURE_ALG=1 make_EE md5signature-expired 'CN=Test MD5Signature-Expired End-entity' testCA "md5signature-expired.example.com" "-Z MD5" "-w -400"

Thanks for catching this! I fixed this by making this change:

-  EXTRA_ARGS="${5}"
+  EXTRA_ARGS="${5} ${6}"

Note that error was the cause of the behavior I saw when I filed bug 975776. I marked bug 975776 invalid and I corrected test_cert_overrides.js after regenerating the certs.

Comment 16

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/12661b7737d4

Comment 17

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

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #16)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/12661b7737d4

This was checked in with the commit message "Allow cert error overrides when insanity::pkix is used, r?cviecco, r?keeler," but both patches *were* reviewed. I just forgot to replace the "?"s with "+"s.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/12661b7737d4