Bug 1395508 (CVE-2018-5117)

Firefox address bar spoof using RTL language and references

VERIFIED FIXED in Firefox -esr52

Status

()

P1
normal
VERIFIED FIXED
2 years ago
5 months ago

People

(Reporter: xisigr, Assigned: mak)

Tracking

({csectype-spoof, sec-moderate})

55 Branch
Firefox 59
csectype-spoof, sec-moderate
Points:
---
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox-esr5258+ fixed, firefox55 wontfix, firefox56- wontfix, firefox57+ wontfix, firefox58 verified, firefox59 verified)

Details

(Whiteboard: [fxsearch][adv-main58+][adv-esr52.6+][post-critsmash-triage])

Attachments

(5 attachments, 12 obsolete attachments)

1.46 MB, image/png
Details
427 bytes, text/html
Details
645.91 KB, image/png
Details
3.35 KB, patch
Details | Diff | Splinter Review
3.29 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

2 years ago
Posted image spoof.png
Firefox address bar spoof using hebrew on Windows/macOS.

poc.html

<meta http-equiv="content-type" content="text/html;charset=utf-8">
<script>
    
    function aa(){
        var link = document.createElement('a');
        link.href = "https://xn--ggbla1c4e.xn--ngbc5azd/#"+Array(0x200).join("%20")+"סוֹ.סח";
        link.target="aaaa";
        document.body.appendChild(link);
        link.click();
    }
    
</script>
    
<a onclick="aa();" href="javascript:void(0);">CLICK ME</a>
    
Expected Result:
https://اسماء.شبكة

Rendered Results:
https://no.io
(Reporter)

Comment 1

2 years ago
Posted file poc.html
Keywords: csectype-spoof, sec-moderate
Summary: Firefox address bar spoof using hebrew → Firefox address bar spoof using LTR language and references
Works on Linux too.

Dan: did you mean "RTL language" in the new title?

So the trick here is that the Arabic domain is RTL, so it appears on the right of the address bar, but then it gets pushed off by all the spaces and Hebrew text, because we are still left-justifying the contents? IOW, the fix is to make sure that the justification rules on the address bar are such that the actual domain is always visible?

Gerv
Works on Windows too. Clever.
Status: UNCONFIRMED → NEW
status-firefox55: --- → wontfix
status-firefox56: --- → affected
status-firefox57: --- → affected
status-firefox-esr52: --- → affected
tracking-firefox56: --- → ?
tracking-firefox57: --- → ?
tracking-firefox-esr52: --- → ?
Ever confirmed: true
OS: Unspecified → All
Hardware: Unspecified → All
Since this issue has a sec-moderate rating right now I don't want to track it for 56. We're getting close to the 56 release. Happy to take a patch for 56 if we land one. 

mak, can you help find someone to work on this? 
Do you think it is urgent to fix for 56/57?
Flags: needinfo?(mak77)
(Assignee)

Comment 5

2 years ago
Forwarding to Panos to evaluate the available resources.
It would be nice to fix it for sure, I don't know if it's as urgent to be in 56, the spoofed font is somehow recognizable and the identity panel points to the right domain. I can see it as a concern for a less expert user.

Surely the best thing would be to always scroll the locationbar to show the domain.
Flags: needinfo?(mak77) → needinfo?(past)

Comment 6

2 years ago
(In reply to Marco Bonardo [::mak] from comment #5)
> Forwarding to Panos to evaluate the available resources.
> It would be nice to fix it for sure, I don't know if it's as urgent to be in
> 56, the spoofed font is somehow recognizable and the identity panel points
> to the right domain. I can see it as a concern for a less expert user.
> 
> Surely the best thing would be to always scroll the locationbar to show the
> domain.

We already do that (force selectionStart + End to 0). It's not clear to me off-hand what part of it isn't working somehow.
(Assignee)

Comment 7

2 years ago
(In reply to :Gijs (queue backed up, slow) from comment #6)
> We already do that (force selectionStart + End to 0). It's not clear to me
> off-hand what part of it isn't working somehow.

The url is LTR aligned (https is on the left), but the RTL chars are RTL aligned. And looks like there can be mixed direction domains. The input field is LTR in an LTR build.
There's some background in bug 993806 and bug 641238 for the braves.

Fwiw, Chrome puts the scheme in a static element, and scrolls all the rest of the string to the right. It may not be necessary to keep showing the scheme considered the identity panel? (they also don't decode the %20)
Flags: needinfo?(past)
Priority: -- → P1
Whiteboard: [fxsearch]
Panos asked me to look at this, but with the understanding that it can wait until Sept. 15, the Photon soft code freeze.  So I'll assign myself now, but if anyone else wants to take it before then, please feel free.
Assignee: nobody → adw
Status: NEW → ASSIGNED
(In reply to :Gijs from comment #6)
> (In reply to Marco Bonardo [::mak] from comment #5)

> > Surely the best thing would be to always scroll the locationbar to show the
> > domain.
> 
> We already do that (force selectionStart + End to 0). It's not clear to me
> off-hand what part of it isn't working somehow.

The trouble is, the domain is nowhere near the start (offset 0) of the urlbar content. Setting selectionStart/End to 0 ensures that the left-hand edge of the "https://..." scheme is visible; but then the domain is at the far right of the urlbar content, out of sight.

If I arbitrarily hack browser.js so that it sets selectionStart/End to 8 instead of 0 (i.e. placing the selection immediately after "https://"), then the testcase here behaves better, with the (Arabic-script) domain visible -- although not perfectly, because of the fade-out effect is still applied at the right-hand end, so the beginning of the domain name fades away. And of course now the scheme is no longer visible. So even if we did this (more correctly, by checking the actual offset of the domain rather than hard-coding "8"), it wouldn't be very satisfactory.

If we could avoid decoding the %20 characters, that would help quite a lot, although it could make some legitimate URLs look more ugly. (Note that the %20s in question are part of the fragment identifier here, not part of an actual domain label.) Offhand I'm not sure where to hack that, though.
(Assignee)

Comment 10

2 years ago
(In reply to Jonathan Kew (:jfkthame) from comment #9)
> And of
> course now the scheme is no longer visible.

That's why I'm wondering if, with the identity icon always visible, we still care about the scheme.
If we care, Chrome solution (sticky scheme) sounds feasible.
(In reply to Marco Bonardo [::mak] from comment #10)
> (In reply to Jonathan Kew (:jfkthame) from comment #9)
> > And of
> > course now the scheme is no longer visible.
> 
> That's why I'm wondering if, with the identity icon always visible, we still
> care about the scheme.
> If we care, Chrome solution (sticky scheme) sounds feasible.

There's also the issue that the fade-out truncation we apply at the (right-hand) end of the urlbar results in fading the beginning of the domain name.
We could drop the fade out at the end and instead truncate in the middle with an ellipsis (…) so that the beginning and end of the URL are always visible.  Is that what you mean by a sticky scheme, Marco?

I agree with Jonathan that not decoding the spaces would help a lot.  I'd go further and say that by itself is an acceptable fix for this case.  But I think I like the truncation idea, too.
Posted image Chrome screenshot (obsolete) —
(In reply to Drew Willcoxon :adw from comment #12)
> Is that what you mean by a sticky scheme, Marco?

I guess so.  Here's a Chrome screenshot.
(Assignee)

Comment 14

2 years ago
(In reply to Drew Willcoxon :adw from comment #13)
> Created attachment 8905279 [details]
> Chrome screenshot
> 
> (In reply to Drew Willcoxon :adw from comment #12)
> > Is that what you mean by a sticky scheme, Marco?
> 
> I guess so.  Here's a Chrome screenshot.

yes, even if I'd not define that "in the middle", it looks like they put ellipsis just after the scheme (so the scheme is "sticky" positioned and the rest of the url is just scrolled to the far right).
Putting ellipsis in the middle, with that many spaces could still be confusing, especially on 16:9 and ultrawide (21:9) screens, the real domain would still be at the far right, while the fake one would still appear just after the scheme.
(In reply to Marco Bonardo [::mak] from comment #14)
> (In reply to Drew Willcoxon :adw from comment #13)
> > Created attachment 8905279 [details]
> > Chrome screenshot
> > 
> > (In reply to Drew Willcoxon :adw from comment #12)
> > > Is that what you mean by a sticky scheme, Marco?
> > 
> > I guess so.  Here's a Chrome screenshot.
> 
> yes, even if I'd not define that "in the middle", it looks like they put
> ellipsis just after the scheme (so the scheme is "sticky" positioned and the
> rest of the url is just scrolled to the far right).

My interpretation of Chrome's behavior is that the ellipsis is at the (logical) end of the text, which means it appears at the left-hand end of the (right-to-left) text.

Something like this could possibly be achieved by wrapping the URL (not including the scheme) in an inline-block element with dir=auto and styling it with text-overflow:ellipsis, overflow:hidden, and width:.... umm, not sure what. I don't think -moz-available works here, so it might have to be computed somehow.

(Applying text-overflow:ellipsis to the URL as a whole (including scheme) cannot give this result, AFAICT from the spec for text-overflow.)

I suspect forcing the spaces to be displayed as %20 is much easier to implement as a short-term fix, however. Reworking the elements within the urlbar to separate the scheme from the rest of the URL (and allow us to align/truncate them separately) sounds....risky and disruptive.
status-firefox56: affected → fix-optional
tracking-firefox56: ? → -
tracking-firefox57: ? → +
mak, anyone else who might work on this? 
We can still take a patch for 57, but I am dropping tracking on this bug since we are in mid-beta at this point.
status-firefox57: affected → fix-optional
status-firefox58: --- → ?
Flags: needinfo?(mak77)
(Assignee)

Updated

2 years ago
Flags: needinfo?(mak77) → needinfo?(past)
(Assignee)

Comment 17

2 years ago
So, I think that in any case we should show the %20, the spaces are just too confusing.
The 2 proposed solutions:
1. ellipsis in the middle:
   - simpler and potentially upliftable
   - on overflow it may still look confusing
     scheme://fake.domain%20%20%20%20%20%20%20%20%20%20...%20%20%20%20%20%20%20%20%20%20%20%20real.domain
2. separate the scheme from the url, ellipsis at the end of the url
   - more complex to do
   - on overflow it's less confusing
     scheme://...e.domain%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20real.domain

if there's no overflow, the 2 solutions look the same, and the confusion may be solved by the domain hilight. Regardless the hilight helps if we always show the domain, so probably solution 1 has better cost/benefit balance.
Just ensuring the spaces are displayed as %20 goes a long way towards mitigating this, IMO. I think we should do that immediately (and uplift it to 57), and then look into further improvements (either (1) or (2) above) as a followup for 58 or later.
(Assignee)

Comment 19

2 years ago
Another interesting POC is using %e30%80%80 instead of %20. Pretty much any recognized whitespace.
To avoid decoding these, it may be enough to fix losslessDecodeURI that currently skips whitespaces on purpose, probably for beautification reasons.
I can try to patch it and see if any tests are failing. It will also need a test for these 2 cases, at least.
Assignee: adw → mak77
Flags: needinfo?(past)
(Assignee)

Comment 20

2 years ago
I actually meant %E3%80%80, not sure how a zero slipped into that :\
(Assignee)

Comment 21

2 years ago
And here comes back Bug 740910 that makes cropping in the middle a nightmare. It may be easier to actually split the scheme considered that bug is open from 6 years and the proposals got few interest in W3C.

At this point I agree with comment 18, let's just show the encoded whitespaces and I'll file a separate public bug about cropping the urls better.
(Assignee)

Comment 22

2 years ago
Posted patch don't decode whitespaces (obsolete) — Splinter Review
This just avoids decoding whitespaces, so they will stay % encoded.
Cropping is a more complex problem we'd better handle separately, I suggest a normal public bug in the Address Bar for that.
Does this sound sufficiently good to you?
Attachment #8917347 - Flags: review?(jfkthame)
Attachment #8917347 - Flags: feedback?(dveditz)
Decoding whitespaces is a nice UX win. Can we refuse to decode them only when there's more than N of them, or a run of more than N of them, or something?

Gerv
(Assignee)

Comment 24

2 years ago
(In reply to Gervase Markham [:gerv] from comment #23)
> Decoding whitespaces is a nice UX win.

We will keep decoding them for javascript or exotic urls, the change only affects http, https, file and ftp schemes. For these I'm not sure there's any UX win.
While your suggestion is technically feasible, it would make the behavior more unpredictable and maybe even surprising, imo.
(In reply to Marco Bonardo [::mak] from comment #24)
> We will keep decoding them for javascript or exotic urls, the change only
> affects http, https, file and ftp schemes. For these I'm not sure there's
> any UX win.

Really? I'd say that:

https://wiki.mozilla.org/CA/CA Information Checklist

in the URL bar looks rather a lot nicer than:

https://wiki.mozilla.org/CA/CA%20Information%20Checklist

Gerv
(Assignee)

Comment 26

2 years ago
Fwiw, all the wikis I tried rewrite %20 to underscores, makes more sense imo, web devs should use underscores if they care about readability of their urls. Your example in reality would be https://wiki.mozilla.org/CA/CA_Information_Checklist
To look into the other browsers, and understand if we're breaking users expectations: Chrome doesn't decode %20, as well as Edge. I don't have Safari at hand atm.

How would you measure the right N? The window and, as a consequence, the Address Bar, can be small. To hide the smaller host possible you may not need a lot of spaces, indeed even if invalid per RFC, some dns may support single letter hostnames.
If we only care to mitigate the problem, we could maybe allow single isolated spaces.
Let's see what the others think.

Comment 27

2 years ago
FWIW, I too think it would be preferable to find a way to keep decoding spaces in the "non-spoofy" cases for UX reasons, even if that makes the behaviour less predictable, because it'll keep the 'nice' behaviour for the common case while defeating spoofing attempts (if done right :-) ).
(Assignee)

Comment 28

2 years ago
The only "right" way I can think of currently is to decode only isolated whitespaces.
(Assignee)

Comment 29

2 years ago
(In reply to Marco Bonardo [::mak] from comment #28)
> The only "right" way I can think of currently is to decode only isolated
> whitespaces.

Even if, something like "http://fake.domain . . . . . . . . . . . . . . . . . . . . . . . . . real.domain" (or some unicode glyph even more subtle than a dot) could still be quite confusing.
The correct fix here is making sure that the real domain is always visible. Combine that with domain highlighting, and we are fine even if spaces are not decoded at all. Let's focus on that. If we want to stopgap by refusing to decode long runs of spaces, let's do it. But we are already at the edge case of edge cases. Phishers don't bother with this stuff for good reasons. It works fine without.

Gerv
(Assignee)

Comment 31

2 years ago
(In reply to Gervase Markham [:gerv] from comment #30)
> The correct fix here is making sure that the real domain is always visible.

If we want the fix in 57 the only way would be to hide the scheme, and I'm not sure we're comfortable with that.
If we can wait, then surely we could look into the static scheme solution (that off-hand looks easier than waiting for Bug 740910).
I think we can wait.

Gerv
(Assignee)

Updated

2 years ago
Attachment #8917347 - Flags: review?(jfkthame)
Attachment #8917347 - Flags: feedback?(dveditz)
(In reply to Marco Bonardo [::mak] from comment #28)
> The only "right" way I can think of currently is to decode only isolated
> whitespaces.

How about a regex something like

  /%(?!3B|2F|3F|3A|40|26|3D|2B|24|2C|23)|\s(?=\s)|\s$/

This would mean we'll encode any whitespaces that are followed by a further whitespace, or that occur at the end of the URL. (I'd have liked to catch any whitespaces -preceded- by another whitespace, too, but that would have needed a lookbehind assertion, which AFAIK isn't available in JS regex.)

I agree the proper solution is to make sure the real domain is always visible and highlighted, but I don't think we're going to fix that for 57, so I'd suggest doing a whitespace fix like this as an interim fix that could be safely uplifted (also to ESR if desired).
(Assignee)

Comment 34

2 years ago
(In reply to Jonathan Kew (:jfkthame) from comment #33)
>   /%(?!3B|2F|3F|3A|40|26|3D|2B|24|2C|23)|\s(?=\s)|\s$/

We should still always encode \r\t\n, even if they are not followed by another whitespace char. In my patch I just replaced them with \s because it was a catch-all solution. Surely it could be adapted, something like [\r\n\t]|\s(?=\s)|\s$

Though, gerv is right the only real solution is to show and hilight the real domain. I was looking for an "almost whitespace char" to see the effect. Replacing isolated spaces in this case doesn't help much, and I didn't search the whole unicode.
http://www.fake.domain/(%20%DC%82 repeated many times)/real.domain
(In reply to Marco Bonardo [::mak] from comment #34)
> (In reply to Jonathan Kew (:jfkthame) from comment #33)
> >   /%(?!3B|2F|3F|3A|40|26|3D|2B|24|2C|23)|\s(?=\s)|\s$/
> 
> We should still always encode \r\t\n, even if they are not followed by
> another whitespace char.

Ah, right; we should do those in all cases.

>  I was looking for an "almost whitespace char" to see the effect.
> Replacing isolated spaces in this case doesn't help much, and I didn't
> search the whole unicode.
> http://www.fake.domain/(%20%DC%82 repeated many times)/real.domain

At that point, we're getting into territory that's more like the "barely noticeable combining mark" cases -- a glyph that is unambiguously visible (assuming non-broken fonts!), but nevertheless easily overlooked by users who aren't accustomed to noticing it.
(Assignee)

Comment 36

2 years ago
Considering the approvals for 57 were just made strict to the point of "only if it may cause a chemspill release", I'd say we will just skip the stopgap solution.
(Assignee)

Comment 37

a year ago
Posted patch patch v2 (obsolete) — Splinter Review
This is a possible patch. It still lacks tests, but I'd first like to collect some feedback.
* multiple spaces are encoded as suggested by jfkthame, single spaces are not, so we retain ux advantage.
* the address bar gradient is on the left or on the right, depending on the overflow direction.
* there is an overlay label that always shows the scheme when it is overflowed and the url has a host.
* I didn't use ellipsis because it's impossible without proper text-overflow support. If we'd have a text-overflow where one could define at which char to put the ellipsis, it would be trivial.

I tested this with an ltr long url, and this rtl-host long url, in ltr, rtl and dark theme modes. In all the cases both the scheme and the host are visible and the gradient is on the right side. 

The code is not the cleanest, because while it's true we can set the selection to the end of the "scheme://", that still creates situations where the url is overflowed on both sides when moving between states.
It would be much simpler if we'd strip the scheme, but that creates more risks of regressions when we have to put it back, and we'd only strip some schemes, that would make things even more foggy.

Another alternative I tried, was to create a special kind of selection like SELECTION_OMIT, that would not be drawn to the textrun. Then I could set that on the scheme, and always show the scheme in the separate label. It should be less regression prone, since the actual input value wouldn't change.
Unfortunately I'm not very good at layout code, I successfully introduced the selection type globally, but my changes to nsTextFrame.cpp didn't seem to work. Thus I don't know if it would properly align. Regardless, I'd need help implementing the omit selection type in the text frame, if we want to try that approach.

What do you think?
Attachment #8917347 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Flags: needinfo?(jfkthame)
Flags: needinfo?(dao+bmo)
(Assignee)

Comment 38

a year ago
Since Gijs is away, I cc-ed Dao, who previously worked on formatting the urlbar text.
From the comments above it sounds like we aren't going to create a low-risk patch for 56 and perhaps even 57.
status-firefox56: fix-optional → wontfix
Keywords: stale-bug
(In reply to Marco Bonardo [::mak] from comment #37)
> Created attachment 8919079 [details] [diff] [review]
> patch v2
> 
> This is a possible patch. It still lacks tests, but I'd first like to
> collect some feedback.
> * multiple spaces are encoded as suggested by jfkthame, single spaces are
> not, so we retain ux advantage.
> * the address bar gradient is on the left or on the right, depending on the
> overflow direction.
> * there is an overlay label that always shows the scheme when it is
> overflowed and the url has a host.
> * I didn't use ellipsis because it's impossible without proper text-overflow
> support. If we'd have a text-overflow where one could define at which char
> to put the ellipsis, it would be trivial.
> 
> I tested this with an ltr long url, and this rtl-host long url, in ltr, rtl
> and dark theme modes. In all the cases both the scheme and the host are
> visible and the gradient is on the right side.

I tried this patch in a local build (based on current m-c tip), and while it seems like a possible way forward, I observed a couple of glitches that will need further work. As a testcase, I took the URL generated by the PoC here (comment 1).

(a) When I paste this URL into the address bar of a new tab and press Enter, it is displayed with the https:// scheme visible (as expected), but then the URL is left-aligned, meaning that the fake "no.io" domain (actually part of the fragment id) appears after the scheme, followed by the sequence of spaces (all but one %20-encoded). The same occurs if I navigate to the URL by choosing it from the History menu. Also, note that the fade-out gradient is missing at this point.

(b) If I switch to a different tab, and then return to my test tab with the long URL, it now displays right-aligned, so that the real domain (اسماء.شبكة) is visible. However, the sequence of %20-encoded spaces in the fragment identifier are overprinting the still-visible scheme.

I'll attach screenshots of (a) and (b) as seen on my system.
(In reply to Panos Astithas [:past] (please ni?) from comment #39)
> From the comments above it sounds like we aren't going to create a low-risk
> patch for 56 and perhaps even 57.

Personally, I'd still favor doing a minimal patch that %-encodes multiple spaces (i.e. just the regex change in function losslessDecodeURI) and trying to uplift that to 57, as I think it's a useful mitigation with minimal risk. Then all further improvements would go into a future release. But I recognize I'm not an owner/peer here, so it's not my call.
(Assignee)

Comment 44

a year ago
(In reply to Jonathan Kew (:jfkthame) from comment #42)
> Created attachment 8919294 [details]
> screenshot (b), after switching to a different tab and then back to the
> testcase tab

uh, this is strange, did you use --purgecaches? In particular it's strange that the label is missing the background since it's part of its css :\ I'll try to reproduce btw.
(In reply to Marco Bonardo [::mak] from comment #44)
> (In reply to Jonathan Kew (:jfkthame) from comment #42)
> > Created attachment 8919294 [details]
> > screenshot (b), after switching to a different tab and then back to the
> > testcase tab
> 
> uh, this is strange, did you use --purgecaches?

No, I wasn't aware of that. Just tried it, though, and I'm still seeing the issues. The exact result (a) seems to vary -- a couple of times, it's appeared with the gradient on the scheme at the left, though still showing the wrong part of the URL; case (b) with the overpainted scheme after switching tabs seems consistently reproducible.
(Assignee)

Comment 46

a year ago
Ah, problem b) is related to the fact I was testing using Dark Theme. The css needs some tweaks for the default theme.
Problem a) seems to depend on whether the text was already overflowed before pasting.
(Assignee)

Comment 47

a year ago
Posted patch patch v2.1 (obsolete) — Splinter Review
This should fix the previous problems.
Though, I had some problems reproducing the issue a), so please check again and if you can provide me better STR I'd appreciate that.
Attachment #8905279 - Attachment is obsolete: true
Attachment #8919079 - Attachment is obsolete: true
Attachment #8919294 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Flags: needinfo?(jfkthame)
This definitely looks better; I can no longer reproduce (b) at all, AFAICT.

I can still see some behavioral glitches, though, including a form of (a) in certain circumstances. I'll try to give STR that reliably misbehave for me:

(a) Visit the test URL from the PoC in comment 1, so that it gets into History.
    Open a new browser window (NOT a new tab in the same window).
    Choose the test site's from the History menu.
    >> The URL is displayed left-aligned, so the wrong part of it (the fake "no.io") is visible.
    Note that this only seems to happen for the first tab in a window.

(b) Visit the test URL from the PoC.
    Copy the entire long URL to the clipboard.
    Paste it back into the URL bar (overwriting the copy that's already there).
    Press Enter to confirm ("navigating" to the page we're already on).
    >> The URL is displayed left-aligned, with a fade applied to the left-hand end.
    >> Also, note that the https:// scheme label is missing.

(c) Visit the test URL from the PoC.
    Switch to a different tab, then return to the PoC tab.
    (In my experience, this fixes glitches such as (a) and (b) above, so the URL is now displayed as intended.)
    >> The scheme label (https://) is slightly misaligned, about a pixel or two higher than the text of the URL.

(d) Open a new tab.
    In the URL bar, start typing a really long URL. (Its content is irrelevant, just random text will do.)
    Observe what happens when the text reaches the end of the visible URL bar width.
    >> At the point where we'd expect it to start scrolling horizontally, instead the insertion point jumps back the beginning.
Flags: needinfo?(jfkthame)
(Assignee)

Comment 49

a year ago
Posted patch patch v2.2 (obsolete) — Splinter Review
Thank you, that was useful, all the reported issues should be fixed.
I'm not 100% sure about the text misalignment, since so far I only tested on Win, but I changed the alignment approach for a simple box centering.

I'll start looking into what I can add a test for.
Attachment #8919810 - Attachment is obsolete: true
(In reply to Jonathan Kew (:jfkthame) from comment #43)
> (In reply to Panos Astithas [:past] (please ni?) from comment #39)
> > From the comments above it sounds like we aren't going to create a low-risk
> > patch for 56 and perhaps even 57.
> 
> Personally, I'd still favor doing a minimal patch that %-encodes multiple
> spaces (i.e. just the regex change in function losslessDecodeURI) and trying
> to uplift that to 57, as I think it's a useful mitigation with minimal risk.
> Then all further improvements would go into a future release. But I
> recognize I'm not an owner/peer here, so it's not my call.

I agree. This patch is a bit hard to evaluate since it tries to do different mostly unrelated things, there are many edge cases to consider, and the code isn't the cleanest, as Marco admitted. Can we just encode spaces here and move the other ideas to followups?
Flags: needinfo?(dao+bmo)
(Assignee)

Comment 51

a year ago
The newest patch is much cleaner, surely there are a few edge cases, but in practice it relies on the formatting code now, that is well tested. Have a look at it, I think it's a pretty good MVP.

We can surely land the fixed at different times, but there are 2 reasons I didn't:
1. uplifiting to 57 is very unlikely, per rel-eng decisions, so the benefit of splitting for 58 is minor
2. even with the whitespaces fix, the bug is not really fixed, since the host is still not visible. It would make the url less confusable just for more savvy users. And by landing the first part we'd practically expose the problem without having the final fix at hand.
(Assignee)

Comment 52

a year ago
Posted patch patch 2.3 (obsolete) — Splinter Review
Added a test. Tomorrow I'll do some manual testing on Mac.
Attachment #8920064 - Attachment is obsolete: true
I tested briefly again with the new patch; this is definitely a lot more stable. At this point, I've only noticed a couple of relatively minor glitches (again, I'm testing on a Mac, in case this has any significance):

1. Open a new window.
2. Load the test URL from the PoC above (the "fake no.io").
   > It displays correctly, with the Arabic-script domain visible at the right, the scheme visible at the left, and the long URL truncated (with fade-out) before it hits the scheme
3. Now, assume we want to edit the URL. So click in it, let's say at the dot in the domain name.
   > The focus goes to the URL bar, and the entire URL is selected
   > HOWEVER, sometimes (maybe one in three or four times, the URL "jumps" horizontally so that the domain is no longer visible, and I'm looking at a run of encoded spaces (%20%20%20%20...)
4a. Click in the (now-selected) URL a second time, to place an insertion point for editing.
    > If the URL didn't "jump-scroll" in the previous step, this behaves fine. If it did, the insertion point seems to be misplaced by a few characters from where I clicked.
4b. Instead of clicking in the selected URL to begin editing, I change my mind and click back in the content of the page.
    > The URL bar loses focus, but in the process its content momentarily jumps to the wrong end of its scroll range, before reverting to the expected right-aligned scroll position. This produces a distracting visual flicker as the text skips back and forth.
(Assignee)

Comment 54

a year ago
(In reply to Jonathan Kew (:jfkthame) from comment #53)
> 3. Now, assume we want to edit the URL. So click in it, let's say at the dot
> in the domain name.

I fear the jumping issue is due to the underlying editor and I'd not have much control over that from the urlbar code.
If it just happens for overflowed urls with RTL domains, it sounds like an edge case, so we may ignore it for now. I'll have a look at it.

>     > The URL bar loses focus, but in the process its content momentarily
> jumps to the wrong end of its scroll range, before reverting to the expected
> right-aligned scroll position. This produces a distracting visual flicker as
> the text skips back and forth.

This may be unavoidable unless we have a way to tell if the first char after the scheme is RTL. Do you know if we have something like that exposed to js?
(Assignee)

Comment 55

a year ago
(In reply to Jonathan Kew (:jfkthame) from comment #53)
> 3. Now, assume we want to edit the URL. So click in it, let's say at the dot
> in the domain name.
>    > The focus goes to the URL bar, and the entire URL is selected
>    > HOWEVER, sometimes (maybe one in three or four times, the URL "jumps"
> horizontally so that the domain is no longer visible, and I'm looking at a
> run of encoded spaces (%20%20%20%20...)

After some testing, I think this is a bug somewhere in the editor or the selection code, that is uncovered by the rtl selection. Indeed the only thing we do is a call to .selectAll(). I don't have much control over that, but it seems rare enough to not be a blocker.
(Assignee)

Comment 56

a year ago
Posted patch patch 2.4 (obsolete) — Splinter Review
Attachment #8920250 - Attachment is obsolete: true
(In reply to Marco Bonardo [::mak] from comment #55)
> (In reply to Jonathan Kew (:jfkthame) from comment #53)
> > 3. Now, assume we want to edit the URL. So click in it, let's say at the dot
> > in the domain name.
> >    > The focus goes to the URL bar, and the entire URL is selected
> >    > HOWEVER, sometimes (maybe one in three or four times, the URL "jumps"
> > horizontally so that the domain is no longer visible, and I'm looking at a
> > run of encoded spaces (%20%20%20%20...)
> 
> After some testing, I think this is a bug somewhere in the editor or the
> selection code, that is uncovered by the rtl selection. Indeed the only
> thing we do is a call to .selectAll(). I don't have much control over that,
> but it seems rare enough to not be a blocker.

It's not limited to cases that involve RTL; I've also seen it with purely LTR long URLs. But on checking, I find that I can also reproduce it (intermittently) in a current nightly (i.e. without the patch here), in the existing URL bar. So as you say, it's not an issue with this patch; it seems to be a bug elsewhere in editor/selection handling.
(Assignee)

Comment 58

a year ago
Posted patch patch v2.5 (obsolete) — Splinter Review
Ok, I did some more testing on Win and Mac and modulo the bug with SelectAll() unrelated to this specific change, it is working. Thus moving to review.
Attachment #8921148 - Attachment is obsolete: true
Attachment #8921447 - Flags: review?(dao+bmo)
Too late for 57, let's try to get this in 58.
status-firefox57: fix-optional → wontfix
status-firefox58: ? → affected

Comment 60

a year ago
Comment on attachment 8921447 [details] [diff] [review]
patch v2.5

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

::: browser/base/content/urlbarBindings.xml
@@ +482,5 @@
> +          // its beginning.
> +          BrowserUtils.promiseLayoutFlushed(document, "layout", () => {
> +            requestAnimationFrame(() => {
> +              if (!this.focused) {
> +                this.selectionStart = this.selectionEnd = schemeWSlashes.length;

Drive-by (sorry) - this should have a check against re-entrancy (ditto for the further-nested updates a bit further down).
(In reply to Marco Bonardo [::mak] from comment #51)
> The newest patch is much cleaner, surely there are a few edge cases, but in
> practice it relies on the formatting code now, that is well tested. Have a
> look at it, I think it's a pretty good MVP.

I'm afraid the whole .urlbar-scheme-box[scheme][textoverflow="start"] thing is an edge case. :/
It will be easy to regress and we won't necessarily catch that in nightly testing.

> We can surely land the fixed at different times, but there are 2 reasons I
> didn't:
> 1. uplifiting to 57 is very unlikely, per rel-eng decisions, so the benefit
> of splitting for 58 is minor
> 2. even with the whitespaces fix, the bug is not really fixed, since the
> host is still not visible. It would make the url less confusable just for
> more savvy users. And by landing the first part we'd practically expose the
> problem without having the final fix at hand.

With the whitespce fix, this bug doesn't seem severe enough to me to be concerned about that. But we can land without a commit message and keep this bug marked security-sensitive for as long as we want.
Comment on attachment 8921447 [details] [diff] [review]
patch v2.5

I'm getting arbitrary results when repeatedly switching from/to the tab with the testcase loaded. Sometimes the scheme is displayed, sometimes it's missing. Also its horizontal alignment seems off by a pixel. Tested on Linux.

>+          this.schemeBox.firstChild.style.opacity = 0.5;

Hmm, this isn't how SELECTION_URLSECONDARY works. It explicitly doesn't use the alpha channel since this might get us grayscale anti-aliasing:
http://searchfox.org/mozilla-central/rev/ed212c79cfe86357e9a5740082b9364e7f6e526f/layout/generic/nsTextFrame.cpp#3972
Attachment #8921447 - Flags: review?(dao+bmo)
(Assignee)

Comment 63

a year ago
(In reply to Dão Gottwald [::dao] from comment #61)
> I'm afraid the whole .urlbar-scheme-box[scheme][textoverflow="start"] thing
> is an edge case. :/
> It will be easy to regress and we won't necessarily catch that in nightly
> testing.

It is sort-of an edge case, but this bug is about an edge case.
I agree it's something we may miss in Nightly, that's why I added a test, so we don't regress.

> With the whitespce fix, this bug doesn't seem severe enough to me to be
> concerned about that. But we can land without a commit message and keep this
> bug marked security-sensitive for as long as we want.

If dveditz is fine with that, no problem on my side.
I still think not showing the domain doesn't really fix anything for the non-technical users, it will just be a weak indicator: "something looks strange, but I see a host here".
But for the next version I will split the patch into the 2 parts.

(In reply to Dão Gottwald [::dao] from comment #62)
> I'm getting arbitrary results when repeatedly switching from/to the tab with
> the testcase loaded. Sometimes the scheme is displayed, sometimes it's
> missing. Also its horizontal alignment seems off by a pixel. Tested on Linux.

I want to address the re-entrancy Gijs pointed out and re-test for this tab switching case, I didn't see it on Win/Mac.
I suppose the 1px offset is not a critical problem, considered as you said this is an edge case, and security comes first.

> >+          this.schemeBox.firstChild.style.opacity = 0.5;
> 
> Hmm, this isn't how SELECTION_URLSECONDARY works. It explicitly doesn't use
> the alpha channel since this might get us grayscale anti-aliasing:
> http://searchfox.org/mozilla-central/rev/
> ed212c79cfe86357e9a5740082b9364e7f6e526f/layout/generic/nsTextFrame.cpp#3972

I know, but I have no way to reproduce what the textframe is doing.
Is it critical to have subpixel AA in a small label shown in edge cases? Any alternative suggestion on how to achieve that?
(In reply to Marco Bonardo [::mak] from comment #63)
> (In reply to Dão Gottwald [::dao] from comment #62)
> > I'm getting arbitrary results when repeatedly switching from/to the tab with
> > the testcase loaded. Sometimes the scheme is displayed, sometimes it's
> > missing. Also its horizontal alignment seems off by a pixel. Tested on Linux.
> 
> I want to address the re-entrancy Gijs pointed out and re-test for this tab
> switching case, I didn't see it on Win/Mac.
> I suppose the 1px offset is not a critical problem, considered as you said
> this is an edge case, and security comes first.

If we want to commit to doing this, I'd rather do it right. These kind of glitches that aren't covered by the test and will only add up over time are exactly what concerns me.

> > >+          this.schemeBox.firstChild.style.opacity = 0.5;
> > 
> > Hmm, this isn't how SELECTION_URLSECONDARY works. It explicitly doesn't use
> > the alpha channel since this might get us grayscale anti-aliasing:
> > http://searchfox.org/mozilla-central/rev/
> > ed212c79cfe86357e9a5740082b9364e7f6e526f/layout/generic/nsTextFrame.cpp#3972
> 
> I know, but I have no way to reproduce what the textframe is doing.
> Is it critical to have subpixel AA in a small label shown in edge cases? Any
> alternative suggestion on how to achieve that?

Perhaps use a textframe with SELECTION_URLSECONDARY?
(Assignee)

Comment 65

a year ago
This is the part we agree upon: don't decode consecutive spaces. It preserves UX for urls having single spaces to separate words, but makes long runs of spaces visible and likely warnings the user something may be wrong.

I'm still looking at the second part (always show the domain) to see if I can reproduce the reported intermittent issues.

Daniel, would you be fine with landing the first part for now, so that consecutive spaces are shown encoded, and keep the second part for later when we are more comfortable with it?
Do you think there's risk exposing a partial fix to the problem?
Attachment #8926403 - Flags: review?(dao+bmo)
Attachment #8926403 - Flags: feedback?(dveditz)
Comment on attachment 8926403 [details] [diff] [review]
part 1 - Don't decode adjacent spaces

>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js
>@@ -2761,18 +2761,20 @@ function losslessDecodeURI(aURI) {
>       try {
>         value = decodeURI(value)
>                   // 1. decodeURI decodes %25 to %, which creates unintended
>                   //    encoding sequences. Re-encode it, unless it's part of
>                   //    a sequence that survived decodeURI, i.e. one for:
>                   //    ';', '/', '?', ':', '@', '&', '=', '+', '$', ',', '#'
>                   //    (RFC 3987 section 3.2)
>                   // 2. Re-encode whitespace so that it doesn't get eaten away
>-                  //    by the location bar (bug 410726).
>-                  .replace(/%(?!3B|2F|3F|3A|40|26|3D|2B|24|2C|23)|[\r\n\t]/ig,
>+                  //    by the location bar. We explicitly want to re-encode
>+                  //    multiple whitespaces, because they make spoofing
>+                  //    attempts easier.

I'd leave the reference to that old bug and tweak the wording a bit:

"Re-encode select whitespace so that it doesn't get eaten away by the location bar (bug 410726). Re-encode all adjacent whitespace to prevent spoofing attempts where invisible characters would push part of the URL to overflow the location bar."

Maybe also add a reference to this bug. Blame is a bit annoying here because these lines tend to get touched again and again.
Attachment #8926403 - Flags: review?(dao+bmo) → review+
(Assignee)

Comment 67

a year ago
(In reply to Dão Gottwald [::dao] from comment #66)
> Maybe also add a reference to this bug. Blame is a bit annoying here because
> these lines tend to get touched again and again.

I've updated the comment locally, though now it's basically exposing what this bug is about and how to reproduce it.
Imo that forces us to move the remaining work to a separate bug. Let's see what Daniel thinks.
(In reply to Marco Bonardo [::mak] from comment #67)
> (In reply to Dão Gottwald [::dao] from comment #66)
> > Maybe also add a reference to this bug. Blame is a bit annoying here because
> > these lines tend to get touched again and again.
> 
> I've updated the comment locally, though now it's basically exposing what
> this bug is about and how to reproduce it.

Not quite, crucial details like the RTL stuff are missing.
Summary: Firefox address bar spoof using LTR language and references → Firefox address bar spoof using RTL language and references
(Assignee)

Comment 69

a year ago
This fixes the opacity AA, the alignment and the re-entrancy. I didn't have a chance to test the Linux intermittent problem. Testing is welcome.
Attachment #8921447 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8926654 - Attachment description: Part 2 - Alwaus show domain and scheme (ftp, https) → Part 2 - Always show domain and scheme (ftp, https)
(Assignee)

Comment 70

a year ago
Attachment #8926654 - Attachment is obsolete: true
(Assignee)

Comment 71

a year ago
Dao, could you please give a quick test to the latest version? It fixes alignment, colors and LWTs.
Flags: needinfo?(dao+bmo)
Comment on attachment 8926403 [details] [diff] [review]
part 1 - Don't decode adjacent spaces

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

::: browser/base/content/browser.js
@@ +2768,5 @@
>                    // 2. Re-encode whitespace so that it doesn't get eaten away
> +                  //    by the location bar. We explicitly want to re-encode
> +                  //    multiple whitespaces, because they make spoofing
> +                  //    attempts easier.
> +                  .replace(/%(?!3B|2F|3F|3A|40|26|3D|2B|24|2C|23)|[\r\n\t]|\s(?=\s)|\s$/ig,

I agree w/Dão that we shouldn't remove the old bug reference and should add a reference to this bug.

Should be fine to land just this part -- spaces are already known to be spoofy and nothing here references the RTL aspect.

::: browser/base/content/test/urlbar/browser_urlbarCopying.js
@@ +166,5 @@
>    },
>    {
> +    loadURL: "http://example.com/a%20test",
> +    expectedURL: "example.com/a test",
> +    copyExpected: "http://example.com/a%20test"

Can we add a test with two embedded spaces (possibly different ones, like %20%C2%A0) to show the expected "%20 " behavior and not " %C2%A0"?
Attachment #8926403 - Flags: feedback?(dveditz) → feedback+
(In reply to Marco Bonardo [::mak] from comment #65)
> Daniel, would you be fine with landing the first part for now

Yep.

(In reply to Marco Bonardo [::mak] from comment #67)
> I've updated the comment locally, though now it's basically exposing what
> this bug is about and how to reproduce it.

What did you update the comment to? If you take the comment I saw and just stick "(bug 1395508)" on the end it's not revealing the key part of this. Even if it does I'd prefer keeping the work in this bug. Something like

>+                  //    by the location bar (bug 410726)). We explicitly
>+                  //    want to re-encode multiple whitespaces, because
>+                  //    they make spoofing attempts easier (bug 1395508).

should be fine, and then keep the remaining work in this bug.
(Assignee)

Comment 74

a year ago
(In reply to Daniel Veditz [:dveditz] from comment #73)
> (In reply to Marco Bonardo [::mak] from comment #67)
> > I've updated the comment locally, though now it's basically exposing what
> > this bug is about and how to reproduce it.
> 
> What did you update the comment to?

The one suggested in comment 66.
(Assignee)

Comment 75

a year ago
I should have set a needinfo. Please check the suggested text in comment 66 and let me know if it's ok to ship it or if you would like modifications.
Flags: needinfo?(dveditz)
The proposed text in comment 66 looks fine.
Flags: needinfo?(dveditz)
(Assignee)

Comment 77

a year ago
To simplify tracking of this bug (likely we'd uplift part 1) I'm going to split part 2 to a separate cloned bug.
Flags: needinfo?(dao+bmo)
(Assignee)

Updated

a year ago
Blocks: 1419391
(Assignee)

Comment 78

a year ago
(In reply to Daniel Veditz [:dveditz] from comment #72)
> Can we add a test with two embedded spaces (possibly different ones, like
> %20%C2%A0) to show the expected "%20 " behavior and not " %C2%A0"?

We will actually show "%20%C2%A0" and not " %C2%A0" because they are considered consecutive spaces and thus re-encoded by this patch. I still added the test, I hope it's ok, it should be from the point of view of this report.
(Assignee)

Updated

a year ago
Attachment #8927028 - Attachment is obsolete: true
(Assignee)

Comment 79

a year ago
Attachment #8926403 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/a14cc3bd9643
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox58: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
(Assignee)

Comment 82

a year ago
Not sure why this has been marked as fixed on 58, likely a mistake.
status-firefox58: fixed → affected
(Assignee)

Comment 83

a year ago
Comment on attachment 8930465 [details] [diff] [review]
re-encode consecutive spaces

Approval Request Comment
[Feature/Bug causing the regression]: no regression
[User impact if declined]: Spoofing attempts using adjacent whitespaces in the Address Bar are easier
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: it's a regex change
[String changes made/needed]: none
Attachment #8930465 - Flags: approval-mozilla-beta?
(Assignee)

Comment 84

a year ago
Comment on attachment 8930465 [details] [diff] [review]
re-encode consecutive spaces

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: spoofing Address bar fix
User impact if declined: spoofing through spaces in the Address Bar is easier
Fix Landed on Version: 59
Risk to taking this patch (and alternatives if risky): should not be risky, simple regex change
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8930465 - Flags: approval-mozilla-esr52?
Hi dveditz,
Since this is sec-moderate, do you think we need to take this in Beta58 & ESR52?
Flags: needinfo?(dveditz)
status-firefox59: --- → fixed
Group: firefox-core-security → core-security-release
(In reply to Gerry Chang [:gchang] from comment #85)
> Since this is sec-moderate, do you think we need to take this in Beta58 & ESR52?

We should, because these spoofs are easy to construct once people think about it, and the patch is small and straightforward.
Flags: needinfo?(dveditz)
Comment on attachment 8930465 [details] [diff] [review]
re-encode consecutive spaces

Fix a sec-moderate. Beta58+ & ESR52+.
Attachment #8930465 - Flags: approval-mozilla-esr52?
Attachment #8930465 - Flags: approval-mozilla-esr52+
Attachment #8930465 - Flags: approval-mozilla-beta?
Attachment #8930465 - Flags: approval-mozilla-beta+
tracking-firefox-esr52: ? → 58+
Looks like ESR52 needs rebasing around bug 1402715.
Flags: needinfo?(mak77)
(Assignee)

Comment 90

a year ago
Posted patch esr52 patchSplinter Review
This seems to pass tests on Try, the test is the same as in Nightly. It makes sense since bug 1402715 fixes a regression that happened in v53.
Ryan, could you please take care of landing it?
Flags: needinfo?(mak77) → needinfo?(ryanvm)
https://hg.mozilla.org/releases/mozilla-esr52/rev/263165eacc54
status-firefox-esr52: affected → fixed
Flags: needinfo?(ryanvm)
Whiteboard: [fxsearch] → [fxsearch][adv-main58+][adv-esr52.6+]
Alias: CVE-2018-5117
Whiteboard: [fxsearch][adv-main58+][adv-esr52.6+] → [fxsearch][adv-main58+][adv-esr52.6+][post-critsmash-triage]
I tried to reproduce and verify the fix of this issue. Based on Comment 31, I suppose that the fixed builds should displayed the real domain (سماء.شبكة). Compared with Chrome, where the real domain is always in focus, on Firefox the domain is displayed only at the end of the URL (can be seen only by using the horizontal scroll). Not sure what the fix should be here! Am I missing something?

Configuration used: 
- OS: Windows 10 x64, macOS 10.13.2
- Affected builds: 57.0a1 (2017-08-31), Release 57.0.4
- Fixed builds: Latest Nightly (2018-01-22), Beta 58.0b16
Flags: needinfo?(mak77)
(Assignee)

Comment 93

a year ago
(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #92)
> I tried to reproduce and verify the fix of this issue. Based on Comment 31,
> I suppose that the fixed builds should displayed the real domain
> (سماء.شبكة). Compared with Chrome, where the real domain is always in focus,
> on Firefox the domain is displayed only at the end of the URL (can be seen
> only by using the horizontal scroll). Not sure what the fix should be here!
> Am I missing something?

Yes, the "always show domain" fix didn't land here but has been moved to bug 1419391, because the patch is fragile due to missing editor APIs and there's some discussion about a completely new approach for the Address Bar that would be less likely to suffer from spoofing attempts. That didn't happen yet.

What landed here is the patch that decodes multiple whitespaces when they are adjacent, that by itself makes the user less likely to fall for the spoofing attempt since there's no long empty space after the fake domain making it look good.
Flags: needinfo?(mak77)
(In reply to Marco Bonardo [::mak] from comment #93)
> What landed here is the patch that decodes multiple whitespaces when they
> are adjacent, that by itself makes the user less likely to fall for the
> spoofing attempt since there's no long empty space after the fake domain
> making it look good.

I retested this issue, by taking into account the information mentioned above, on latest Nightly 60.0a1 (2018-01-22) and on Release 58.06 - build 6 under Windows 10 x64, Ubuntu 12.04 x32 and macOS 10.13. I can mark this bug as verified fixed.
Status: RESOLVED → VERIFIED
status-firefox58: fixed → verified
status-firefox59: fixed → verified
Flags: sec-bounty?
Flags: sec-bounty? → sec-bounty+
Group: core-security-release
(Assignee)

Updated

5 months ago
Duplicate of this bug: 1403812
You need to log in before you can comment on or make changes to this bug.