rework SHA1 telemetry to measure the possible policy configurations

RESOLVED FIXED in Firefox 46

Status

()

Core
Security: PSM
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: keeler, Assigned: keeler)

Tracking

Trunk
mozilla46
Points:
---

Firefox Tracking Flags

(firefox45 affected, firefox46 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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.)
(Assignee)

Comment 1

2 years ago
Created attachment 8707624 [details]
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/
Attachment #8707624 - Flags: review?(rlb)
Attachment #8707624 - Flags: review?(mgoodwin)
Attachment #8707624 - Flags: review?(cykesiopka.bmo)
Attachment #8707624 - Flags: review?(rlb)
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.
(Assignee)

Comment 3

2 years ago
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 :)
(Assignee)

Comment 4

2 years ago
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/
Attachment #8707624 - Flags: review?(cykesiopka.bmo) → review?(rlb)
(Assignee)

Updated

2 years ago
Attachment #8707624 - Flags: review?(cykesiopka.bmo)

Comment 5

2 years ago
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.
Attachment #8707624 - Flags: review?(cykesiopka.bmo) → review+
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.
Attachment #8707624 - Flags: review?(rlb) → review+
(Assignee)

Comment 7

2 years ago
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 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!
Attachment #8707624 - Flags: review?(mgoodwin) → review+
(Assignee)

Comment 10

2 years ago
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/
Attachment #8707624 - Attachment description: MozReview Request: bug 1239455 - rework telemetry for SHA-1 certificates to reflect possible policy states r?rbarnes r?mgoodwin r?Cykesiopka → MozReview Request: bug 1239455 - rework telemetry for SHA-1 certificates to reflect possible policy states r=rbarnes r=Cykesiopka r=mgoodwin

Comment 12

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bb6bfd172d6e
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox46: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
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

Updated

2 years ago
Depends on: 1241480
(Assignee)

Comment 14

2 years ago
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
Attachment #8707624 - Flags: approval-mozilla-aurora?
status-firefox45: --- → affected
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.
Attachment #8707624 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
You need to log in before you can comment on or make changes to this bug.