1147497 - (CVE-2015-2741) key pinning checks for overridable errors do not work as intended by default

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

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

In bug 1066190, we discovered that encountering an overridable certificate error could cause pinning checks to be skipped. So, if building a trusted chain failed in CertVerifier::VerifyCert, we made sure that we called CertListContainsExpectedKeys to do pinning checks. CertListContainsExpectedKeys expects a chain of certificates ending in a trusted root. The default pinning setting is to skip the checks if the root isn't a built-in root. In VerifyCert, if building a chain failed, we don't have a chain - we just pass the certificate itself to CertListContainsExpectedKeys. This does the built-in root check on that certificate, which almost certainly won't be a built-in. So, we skip the pinning checks and use the original error, which would allow overrides.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

This problem would have been avoided if my suggestion would have been taken: If a site has a valid pinset, then don't allow cert error overrides at all, full stop. There's no good reason to allow cert error overrides for pinned sites.

Comment 2

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

Attachment: patch

This is a minimal patch with the intention that it's the safest, smallest change to uplift. The real fix is the API I mention in the comment, but that will require some refactoring, etc.

Comment 3

[[:mmc] Monica Chew (no longer reading bugmail)]

Brian, do you think that the pinning mode (strict or mitm) should have an affect on whether an orthogonal error should be overridable? We are thinking it shouldn't matter.

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to [:mmc] Monica Chew (please use needinfo) from comment #3)
> Brian, do you think that the pinning mode (strict or mitm) should have an
> affect on whether an orthogonal error should be overridable? We are thinking
> it shouldn't matter.

It seems less error-prone to just disallow cert error overrides, and reducing the chances of a bug in this logic seems much more important than whatever benefit allowing cert error overrides for pinned sites (which seems close to zero, if not negative) provides.

Comment 5

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8583297 [details] [diff] [review]
patch

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

OK, thanks. After talking it over with keeler it is too late for 37, since this has already gone to RC, and we should go with the original solution that requires an API "do we have pinning information for this host".

Comment 6

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

Attachment: patch v2

Here's the patch with the new API.
Brian, if you're interested, I'd appreciate any feedback you have (not a big deal, though - I understand reviewing PSM code is not high on your priority or interest list).

Comment 7

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8584790 [details] [diff] [review]
patch v2

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

Looks much better, thanks. I made some slightly disruptive comments around legacy function names, feel free to ignore them if you don't think it's worthwhile.

::: docshell/base/nsDocShell.cpp
@@ +5052,4 @@
>            cssClass.AssignLiteral("badStsCert");
>            // measuring STS separately allows us to measure click through
>            // rates easily
>            bucketId = nsISecurityUITelemetry::WARNING_BAD_CERT_TOP_STS;

This conflates telemetry for STS and HPKP. Maybe make another clause for isPinnedHost with just "badStsCert"?

::: security/manager/boot/src/PublicKeyPinningService.cpp
@@ +181,2 @@
>  /**
>   * Check PKPins on the given certlist against the specified hostname

Bad comment and name. Returns true if any pins on the certlist pass for the given hostname, or pins don't exist.

Also "Check" is a bad name because it doesn't say what to do if "Check" fails. PinsPassForHostname or something would be better.

@@ +193,5 @@
>    }
>  
> +  nsresult rv = FindPinningInformation(hostname, time,
> +    [&](TransportSecurityPreload* staticFingerprints,
> +        const nsTArray<nsCString>* dynamicFingerprints) {

This signature shows up twice. Time to typedef? I also think it would be easier if the callback were named instead of anonymous.

@@ +203,5 @@
> +      }
> +      return NS_OK;
> +    }
> +    if (staticFingerprints) {
> +      bool result = EvalChainWithPinset(certList, staticFingerprints->pinset);

Since you are touching this code, how about renaming these to CertChainMatchesPinset and friends? Eval is so generic. Same for EvalChainWithHashtype above.

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +490,2 @@
>    bool strictTransportSecurityEnabled = false;
> +  bool havePinningInformation = false;

s/have/has

@@ +567,5 @@
>               ("[%p][%p] All errors covered by override rules\n",
>               mFdForLogging, this));
>        return new SSLServerCertVerificationResult(mInfoObject, 0);
>      }
>    } else {

Slightly cleaner to move this up top since it's shorter, but no big deal.

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8584790 [details] [diff] [review]
patch v2

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

This seems to implement what I suggested. But, to be honest, I quickly found that I'm so unfamiliar with the PublicKeyPinningService code that any specific feedback I can offer on it is probably not very useful. :(

::: docshell/base/nsDocShell.cpp
@@ +5052,4 @@
>            cssClass.AssignLiteral("badStsCert");
>            // measuring STS separately allows us to measure click through
>            // rates easily
>            bucketId = nsISecurityUITelemetry::WARNING_BAD_CERT_TOP_STS;

I agree. It would be better to have a separate telemetry probe.

::: security/certverifier/CertVerifier.cpp
@@ -467,5 @@
> -                                                          time, mPinningMode);
> -      if (pinningResult != Success) {
> -        rv = pinningResult;
> -      }
> -    }

Great!

::: security/manager/boot/src/PublicKeyPinningService.cpp
@@ +175,5 @@
>  
> +template<typename Callback>
> +static nsresult
> +FindPinningInformation(const char* hostname, mozilla::pkix::Time time,
> +                       Callback callback);

I suggest that you avoid making this a template, by having the Callback function take enough parameter. that it doesn't need to capture any variables. C++ lambdas have a special case for non-capturing lambdas ([] instead of [&]) that allow them to be converted into a regular function pointer. (I'm hoping to do similar in mozilla::pkix soon, if possible.)

@@ +182,5 @@
>   * Check PKPins on the given certlist against the specified hostname
>   */
>  static bool
>  CheckPinsForHostname(const CERTCertList *certList, const char *hostname,
>                       bool enforceTestMode, mozilla::pkix::Time time)

I agree with mmc. "CheckXXX" functions should return an error/status code, not a boolean. If a boolean result is needed then the function should be named something that makes it obvious what a true result means and what a false result means.

Comment 9

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

Attachment: patch v3

Thank you both for the reviews/feedback. I discussed this with Monica yesterday, and (as I understand it) we came to the conclusion that while the pinning implementation could benefit from improved documentation and some function renaming, it would be best to first prepare a more uplift-friendly patch that has fewer overall changes (i.e. basically just code changes), so that's what I did here. It does change all uses of boolean return values to an nsresult / output variable paradigm. I also re-worked the template/lambda approach since it wasn't necessary. I'm intending for the remaining documentation/naming fixups to be done as part of bug 1102436 or maybe in its own follow-up bug.

Comment 10

[:Cykesiopka]

Comment on attachment 8588051 [details] [diff] [review]
patch v3

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

LGTM. Unfortunately I'm not really familiar with this code either.
I did however go over the existing code, so I believe I understand the existing code and the changes.

::: security/manager/boot/src/PublicKeyPinningService.cpp
@@ +81,5 @@
>        if (base64Out.Equals(fingerprints->data[i])) {
>          PR_LOG(gPublicKeyPinningLog, PR_LOG_DEBUG,
>                 ("pkpin: found pin base_64 ='%s'\n", base64Out.get()));
> +       certMatchesPinset = true;
> +       return NS_OK;

Nit: indentation here looks off.

@@ +91,5 @@
>        if (base64Out.Equals((*dynamicFingerprints)[i])) {
>          PR_LOG(gPublicKeyPinningLog, PR_LOG_DEBUG,
>                 ("pkpin: found pin base_64 ='%s'\n", base64Out.get()));
> +       certMatchesPinset = true;
> +       return NS_OK;

Here as well.

@@ +174,5 @@
>  /**
>    Comparator for the is public key pinned host.
>  */
>  static int
> +TransportSecurityPreloadCompare(const void *key, const void *entry)

Nit: |const void*| while you're here.

::: security/manager/ssl/tests/unit/head_psm.js
@@ +575,5 @@
>      throw Components.results.NS_ERROR_NO_INTERFACE;
>    },
>  }
> +
> +// Utility functions for adding tests relating to certificate error orverrides

Nit: s/orverrides/overrides/

Comment 11

[J.C. Jones [:jcj] (he/they)]

keeler: I'm afraid I barely got started on this patch, and I have to leave for a bit of PTO. I'm not going to be able to finish this review until Monday, I'm sorry. :(

The couple files I looked at looked fine, but like Cykesiopka, I would need to read around the other files to properly review.

Comment 12

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8588051 [details] [diff] [review]
patch v3

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

Keeler,

I've dug through the changes and the associated parts of PSM and it looks alright. I made some nit comments to go along with Cykesiopka's, but given the intent to refactor I don't see anything that should hold up a merge. You can mark r=jcj.

Sorry for the delay!

::: security/manager/boot/src/PublicKeyPinningService.cpp
@@ +52,4 @@
>  }
>  
>  /*
>   * Returns true if a given cert matches any hashType fingerprints from the

Nit: Now it returns NS_OK unless there are no static or dynamic pinned fingerprints.

@@ +100,4 @@
>  }
>  
>  /*
>   * Returns true if a given chain matches any hashType fingerprints from the

Nit: Sets the outvar if there's an intersection; returns NS_OK unless fingerprints/dynamicFingerprints changes between lines 125 and 137.

@@ +149,4 @@
>  }
>  
>  /**
>   * Given a pinset and certlist, return true if one of the certificates on

Nit: Note out variable

::: security/manager/boot/src/PublicKeyPinningService.h
@@ +22,1 @@
>     * the given certificate chain matches the pin set specified by the

Nit: Note out variable

@@ +35,2 @@
>    /**
>     * Returns true if there is any intersection between the certificate list

Nit: Note out variable

Comment 13

[:Cykesiopka]

Comment on attachment 8588051 [details] [diff] [review]
patch v3

Setting (I guess accidental) r? back to r+.

Comment 14

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

Attachment: patch v3.1

Thank you for the reviews. I'm deferring preexisting style and comment nits to the follow-up bug (or potentially bug 1102436).

This also needs a docshell peer review. jst: the idea is to check for and prevent certificate error overrides for hosts we have pinning information for (much in the way we already do for HSTS hosts).

Comment 15

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8588677 [details] [diff] [review]
patch v3.1

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

LGTM. Thanks for working on this.

Comment 16

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

Comment on attachment 8588677 [details] [diff] [review]
patch v3.1

Olli said he would be able to review the docshell changes early next week.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8588677 [details] [diff] [review]
patch v3.1

Not super happy to see 2 sync SendIsSecureURI calls in a row, but this code
should be called rather rarely.

Comment 18

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

Attachment: patch v3.2

Turns out there was a devtools test that needed to explicitly enable pinning after these changes. Panos, would you please review the changes to toolkit/devtools/webconsole/test/unit/test_security-info-static-hpkp.js? Thanks.

Comment 19

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e78516e315d
https://treeherder.mozilla.org/#/jobs?repo=try&revision=877caf6501bf

Comment 20

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8593007 [details] [diff] [review]
patch v3.2

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

LGTM.

Comment 21

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

Thanks!
https://hg.mozilla.org/integration/mozilla-inbound/rev/98059179549d

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/98059179549d

I assume we want to nominate this for Aurora/Beta/b2g37 approval?

Comment 23

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

Attachment: patch for esr-38

Approval Request Comment
[Feature/regressing bug #]: key pinning
[User impact if declined]: key pinning won't be as effective at preventing MITM attacks (the user would still have to click through an override page)
[Describe test coverage new/current, TreeHerder]: has tests
[Risks and why]: this does touch a fair bit of code, but since it basically copies similar modifications that were made for HSTS, I think the risk is mitigated somewhat (i.e. we've done something like this before, so it probably won't break anything else)
[String/UUID change made/needed]: none

Comment 24

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

Comment on attachment 8593007 [details] [diff] [review]
patch v3.2

Approval Request Comment
see comment 23

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Looks like b2g37 will need rebasing too.

Comment 26

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8593007 [details] [diff] [review]
patch v3.2

It is a sec moderate, we are past the middle cycle for beta and the patch is quite big.
I am not sure we want to take the risk here for beta.
Anyway, taking it for aurora.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5ab6348ad96e

Comment 28

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

(In reply to Sylvestre Ledru [:sylvestre] from comment #26)
> Comment on attachment 8593007 [details] [diff] [review]
> patch v3.2
> 
> It is a sec moderate, we are past the middle cycle for beta and the patch is
> quite big.
> I am not sure we want to take the risk here for beta.
> Anyway, taking it for aurora.

It would be good to at least get this on ESR 38.

Comment 29

[:Cykesiopka]

FWIW, it looks like the public Bug 1156073 (filed yesterday) is a dupe of this bug (assuming I'm not misinterpreting anything). Not sure what the proper procedure here is...

Comment 31

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

Attachment: patch for b2g37

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
see comment 23

Comment 32

[Sylvestre Ledru [:Sylvestre]]

(In reply to David Keeler [:keeler] (use needinfo?) from comment #28)
> It would be good to at least get this on ESR 38.
ESR 38 is based on the release and doesn't receive much testing coverage (as the code is pretty much the same in esr and release). If we take it for ESR, we have to take it for 38.

Comment 33

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8594188 [details] [diff] [review]
patch for esr-38

As mentioned in comment #26, I am not ready to take the risk as this release is going to get marketing exposure.

Comment 34

[Richard Barnes [:rbarnes]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #33)
> Comment on attachment 8594188 [details] [diff] [review]
> patch for beta
> 
> As mentioned in comment #26, I am not ready to take the risk as this release
> is going to get marketing exposure.

I'm confused.  ISTM that the marketing risk here is that we'll be noted as non-compliant with the pinning spec and provided degraded security to pinned sites.

Comment 35

[Sylvestre Ledru [:Sylvestre]]

(In reply to Richard Barnes [:rbarnes] from comment #34)
> I'm confused.  ISTM that the marketing risk here is that we'll be noted as
> non-compliant with the pinning spec and provided degraded security to pinned
> sites.
I understand your point (and I asked Lawrence's opinion before rejecting this uplift) but understand that we are working on stabilizing the release, it is late in the cycle and it is not uncommon that security changes affect the release.
We can just let it ride the train from 39.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

Might be worth considering this for ESR 38.1 nomination if 39 is looking stable once on the beta channel?

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/8fbdb08d1cef

Comment 38

[Ryan VanderMeulen [:RyanVM]]

[Tracking Requested - why for this release]: Requesting 39+ status for potential landing in 38.1. It's a security and spec compliance issue.

David, you'll still need to request esr38 approval on the beta patch some time next week after 38.0 ships.

Comment 39

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

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #38)

Thanks - will do.

Comment 40

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

Comment on attachment 8594188 [details] [diff] [review]
patch for esr-38

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: potential bypass of key pinning protections (i.e. site impersonation)
Fix Landed on Version: 40, uplifted to 39
Risk to taking this patch (and alternatives if risky): some risk, but we haven't seen any issues yet, and it's been a few weeks
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr38/rev/ed87585a9fbc