Closed Bug 1153444 Opened 9 years ago Closed 9 years ago

key pinning telemetry probes need to be re-worked

Categories

(Core :: Security: PSM, defect)

Product:
Core
Component:
Security: PSM
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla43
Tracking Flags:
Tracking Status
firefox43 --- fixed

People

(Reporter: keeler, Assigned: mgoodwin)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 1153444 - Fix up Key Pinning Telemetry.patch
36.35 KB, patch
keeler
: review+
Details | Diff | Splinter Review 
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: nobody → mgoodwin
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)
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+
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
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)
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
Closed: 9 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.

Attachment

General

Creator:
Created:
Updated:
Size: