1402368 - (CVE-2018-5121) URL spoofing with using U+0F37 and U+0F35 Tibetan

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect)

Description

[Khalil Zhani]

http://xn--google-usv.com/   (U+0F37 after 'e').
http://xn--google-gsv.com/ (U+0F35 after 'e').

Comment 1

[Khalil Zhani]

Attachment: Screen Shot 2017-09-25 at 16.10.26.png

Comment 2

[Khalil Zhani]

U+0F37 + U+0F35 rendered as 'space' on Mac e.g. http://www.google.com༵༷༵༷༵༷༵༷༵༷༵༷༵༷༵༷༵༷༵༷༵༷༵༷༵༷༵༷༵༷༵༷༵༷༵༷༵༷༵༷༵༷༵༷༵༷༵༷༵༷.bntk.pl/

Comment 3

[Daniel Veditz [:dveditz]]

Jonathan: will bug 1370497 (spoofy combining marks) address this one, too?

Also like the Arabic character that inspired that bug, the mark in this bug is below the bottom of the text box we show in the Address Bar, although it looks like there ought to be enough whitespace to have shown it. Should that be filed as a separate UX bug? On Mac, anyway, it looks like a normal 'e' with space after it, but if you copy-paste it elsewhere you'll see the dot/ring thing that's below the e further down than the 'g' descender goes (the display box seems to stop at or maybe a pixel below the 'g' descender).

Comment 4

[Daniel Veditz [:dveditz]]

Since the combining marks are certainly a dupe of bug 1370497 maybe we should morph this bug into one to cover us not showing the full height required for some characters/combinations.

Comment 5

[Khalil Zhani]

This bug seems like bug 1390980 because U+0F37 + U+0F35 turned to blank e.g http://www.mail.google.com༵༷༵༷༵༷༵༷༵༷༵༷༵༷༵༷༵༷༵༷༵༷༵༷༵༷༵༷༵༷༵༷༵༷༵༷༵༷༵༷༵༷༵༷༵༷༵༷༵༷.bntk.pl/

Comment 6

[Khalil Zhani]

Attachment: Screen Shot 2017-09-25 at 22.48.44.png

Comment 7

[Jonathan Kew [:jfkthame]]

(In reply to Daniel Veditz [:dveditz] from comment #3)
> Jonathan: will bug 1370497 (spoofy combining marks) address this one, too?

AFAICS, it should do. These marks have Script=Tibetan, so the patch there will reject them (i.e. force punycode display) if they're applied to Latin letters.

Comment 8

[Jonathan Kew [:jfkthame]]

(In reply to Khalil Zhani from comment #5)
> This bug seems like bug 1390980 because U+0F37 + U+0F35 turned to blank e.g
> http://www.mail.google.com༵༷༵༷༵༷༵༷༵༷༵༷༵༷༵༷༵༷༵༷༵༷༵༷༵༷༵༷༵༷༵༷༵༷༵༷༵༷༵༷༵༷༵༷༵༷༵༷༵༷.
> bntk.pl/

They're not actually blank in any of the Mac fonts (at least on my 10.12 system), which was the issue with several other Tibetan characters in bug 1390980. But as Dan notes, they are sufficiently low down that they are clipped (or largely clipped -- see the residual dots in the "blank" space in attachment 8912018 [details]) from the URL bar field, although they show in the tab title.

Comment 9

[:Gijs (he/him)]

The line-height of the <html:input> at the heart of the urlbar is set to `inherit` which inherits `-moz-use-system-font`. Why is that not sufficient to display the entirety of the text? (Changing it to e.g. 1.6em seems like it might help, but...)

Do we clip these on Windows/Linux as well? Or only on OSX?

Comment 10

[Jonathan Kew [:jfkthame]]

(In reply to :Gijs from comment #9)
> The line-height of the <html:input> at the heart of the urlbar is set to
> `inherit` which inherits `-moz-use-system-font`. Why is that not sufficient
> to display the entirety of the text?

Because we're seeing characters that aren't supported by the system font (SFNS Text), so font fallback comes into play, and is finding large-repertoire CJK fonts where some of these characters are positioned significantly lower than the metrics of the system font would allow for.

Comment 11

[:Gijs (he/him)]

Attachment: Patch v0.1

Something like this seems to work on OS X?

Comment 12

[:Gijs (he/him)]

(In reply to Jonathan Kew (:jfkthame) from comment #10)
> (In reply to :Gijs from comment #9)
> > The line-height of the <html:input> at the heart of the urlbar is set to
> > `inherit` which inherits `-moz-use-system-font`. Why is that not sufficient
> > to display the entirety of the text?
> 
> Because we're seeing characters that aren't supported by the system font
> (SFNS Text), so font fallback comes into play, and is finding
> large-repertoire CJK fonts where some of these characters are positioned
> significantly lower than the metrics of the system font would allow for.

So, we can compensate for the characters in question with a simple CSS fix, I think, but I'm not sure that will be sufficient if other such characters appear, because I assume that, in principle, there is no limit to how tall the characters will be. Is that right?

I don't see any overflow set on the input or its descendants or its ancestors, so I can only assume that things being cut off is the result of layout treating HTML <input> things "specially", so I doubt we can do anything about that...

Comment 13

[Jonathan Kew [:jfkthame]]

There's a property "overflow: -moz-hidden-unscrollable" in urlbar-searchbar.inc.css, which I think has some relevance here, but AFAICT just changing this (e.g. to visible) is not sufficient to solve things here. It does mean you can drag-scroll the content of the URL-bar upwards to see the low marks, but I don't think we really want that result anyhow.

Comment 14

[Jonathan Kew [:jfkthame]]

Comment on attachment 8912185 [details] [diff] [review]
Patch v0.1

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

This seems reasonable to me, and certainly helps, but I'm not really a valid reviewer for front-end stuff.

FWIW, in my testing setting a line-height of 1.7em causes a *very* slight shift of the text baseline in the URL bar, though it's too slight to matter, I expect. But you might want to consider 1.67em, which AFAICS causes no visible shift at all. (With current system fonts on 10.12; of course, font changes in future OS versions might affect this.)

Comment 15

[Dão Gottwald [:dao]]

(In reply to Jonathan Kew (:jfkthame) from comment #13)
> There's a property "overflow: -moz-hidden-unscrollable" in
> urlbar-searchbar.inc.css, which I think has some relevance here

I don't think so. That's applied on the <xul:textbox/>, but text already can't overflow the anonymous <html:input/>.

Comment 16

[Jonathan Kew [:jfkthame]]

(In reply to :Gijs from comment #12)
> So, we can compensate for the characters in question with a simple CSS fix,
> I think, but I'm not sure that will be sufficient if other such characters
> appear, because I assume that, in principle, there is no limit to how tall
> the characters will be. Is that right?

That's true, but it seems unlikely things will get much worse than this. Note that we're only dealing with the system fonts that may get used as a result of fallback for obscure characters; we don't have to worry about a malicious site deploying a webfont with extremely high or low glyphs, as they won't be used in the URL bar anyhow.

Comment 17

[:Gijs (he/him)]

(In reply to Dão Gottwald [::dao] from comment #15)
> (In reply to Jonathan Kew (:jfkthame) from comment #13)
> > There's a property "overflow: -moz-hidden-unscrollable" in
> > urlbar-searchbar.inc.css, which I think has some relevance here
> 
> I don't think so. That's applied on the <xul:textbox/>, but text already
> can't overflow the anonymous <html:input/>.

Right, I tried removing this or replacing with an explicit overflow:visible, and that had no effect.

(In reply to Jonathan Kew (:jfkthame) from comment #14)
> This seems reasonable to me, and certainly helps, but I'm not really a valid
> reviewer for front-end stuff.

Yep, this is why I also asked Dão for a review. I wanted both of your opinions. :-)

> FWIW, in my testing setting a line-height of 1.7em causes a *very* slight
> shift of the text baseline in the URL bar, though it's too slight to matter,
> I expect. But you might want to consider 1.67em, which AFAICS causes no
> visible shift at all. (With current system fonts on 10.12; of course, font
> changes in future OS versions might affect this.)

Right, I noticed this, but I didn't think it was a big deal, effectively. The overall size of the urlbar is unchanged, as far as I can see, and the identity block text is shifted in the same way so the alignment doesn't really change, either.

When using 1.67em, there is indeed no shift but these characters are also slightly cut-off at the bottom still. I guess the question is how important that is, and I don't speak Tibetan, nor do I know if there are other languages that would be implicated here, so I don't know how to determine that.

Comment 18

[Dão Gottwald [:dao]]

Comment on attachment 8912185 [details] [diff] [review]
Patch v0.1

This makes the location bar slightly taller in compact mode at least on Ubuntu.

Comment 19

[:Gijs (he/him)]

(In reply to Dão Gottwald [::dao] from comment #18)
> Comment on attachment 8912185 [details] [diff] [review]
> Patch v0.1
> 
> This makes the location bar slightly taller in compact mode at least on
> Ubuntu.

Do you have a suggestion for what to do instead? Should we restrict the change to OSX, or do you have an alternative idea?

Comment 20

[Dão Gottwald [:dao]]

Did bug 1399567 perhaps fix this on Mac?

Comment 21

[:Gijs (he/him)]

(In reply to Dão Gottwald [::dao] from comment #20)
> Did bug 1399567 perhaps fix this on Mac?

Unfortunately not. Re-needinfo'ing for comment #19...

Comment 22

[:Gijs (he/him)]

I do notice that now, it no longer seems to be the case that the EV label moves, so if we do this only on mac, I guess we have to apply the same line-height to the EV label.

Comment 23

[Dão Gottwald [:dao]]

Mac-specific patch works for me. It makes me a bit nervous that this can affect the location bar height at all, but I suppose that's only a hypothetical concern on Mac. I also wonder if this affects the vertical text alignment. Have you checked that normal URLs (no Tibetan characters) are positioned as before?

Comment 24

[:Gijs (he/him)]

Attachment: Patch v0.2

Tested on both compact and normal mode, this doesn't vertically shift the text or change the height of the urlbar. Confusingly, a slightly lower value of 1.7em does vertically shift the text (but doesn't change the height of the textbox), and then a slightly higher value puts it back. I assume there's a rounding issue somewhere, but given that this works I think it's OK. Note that the 1.745 value matches 24px (default font size is 11px but the urlbar gets font-size: 1.25em, so 13.75, and 24/13.75 is 1.745). I can also use 24px but I figured we should stick with em as that's what we use elsewhere.

Comment 25

[:Gijs (he/him)]

Note that bug 1399939 mitigates this by turning the IDN restrictions up to 'high' (you can test by simply flipping the pref back to "moderate", which works at runtime). I figure taking this is probably still a good idea to avoid not displaying certain text on OS X, or in case we have to back out the IDN restriction change if we run into web compact issues or something like that.

Comment 26

[:Gijs (he/him)]

(In reply to :Gijs from comment #24)
> Confusingly, a slightly lower value
> of 1.7em does vertically shift the text (but doesn't change the height of
> the textbox), and then a slightly higher value puts it back. I assume
> there's a rounding issue somewhere, but given that this works I think it's
> OK.

To be clear, this was a barely-observable shift of 1 device pixel (ie 0.5 css px) on a hidpi device, so it's not exactly earth-shattering, but hey.

Comment 27

[Dão Gottwald [:dao]]

(In reply to :Gijs (no reviews, PTO EOB on 11th) from comment #25)
> Note that bug 1399939 mitigates this by turning the IDN restrictions up to
> 'high' (you can test by simply flipping the pref back to "moderate", which
> works at runtime). I figure taking this is probably still a good idea to
> avoid not displaying certain text on OS X,

Assuming bug 1399939 sticks, is this still a more realistic concern on Mac than on other platforms? If not, I suggest we just close this as fixed.

Comment 28

[:Gijs (he/him)]

(In reply to Dão Gottwald [::dao] from comment #27)
> (In reply to :Gijs (no reviews, PTO EOB on 11th) from comment #25)
> > Note that bug 1399939 mitigates this by turning the IDN restrictions up to
> > 'high' (you can test by simply flipping the pref back to "moderate", which
> > works at runtime). I figure taking this is probably still a good idea to
> > avoid not displaying certain text on OS X,
> 
> Assuming bug 1399939 sticks, is this still a more realistic concern on Mac
> than on other platforms? If not, I suggest we just close this as fixed.

Well, there are scripts where these characters are legitimate, so we're basically still cropping bits that we shouldn't. I don't know if that's a concern given the font thingymajig that's going on with this particular character vs. more "normal" text. Jonathan can hopefully answer that question more conclusively.

Comment 29

[Jonathan Kew [:jfkthame]]

The location bar on OS X currently crops some legitimate diacritics, e.g. in Arabic, which could lead to ambiguity/spoofability for purely Arabic-script IDNs. As such, I think it's worth taking the patch here in order to reduce the risk of such clipping.

Comment 30

[Dão Gottwald [:dao]]

Comment on attachment 8914308 [details] [diff] [review]
Patch v0.2

>+++ b/browser/base/content/browser.css

>+%ifdef XP_MACOSX
>+html|input.urlbar-input {
>+  line-height: 1.745em;
>+}
>+%endif

Can you please move this here:

http://searchfox.org/mozilla-central/rev/1a4a26905f923458679a59a4be1e455ebc53c333/browser/themes/osx/browser.css#268

and also document it?

Comment 31

[:Gijs (he/him)]

Attachment: Patch v0.3

Comment 32

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/747cfc86a8cccb5a80fb224a9d15c017fd3c4937

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/747cfc86a8cc

Comment 34

[Ryan VanderMeulen [:RyanVM]]

Is this something we should consider for backport to 57 or can it ride the 58 train?

Comment 35

[Jonathan Kew [:jfkthame]]

IMO, with bug 1399939 having been uplifted, the remaining issue here isn't critical enough to justify backporting. (But if ::dao wants to disagree, go ahead...)

Comment 36

[Dão Gottwald [:dao]]

Agreed.

Comment 37

[Bogdan Maris, Desktop Test Engineering]

I reproduced this issue on Nightly 58.0a1 (2017-09-22) under macOS 10.13. The issue is fixed on Beta 58.0b16 and on latest Nightly (2018-01-22) under macOS 10.13, macOS 10.12 and OS X 10.11.

Comment 38

[Mats Palmgren (inactive)]

Note that as of v59, <input> no longer clips the text at the content edge
(in the vertical direction), but instead at the padding edge (by default).
So that should also give some more room to display text in the URL bar, IIUC.
(bug 752790)

Comment 39

[Khalil Zhani]

Is this report qualified for a bounty? Thanks!

Comment 40

[:Gijs (he/him)]

(In reply to Khalil Zhani from comment #39)

Is this report qualified for a bounty? Thanks!

Comment 41

[Tom Ritter [:tjr]]

(In reply to Khalil Zhani from comment #39)
Is this report qualified for a bounty? Thanks!

Because of its severity rating, it did not qualify for a bounty (this happened in Dan's comment back in 2017.) The flag update I just performed was to ensure that your name does get added to our Hall of Fame however.