Closed Bug 1402368 (CVE-2018-5121) Opened 2 years ago Closed 2 years ago

URL spoofing with using U+0F37 and U+0F35 Tibetan

Categories

(Firefox :: Address Bar, defect)

57 Branch
Unspecified
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 58
Tracking Status
firefox-esr52 --- wontfix
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 --- verified

People

(Reporter: chromium.khalil, Assigned: Gijs)

Details

(Keywords: csectype-spoof, sec-low, Whiteboard: [adv-main58+][post-critsmash-triage])

Attachments

(3 files, 2 obsolete files)

http://xn--google-usv.com/   (U+0F37 after 'e').
http://xn--google-gsv.com/ (U+0F35 after 'e').
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).
Flags: needinfo?(jfkthame)
Flags: needinfo?(gijskruitbosch+bugs)
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.
(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.
Flags: needinfo?(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.
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?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(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.
Flags: needinfo?(jfkthame)
Attached patch Patch v0.1 (obsolete) — Splinter Review
Something like this seems to work on OS X?
Attachment #8912185 - Flags: review?(jfkthame)
Attachment #8912185 - Flags: review?(dao+bmo)
Assignee: nobody → gijskruitbosch+bugs
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(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...
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 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.)
Attachment #8912185 - Flags: review?(jfkthame) → review+
(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/>.
(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.
(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 on attachment 8912185 [details] [diff] [review]
Patch v0.1

This makes the location bar slightly taller in compact mode at least on Ubuntu.
Attachment #8912185 - Flags: review?(dao+bmo) → review-
(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?
Flags: needinfo?(dao+bmo)
Did bug 1399567 perhaps fix this on Mac?
Flags: needinfo?(dao+bmo)
(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...
Flags: needinfo?(dao+bmo)
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.
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?
Flags: needinfo?(dao+bmo)
Attached patch Patch v0.2 (obsolete) — Splinter Review
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.
Attachment #8914308 - Flags: review?(dao+bmo)
Attachment #8912185 - Attachment is obsolete: true
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.
(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.
(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.
Flags: needinfo?(gijskruitbosch+bugs)
(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.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(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.
Flags: needinfo?(jfkthame)
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?
Attachment #8914308 - Flags: review?(dao+bmo)
Attached patch Patch v0.3Splinter Review
Attachment #8917408 - Flags: review?(dao+bmo)
Attachment #8914308 - Attachment is obsolete: true
Attachment #8917408 - Flags: review?(dao+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/747cfc86a8cc
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Is this something we should consider for backport to 57 or can it ride the 58 train?
Flags: needinfo?(dao+bmo)
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...)
Agreed.
Flags: needinfo?(dao+bmo)
Group: firefox-core-security → core-security-release
Flags: sec-bounty?
Flags: sec-bounty? → sec-bounty-
Keywords: sec-moderatesec-low
Whiteboard: [adv-main58+]
Alias: CVE-2018-5121
Whiteboard: [adv-main58+] → [adv-main58+][post-critsmash-triage]
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.
Status: RESOLVED → VERIFIED
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)
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.