Closed Bug 1159155 Opened 5 years ago Closed 4 years ago

Add telemetry probe for SHA-1 usage

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox40 --- affected
firefox42 --- fixed

People

(Reporter: mgoodwin, Assigned: mgoodwin)

References

Details

Attachments

(2 files, 10 obsolete files)

38.57 KB, patch
mgoodwin
: review+
Details | Diff | Splinter Review
20.28 KB, patch
mgoodwin
: review+
Details | Diff | Splinter Review
No description provided.
Attachment #8615972 - Flags: review?(dkeeler)
Attachment #8615972 - Attachment is obsolete: true
Attachment #8615972 - Flags: review?(dkeeler)
Attachment #8615973 - Flags: review?(dkeeler)
This will 'work' in the sense that it will give us telemetry on how many cert chains contain certificates that use SHA-1 signatures but I'm not completely happy with this approach for the following reasons:
1) it would be nice to have separate bins for root / intermediate / ee
2) this approach involves adding (yet another) args to the NSSCertDBTrustDomain constructor which already has a long arg list (could we have a TrustDomainOpts which means we can change options without breaking all the call sites every time?)
3) I notice there's an optional out param for getting the cert chain - could we inspect that for the signature algorithm?
Flags: needinfo?(dkeeler)
(In reply to Mark Goodwin [:mgoodwin] from comment #3)
> This will 'work' in the sense that it will give us telemetry on how many
> cert chains contain certificates that use SHA-1 signatures but I'm not
> completely happy with this approach for the following reasons:
> 1) it would be nice to have separate bins for root / intermediate / ee

This is doable. First thing to note is that we don't actually check the signatures on roots (because it's not necessary). One change that would have to be made is to pass in the EndEntityOrCA parameter to CheckSignatureDigestAlgorithm in pkixcheck.cpp. However, to measure accurately we would probably have to do the "try some sort of strict mode and then fall back" bit for each of (disallow sha1 completely, allow only for end-entities, allow only for intermediates) or whatever we wanted.

> 2) this approach involves adding (yet another) args to the
> NSSCertDBTrustDomain constructor which already has a long arg list (could we
> have a TrustDomainOpts which means we can change options without breaking
> all the call sites every time?)

I see what you mean. I would be open to some refactoring.

> 3) I notice there's an optional out param for getting the cert chain - could
> we inspect that for the signature algorithm?

Unfortunately this could lead to over-counting the compatibility risk of disabling sha1. Suppose there were two valid chains but the library found and returned the one with a sha1 signature (where the other chain didn't have a sha1 signature). Such a measurement would count that as a potential risk when it wasn't in fact problematic.
Flags: needinfo?(dkeeler)
Comment on attachment 8615973 [details] [diff] [review]
Add telemetry probe for sha1 certificates

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

This looks really good, and is almost exactly what we want, functionality-wise (one thing is I think we do need to handle the "already bad" case as well, so we have a more complete picture of relative failure rates). The other thing is there's a lot of code duplication, so I think we should look into what refactoring we can do. Having some sort of options mechanism (or maybe being able to set the options directly?) sounds reasonable. r- for now to see what we can do there.
Another thought is should we measure failures with EV separately?

::: security/certverifier/CertVerifier.cpp
@@ +378,2 @@
>            *keySizeStatus = KeySizeStatus::AlreadyBad;
>          }

This is where we would add the already bad case to the signature digest status telemetry.

::: security/certverifier/CertVerifier.h
@@ +23,5 @@
>    AlreadyBad = 3,
>  };
>  
> +// These values correspond to the CERT_CHAIN_SIGNATURE_DIGEST telemetry.
> +enum class SigDigestStatus {

Let's call this "SignatureDigestStatus" to be more consistent with the CheckSignatureDigestAlgorithm function.

@@ +25,5 @@
>  
> +// These values correspond to the CERT_CHAIN_SIGNATURE_DIGEST telemetry.
> +enum class SigDigestStatus {
> +  NeverChecked = 0,
> +  GoodAlgorithmsOnly = 1,

Maybe "StrongAlgorithmsSucceeded"?

@@ +26,5 @@
> +// These values correspond to the CERT_CHAIN_SIGNATURE_DIGEST telemetry.
> +enum class SigDigestStatus {
> +  NeverChecked = 0,
> +  GoodAlgorithmsOnly = 1,
> +  WeakDigestAlgorithmPresent = 2,

To be consistent, maybe "CompatibilityRisk"?
We should have an "AlreadyBad" value as well.

::: security/certverifier/NSSCertDBTrustDomain.cpp
@@ +800,3 @@
>  {
> +  if (aAlg == DigestAlgorithm::sha1) {
> +    return mPreventSHA1 ? Result::ERROR_BAD_SIGNATURE : Success;

Let's use Result::ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED

::: security/certverifier/NSSCertDBTrustDomain.h
@@ +55,5 @@
>                         CertVerifier::OcspGetConfig ocspGETConfig,
>                         uint32_t certShortLifetimeInDays,
>                         CertVerifier::PinningMode pinningMode,
>                         unsigned int minRSABits,
> +          /*optional*/ const bool preventSHA1 = false,

bools aren't very descriptive as arguments. I think it would be best to define another enum type. Maybe it could look something like this:

enum class SignatureDigestConfig {
  AcceptAllAlgorithms = 0,
  DisableSHA1 = 1,
};

::: toolkit/components/telemetry/Histograms.json
@@ +7297,5 @@
>    },
> +  "CERT_CHAIN_SIGNATURE_DIGEST_STATUS": {
> +    "expires_in_version": "default",
> +    "kind": "enumerated",
> +    "n_values": 6,

Why 6? Shouldn't we do 4 like in CERT_CHAIN_KEY_SIZE_STATUS? (the 4th bucket is the "already bad" bucket)
Attachment #8615973 - Flags: review?(dkeeler) → review-
Developing on the previous idea. In particular:
* looping over different key size / signature algorithm options to cut down on duplicated code
* distinguishing between ee and CA certs

I've not finished cleaning up (so I'm aware there are nits) but I wanted your OK on the approach before I spent more time.
Attachment #8615973 - Attachment is obsolete: true
Attachment #8622589 - Flags: feedback?(dkeeler)
Comment on attachment 8622589 [details] [diff] [review]
Add telemetry probe for sha1 certificates

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

Cool - this looks like a reasonable approach.

::: security/certverifier/CertVerifier.cpp
@@ +193,5 @@
>      }
>      stapledOCSPResponse = &stapledOCSPResponseInput;
>    }
>  
> +  TrustOptions allowAll;

Maybe allowAllDigestAlgorithms? Also, maybe these should be in the same order as listed in digestAlgorithmOptions.

@@ +246,5 @@
> +        SigDigestStatus::WeakCACert,
> +        SigDigestStatus::WeakCAAndEE
> +      };
> +
> +      int digestAlgorithmOptionsCount = 3;

Since we statically know the sizes of the arrays we're using, let's just use MOZ_ARRAY_LENGTH (indeed - isn't this incorrect at the moment?)

@@ +248,5 @@
> +      };
> +
> +      int digestAlgorithmOptionsCount = 3;
> +
> +      bool success = false;

We already have the Result rv to indicate success, so I'm not sure we need another one.

@@ +263,5 @@
>        SECOidTag evPolicyOidTag;
>        SECStatus srv = GetFirstEVPolicy(cert, evPolicy, evPolicyOidTag);
> +
> +      for (int i=0; i<digestAlgorithmOptionsCount && success == false; i++) {
> +        if (srv == SECSuccess) {

The extra level of indentation is a bit unfortunate here - maybe move 'src == SECSuccess' into the loop condition?

@@ +282,5 @@
> +            if (evOidPolicy) {
> +              *evOidPolicy = evPolicyOidTag;
> +            }
> +            if (sigDigestStatus) {
> +              *sigDigestStatus = digestAlgorithmStatuses[i];

Should we maybe keep separate telemetry for EV?
Attachment #8622589 - Flags: feedback?(dkeeler) → feedback+
Feedback addressed.

(In reply to David Keeler [:keeler] (use needinfo?) from comment #7)
> Comment on attachment 8622589 [details] [diff] [review]
> Add telemetry probe for sha1 certificates
>
> Should we maybe keep separate telemetry for EV?

Yes. I'm guessing this would make a good follow up?
Attachment #8622589 - Attachment is obsolete: true
Attachment #8623703 - Flags: review?(dkeeler)
Comment on attachment 8623703 [details] [diff] [review]
Add telemetry probe for sha1 certificates.patch

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

Looking good. I made a few comments, and I think I should have a final look with those addressed, so r- for now.

::: security/certverifier/CertVerifier.cpp
@@ +129,5 @@
>    MOZ_LOG(gCertVerifierLog, LogLevel::Debug, ("Top of VerifyCert\n"));
>  
>    PR_ASSERT(cert);
>    PR_ASSERT(usage == certificateUsageSSLServer || !(flags & FLAG_MUST_BE_EV));
>    PR_ASSERT(usage == certificateUsageSSLServer || !keySizeStatus);

We should also add a "PR_ASSERT(usage == certificateUsageSSLServer || !sigDigestStatus);" line and associated non-debug error handling (see how keySizeStatus is handled).

@@ +244,5 @@
> +        SigDigestStatus::GoodAlgorithmsOnly,
> +        SigDigestStatus::WeakEECert,
> +        SigDigestStatus::WeakCACert,
> +        SigDigestStatus::WeakCAAndEE
> +      };

We should probably add a static_assert that MOZ_ARRAY_LENGTH(digestAlgorithmOptions) == MOZ_ARRAY_LENGTH(digestAlgorithmStatuses) (and similarly for keySizeOptions).

@@ +261,5 @@
>        CertPolicyId evPolicy;
>        SECOidTag evPolicyOidTag;
>        SECStatus srv = GetFirstEVPolicy(cert, evPolicy, evPolicyOidTag);
> +
> +      for (int i=0;

nit: 'size_t i = 0;'

@@ +268,2 @@
>          NSSCertDBTrustDomain
> +            trustDomain(trustSSL, evOCSPFetching,

nit: looks like this got indented two more spaces than necessary

@@ +311,5 @@
> +      };
> +
> +      int keySizeOptionsCount = MOZ_ARRAY_LENGTH(keySizeStatuses);
> +
> +      for (int i=0; i<keySizeOptionsCount && rv != Success; i++) {

nit: 'size_t i = 0; i < keySizeOptionsCount...' etc.

@@ +314,5 @@
> +
> +      for (int i=0; i<keySizeOptionsCount && rv != Success; i++) {
> +        for (int j=0; j<digestAlgorithmOptionsCount && rv != Success; j++) {
> +          NSSCertDBTrustDomain trustDomain(trustSSL, defaultOCSPFetching,
> +                                          mOCSPCache, pinArg, ocspGETConfig,

nit: alignment

::: security/certverifier/CertVerifier.h
@@ +23,5 @@
>    AlreadyBad = 3,
>  };
>  
> +// These values correspond to the CERT_CHAIN_SIGNATURE_DIGEST telemetry.
> +enum class SigDigestStatus {

It's longer, but I think it might be worth it to not abbreviate Signature here for clarity: "SignatureDigestStatus"

::: security/certverifier/NSSCertDBTrustDomain.cpp
@@ +801,3 @@
>  {
> +  MOZ_LOG(gCertVerifierLog, LogLevel::Debug,
> +           ("NSSCertDBTrustDomain: CheckSignatureDigestAlgorithm"));

nit: alignment - move this line one space left

@@ +804,5 @@
> +  if (aAlg == DigestAlgorithm::sha1) {
> +    if (endEntityOrCA == EndEntityOrCA::MustBeCA) {
> +    MOZ_LOG(gCertVerifierLog, LogLevel::Debug, ("CA cert is SHA1"));
> +      return mTrustOptions.signatureDigestOptionCA == DisableSHA1 ?
> +          Result::ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED : Success;

Let's reformat these ternaries:

return mTrustOptions.signatureDigestOptionCA == DisableSHA1
       ? Result::ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED
       : Success;

::: security/certverifier/NSSCertDBTrustDomain.h
@@ +42,5 @@
> +};
> +
> +struct TrustOptions {
> +  SignatureDigestOption signatureDigestOptionEE;
> +  SignatureDigestOption signatureDigestOptionCA;

I think we can do away with a level of indirection by just defining SignatureDigestOption as:

AcceptAllAlgorithms,
DisableSHA1ForEE,
DisableSHA1ForCA,
DisableSHA1Everywhere

@@ +65,5 @@
>                         CertVerifier::OcspGetConfig ocspGETConfig,
>                         uint32_t certShortLifetimeInDays,
>                         CertVerifier::PinningMode pinningMode,
>                         unsigned int minRSABits,
> +                       TrustOptions& trustOptions,

We can pass this by value - it doesn't need to be a reference.
Attachment #8623703 - Flags: review?(dkeeler) → review-
Feedback addressed. In particular:
* Lots of nits addressed
* TrustOptions replaced with a larger set of SignatureDigestOptions
Attachment #8623703 - Attachment is obsolete: true
Attachment #8624311 - Flags: review?(dkeeler)
Comment on attachment 8624311 [details] [diff] [review]
Add telemetry probe for sha1 certificates.patch

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

This looks great. Just some formatting nits.

::: security/certverifier/CertVerifier.cpp
@@ +258,5 @@
>        CertPolicyId evPolicy;
>        SECOidTag evPolicyOidTag;
>        SECStatus srv = GetFirstEVPolicy(cert, evPolicy, evPolicyOidTag);
> +
> +      for (size_t i=0;

nit: spaces around '='

@@ +313,5 @@
> +
> +      size_t keySizeOptionsCount = MOZ_ARRAY_LENGTH(keySizeStatuses);
> +
> +      for (size_t i=0; i<keySizeOptionsCount && rv != Success; i++) {
> +        for (size_t j=0; j<digestAlgorithmOptionsCount && rv != Success; j++) {

nit: spaces around '=' and '<'

::: security/certverifier/NSSCertDBTrustDomain.cpp
@@ +806,5 @@
> +      return Result::ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED;
> +    }
> +
> +    if (endEntityOrCA == EndEntityOrCA::MustBeCA) {
> +    MOZ_LOG(gCertVerifierLog, LogLevel::Debug, ("CA cert is SHA1"));

nit: indentation

@@ +811,5 @@
> +      return mSignatureDigestOption == DisableSHA1ForCA
> +             ? Result::ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED
> +             : Success;
> +    } else {
> +    MOZ_LOG(gCertVerifierLog, LogLevel::Debug, ("EE cert is SHA1"));

nit: indentation

::: security/certverifier/NSSCertDBTrustDomain.h
@@ +62,5 @@
>                         CertVerifier::OcspGetConfig ocspGETConfig,
>                         uint32_t certShortLifetimeInDays,
>                         CertVerifier::PinningMode pinningMode,
>                         unsigned int minRSABits,
> +                       SignatureDigestOption,

nit: let's give this argument a name here
Attachment #8624311 - Flags: review?(dkeeler) → review+
Modified to address xpcshell test failures. In particular, I've added code to break out of the digest algorithm loop if the error is not ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED
Attachment #8624311 - Attachment is obsolete: true
Attachment #8625580 - Flags: review?(dkeeler)
Comment on attachment 8625580 [details] [diff] [review]
Add telemetry probe for sha1 certificates.patch

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

As we discussed in person, fixing the tests would probably be the best approach, so I'll clear this review for now.
Attachment #8625580 - Flags: review?(dkeeler)
Now with less bitrot (and the early-loop-exit for non-signature failure removed)
Attachment #8625580 - Attachment is obsolete: true
Attached patch some_tests.patch (obsolete) — Splinter Review
Test failures due to the changed request counts / pinning histogram values.

Test modified to make it clearer what each case is requesting (and why)
Attachment #8628511 - Flags: review?(dkeeler)
Attachment #8628513 - Flags: review?(dkeeler)
Comment on attachment 8628511 [details] [diff] [review]
Add telemetry probe for sha1 certificates.patch

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

Great! r=me with nits addressed.

::: security/certverifier/CertVerifier.cpp
@@ +240,5 @@
> +        SignatureDigestStatus::WeakCACert,
> +        SignatureDigestStatus::WeakCAAndEE
> +      };
> +
> +      size_t digestAlgorithmOptionsCount = MOZ_ARRAY_LENGTH(digestAlgorithmStatuses);

nit: maybe put the assert before this line

@@ +258,5 @@
>  
>        CertPolicyId evPolicy;
>        SECOidTag evPolicyOidTag;
>        SECStatus srv = GetFirstEVPolicy(cert, evPolicy, evPolicyOidTag);
> +      for (size_t i=0;

nit: spaces around '='

@@ +312,5 @@
> +                    "keySize array lengths differ");
> +
> +      size_t keySizeOptionsCount = MOZ_ARRAY_LENGTH(keySizeStatuses);
> +
> +      for (size_t i=0; i<keySizeOptionsCount && rv != Success; i++) {

nit: spaces around '=' and '<'

@@ +313,5 @@
> +
> +      size_t keySizeOptionsCount = MOZ_ARRAY_LENGTH(keySizeStatuses);
> +
> +      for (size_t i=0; i<keySizeOptionsCount && rv != Success; i++) {
> +        for (size_t j=0; j<digestAlgorithmOptionsCount && rv != Success; j++) {

here too

::: security/certverifier/NSSCertDBTrustDomain.cpp
@@ +808,5 @@
> +      return Result::ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED;
> +    }
> +
> +    if (endEntityOrCA == EndEntityOrCA::MustBeCA) {
> +    MOZ_LOG(gCertVerifierLog, LogLevel::Debug, ("CA cert is SHA1"));

nit: indentation

@@ +813,5 @@
> +      return mSignatureDigestOption == DisableSHA1ForCA
> +             ? Result::ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED
> +             : Success;
> +    } else {
> +    MOZ_LOG(gCertVerifierLog, LogLevel::Debug, ("EE cert is SHA1"));

nit: indentation
Attachment #8628511 - Flags: review?(dkeeler) → review+
Comment on attachment 8628513 [details] [diff] [review]
some_tests.patch

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

This looks good, but the changes to test_ocsp_caching.js don't really result in a clear, easy-to-understand test (this is in large part due to its current lack of clarity). I think we can change it to be more like test_ocsp_stapling_expired.js, which will make it easier to understand going forward (see the comment about that). I also noted some nits. r- for now.

::: security/manager/ssl/tests/unit/test_ocsp_caching.js
@@ +48,4 @@
>    });
>    ocspResponder.start(8888);
>  
> +

nit: unnecessary blank line

@@ +76,5 @@
>                                12000);
> +    setup_responses([], "expected zero OCSP requests for a short-lived certificate");
> +    run_next_test();
> +  });
> +  add_connection_test("ocsp-stapling-none.example.com", PRErrorCodeSuccess,

I think we should restructure this test file to be more like test_ocsp_stapling_expired.js. Instead of calling add_connection_test directly, we should call a function like add_ocsp_test with the sequence of responses to serve. Then, we check that they were all consumed before moving on to the next testcase. In between there can be calls to add_test where we set appropriate preferences or clear related state as needed.

::: security/manager/ssl/tests/unit/test_ocsp_stapling_expired.js
@@ +21,5 @@
>        gCurrentOCSPResponse = aOCSPResponseToServe;
>        gOCSPRequestCount = 0;
>      },
>      function() {
> +      do_print("gOCSPRequestCount is "+gOCSPRequestCount);

nit: spaces around '+'

@@ +51,5 @@
> +// sometimes we expect a result without re-fetch
> +let willNotRetry = 1;
> +// but sometimes, since a bad response is in the cache, OCSP fetch will be
> +// attempted for each validation - in practice, for these test certs, this
> +// means 4 requests

I would add a comment that this is currently due to retrying with different hash algorithm settings for telemetry.
Attachment #8628513 - Flags: review?(dkeeler) → review-
Comment on attachment 8628513 [details] [diff] [review]
some_tests.patch

Just so you're aware, it looks like some of the changes here will conflict with the patch for Bug 1174389.

Sorry for not noticing earlier!
(In reply to Cykesiopka from comment #18)
> Sorry for not noticing earlier!

No problem; thanks for letting me know.
Un-bit-rotted. Carrying r+
Attachment #8628511 - Attachment is obsolete: true
Attachment #8630707 - Flags: review+
Attached patch some tests.patch (obsolete) — Splinter Review
Modified extensively make things clearer - more similar to test_ocsp_stapling_expired.js.
Attachment #8628513 - Attachment is obsolete: true
Attachment #8630709 - Flags: review?(dkeeler)
Comment on attachment 8630709 [details] [diff] [review]
some tests.patch

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

This looks great. I just had one question in particular it would be good to clear up (see below), but otherwise r=me.

::: security/manager/ssl/tests/unit/test_ocsp_caching.js
@@ +59,3 @@
>  
>      do_print("gFetchCount: " + gFetchCount);
> +    let func = gResponsePattern[gFetchCount];

maybe s/func/responseFunction/ ?

@@ +144,5 @@
>      gGoodOCSPResponse = generateGoodOCSPResponse();
>      run_next_test();
>    });
> +  add_ocsp_test("ocsp-stapling-none.example.com", PRErrorCodeSuccess,
> +                [respondWithError, respondWithError, respondWithGoodOCSP],

Does just "[respondWithGoodOCSP]" work for this test case? I think that's the behavior we want...
Attachment #8630709 - Flags: review?(dkeeler) → review+
(In reply to David Keeler [:keeler] (use needinfo?) from comment #22)
> maybe s/func/responseFunction/ ?

Yes, absolutely.

> > +  add_ocsp_test("ocsp-stapling-none.example.com", PRErrorCodeSuccess,
> > +                [respondWithError, respondWithError, respondWithGoodOCSP],
> 
> Does just "[respondWithGoodOCSP]" work for this test case? I think that's
> the behavior we want...

It works; what threw me is that the test used to always give 2 error responses before any good OCSP response. The modified test reproduced that behavior but, as you point out, that's not necessary.
Minor tweak (declaration order change) to address compilation issues with some gcc versions. Carrying r+ as per IRC conversation with keeler
Attachment #8630707 - Attachment is obsolete: true
Attachment #8631433 - Flags: review+
Attached patch some tests.patchSplinter Review
Keeler's feedback addressed
Attachment #8630709 - Attachment is obsolete: true
Attachment #8631434 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/31d0ae4d8c62
https://hg.mozilla.org/mozilla-central/rev/9b18e545fe2d
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.