Closed Bug 1065909 (CVE-2015-0832) Opened 5 years ago Closed 5 years ago

HPKP and HSTS can be bypassed with extra dot in hostname

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla37
Tracking Status
firefox33 --- wontfix
firefox34 --- wontfix
firefox35 - wontfix
firefox36 + verified
firefox37 + verified
firefox-esr31 --- wontfix
b2g-v1.4 --- wontfix
b2g-v2.0 --- wontfix
b2g-v2.0M --- wontfix
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: sdna.muneaki.nishimura, Assigned: keeler)

References

Details

(Keywords: sec-moderate, Whiteboard: [reporter-external][adv-main36+][b2g-adv-main2.2+])

Attachments

(3 files, 2 obsolete files)

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.
Flags: needinfo?(mmc)
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).
Flags: needinfo?(dkeeler)
Flags: needinfo?(cviecco)
Flags: sec-bounty?
Whiteboard: [reporter-external]
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'.
Flags: needinfo?(dkeeler)
See Also: → 134402
Component: Untriaged → Security: PSM
Product: Firefox → Core
Version: 35 Branch → unspecified
OS: Windows 7 → All
Hardware: x86_64 → All
Seems like a legit bug. We will need to update the pinning logic to ignore any number of traling '.'
Flags: needinfo?(cviecco)
Flags: needinfo?(mmc)
Who will own the fix for this?
Calling this sec-moderate because it's not a direct attack on Firefox but bypassing a second line of defense against compromised CAs.
Keywords: sec-moderate
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./
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → cviecco
Attached patch normalization-hostname-tests (obsolete) — Splinter Review
This patch illustrates the issue of normalization in general.
Attached patch normalize-hostname-certverify (obsolete) — Splinter Review
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).
Attachment #8499147 - Flags: feedback?(dkeeler)
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?
Attachment #8499147 - Flags: feedback?(dkeeler) → feedback-
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.
Summary: HPKP can be bypassed with extra dot in hostname → HPKP and HSTS can be bypassed with extra dot in hostname
Duplicate of this bug: 1085091
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.
Flags: needinfo?(dveditz)
Try Keeler since he's looked at the patches, but maybe JC would like to take this one.
Assignee: cviecco → dkeeler
Depends on: 1063281
Flags: needinfo?(dveditz)
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?
Flags: needinfo?(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.
Attached patch patchSplinter Review
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).
Attachment #8499036 - Attachment is obsolete: true
Attachment #8499147 - Attachment is obsolete: true
Attachment #8544901 - Flags: review?(mmc)
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.
Attachment #8544901 - Flags: review?(mmc) → review+
(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.."
Huh - maybe that's linux-specific.
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.
https://hg.mozilla.org/mozilla-central/rev/f0cd9e9e87aa
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Flags: sec-bounty? → sec-bounty+
David, could you fill the uplift request for 36 (currently beta) and 37 (aurora)? Thanks
Flags: needinfo?(dkeeler)
Attached patch patch for betaSplinter Review
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
Flags: needinfo?(dkeeler)
Attachment #8548366 - Flags: review+
Attachment #8548366 - Flags: approval-mozilla-beta?
Attachment #8548366 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attached patch patch for b2g34Splinter Review
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
Flags: needinfo?(dkeeler)
Attachment #8549282 - Flags: review+
Attachment #8549282 - Flags: approval-mozilla-b2g34?
Attachment #8549282 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+
Whiteboard: [reporter-external] → [reporter-external][adv-main36+]
Alias: CVE-2015-0832
Verified fixed using Fiddler and the steps outlined in the bug description.
Status: RESOLVED → VERIFIED
Whiteboard: [reporter-external][adv-main36+] → [reporter-external][adv-main36+][b2g-adv-main2.2+]
Group: core-security → core-security-release
Group: core-security-release
Duplicate of this bug: 774769
You need to log in before you can comment on or make changes to this bug.