Closed Bug 1528939 (CVE-2019-11699) Opened 3 years ago Closed 3 years ago

Awesomebar UI mismatch could lead to successful phishing of Firefox's users

Categories

(Firefox :: Address Bar, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 67
Tracking Status
firefox-esr60 --- wontfix
firefox65 --- wontfix
firefox66 --- wontfix
firefox67 --- verified
firefox68 --- verified

People

(Reporter: tzachyr, Assigned: mak)

References

()

Details

(Keywords: csectype-spoof, sec-low, Whiteboard: [reporter-external] [client-bounty-form] [verif?] [post-critsmash-triage][adv-main67+])

Attachments

(4 files, 1 obsolete file)

Attached file Awesomebar_UI_Mismatch.zip (obsolete) —

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:\\someExfiltirateDdataCouldBeUsedHereToMaskThings@www.cnn.com//rest/path/to/exfiltrate/data/to/the/real/domain/that/we/are/going/to#someMoreData

Flags: sec-bounty?

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.

Component: Security → Address Bar

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 \.

Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(gijskruitbosch+bugs)

So what happens is that the value:

http://foopy:\\blah@somewhere.com//whatever

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

"http://foopy/blah@somewhere.com//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

  1. 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?

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(mak77)

(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:password@example.org/ https://user:password@example.org/

Which makes very little sense to me.)

Flags: needinfo?(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>.)

Flags: needinfo?(annevk)

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));

(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...

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.

Assignee: nobody → mak77
Status: NEW → ASSIGNED
Flags: needinfo?(mak77)

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.

Attached file Bug 1528939. r=gijs!
Group: firefox-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67

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

(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.

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

Flags: sec-bounty? → sec-bounty-

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

Flags: qe-verify+
Whiteboard: [reporter-external] [client-bounty-form] [verif?] → [reporter-external] [client-bounty-form] [verif?] [post-critsmash-triage]

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?

Flags: needinfo?(gijskruitbosch+bugs)

(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?

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(mak77)

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).

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.

Status: RESOLVED → REOPENED
Flags: needinfo?(mak77)
Resolution: FIXED → ---
Attached file Bug 1528939. r=gijs
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED

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

Flags: needinfo?(mak77)

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
Flags: needinfo?(mak77)
Attachment #9056581 - Flags: approval-mozilla-beta?
Flags: qe-verify+ → qe-verify?

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 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.

Attachment #9056581 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify? → qe-verify+
QA Whiteboard: [qa-triaged]

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.

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

Status: RESOLVED → VERIFIED
Flags: qe-verify+
Whiteboard: [reporter-external] [client-bounty-form] [verif?] [post-critsmash-triage] → [reporter-external] [client-bounty-form] [verif?] [post-critsmash-triage][adv-main67+]
Alias: CVE-2019-11699
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.