1183822 - sha-1 telemetry causing OCSP verification failures

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

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

Bug 1159155 added fallback verification for use of sha-1 in certificate signatures. It works by disabling sha-1 and re-trying if that fails. If an OCSP responder certificate was signed with sha-1, verifying it fails, resulting in a value of Result::ERROR_OCSP_RESPONDER_CERT_INVALID being cached. When we try the fallback with sha-1 enabled, however, we see the cached result and use that without re-verifying the responder certificate (which would succeed).

(Note that with the pref security.OCSP.require set to the default of false, this results in silently ignoring all OCSP responses signed by sha-1 responders. If security.OCSP.require is set to true, Firefox won't connect to sites where it sees such responses.)

Comment 2

[Mark Goodwin [:mgoodwin]]

The plan here is to bypass the fallback for OCSP requests. We lose telemetry for certs used in OCSP signatures - but at least stuff will work.

Comment 3

[Daniel Veditz [:dveditz]]

bug 1183405 lists a number of high profile sites broken if you flip the pref to require OCSP. Ironically our telemetry site is one of the victims given this is due to an attempt to acquire more telemetry.

Comment 4

[Mark Goodwin [:mgoodwin]]

Attachment: Bug 1183822.patch

Existing tests in security/manager/ssl/tests/unit pass (obviously I need to add a sha-1 ocsp signer test - or this wouldn't have happened in the first place).

Also, selected sites in dveditz' list work with security.OCSP.require set.

How do you feel about this approach, Keeler?

Comment 5

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

Comment on attachment 8633750 [details] [diff] [review]
Bug 1183822.patch

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

This seems like a reasonable approach for the moment. We should definitely add appropriate tests and a comment explaining this, as well as a comment in the constructor of NSSCertDBTrustDomain that if it ever does something non-trivial, we'll have to see if it affects this addition.

Comment 6

[Mark Goodwin [:mgoodwin]]

Attachment: Bug 1183822.patch

Added comments

Comment 7

[Mark Goodwin [:mgoodwin]]

Attachment: Bug 1183822 tests.patch

I'd like some help with this, please. My understanding was that I simply needed to make a connection to an endpoint with a different certificate, for which the OCSP response's signer had a SHA-1 certificate. From my reading of GenerateOCSPResponse, I should be able to specify the cert I want a response for, as well as an alternate with which to sign the response...

I've created a SHA-1 cert for a delegated signer - but using this doesn't cause a test failure when I run without the patch for this bug.

I am clearly not reproducing the error condition correctly; what have I done wrong?

Comment 8

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8634170 [details] [diff] [review]
Bug 1183822 tests.patch

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

::: security/manager/ssl/tests/unit/tlsserver/generate_certs.sh
@@ +305,5 @@
>  make_EE expiredissuer 'CN=Test End-entity with expired issuer' expiredINT "expiredissuer.example.com"
>  make_INT notYetValidINT 'CN=Not Yet Valid Test Intermediate' testCA "-w 400"
>  make_EE notYetValidIssuer 'CN=Test End-entity with not yet valid issuer' notYetValidINT "notyetvalidissuer.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 sha1signature 'CN=Test End-entity with SHA1 signature' testCA "localhost,*.example.com" "-Z SHA1"

Ignore this - this is leftover from another tack I tried.

Comment 10

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

Comment on attachment 8634170 [details] [diff] [review]
Bug 1183822 tests.patch

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

Turns out we needed to add a new mapping to GenerateOCSPResponse. Also, generate_certs has some gotchas, but we're getting rid of it soon anyway (hopefully). See comments below.

::: security/manager/ssl/tests/unit/test_ocsp_caching.js
@@ +33,5 @@
>    response.bodyOutputStream.write(body, body.length);
>  }
>  
> +function generateSHA1OCSPResponse() {
> +  let args = [ ["good", "sameIssuerEE", "delegatedSHA1Signer" ] ];

In GenerateOCSPResponse.cpp, we'll have to add a mapping like { "good-delegated", ORTDelegatedIncluded } to kOCSPResponseNameList to enable it to generate delegated OCSP responses (and then use "good-delegated" here instead of "good"). (The code we want to run is here: https://hg.mozilla.org/mozilla-central/annotate/e7e69cc8c07b/security/manager/ssl/tests/unit/tlsserver/lib/OCSPCommon.cpp#l99 so the OCSP response type needs to be ORTDelegatedIncluded instead of just ORTGood.)

@@ +206,5 @@
> +    Services.prefs.setBoolPref("security.OCSP.require", true);
> +    run_next_test();
> +  });
> +
> +  add_ocsp_test("ocsp-stapling-none-sha1.example.com", PRErrorCodeSuccess,

I think you can just use ocsp-stapling-none.example.com here.

::: security/manager/ssl/tests/unit/tlsserver/generate_certs.sh
@@ +320,5 @@
>  export_cert inadequatekeyusage inadequatekeyusage-ee.der
>  make_EE selfsigned-inadequateEKU 'CN=Self-signed Inadequate EKU Test End-entity' unused "selfsigned-inadequateEKU.example.com" "--keyUsage keyEncipherment,dataEncipherment --extKeyUsage serverAuth" "-x"
>  
>  make_delegated delegatedSigner 'CN=Test Delegated Responder' testCA "--extKeyUsage ocspResponder"
> +make_delegated delegatedSHA1Signer 'CN=Test SHA1 Delegated Responder' testCA "--extKeyUsage ocspResponder" "-Z SHA1"

Looks like this last part should be "--extKeyUsage ocspResponder -Z SHA1" all together instead of as two arguments (yay bash).

Comment 11

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

(In reply to Mark Goodwin [:mgoodwin] from comment #7)
> Created attachment 8634170 [details] [diff] [review]
> Bug 1183822 tests.patch
> 
> I'd like some help with this, please. My understanding was that I simply
> needed to make a connection to an endpoint with a different certificate, for
> which the OCSP response's signer had a SHA-1 certificate. From my reading of
> GenerateOCSPResponse, I should be able to specify the cert I want a response
> for, as well as an alternate with which to sign the response...

Right, but in this case we don't actually need to use a different end-entity certificate. We just need to specify the delegated signer and have it actually do a delegated signing (see comment 10).

Comment 12

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

Comment on attachment 8634164 [details] [diff] [review]
Bug 1183822.patch

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

Sorry to change direction on you, but upon considering this some more, I think we should take a slightly different approach (see below). It will mean more code, but it should be more clear what's going on.

::: security/certverifier/NSSCertDBTrustDomain.cpp
@@ +66,5 @@
>    , mBuiltChain(builtChain)
>    , mCertBlocklist(do_GetService(NS_CERTBLOCKLIST_CONTRACTID))
>    , mOCSPStaplingStatus(CertVerifier::OCSP_STAPLING_NEVER_CHECKED)
>  {
> +  // If this constructor changes to do something non-trivial, check the creation

I think this will be obviated by my new suggestion (below).

@@ +645,5 @@
> +  // we create a new TrustDomain here, rather than using *this, since OCSP
> +  // failures are cached. Because of this, the 'try and fallback' approach for
> +  // accumulating telemetry would cause OCSP to fail where the response
> +  // signer's certificate uses a weak signature digest algorithm.
> +  NSSCertDBTrustDomain trustDomain(mCertDBTrustType, mOCSPFetching, mOCSPCache,

I think it would be best to leverage the type system a bit more for this. That is, we should define a new TrustDomain that accepts all signature algorithms but returns Result::ERROR_LIBRARY_FAILURE for any function other than the few we're expecting to be called on it (e.g. FindIssuer, CheckRevocation, and IsChainValid should never be called on it). I guess for things like CheckECDSACurveIsAcceptable we should pass through to the original NSSCertDBTrustDomain.

Comment 13

[Mark Goodwin [:mgoodwin]]

Attachment: Bug 1183822 tests.patch

The tests work now. I added the new mapping for good-delegated responses and modified the tests and generation scripts accordingly.

Comment 14

[Mark Goodwin [:mgoodwin]]

Attachment: Bug 1183822.patch

Adds a new TrustDomain for OCSP Signers which will always allow all acceptible signature digest algorithms. Calls to most other TrustDomain methods are passed through to the owning NSSCertDBTrustDomain.

Comment 15

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

Comment on attachment 8634678 [details] [diff] [review]
Bug 1183822.patch

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

Great - just a few comments, but otherwise r=me.

::: security/certverifier/NSSCertDBTrustDomain.cpp
@@ +642,5 @@
> +
> +  // we create a new OCSPSignerTrustDomain here, rather than using *this, since
> +  // OCSP failures are cached. Because of this, the 'try and fallback' approach
> +  // for accumulating telemetry would cause OCSP to fail where the response
> +  // signer's certificate uses a weak signature digest algorithm.

I might restructure this a bit. Start with that we are doing the 'try and fallback' approach for signature digest algorithms. If a delegated OCSP response signing certificate was issued with a weak signature digest algorithm, verification initially fails. We cache the failure and then re-use that result even when doing fallback (i.e. when weak signature digest algorithms should succeed).

::: security/certverifier/OCSPSignerTrustDomain.cpp
@@ +9,5 @@
> +using namespace mozilla;
> +using namespace mozilla::pkix;
> +
> +OCSPSignerTrustDomain::OCSPSignerTrustDomain(
> +                         NSSCertDBTrustDomain& certDBTrustDomain)

If this is too long for one line, I would just indent the second line two spaces from the start of the line.

@@ +27,5 @@
> +
> +
> +Result
> +OCSPSignerTrustDomain::FindIssuer(Input encodedIssuerName,
> +                                  IssuerChecker& checker, Time time)

nit: unused arguments don't need to have names here

@@ +34,5 @@
> +  return Result::FATAL_ERROR_LIBRARY_FAILURE;
> +}
> +
> +Result
> +OCSPSignerTrustDomain::IsChainValid(const DERArray& certArray, Time time)

nit: same, etc.

@@ +58,5 @@
> +{
> +  // The reason for wrapping the NSSCertDBTrustDomain in an
> +  // OCSPSignerTrustDomain is to allow us to bypass the weaker signature
> +  // algorithm check - thus all allowable signature digest algorithms should
> +  // always be accepted.

I would add that this is more or less temporary while we gather telemetry on SHA-1. When we disable SHA-1, we can undo this.

::: security/certverifier/moz.build
@@ +13,5 @@
>      'CertVerifier.cpp',
>      'NSSCertDBTrustDomain.cpp',
>      'OCSPCache.cpp',
>      'OCSPRequestor.cpp',
> +    'OCSPSignerTrustDomain.cpp',

Maybe OCSPVerificationTrustDomain?

Comment 16

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

Comment on attachment 8634676 [details] [diff] [review]
Bug 1183822 tests.patch

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

Awesome!

::: security/manager/ssl/tests/unit/test_ocsp_caching.js
@@ +19,5 @@
>    response.write(gGoodOCSPResponse);
>  }
>  
> +function respondWithSHA1OCSP(request, response) {
> +  do_print("returning 200 OK");

Maybe add "... with response from delegated responder issued with SHA-1 signature" (or something shorter)

Comment 17

[Mark Goodwin [:mgoodwin]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e335112b28e

Comment 18

[Mark Goodwin [:mgoodwin]]

Attachment: Bug 1183822 tests.patch

Nit addressed (added sha-1 delegated response to the HTTP response debug)

Comment 19

[Mark Goodwin [:mgoodwin]]

Attachment: Bug 1183822.patch

Addressed nits. In particular:
 - formatting nits
 - unused arguments are no longer named
 - OCSPSignerTrustDomain renamed to OCSPVerificationTrustDomain
 - comments expanded and clarified

Comment 20

[Mark Goodwin [:mgoodwin]]

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/fb6cbb4ada544b1d4e690b8dad1e067c2e3e609b
changeset:  fb6cbb4ada544b1d4e690b8dad1e067c2e3e609b
user:       Mark Goodwin <[e-mail]>
date:       Fri Jul 17 10:03:56 2015 +0100
description:
Bug 1183822 - fix OCSP verification failures (r=keeler)
Adds a new TrustDomain for OCSP Signers which will always allow all acceptible
signature digest algorithms. Calls to most other TrustDomain methods are passed
through to the owning NSSCertDBTrustDomain.

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/f324dcfaab40316a32888591053bd27b4c353658
changeset:  f324dcfaab40316a32888591053bd27b4c353658
user:       Mark Goodwin <[e-mail]>
date:       Fri Jul 17 10:04:17 2015 +0100
description:
Bug 1183822 - Add an OCSP test for signers with SHA-1 certificates (r=keeler)

Comment 21

[Mark Goodwin [:mgoodwin]]

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/91754a3281fbaf6049241103e4b02d251cfb923d
changeset:  91754a3281fbaf6049241103e4b02d251cfb923d
user:       Mark Goodwin <[e-mail]>
date:       Fri Jul 17 10:36:56 2015 +0100
description:
Backed out changeset f324dcfaab40 (bug 1183822)

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/99b36484d3bce5f278764fc617981560ca6b46a6
changeset:  99b36484d3bce5f278764fc617981560ca6b46a6
user:       Mark Goodwin <[e-mail]>
date:       Fri Jul 17 10:36:58 2015 +0100
description:
Backed out changeset fb6cbb4ada54 (bug 1183822)

Comment 22

[Mark Goodwin [:mgoodwin]]

Backed out: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=d2f8da515976

Comment 23

[Mark Goodwin [:mgoodwin]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f29ee2ca66a

Comment 24

[Mark Goodwin [:mgoodwin]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e81b2a01218

Comment 25

[Mark Goodwin [:mgoodwin]]

Attachment: Bug 1183822.patch

Marked constructor as explicit to fix static checking build failures.

Comment 26

[Mark Goodwin [:mgoodwin]]

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/4caca8feef1fe207d00a1f43bb6859db685000d5
changeset:  4caca8feef1fe207d00a1f43bb6859db685000d5
user:       Mark Goodwin <[e-mail]>
date:       Fri Jul 17 17:07:48 2015 +0100
description:
Bug 1183822 - fix OCSP verification failures (r=keeler)
Adds a new TrustDomain for OCSP Signers which will always allow all acceptible
signature digest algorithms. Calls to most other TrustDomain methods are passed
through to the owning NSSCertDBTrustDomain.

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/9d4dd81565a740bbdf500683e549adfec5fc1d52
changeset:  9d4dd81565a740bbdf500683e549adfec5fc1d52
user:       Mark Goodwin <[e-mail]>
date:       Fri Jul 17 17:07:50 2015 +0100
description:
Bug 1183822 - Add an OCSP test for signers with SHA-1 certificates (r=keeler)

Comment 27

[Mark Goodwin [:mgoodwin]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ee53e4a4aa4 <- a clean try run, this time with static checking

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4caca8feef1f
https://hg.mozilla.org/mozilla-central/rev/9d4dd81565a7