921885 - Use insanity::pkix for EV (extended validation) certificate verification

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P1)

Description

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

+++ This bug was initially created as a clone of Bug #915930 +++

Currently we use libpkix for EV even when insanity::pkix is used for non-EV verification. We should use insanity::pkix for EV certificate verification too.

Comment 1

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

Camilo wrote the initial version of this, though it will need to be rebased and perhaps modified.

Comment 2

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

Attachment: insanity-ev.patch

Regarding the tests: I modified the existing test coverage in this patch. I am still working on another patch to extend the test coverage, which I will attach as a separate patch to this bug.

Comment 3

[Camilo Viecco (:cviecco)]

Comment on attachment 8380881 [details] [diff] [review]
insanity-ev.patch

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

::: b2g/confvars.sh
@@ +25,5 @@
>  MOZ_WEBSMS_BACKEND=1
>  MOZ_DISABLE_CRYPTOLEGACY=1
>  MOZ_APP_STATIC_INI=1
>  NSS_NO_LIBPKIX=1
> +MOZ_NO_EV_CERTS=1

I just remembered asking for the name of the config to be this :)

::: security/certverifier/CertVerifier.cpp
@@ +443,5 @@
> +  if (mImplementation == insanity) {
> +    return InsanityVerifyCert(cert, usage, time, pinArg, flags,
> +                              stapledOCSPResponse, validationChain,
> +                              evOidPolicy);
> +  }

you are dupping the logic by moving this so far up on the call. Please move it to after sanity checks/ init values.

::: security/certverifier/NSSCertDBTrustDomain.cpp
@@ +124,5 @@
> +      if (CertIsAuthoritativeForEVPolicy(candidateCert, policy)) {
> +        *trustLevel = TrustAnchor;
> +        return SECSuccess;
> +      }
> +#endif

Inherits trust sounds incorrect for the return value on failure, but I cannot think of a better name for the enum.

@@ +191,5 @@
> +      (endEntityOrCA == MustBeCA && (mOCSPFetching == FetchOCSPForDVHardFail ||
> +                                     mOCSPFetching == FetchOCSPForDVSoftFail))) {
> +    return SECSuccess;
> +  }
> +

This logic is just convoluted. I think it will be simpler if you use my suggestion for enums

::: security/certverifier/NSSCertDBTrustDomain.h
@@ +52,5 @@
> +    FetchOCSPForDVSoftFail = 1,
> +    FetchOCSPForDVHardFail = 2,
> +    FetchOCSPForEV = 3,
> +    LocalOnlyOCSPForEV = 4,
> +  };

I think this is a regresion on readability. I would keep the localonly flag and two enums:

revocation_depth {
  ee_only = 0;
  ee_and_intermediates = 1;
}

and another one 
revocation_strictness{
  softFail = 0;
  mandatoryOnOCSPURL = 1; //ie strict revocation, stapling is ok
  mandatory = 2;     // what ev actualy uses

}


Then your logic of processing options would be much more readably.

::: security/manager/ssl/tests/unit/head_psm.js
@@ +18,4 @@
>  const isDebugBuild = Cc["@mozilla.org/xpcom/debug;1"]
>                         .getService(Ci.nsIDebug2).isDebugBuild;
>  
>  const SEC_ERROR_BASE = Ci.nsINSSErrorsService.NSS_SEC_ERROR_BASE;

Just comment this to be // -8192 instead of the comments below

Comment 4

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

Comment on attachment 8380881 [details] [diff] [review]
insanity-ev.patch

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

::: security/certverifier/CertVerifier.cpp
@@ +443,5 @@
> +  if (mImplementation == insanity) {
> +    return InsanityVerifyCert(cert, usage, time, pinArg, flags,
> +                              stapledOCSPResponse, validationChain,
> +                              evOidPolicy);
> +  }

Like in bug 975122, the duplication is temporary. This way, we can see that the entire rest of the function can be deleted when we fix bug 975229 so that we're only using insanity::pkix. In fact, we'll be able to just rename "InsanityVerifyCert" to "VerifyCert" (rearranging the parameter order) and delete the current VerifyCert.

Consequently, I'd like to keep the code the way I wrote it here, as long as it is correctly functioning.

::: security/certverifier/NSSCertDBTrustDomain.h
@@ +52,5 @@
> +    FetchOCSPForDVSoftFail = 1,
> +    FetchOCSPForDVHardFail = 2,
> +    FetchOCSPForEV = 3,
> +    LocalOnlyOCSPForEV = 4,
> +  };

I thought about doing this, but I avoided doing so because it results in a bigger matrix of possibilities than what I actually implemented. Consequently, we'd either waste time writing tests for combinations that we don't use, or we'd avoid testing cases that the code would be trying to handle. So, I would like to avoid making this change too.

::: security/manager/ssl/tests/unit/head_psm.js
@@ +18,4 @@
>  const isDebugBuild = Cc["@mozilla.org/xpcom/debug;1"]
>                         .getService(Ci.nsIDebug2).isDebugBuild;
>  
>  const SEC_ERROR_BASE = Ci.nsINSSErrorsService.NSS_SEC_ERROR_BASE;

The comments below are actually much more helpful than just adding "// -8192" here. When the tests fail, we get a message like "TEST-UNEXPECTED-FAIL: -8172 != -8032". Currently I have to use a calculator to figure out what the expected error is and/or what the actual error is. This way, I can just look things up by number.

(I built up this set of comments by debugging messages like "TEST-UNEXPECTED-FAIL: -8172 != -8032" during the course of modifying this test.)

Comment 5

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

Comment on attachment 8380881 [details] [diff] [review]
insanity-ev.patch

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

I think there could be some more explanatory comments in NSSCertDBTrustDomain::CheckRevocation, but overall I think this looks good.

::: configure.in
@@ +6284,5 @@
>  fi
>  AC_SUBST(MOZ_DISABLE_CRYPTOLEGACY)
>  
>  dnl ========================================================
>  dnl = Disable libpkix

"Disable EV certificates"?

::: security/certverifier/CertVerifier.cpp
@@ +244,5 @@
> +  if (evOidPolicy) {
> +    *evOidPolicy = SEC_OID_UNKNOWN;
> +  }
> +
> +  if (!cert || (usage != certificateUsageSSLServer && (flags & FLAG_MUST_BE_EV))) {

nit: I think this line is >80 chars.

::: security/certverifier/NSSCertDBTrustDomain.cpp
@@ +191,5 @@
> +      (endEntityOrCA == MustBeCA && (mOCSPFetching == FetchOCSPForDVHardFail ||
> +                                     mOCSPFetching == FetchOCSPForDVSoftFail))) {
> +    return SECSuccess;
> +  }
> +

I agree the logic is convoluted, but I'm not sure anything could make it much more clear. Comments might help. Also, FWIW, I did manage to convince myself that it's correct.

@@ +206,5 @@
> +  // interpretation of "strict" revocation checking in the face of a
> +  // certificate that lacks an OCSP responder URI.
> +  if (!url) {
> +    if (stapledOCSPResponse) {
> +      PR_SetError(SEC_ERROR_OCSP_OLD_RESPONSE, 0);

The comment doesn't really match the behavior here.

@@ +233,5 @@
> +  if (!response) {
> +    if (mOCSPFetching != FetchOCSPForDVSoftFail) {
> +      PR_LOG(gCertVerifierLog, PR_LOG_DEBUG,
> +        ("NSSCertDBTrustDomain: returning SECFailure after "
> +        "CERT_PostOCSPRequest failure"));

nit: indentation

@@ +239,5 @@
>      }
>  
> +    PR_LOG(gCertVerifierLog, PR_LOG_DEBUG,
> +      ("NSSCertDBTrustDomain: returning SECSuccess after "
> +        "CERT_PostOCSPRequest failure"));

nit: indentation

::: security/manager/ssl/tests/unit/head_psm.js
@@ +18,4 @@
>  const isDebugBuild = Cc["@mozilla.org/xpcom/debug;1"]
>                         .getService(Ci.nsIDebug2).isDebugBuild;
>  
>  const SEC_ERROR_BASE = Ci.nsINSSErrorsService.NSS_SEC_ERROR_BASE;

Wasn't there a site that would take any xpcom/nss/etc. error and decode it? I saw it once but now can't remember what it was.

Comment 6

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

Comment on attachment 8380881 [details] [diff] [review]
insanity-ev.patch

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

::: security/manager/ssl/tests/unit/head_psm.js
@@ +18,4 @@
>  const isDebugBuild = Cc["@mozilla.org/xpcom/debug;1"]
>                         .getService(Ci.nsIDebug2).isDebugBuild;
>  
>  const SEC_ERROR_BASE = Ci.nsINSSErrorsService.NSS_SEC_ERROR_BASE;

http://james-ross.co.uk/mozilla/misc/nserror.

Trust me, these comments still save a lot of effort.

Comment 7

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

Comment on attachment 8380881 [details] [diff] [review]
insanity-ev.patch

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

::: security/certverifier/NSSCertDBTrustDomain.cpp
@@ +191,5 @@
> +      (endEntityOrCA == MustBeCA && (mOCSPFetching == FetchOCSPForDVHardFail ||
> +                                     mOCSPFetching == FetchOCSPForDVSoftFail))) {
> +    return SECSuccess;
> +  }
> +

Talked to cviecco. He is also convinced the code is correct. He said that after this lands, he will give a go at trying to make it clearer.

FWIW, I did try Camilo's suggestion as my first attempt, and I found that it was hard to convince myself it was correct, so I changed to this approach. I would be happy to see somebody improve upon the code though.

@@ +206,5 @@
> +  // interpretation of "strict" revocation checking in the face of a
> +  // certificate that lacks an OCSP responder URI.
> +  if (!url) {
> +    if (stapledOCSPResponse) {
> +      PR_SetError(SEC_ERROR_OCSP_OLD_RESPONSE, 0);

I moved the comment to [1].

@@ +212,5 @@
>      }
> +    if (mOCSPFetching == FetchOCSPForEV) {
> +      PR_SetError(SEC_ERROR_OCSP_UNKNOWN_CERT, 0);
> +      return SECFailure;
> +    }

[1] I moved the comment here.

Comment 8

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

Attachment: insanity-ev.patch [review comments addressed]

r=cviecco, r=keeler.

Thanks for the reviews!

Comment 9

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/b7030189c2ca

I also filed follow-up bug 977307 for expanding the tests since I noticed the test coverage here is still a little too limited. It isn't the most urgent thing to do now since the EV vs. DV indicator isn't as critical as other things that need more testing.

Comment 10

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

Comment on attachment 8381960 [details] [diff] [review]
insanity-ev.patch [review comments addressed]

[Approval Request Comment]
See bug 915931 comment 43.

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Brian, could you land this patch in m-c before? thanks

Comment 12

[Carsten Book [:Tomcat]]

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

Comment 13

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/836391ce81b8

Comment 14

[Philip Chee]

I hope I'm not exposing the depths of my ignorance by asking the following question:

> +++ b/security/certverifier/ExtendedValidation.cpp
> +#include "insanity/nullptr.h"
Is there a reason we can't use:
#include "mozilla/NullPtr.h"

I has just written a patch to stop ExtendedValidation.cpp from burning SeaMonkey-aurora due to our compiler toolchain not understanding nullptr, but I see you're ahead of me here.

Comment 15

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

(In reply to Philip Chee from comment #14)
> I hope I'm not exposing the depths of my ignorance by asking the following
> question:
> 
> > +++ b/security/certverifier/ExtendedValidation.cpp
> > +#include "insanity/nullptr.h"
> Is there a reason we can't use:
> #include "mozilla/NullPtr.h"

One of the goals of insanity::pkix and the certificate verifier (certverifier) that uses it is to only depend on low-level Mozilla libraries like NSPR and NSS. That way, it can be used by other projects without requiring the rest of the tree.