key pinning telemetry probes need to be re-worked

RESOLVED FIXED in Firefox 43

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: keeler, Assigned: mgoodwin)

Tracking

unspecified
mozilla43
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 years ago
Consider the following scenario: end-entity certificate A was issued by some intermediate B issued by root C. Suppose root D cross-signs B and issues B'. Suppose a site using certificate A specifies a pinset consisting of the key from root C. When attempting to verify that certificate, we may find the path A -- B' -- D. D is a trusted root, so we check pinning. This fails and we accumulate telemetry for that result. However, path building continues and we find path A -- B -- C. We again check pinning, which succeeds. Thus, we have gathered telemetry for a spurious key pinning failure. We need to instead gather telemetry in a manner similar to the fix in bug 1107666, where we essentially take notes during the process of path building but only accumulate telemetry once, at the end of that process.
(Assignee)

Updated

3 years ago
Assignee: nobody → mgoodwin
(Assignee)

Comment 1

3 years ago
Created attachment 8641763 [details] [diff] [review]
Bug 1153444 - Fix up Key Pinning Telemetry.patch

The approach I've taken here is to pass something to collect pinning telemetry information into the cert verifier, through NSSCertDBTrustDomain and into the PublicKeyPinningService - each time we attempt to build a chain, we reset this so that when we get back to SSLServerCertVerifier we have a single source of information from which to accumulate telemetry.

It might even be worth renaming this to TelemetryInfo and adding all of the other telemetry related out values so we can clean up those interfaces. What do you think?
Attachment #8641763 - Flags: feedback?(dkeeler)
(Reporter)

Comment 2

3 years ago
Comment on attachment 8641763 [details] [diff] [review]
Bug 1153444 - Fix up Key Pinning Telemetry.patch

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

I'll have a better look at this early next week, but I wanted to go ahead and give some feedback before the weekend. Looks like a good approach so far.

::: security/certverifier/CertVerifier.cpp
@@ +264,5 @@
>             i < digestAlgorithmOptionsCount && rv != Success && srv == SECSuccess;
>             i++) {
> +        // invalidate any telemetry info relating to failed chains
> +        if (pinningTelemetryInfo) {
> +          pinningTelemetryInfo->accumulateResult = false;

Maybe just have a function we can call like 'pinningTelemetryInfo->Reset();'?

::: security/manager/ssl/PublicKeyPinningService.h
@@ +32,5 @@
>                                      const char* hostname,
>                                      mozilla::pkix::Time time,
>                                      bool enforceTestMode,
> +                            /*out*/ bool& chainHasValidPins,
> +                                    PinningTelemetryInfo* pinningTelemetryInfo);

Let's annotate this as /*optional out*/?

::: security/manager/ssl/RootCertificateTelemetryUtils.cpp
@@ +14,1 @@
>  namespace mozilla { namespace psm { 

Let's get rid of this trailing space while we're here.
Attachment #8641763 - Flags: feedback?(dkeeler) → feedback+
(Reporter)

Comment 3

3 years ago
Comment on attachment 8641763 [details] [diff] [review]
Bug 1153444 - Fix up Key Pinning Telemetry.patch

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

I guess I didn't really have anything more to add except a small nit.

::: security/manager/ssl/RootCertificateTelemetryUtils.h
@@ +10,5 @@
>  #include "mozilla/Telemetry.h"
>  #include "certt.h"
>  
>  namespace mozilla { namespace psm {
> +// Note: New CAs will show up as UNKNOWN_ROOT until

nit: let's have a blank line before this
(Assignee)

Comment 5

3 years ago
Created attachment 8649345 [details] [diff] [review]
Bug 1153444 - Fix up Key Pinning Telemetry.patch

Feedback addressed; specifically:
 - Reset() added to PinningTelemetryInfo to simplify resetting telemetry info state
 - Annotated PublicKeyPinningService::ChainHasValidPins to specify pinningTelemetryInfo as optional out
 - Fixed whitespace nits in RootCertificateTelemetryUtils(.h|.cpp)
Attachment #8641763 - Attachment is obsolete: true
Attachment #8649345 - Flags: review?(dkeeler)
(Reporter)

Comment 6

3 years ago
Comment on attachment 8649345 [details] [diff] [review]
Bug 1153444 - Fix up Key Pinning Telemetry.patch

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

Great! r=me with nits addressed.
In the future, we should start using mozreview. The interface is much better than splinter.

::: security/certverifier/CertVerifier.cpp
@@ +262,5 @@
>        SECStatus srv = GetFirstEVPolicy(cert, evPolicy, evPolicyOidTag);
>        for (size_t i=0;
>             i < digestAlgorithmOptionsCount && rv != Success && srv == SECSuccess;
>             i++) {
> +        // invalidate any telemetry info relating to failed chains

I would maybe be a bit more verbose here: "Because of the try-strict and fallback approach, we have to clear any previously noted telemetry information" or something

::: security/certverifier/CertVerifier.h
@@ +31,5 @@
>    WeakCAAndEE = 4,
>    AlreadyBad = 5,
>  };
>  
> +class PinningTelemetryInfo {

nit: { on the next line

@@ +76,5 @@
>        /*optional out*/ SECOidTag* evOidPolicy = nullptr,
>        /*optional out*/ OCSPStaplingStatus* ocspStaplingStatus = nullptr,
>        /*optional out*/ KeySizeStatus* keySizeStatus = nullptr,
> +      /*optional out*/ SignatureDigestStatus* sigDigestStatus = nullptr,
> +                       PinningTelemetryInfo* pinningTelemetryInfo = nullptr);

We should annotate this as /*optional out*/ here as well

@@ +91,5 @@
>     /*optional out*/ SECOidTag* evOidPolicy = nullptr,
>     /*optional out*/ OCSPStaplingStatus* ocspStaplingStatus = nullptr,
>     /*optional out*/ KeySizeStatus* keySizeStatus = nullptr,
> +   /*optional out*/ SignatureDigestStatus* sigDigestStatus = nullptr,
> +                    PinningTelemetryInfo* pinningTelemetryInfo = nullptr);

Here too

::: security/certverifier/NSSCertDBTrustDomain.cpp
@@ +50,5 @@
>                                             CertVerifier::PinningMode pinningMode,
>                                             unsigned int minRSABits,
>                                             ValidityCheckingMode validityCheckingMode,
>                                             SignatureDigestOption signatureDigestOption,
> +                                           PinningTelemetryInfo* pinningTelemetryInfo,

Let's annotate this as /*optional*/

@@ +793,5 @@
>      bool enforceTestMode =
>        (mPinningMode == CertVerifier::pinningEnforceTestMode);
>      bool chainHasValidPins;
>      nsresult nsrv = PublicKeyPinningService::ChainHasValidPins(
> +      certList, mHostname, time, enforceTestMode, chainHasValidPins, mPinningTelemetryInfo);

nit: long line

::: security/certverifier/NSSCertDBTrustDomain.h
@@ +69,5 @@
>                         CertVerifier::PinningMode pinningMode,
>                         unsigned int minRSABits,
>                         ValidityCheckingMode validityCheckingMode,
>                         SignatureDigestOption,
> +          /*optional*/ PinningTelemetryInfo* pinningTelemetryInfo,

Well, if this is really optional, we should set its default to nullptr.

::: security/manager/ssl/PublicKeyPinningService.cpp
@@ +271,5 @@
>  static nsresult
>  CheckPinsForHostname(const CERTCertList* certList, const char* hostname,
>                       bool enforceTestMode, mozilla::pkix::Time time,
> +             /*out*/ bool& chainHasValidPins,
> +                     PinningTelemetryInfo* pinningTelemetryInfo)

Annotate /*optional out*/

@@ +328,5 @@
> +        pinningTelemetryInfo->certPinningResultHistogram = histogram;
> +        pinningTelemetryInfo->certPinningResultBucket = bucket;
> +      } else {
> +        pinningTelemetryInfo->accumulateResult = true;
> +        pinningTelemetryInfo->certPinningResultHistogram = histogram;

We might as well factor out these two common lines.
Attachment #8649345 - Flags: review?(dkeeler) → review+
https://hg.mozilla.org/mozilla-central/rev/fc86e9f2d6ea
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox43: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.