1239455 - rework SHA1 telemetry to measure the possible policy configurations

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

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

Currently we have telemetry on whether or not SHA1 is being used in end-entity certificates and/or intermediates for TLS handshakes. However, the policy configuration doesn't differentiate between end-entities and intermediates but rather between whether or not each certificate has a notBefore value after 2015 (i.e. >= 0:00:00 1 January 2016 UTC). We really need telemetry based on the possible policy configurations. The end-entity vs. intermediate information is interesting but not as useful to us at the moment. (I suppose we could find a way to combine both, but that would be complicated and this is a time-sensitive matter.)

Comment 1

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

Attachment: MozReview Request: bug 1239455 - rework telemetry for SHA-1 certificates to reflect possible policy states r=rbarnes r=Cykesiopka r=mgoodwin

Before this patch, we were measuring where SHA-1 was being used in TLS
certificates: nowhere, in end-entities, in intermediates, or in both. However,
the possible SHA-1 policies don't differentiate between end-entities and
intermediates and instead depended on whether or not each certificate has a
notBefore value after 2015 (i.e. >= 0:00:00 1 January 2016 UTC). We need to
gather telemetry on the possible policy configurations.

Review commit: https://reviewboard.mozilla.org/r/30791/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/30791/

Comment 2

[Richard Barnes [:rbarnes]]

Comment on attachment 8707624 [details]
MozReview Request: bug 1239455 - rework telemetry for SHA-1 certificates to reflect possible policy states r=rbarnes r=Cykesiopka r=mgoodwin

https://reviewboard.mozilla.org/r/30791/#review27621

Overall, I think this is on the right track.  Couple of minor issues with telemetry, documentation, and testing.

::: security/certverifier/CertVerifier.h:25
(Diff revision 1)
> -// These values correspond to the CERT_CHAIN_SIGNATURE_DIGEST telemetry.
> +// These values correspond to the CERT_CHAIN_SHA1_POLICY_STATUS telemetry.

I'm kind of puzzled by the values here.  It seems like we should have one value here per SHA-1 mode, and then the measurement will reflect the strictest policy under which a certificate verified.   Can we just make these the same as the SHA1Mode values?  (Maybe with an offset)

::: security/certverifier/CertVerifier.h:111
(Diff revision 1)
> +    OnlyBefore2016ExceptImportedRoots = 3,

Nit: It would be a bit clearer to say "Before2016OrImportedRoot".  We can probably drop the "Only" from option 2 as well.

::: security/certverifier/CertVerifier.cpp:143
(Diff revision 1)
> +  if (mSHA1Mode == SHA1Mode::Forbidden) {

Nit: Clearer to write this as a switch?

::: security/certverifier/CertVerifier.cpp:152
(Diff revision 1)
> +  return false;

Assert(false) instead?

::: security/certverifier/CertVerifier.cpp:309
(Diff revision 1)
> +        // the sha1 mode option we're on.

Nit: I would reword this as "Only attempt verification if the SHA-1 option we're on is more restrictive than mSHA1Mode."

The most important thing here is that we can measure what the incremental failure rate would be if we made things more strict.  It took me a few seconds to tell if that was happening based on this comment :)

::: security/certverifier/CertVerifier.cpp:329
(Diff revision 1)
> +        // terminate with an imported root, we must reject it.

I would like to have more explanation here of how this process works, namely:
* We attempt to verify in order of strictness
* In particular, we try to verify "SHA-1 disabled after 2016" before we try "SHA-1 allowed for installed root"
* So if we verify under "SHA-1 allowed for installed root" at all, we already know that it can't verify without SHA-1

Trying to avoid someone reordering the checks and causing the policy not to be enforced properly.

::: security/manager/ssl/tests/unit/test_cert_sha1.js:91
(Diff revision 1)
> +  // happens to be all of our test certificates).

It makes me a little uncomfortable that we're not testing that this mode rejects post-2015 SHA-1 certs.

Is there a way we can mark a specific installed root should be treated in the same way as default roots?  For example, maybe something like the following in IsCertBuiltInRoot:

```
#ifdef NIGHTLY
  // Get cert fingerprint from a pref
  // If cert matches fingerprint, return true
#undef
```

::: toolkit/components/telemetry/Histograms.json:8032
(Diff revision 1)
> -  "CERT_CHAIN_SIGNATURE_DIGEST_STATUS": {
> +  "CERT_CHAIN_SHA1_POLICY_STATUS": {

You should probably check with the telemetry people on this.  I don't think you're allowed to change histograms, just add new ones.

Comment 3

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

https://reviewboard.mozilla.org/r/30791/#review27621

> I'm kind of puzzled by the values here.  It seems like we should have one value here per SHA-1 mode, and then the measurement will reflect the strictest policy under which a certificate verified.   Can we just make these the same as the SHA1Mode values?  (Maybe with an offset)

Part of the reasoning is due to that there are two more result states than policy states - the "never checked" result and the "verification failed regardless" result. More abstractly, though, is to make clear that these are outcomes as a result of trying each policy in order of greatest to least restriction. I updated the name of the enum and its values to make this more clear.

> Nit: It would be a bit clearer to say "Before2016OrImportedRoot".  We can probably drop the "Only" from option 2 as well.

Updated.

> Nit: Clearer to write this as a switch?

Sure, I guess.

> Assert(false) instead?

Fixed this by using switch.

> Nit: I would reword this as "Only attempt verification if the SHA-1 option we're on is more restrictive than mSHA1Mode."
> 
> The most important thing here is that we can measure what the incremental failure rate would be if we made things more strict.  It took me a few seconds to tell if that was happening based on this comment :)

Updated the comment.

> I would like to have more explanation here of how this process works, namely:
> * We attempt to verify in order of strictness
> * In particular, we try to verify "SHA-1 disabled after 2016" before we try "SHA-1 allowed for installed root"
> * So if we verify under "SHA-1 allowed for installed root" at all, we already know that it can't verify without SHA-1
> 
> Trying to avoid someone reordering the checks and causing the policy not to be enforced properly.

Fixed.

> It makes me a little uncomfortable that we're not testing that this mode rejects post-2015 SHA-1 certs.
> 
> Is there a way we can mark a specific installed root should be treated in the same way as default roots?  For example, maybe something like the following in IsCertBuiltInRoot:
> 
> ```
> #ifdef NIGHTLY
>   // Get cert fingerprint from a pref
>   // If cert matches fingerprint, return true
> #undef
> ```

I agree. I think this should be a follow-up bug, though.

> You should probably check with the telemetry people on this.  I don't think you're allowed to change histograms, just add new ones.

Well, I'm not changing it. I'm deleting the old one and adding a new one :)

Comment 4

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

Comment on attachment 8707624 [details]
MozReview Request: bug 1239455 - rework telemetry for SHA-1 certificates to reflect possible policy states r=rbarnes r=Cykesiopka r=mgoodwin

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30791/diff/1-2/

Comment 5

[:Cykesiopka]

Comment on attachment 8707624 [details]
MozReview Request: bug 1239455 - rework telemetry for SHA-1 certificates to reflect possible policy states r=rbarnes r=Cykesiopka r=mgoodwin

https://reviewboard.mozilla.org/r/30791/#review27803

LGTM.

::: security/certverifier/CertVerifier.cpp:339
(Diff revision 2)
> +        // because we try SHA1 configurations in order of decreasting

Nit: typo (decreasting).

::: security/certverifier/CertVerifier.cpp:432
(Diff revision 2)
> +          // decreasting strictness.)

Nit: typo (decreasting).

::: security/certverifier/CertVerifier.cpp:467
(Diff revision 2)
> -        *sigDigestStatus = SignatureDigestStatus::AlreadyBad;
> +      // default. NB: When we change the default, we have to change this.

The relevant pref files probably deserve an equivalent comment as well.

(Also, what do we change this to if the default in firefox.js doesn't match the default in mobile.js?)

::: security/manager/ssl/tests/unit/test_cert_sha1.js:59
(Diff revision 2)
>    // Expected outcomes (accept / reject):

Nit: This needs a new row for the imported root case. Alternatively, maybe this table can be removed in favour of just having people read the code below.

Comment 6

[Richard Barnes [:rbarnes]]

Comment on attachment 8707624 [details]
MozReview Request: bug 1239455 - rework telemetry for SHA-1 certificates to reflect possible policy states r=rbarnes r=Cykesiopka r=mgoodwin

https://reviewboard.mozilla.org/r/30791/#review27859

Thanks for the edits.  I agree that we can fix the test in a follow-on bug.

Comment 7

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

https://reviewboard.mozilla.org/r/30791/#review27803

> Nit: typo (decreasting).

Good catch.

> The relevant pref files probably deserve an equivalent comment as well.
> 
> (Also, what do we change this to if the default in firefox.js doesn't match the default in mobile.js?)

Good call. I imagine we'll just use the default in firefox.js since mobile usage is much less than desktop. Or, I suppose we could introspect what product we're running on.

> Nit: This needs a new row for the imported root case. Alternatively, maybe this table can be removed in favour of just having people read the code below.

I updated the comment.

Comment 8

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8707624 [details]
MozReview Request: bug 1239455 - rework telemetry for SHA-1 certificates to reflect possible policy states r=rbarnes r=Cykesiopka r=mgoodwin

https://reviewboard.mozilla.org/r/30791/#review28367

Looks good to me!

Comment 9

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

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

Comment 10

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

Comment on attachment 8707624 [details]
MozReview Request: bug 1239455 - rework telemetry for SHA-1 certificates to reflect possible policy states r=rbarnes r=Cykesiopka r=mgoodwin

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30791/diff/2-3/

Comment 11

[Pulsebot]

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

Comment 12

[Carsten Book [:Tomcat]]

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

Comment 13

[Jorg K (CEST = GMT+2)]

That broke Thunderbird:

mailnews/extensions/smime/src/nsMsgComposeSecure.cpp:908:51: error: too few arguments to function call, expected at least 6, have 5
or
build/mailnews/extensions/smime/src/nsMsgComposeSecure.cpp(908) : error C2660: 'mozilla::psm::CertVerifier::VerifyCert' : function does not take 5 arguments

https://dxr.mozilla.org/comm-central/source/mailnews/extensions/smime/src/nsMsgComposeSecure.cpp#908

Comment 14

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

Comment on attachment 8707624 [details]
MozReview Request: bug 1239455 - rework telemetry for SHA-1 certificates to reflect possible policy states r=rbarnes r=Cykesiopka r=mgoodwin

Approval Request Comment
[Feature/regressing bug #]: gathering telemetry on SHA1 deprecation
[User impact if declined]: adds 6 weeks to getting the right data on SHA1 usage so we can determine how to best protect our users
[Describe test coverage new/current, TreeHerder]: has tests
[Risks and why]: medium risk - this is a large patch in security-sensitive code. However, it's essentially a variation on preexisting functionality, so that gives us confidence it's correct. Plus, it was reviewed by three different people.
[String/UUID change made/needed]: none

Comment 15

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8707624 [details]
MozReview Request: bug 1239455 - rework telemetry for SHA-1 certificates to reflect possible policy states r=rbarnes r=Cykesiopka r=mgoodwin

Sorry but we cannot take a patch that late in the aurora cycle without baking. The go to build of 45 beta 1 is today.
This will have to ride the train from 46.