1399540 - (CVE-2017-7838) Mozilla Firefox IDN display vulnerability

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect)

Description

[Corey Bonnell]

Attachment: poc.html

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:55.0) Gecko/20100101 Firefox/55.0
Build ID: 20170824053838

Steps to reproduce:

1. Created the attached PoC HTML page, which contains a hyperlink to a fake A-label subdomain (xn--accountlogin) of a valid IDN (xn--google.com).
2. Opened the attached PoC HTML page in the latest released version of Firefox (55.0.3)
3. Hovered over the hyperlink
4. Observed the display of the hyperlink URL


Actual results:

The displayed URL was "xn--accountlogin.xn--google.com".


Expected results:

The displayed URL should be "xn--accountlogin.䕮䕵䕶䕱.com".

An issue that makes this slightly more serious is that some URL scanning and phishing services will fail to detect the URL, since they assume the browser will display “xn--google.com” in its UNICODE format, and not its Punycode format, and that the URL is therefore not deceptive.  They do this to avoid false positives for Punycode domains that happen by chance to have a deceptive substring, but are not themselves deceptive.  The ability to force Firefox to display bare Punycode makes such URLs dangerous again.

The attached Proof of Concept demonstrates that an attacker could register a Punycode spoof domain (such as xn--google.com) and then specify a Fake A-label subdomain so that the Punycode spoof domain name is displayed in its Punycode representation. In this proof of concept, we demonstrate that a Fake A-label subdomain of "xn--accountlogin" incorrectly causes the parent label "xn--google" to be rendered as Punycode. We expect the correct behavior to be that the rendering of parent name labels to be independent of child label values. In other words, we believe the correct rendering of this domain name to be "xn--accountlogin.䕮䕵䕶䕱.com", not "xn--accountlogin.xn--google.com". It should be noted that the latest released version of Chrome and Safari consistently display this domain name as "䕮䕵䕶䕱.com" regardless of child label values.

Comment 1

[Daniel Veditz [:dveditz]]

The spoofiness of xn--google.com is noted in the Unicode specs: http://www.unicode.org/reports/tr36/#Punycode_Spoofs

By itself, or paired with an ascii subdomain, it's handled correctly. Why is adding a xn--accountlogin label breaking things?

The hover text can be spoofed programmatically anyway so I'm less concerned about that than the domain display when you finally do navigate. If we fix that it will fix the hover text.

Comment 2

[Daniel Veditz [:dveditz]]

Although xn--google.com happens to be a valid IDN, some spoofy xn-- names might not be and then they'd stay punycode per spec.

Comment 3

[Jonathan Kew [:jfkthame]]

(In reply to Daniel Veditz [:dveditz] from comment #1)
> By itself, or paired with an ascii subdomain, it's handled correctly. Why is
> adding a xn--accountlogin label breaking things?

Curious.

It looks to me like this is a bug in nsIDNService::ACEtoUTF8. Note the comment at [1] which refers to RFC 3490 (now superseded, but that's beside the point here). This says that "If any step fails, then the original input sequence is returned...", and sure enough, we do that in the loop at [2] (and again for the last fragment at [3]).

Trouble is, the text in RFC 3490 that was referred to here[4] is talking about a *single* label ("a sequence of Unicode code points that make up one label"), not the entire IDN that may be made up of a sequence of labels. But we call ACEtoUTF8 on the complete domain name (e.g. "www.xn--accountlogin.xn--google.com") that we're intending to display, and it handles looping over the string and converting each dot-delimited substring.

So the bug is that when we encounter a decoding failure on *any* of the individual labels in the string ACEtoUTF8 is processing, we immediately return the complete unchanged input for the *whole* IDN. What we should be doing, I think, is considering such failures independently for each label: if decodeACE fails for an individual label, then that individual label (only) should be left in its original form, but the remaining labels may still be separately decoded.

I'll attach a patch that fixes this, though we'll need to check what else it might impact...

[1] https://dxr.mozilla.org/mozilla-central/rev/f9a5e9ed62103c84e4cde915f4d08f1ce71be83e/netwerk/dns/nsIDNService.cpp#300-302
[2] https://dxr.mozilla.org/mozilla-central/rev/f9a5e9ed62103c84e4cde915f4d08f1ce71be83e/netwerk/dns/nsIDNService.cpp#316-319
[3] https://dxr.mozilla.org/mozilla-central/rev/f9a5e9ed62103c84e4cde915f4d08f1ce71be83e/netwerk/dns/nsIDNService.cpp#330-332
[4] https://tools.ietf.org/html/rfc3490#section-4.2

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: Failure to decode an individual label within the IDN should not block decoding of other valid punycode labels

PoC patch, not extensively tested yet.

Comment 5

[Jonathan Kew [:jfkthame]]

(In reply to Daniel Veditz [:dveditz] from comment #1)
> The hover text can be spoofed programmatically anyway so I'm less concerned
> about that than the domain display when you finally do navigate. If we fix
> that it will fix the hover text.

The patch above fixes the domain display when you navigate to the example site; however, it does *not* seem to affect the hover text. I'm not sure offhand what that depends on...

Comment 6

[Jonathan Kew [:jfkthame]]

(In reply to Jonathan Kew (:jfkthame) from comment #5)
> (In reply to Daniel Veditz [:dveditz] from comment #1)
> > The hover text can be spoofed programmatically anyway so I'm less concerned
> > about that than the domain display when you finally do navigate. If we fix
> > that it will fix the hover text.
> 
> The patch above fixes the domain display when you navigate to the example
> site; however, it does *not* seem to affect the hover text. I'm not sure
> offhand what that depends on...

Actually, we don't seem to decode the punycode labels for the hover text even if they're all valid: "http://www.xn--google.com/" displays as such in the hover text, even though it becomes http://www.䕮䕵䕶䕱.com/ in the URL bar.

Comment 7

[Jonathan Kew [:jfkthame]]

Comment on attachment 8908308 [details] [diff] [review]
Failure to decode an individual label within the IDN should not block decoding of other valid punycode labels

AFAICS, this has been handled incorrectly for the case of processing an entire domain (rather than just a single label) ever since the ACEtoUTF8 method was first introduced, back in bug 196717.

We don't appear to have any tests that are affected by this, so I'll look into adding some coverage.

Comment 8

[Gervase Markham [:gerv]]

Comment #3 seems like the right analysis to me. This is a different class of problem to the other "spoofing" issues we've encountered recently; this is simply a bug.

Gerv

Comment 9

[Jonathan Kew [:jfkthame]]

Looks like there's an additional, related problem that we should also fix: calling nsIDNService::ConvertToDisplayIDN with something like "xn--google.䕮䕵䕶䕱.com" (with the default profile) returns, as expected, "䕮䕵䕶䕱.䕮䕵䕶䕱.com".

But calling it with "xn--x.䕮䕵䕶䕱.com" (where "xn--x" is not a valid punycode label) returns the mangled result "xn--xn--x-pm43a.䕮䕵䕶䕱.com".

Simple testcase:

  data:text/html;charset=utf-8,<a href="http://xn--x.%E4%95%AE%E4%95%B5%E4%95%B6%E4%95%B1.com">test</a>

Here, the hover text appears as "xn--xn--x-pm43a.xn--google.com", with incorrect subdomain; and after navigating, the URL bar shows the destination as "xn--xn--x-pm43a.䕮䕵䕶䕱.com".

So an additional patch will be needed, I think.

Comment 10

[Jonathan Kew [:jfkthame]]

Attachment: patch 2 - Handle invalid punycode better in stringPrep to avoid mangling display of fake-punycode labels

The issue in comment 9 arises during stringPrep calls made by nsIDNService::Normalize, because ICU's uidna_labelToUnicode will append U+FFFD to a label that contains invalid punycode, which confuses our subsequent processing. It also returns an error flag, but we're currently ignoring that. :(  By detecting this error and stripping the extra REPLACEMENT CHARACTER from such a label, we can fix the garbled result we're getting for such labels.

Comment 11

[Jonathan Kew [:jfkthame]]

Attachment: Add some IDN testcases with mixed punycode and non-punycode labels

This adds a few IDN tests that relate to the two bugs being fixed here. (It was when trying to create such tests for the first patch that I noticed the broken processing of invalid labels described in comment 9, and fixed by the second patch.) Both the above patches are needed in order for these to pass successfully.

Comment 12

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8908308 [details] [diff] [review]
Failure to decode an individual label within the IDN should not block decoding of other valid punycode labels

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

::: netwerk/dns/nsIDNService.cpp
@@ +327,1 @@
>        }

This does make sense, and it's certainly a step in the right direction.
But other browsers fail URL parsing when failing to decode ACE labels. I'm wondering if we should do the same.

Comment 13

[Jonathan Kew [:jfkthame]]

(In reply to Valentin Gosu [:valentin] from comment #12)
> But other browsers fail URL parsing when failing to decode ACE labels. I'm
> wondering if we should do the same.

In what sense do they fail? AFAICT, with the PoC here Chrome accepts the "xn--accountlogin.xn--google.com" address and tries to navigate there, ending up at a domain-parking page for "www.xn--accountlogin.䕮䕵䕶䕱.com"; this is the same behavior we get once the patches here are applied.

Comment 14

[Valentin Gosu [:valentin] (he/him)]

(In reply to Jonathan Kew (:jfkthame) from comment #13)
> (In reply to Valentin Gosu [:valentin] from comment #12)
> > But other browsers fail URL parsing when failing to decode ACE labels. I'm
> > wondering if we should do the same.
> 
> In what sense do they fail? AFAICT, with the PoC here Chrome accepts the
> "xn--accountlogin.xn--google.com" address and tries to navigate there,
> ending up at a domain-parking page for "www.xn--accountlogin.䕮䕵䕶䕱.com"; this
> is the same behavior we get once the patches here are applied.

I was trying out one of your tests in Chrome:

var url = new URL("http://xn--accountlogin.xn--google"); url.href
> "http://xn--accountlogin.xn--google/"
var url = new URL("http://xn--accountlogin.䕮䕵䕶䕱.com");
> VM146:1 Uncaught TypeError: Failed to construct 'URL': Invalid URL
>     at <anonymous>:1:11
> (anonymous) @ VM146:1

Edge had a similar behaviour. I guess we should leave that for a follow up.

Comment 15

[Jonathan Kew [:jfkthame]]

(In reply to Valentin Gosu [:valentin] from comment #14)
> I was trying out one of your tests in Chrome:
> 
> var url = new URL("http://xn--accountlogin.xn--google"); url.href
> > "http://xn--accountlogin.xn--google/"
> var url = new URL("http://xn--accountlogin.䕮䕵䕶䕱.com");
> > VM146:1 Uncaught TypeError: Failed to construct 'URL': Invalid URL
> >     at <anonymous>:1:11
> > (anonymous) @ VM146:1

Interesting... so Chrome's URL parsing rejects the mixed form "http://xn--accountlogin.䕮䕵䕶䕱.com" but accepts "http://xn--accountlogin.xn--google.com", although AFAICT the two should be equivalent. IMO it would make more sense that either -both- of these should be rejected on the basis that they include an invalid-ACE label, or neither should be rejected (at the parsing level, although the invalid label will never be successfully resolved). But further study of the relevant specs may be needed to see whether there's any clear statement on this.

So yes, sounds like something to consider for a followup.

Comment 16

[Jonathan Kew [:jfkthame]]

(In reply to Jonathan Kew (:jfkthame) from comment #15)
> Interesting... so Chrome's URL parsing rejects the mixed form
> "http://xn--accountlogin.䕮䕵䕶䕱.com" but accepts
> "http://xn--accountlogin.xn--google.com", although AFAICT the two should be
> equivalent. IMO it would make more sense that either -both- of these should
> be rejected on the basis that they include an invalid-ACE label, or neither
> should be rejected (at the parsing level, although the invalid label will
> never be successfully resolved). But further study of the relevant specs may
> be needed to see whether there's any clear statement on this.

To follow up on this, I tried reading the URL parsing algorithm[1] and AFAICT, it looks like Chrome is in error here (which is comforting, given its apparent inconsistency).

The algorithm depends on a "host parser"[2] which in turn uses a "domain to ASCII"[3] algorithm based on Unicode's ToASCII[4]. This basically splits the domain on dots, and then punycode-encodes any labels that contain non-ASCII characters; but I don't see anything that requires an invalid-ACE label like "xn--accountlogin" to result in a validation failure, nor any justification for treating this differently depending on whether a separate label within the domain contained non-ASCII chars (and therefore was punycode-encoded by ToASCII).

It's entirely possible, of course, that I've overlooked something; there are a lot of scattered pieces of spec involved here!

[1] https://url.spec.whatwg.org/#concept-basic-url-parser
[2] https://url.spec.whatwg.org/#concept-host-parser
[3] https://url.spec.whatwg.org/#concept-domain-to-ascii
[4] http://www.unicode.org/reports/tr46/#ToASCII

Comment 17

[Jonathan Kew [:jfkthame]]

And FWIW, Safari agrees with us here, that the two cases "xn--x.xn--google" and "xn--x.䕮䕵䕶䕱" are treated identically:

> new URL("http://xn--x.xn--google.com")
< URL {href: "http://xn--x.xn--google.com/", origin: "http://xn--x.xn--google.com", protocol: "http:", username: "", password: "", …}
> new URL("http://xn--x.䕮䕵䕶䕱.com")
< URL {href: "http://xn--x.xn--google.com/", origin: "http://xn--x.xn--google.com", protocol: "http:", username: "", password: "", …}

Whereas Chrome, as you note, rejects the latter version with a TypeError.

Comment 18

[Jonathan Kew [:jfkthame]]

I've filed https://bugs.chromium.org/p/chromium/issues/detail?id=765922 to see what they think about this.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fbc5b903dbdd
https://hg.mozilla.org/mozilla-central/rev/42b73bde7b4f
https://hg.mozilla.org/mozilla-central/rev/8e697bb08031

Comment 20

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

Confirmed issue in Fx56.0.2.
Verified fixed in Fx57.0b14.