Closed Bug 1147497 (CVE-2015-2741) Opened 5 years ago Closed 5 years ago

key pinning checks for overridable errors do not work as intended by default

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox36 --- wontfix
firefox37 --- wontfix
firefox38 --- wontfix
firefox38.0.5 --- wontfix
firefox39 --- fixed
firefox40 --- fixed
firefox-esr31 --- unaffected
firefox-esr38 39+ fixed
b2g-v1.4 --- unaffected
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- unaffected
b2g-v2.1S --- unaffected
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: keeler, Assigned: keeler)

References

(Blocks 1 open bug)

Details

(Keywords: sec-moderate, Whiteboard: [adv-main39+][adv-esr38.1+])

Attachments

(3 files, 4 obsolete files)

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.
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.
Attached patch patch (obsolete) — Splinter Review
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.
Assignee: nobody → dkeeler
Status: NEW → ASSIGNED
Attachment #8583297 - Flags: review?(mmc)
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.
Flags: needinfo?(brian)
(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.
Flags: needinfo?(brian)
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".
Attachment #8583297 - Flags: review?(mmc)
Attached patch patch v2 (obsolete) — Splinter Review
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).
Attachment #8583297 - Attachment is obsolete: true
Attachment #8584790 - Flags: review?(mmc)
Attachment #8584790 - Flags: feedback?(brian)
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.
Attachment #8584790 - Flags: review?(mmc) → feedback+
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.
Attachment #8584790 - Flags: feedback?(brian) → feedback+
Attached patch patch v3 (obsolete) — Splinter Review
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.
Attachment #8584790 - Attachment is obsolete: true
Attachment #8588051 - Flags: review?(cykesiopka.bmo)
Attachment #8588051 - Flags: feedback?
Attachment #8588051 - Flags: feedback? → feedback?(jjones)
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/
Attachment #8588051 - Flags: review?(cykesiopka.bmo) → review+
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 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
Attachment #8588051 - Flags: review?(cykesiopka.bmo)
Attachment #8588051 - Flags: review+
Attachment #8588051 - Flags: feedback?(jjones)
Attachment #8588051 - Flags: feedback+
Comment on attachment 8588051 [details] [diff] [review]
patch v3

Setting (I guess accidental) r? back to r+.
Attachment #8588051 - Flags: review?(cykesiopka.bmo) → review+
Attached patch patch v3.1 (obsolete) — Splinter Review
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).
Attachment #8588051 - Attachment is obsolete: true
Attachment #8588677 - Flags: review?(jst)
Comment on attachment 8588677 [details] [diff] [review]
patch v3.1

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

LGTM. Thanks for working on this.
Attachment #8588677 - Flags: review+
Comment on attachment 8588677 [details] [diff] [review]
patch v3.1

Olli said he would be able to review the docshell changes early next week.
Attachment #8588677 - Flags: review?(jst) → review?(bugs)
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.
Attachment #8588677 - Flags: review?(bugs) → review+
Attached patch patch v3.2Splinter Review
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.
Attachment #8588677 - Attachment is obsolete: true
Attachment #8593007 - Flags: review?(past)
Comment on attachment 8593007 [details] [diff] [review]
patch v3.2

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

LGTM.
Attachment #8593007 - Flags: review?(past) → review+
https://hg.mozilla.org/mozilla-central/rev/98059179549d

I assume we want to nominate this for Aurora/Beta/b2g37 approval?
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: needinfo?(dkeeler)
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Attached patch patch for esr-38Splinter Review
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
Flags: needinfo?(dkeeler)
Attachment #8594188 - Flags: review+
Attachment #8594188 - Flags: approval-mozilla-beta?
Comment on attachment 8593007 [details] [diff] [review]
patch v3.2

Approval Request Comment
see comment 23
Attachment #8593007 - Flags: approval-mozilla-aurora?
Looks like b2g37 will need rebasing too.
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.
Attachment #8593007 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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.
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...
Duplicate of this bug: 1156073
Attached patch patch for b2g37Splinter Review
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
Attachment #8595474 - Flags: review+
Attachment #8595474 - Flags: approval-mozilla-b2g37?
(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 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.
Attachment #8594188 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
(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.
Flags: needinfo?(sledru)
(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.
Flags: needinfo?(sledru)
Might be worth considering this for ESR 38.1 nomination if 39 is looking stable once on the beta channel?
Attachment #8595474 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
[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.
Flags: needinfo?(dkeeler)
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #38)

Thanks - will do.
Flags: needinfo?(dkeeler)
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.
Attachment #8594188 - Attachment description: patch for beta → patch for esr-38
Attachment #8594188 - Flags: approval-mozilla-esr38?
Attachment #8594188 - Flags: approval-mozilla-esr38? → approval-mozilla-esr38+
Whiteboard: [adv-main39+][adv-esr38.1+]
Alias: CVE-2015-2741
Group: core-security
Blocks: 1181975
You need to log in before you can comment on or make changes to this bug.