1065909 - (CVE-2015-0832) HPKP and HSTS can be bypassed with extra dot in hostname

Status: VERIFIED FIXED

(Core :: Security: PSM, defect)

Description

[Muneaki Nishimura]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/37.0.2062.103 Safari/537.36

Steps to reproduce:

1. Set "security.cert_pinning.enforcement_level" = "2" (strict-mode) on about:config.
1. Enable "Fiddler Web Debugger" with "Decrypt HTTPS traffic" option.
2. Install Fiddler's root certificate on Firefox.
2. Launch https://play.google.com./ (hostname with extra dot)


Actual results:

Google Play is shown on Firefox even though the host is protected with HPKP.



Expected results:

Secure Connection Failed error screen needs to be shown instead of Google Play in strict-mode.

Comment 1

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

One of us will look today. The most likely explanation is that Fiddler's root cert is signed by one of the 70 root pems that Google is pinned to (Chrome pins to 2 intermediates but they don't trust us to do the same, because they don't control our release cycle).

Comment 3

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

See also bug 134402. It does seem bad that if a CA were convinced to issue a certificate for 'play.google.com.', that would essentially bypass pinning. For the purposes of pinning we should probably treat 'play.google.com.' as equivalent to 'play.google.com'.

Comment 4

[Camilo Viecco (:cviecco)]

Seems like a legit bug. We will need to update the pinning logic to ignore any number of traling '.'

Comment 5

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

Who will own the fix for this?

Comment 6

[Daniel Veditz [:dveditz]]

Calling this sec-moderate because it's not a direct attack on Firefox but bypassing a second line of defense against compromised CAs.

Comment 7

[Muneaki Nishimura]

Not only HPKP but also HSTS can be bypassed by same way.

For instance, 'stationary-traveller.eu' is one of preloading HSTS entry but Firefox connects there through HTTP when you put extra dot after hostname like below.

http://stationary-traveller.eu./

Comment 8

[Daniel Veditz [:dveditz]]

The fundamental issue is that we treat "example.com" as a different origin than "example.com." -- that's true for most things, cookies, permissions, HSTS, cert matching.

For HPKP the fraudulent MITM cert would have to contain the form with the dot, but since it's fraudulent cert that might be a simple matter for the attacker to arrange. Hopefully it would raise eyebrows at the CA issuing that cert, but probably wouldn't if it were one of a number of names.

For HSTS that's a second valid attack, but thankfully you shouldn't get any cookies that way. But of course the user might log in that way if the HSTS-using site doesn't redirect all their http traffic back to TLS.

Comment 9

[Camilo Viecco (:cviecco)]

Attachment: normalization-hostname-tests

This patch illustrates the issue of normalization in general.

Comment 10

[Camilo Viecco (:cviecco)]

Attachment: normalize-hostname-certverify

So this solves the issue by normalizing the hostname within certverifier. Are there any issues that I missed (or created) by doing this? This still leaves hsts affected. (and I am not taking care of the foobar.com.. case).

Comment 11

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

Comment on attachment 8499147 [details] [diff] [review]
normalize-hostname-certverify

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

I'm not sure this is the best way to approach this. For one, we have a similar issue with HSTS. It would be nice to have a solution that covers both cases at the same time. I'm not sure what that would be, however. Maybe the normalization should be done in nsISiteSecurityService?

Comment 12

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

I have created some primitives in bug 1063281 for comparing names safely and correctly. See PresentedDNSIDMatchesReferenceDNSID, in particular, which handles trailing dots automatically and correctly. With a little bit of refactoring of that code, we could create a general hostname comparison function that works correctly. I think that is a good long-term solution. We could rework those patches to be acceptable for uplifting, by simply not uplifting the patch that switches Gecko from using CERT_VerifyCertName to CheckCertHostname.

There is another issue that should be considered: Should unqualified names (e.g. "mail") and/or IP addresses (e.g. 192.168.0.1) be able to be HSTS or have pinned keys? I think the answer is clearly "nope, we shouldn't do that" because those names are ambiguous and key pinning and HSTS would be highly problematic for them. I think for HSTS we made an ad-hoc solution that prevented the IP address case. But, again using the primitives in bug 1063281, we can easily create a function that is used by both HSTS and the key pinning code to ensure they are kept in sync.

Consequently, I suggest that we solve this problem on top of the Patches 1-5 of bug 1063281.

Comment 14

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

Can we get a new assignee for this bug and decide on a Firefox release in which it will be fixed? The PresentedDNSIDMatchesReferenceDNSID function has now been r+d in bug 1063281 and I'll check it in as soon as I verify it passes tryserver.

Comment 15

[Daniel Veditz [:dveditz]]

Try Keeler since he's looked at the patches, but maybe JC would like to take this one.

Comment 16

[Lukas Blakk [:lsblakk] use ?needinfo]

Dan - Keeler is on vacation and we're a couple weeks from shipping FF35 - what's your thinking here for this one? Can we wontfix for 35 and get more attention on it for 36 or is there another assignee available to get something ready for Monday's final desktop beta?

Comment 17

[Daniel Veditz [:dveditz]]

Wait for Keeler and fix it in 36. We don't want to uplift bug 1063281 (or backport to ESR31) and it's not severe enough to warrant a one-release hacky band-aide for Fx35.

Comment 18

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

Attachment: patch

This canonicalizes hostnames in nsSiteSecurityService and PublicKeyPinningService by converting to lower-case and removing any trailing '.' (there may be more than one - see bug 1118522) (actually, converting to lower-case may not be necessary - it appears that that already happens before these checks). This patch would probably be simpler after bug 1102589, but it's probably more important to get this fixed first (also, this way will be easier to uplift).

Comment 19

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

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

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

Thanks for including the tests. Are we abandoning the approach in comment 12?

::: security/manager/boot/src/PublicKeyPinningService.h
@@ +42,5 @@
> +  /**
> +   * Given a hostname of indeterminate case with potentially two trailing
> +   * '.', canonicalizes it to lowercase with no trailing '.'.
> +   */
> +  static void CanonicalizeHostname(nsACString& hostname);

It probably makes more sense for this to not mutate the input and return a copy given the way you are calling it.

::: security/manager/ssl/tests/unit/test_pinning.js
@@ +49,5 @@
>  
> +  // Check that using a FQDN doesn't bypass pinning.
> +  add_connection_test("bad.include-subdomains.pinning.example.com.",
> +    getXPCOMStatusFromNSS(MOZILLA_PKIX_ERROR_KEY_PINNING_FAILURE));
> +  // For some reason this is also navigable.

Btw, I couldn't reproduce this before on a current nightly, I get "Server not found". It's fine to include in the test though.

Comment 20

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

(In reply to [:mmc] Monica Chew (please use needinfo) from comment #19)

> Btw, I couldn't reproduce this before on a current nightly, I get "Server
> not found". It's fine to include in the test though.

That is, I get server not found for "example.com.."

Comment 21

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

Huh - maybe that's linux-specific.

Comment 22

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

Thanks for the quick review!

(In reply to [:mmc] Monica Chew (please use needinfo) from comment #19)
> Thanks for including the tests. Are we abandoning the approach in comment 12?

The functionality mentioned in comment 12 provides a way to compare two hostnames. Since HSTS/HPKP information is stored using an off-the-shelf hash table, this doesn't really help us. What we need is a way of canonically representing hostnames when converting them to storage keys, which is what attachment 8544901 [details] [diff] [review] implements. 

> ::: security/manager/boot/src/PublicKeyPinningService.h
> @@ +42,5 @@
> > +  /**
> > +   * Given a hostname of indeterminate case with potentially two trailing
> > +   * '.', canonicalizes it to lowercase with no trailing '.'.
> > +   */
> > +  static void CanonicalizeHostname(nsACString& hostname);
> 
> It probably makes more sense for this to not mutate the input and return a
> copy given the way you are calling it.

Good call - I changed how that works.

Comment 23

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b50babd5975

Comment 24

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/f0cd9e9e87aa

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f0cd9e9e87aa

Comment 26

[Sylvestre Ledru [:Sylvestre]]

David, could you fill the uplift request for 36 (currently beta) and 37 (aurora)? Thanks

Comment 27

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

Attachment: patch for beta

Looks like this got fixed in 37, so we're already covered for aurora.

Approval Request Comment
[Feature/regressing bug #]: key pinning and HSTS
[User impact if declined]: key pinning and HSTS won't be nearly as effective at protecting our uses from MITM attacks
[Describe test coverage new/current, TBPL]: has tests, including new ones
[Risks and why]: low - the only issue I can think of is a slight performance hit since we're doing a bit more string allocation/manipulation
[String/UUID change made/needed]: none

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/82cce51fb174

Is this worth nominating for b2g34 (v2.1) uplift?

Comment 29

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

Attachment: patch for b2g34

This required some branch-specific changes, but not many, so I figured I might as well ask. I think this is a low-risk patch and it fixes a hole in a fairly important feature.

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #): key pinning/HSTS
User impact if declined: key pinning and HSTS won't be nearly as effective at protecting our uses from MITM attacks
Testing completed: has platform tests
Risk to taking this patch (and alternatives if risky): low - doesn't seem to have caused any problems on other branches
String or UUID changes made by this patch: none

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/bacd744d5ac2

Comment 31

[Matt Wobensmith [:mwobensmith][:matt:]]

Verified fixed using Fiddler and the steps outlined in the bug description.