1528939 - (CVE-2019-11699) Awesomebar UI mismatch could lead to successful phishing of Firefox's users

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect)

Description

[Tzachy Horesh]

Attachment: Awesomebar_UI_Mismatch.zip

I was playing a bit with crafting URIs as I know the parser's a mess and needs to handle many cases of each of the parts of the URIs and found the following (It wasn't replicated on Chrome, Explorer, Edge and Opera):

The Firefox awesomebar doesn't parse well specially crafted URIs and displays for a short time the wrong part of the URI as the domain that the browser is being directed to.
This could lead to phishing scenarios where an attacker will craft a shortened URI for example and put it in the 2nd part of the URI (the domain oculus in this case) while making the victim believe they are being directed to www.cnn.com
As the attacker can control the site, they can control how fast the site will load and the UI will change the marked domain to the read directed domain (if oculus is loading fast enough, you can change the domain to some slow site to see it more clearly)
Other parts of the URI can be leveraged to make it look like the real domain had been accessed, exfiltrate data, and so on.

PoC:
http://oculus:\\[e-mail]//rest/path/to/exfiltrate/data/to/the/real/domain/that/we/are/going/to#someMoreData

Comment 1

[Tzachy Horesh]

In the POC provided here I just copied and pasted the URI I was using and in this case, the page code has encoded the "\" to %5c%5c, making the real domain in this case www.cnn.com
In Order to see the issue, please copy and paste the URI to the awesomebar.

Comment 2

[Al Billings [:abillings - ex-MoCo]]

Attachment: Awesomebar-UI MismatchMozilla Firefox.png

Comment 3

[Al Billings [:abillings - ex-MoCo]]

Attachment: Awesomebar-UI MismatchMozilla Firefox2.png

Comment 4

[Daniel Veditz [:dveditz]]

What are we doing special for "\" vs. "%5c%5c". Both look offhand like it's a userinfo section up to the @www.cnn.com but that's not how we parse the \.

Comment 5

[:Gijs (out for now; he/him)]

So what happens is that the value:

http://foopy:\\[e-mail]//whatever

gets "fixed up" by nsIURIFixup / URL parsing to:

"http://foopy/[e-mail]//whatever"

. This makes the actual host "foopy", not "somewhere.com", and actually fires an http request to "foopy" ("oculus" in comment #0). When that fails (because at least in my network, there is no such host; note that this will take longer on most Windows systems), because when pasted in the URL bar this is a user-initiated request, we attempt to fix up the request by going to "www.foopy.com" ("www.oculus.com") instead.

Unfortunately, the URL highlighting code still uses regular expressions. Part of the reason for this is that we're trying to do highlighting on the original input, instead of forcing the location bar to display the parsed URL (which would clobber sloppy escaping on the user's part, especially in the path/query string). We can't just use the original URL object for this because the URL parser will strip out gunk that might still be in the location bar (as evidenced here, where ":\\" has magically become "/"), so any highlighting offsets could be wrong.

This doesn't work well here, because the original input has backslashes in the supposed-username part, which AIUI will cause the "real" URL parser to trash the username part:

https://url.spec.whatwg.org/#authority-state

2. Otherwise, if one of the following is true

• c is the EOF code point, U+002F (/), U+003F (?), or U+0023 (#)

• url is special and c is U+005C ()

then:

1. If @ flag is set and buffer is the empty string, validation error, return failure.

2. Decrease pointer by the number of code points in buffer plus one, set buffer to the empty string, and set state to host state.

But the regular expression we use looks like:

let matchedURL = url.match(/^(([a-z]+:\/\/)(?:[^\/#?]+@)?)(\S+?)(?::\d+)?\s*(?:[\/#?]|$)/);

note in particular that the matching group ending in "@)?" only ignores forward slash, question mark and #, and not backslash.

Hence the confusion - the regex thinks this is a valid username portion, and it really isn't.

Now, the obvious/lazy fix is to just add the backslash to both the [^\/#?] things and move on.

I don't like living with just that fix.

I'd really like us to move away from this regex, or at least enforce that the regex turns up the same hostname as URI fixup, or enforcing that the URI's prePath has to exactly match the initial N bytes in the URL bar (which already strips leading spaces, I think, so just whitespace won't break user input?), otherwise we just clobber the user input for the parsed URL.

Marco, how does that sound to you? Do any of the people currently working on URL bar code have cycles/expertise to take this on?

Comment 6

[:Gijs (out for now; he/him)]

(In Cato style, I also think the URL standard isn't very helpful about username/password things. This is what it says about username/passwords:

Input	Base	Valid	Output
https://user:[e-mail]/		❌	https://user:[e-mail]/

Which makes very little sense to me.)

Comment 7

[Anne (:annevk)]

(The reason is that the username/password portion ought not to be used, so it's not valid. But it still works and can be serialized. Similar to </br> not being valid HTML, but also being equivalent to <br>.)

Comment 8

[Marco Bonardo [:mak]]

I agree we should not trust the regex to define what's the host.
What about we replace the regex with something like:

let {host: domain, userPass, prePath, scheme} = uriInfo.fixedURI;
let fixedPreDomain = prePath.substring(0, prePath.lastIndexOf(domain));
let preDomain = url.substring(0, url.indexOf(domain, fixedPreDomain.length));
let schemeWSlashes = fixedPreDomain.substring(0, fixedPreDomain.lastIndexOf(userPass));

Comment 9

[:Gijs (out for now; he/him)]

(In reply to Marco Bonardo [::mak] from comment #8)

I agree we should not trust the regex to define what's the host.
What about we replace the regex with something like:

let {host: domain, userPass, prePath, scheme} = uriInfo.fixedURI;
let fixedPreDomain = prePath.substring(0, prePath.lastIndexOf(domain));
let preDomain = url.substring(0, url.indexOf(domain, fixedPreDomain.length));

This seems like a good idea, yeah.

let schemeWSlashes = fixedPreDomain.substring(0, fixedPreDomain.lastIndexOf(userPass));

This looks like it'll need some tweaking when there is no userPass or the userPass is not included in the fixedPreDomain.

(In reply to :Gijs (he/him) from comment #5)

Do any of the people currently working on URL bar code have cycles/expertise to take this on?

Marco, still curious about this. I don't have a lot of cycles right now, unfortunately...

Comment 10

[Marco Bonardo [:mak]]

yes, it was mostly pseudocode, it needs some fixes, but we have a good test :)

I'll see if I can take care of this.

Comment 11

[Marco Bonardo [:mak]]

The problem is URIFixup may change the host, especially for things like IPv6, for example [1:2:3:4:5:6:255.255.255.255] becomes [1:2:3:4:5:6:ffff:ffff], so we can't just extract parts from the original url using the fixup info. It would just make it harder to find mistakes, but it would not solve all of them.

I think I'll revert to comparing the guessed host with the fixupURI host, if they differ we'll clobber user's input and re-parse.

Comment 12

[Marco Bonardo [:mak]]

Attachment: Bug 1528939. r=gijs!

Comment 13

[Marco Bonardo [:mak]]

Note sure if Lando can post here, anyway...
https://hg.mozilla.org/integration/autoland/rev/7417187bdee40dd267586fc334b52c1e4d2743d0

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/7417187bdee4

Comment 15

[Marco Bonardo [:mak]]

Considered it's a sec-low I'd not proceed with uplifting... Though let me know if you have concerns with that.

Comment 16

[:Gijs (out for now; he/him)]

(In reply to Marco Bonardo [::mak] from comment #15)

Considered it's a sec-low I'd not proceed with uplifting... Though let me know if you have concerns with that.

No, sgtm.

Comment 17

[Daniel Veditz [:dveditz]]

Thank you for reporting this bug. Unfortunately it does not qualify for a security bug bounty.

Comment 18

[Tzachy Horesh]

Hi,

Can you please tell me why this bug is not eligible for a bounty?
As it can be used to phish users.

Regards,

Tzachy

C

Comment 19

[Anca Soncutean, Desktop QA]

I could reproduced this issue with an affected Nightly 67.0a1 (2019-02-19) on Windows 10 x64.
On the latest Beta 67.0b8 (Windows 10 x64, Ubuntu 18.04 x64 and macOS 10.13) the entire URI is highlighted, no confusion is created regarding the right domain. Is there something else that I should test here to conclude this issue as verified fixed?

Comment 20

[:Gijs (out for now; he/him)]

(In reply to Anca Soncutean [:Anca], Desktop Release QA from comment #19)

I could reproduced this issue with an affected Nightly 67.0a1 (2019-02-19) on Windows 10 x64.
On the latest Beta 67.0b8 (Windows 10 x64, Ubuntu 18.04 x64 and macOS 10.13) the entire URI is highlighted, no confusion is created regarding the right domain. Is there something else that I should test here to conclude this issue as verified fixed?

I don't think there's anything else. I'm a bit surprised the entire URI is highlighted, I thought the patch would clobber the input URL and replace it with the "real" one and only highlight the domain in that. Marco?

Comment 21

[Marco Bonardo [:mak]]

I'm looking into that, it looks like the patch works properly if quantum bar is enabled, but the url is not highlighted otherwise (so I guess we give up due to some error).

Comment 22

[Marco Bonardo [:mak]]

There are 2 problems:

1. the recursion prevention doesn't work because UrlbarSetURI may actually re-enter
2. we must null out userTypedValue, otherwise URLBarSetURI doesn't replace the value

I'm reopening and at this point I think I should add a mochitest-browser.

Comment 23

[Marco Bonardo [:mak]]

Attachment: Bug 1528939. r=gijs

Comment 24

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/011065697f6b27b03cb34b7e09d4d1c99833b4d4

Comment 25

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/011065697f6b

Comment 26

[:Gijs (out for now; he/him)]

Should we mark this as target: 68, or do you want to uplift your latest change (perhaps after more verification) ?

Comment 27

[Marco Bonardo [:mak]]

Comment on attachment 9056581 [details]
Bug 1528939. r=gijs

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: None
• User impact if declined: The fix in this bug doesn't work as expected and in some cases it may cause an unexpected infinite recursion, this fixes it
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: QA already tested the previous fix, and that's how we discovered it not working as expected. Steps are the same.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): simple patch with automated test, just fixes the code that was already modified in 67
• String changes made/needed: none

Comment 28

[Marco Bonardo [:mak]]

I'd like the complete fix to be in 67, because the current fix may sometimes cause an infinite recursion that may hang the browser chrome for a few seconds, not a nice experience.

Comment 29

[Pascal Chevrel:pascalc]

Comment on attachment 9056581 [details]
Bug 1528939. r=gijs

Uplift approved for 67 beta 10, let's have it verified by QA on beta once it's landed.

Comment 30

[Anca Soncutean, Desktop QA]

This issue is verified fixed with Nightly 68.0a1 (2019-04-11) on Windows 10 x64, macOS 10.13 and Ubuntu 18.04 x64 (only the real domain is highlighted as expected).
I will modify the 67 flag to affected since the fix has not yet landed there.

Comment 31

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/730af3807039

Comment 32

[Anca Soncutean, Desktop QA]

This issue is also verified fixed with the latest Beta (67.0b11) on Windows 10 x64, macOS 10.13 and Ubuntu 18.04 x64.