Bug 1221444 (CVE-2015-7211)

Partial URL spoofing using the data URI scheme

RESOLVED FIXED in Firefox 43

Status

()

Core
Networking
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Abdulrahman Alqabandi, Assigned: Gijs)

Tracking

({csectype-spoof, sec-low})

41 Branch
mozilla45
csectype-spoof, sec-low
Points:
---

Firefox Tracking Flags

(firefox43+ fixed, firefox44+ fixed, firefox45+ fixed, firefox-esr38- wontfix)

Details

(Whiteboard: [post-critsmash-triage][adv-main43+])

Attachments

(3 attachments)

(Reporter)

Description

2 years ago
Created attachment 8682949 [details]
ff-spf.html

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:41.0) Gecko/20100101 Firefox/41.0
Build ID: 20151014143721

Steps to reproduce:

The parser that handles data uri schemes does not consider the 'hash' symbol which could lead to tricking a victim into thinking they're in a trusted website. Please view the attached PoC for a better idea of how an attack scenario would look like.

new URL('data:#;,test'); //This is a valid data url, though logically it only points to 'data:' which is supposed to be blocked from access

new URL('data:#');//"TypeError: data:# is not a valid URL."

So we can trick the browser into forming the second url using this flaw.

PoC:

<a href="data:#q;,<b>qab</b><script>location.hash=''</script>">click</a>


Actual results:

We end up in 'data:#' that contains the document from the initial data: url.


Expected results:

The parser should not allow the hash symbol before the ',' character.
(Assignee)

Updated

2 years ago
Group: firefox-core-security → core-security
Component: Untriaged → Networking
Product: Firefox → Core
(Assignee)

Comment 1

2 years ago
Created attachment 8683161 [details] [diff] [review]
Patch
Attachment #8683161 - Flags: review?(bzbarsky)
(Assignee)

Comment 2

2 years ago
Comment on attachment 8683161 [details] [diff] [review]
Patch

So I have no idea if this contravenes the spec, but then the spec is incredibly light on details about all this anyway.

I initially tried to replace with " " but that changes the resulting mimetype of the testcase, and this doesn't, which seems more likely to be web-compatible (we are already more permissive than the spec here as per comments).

An alternative fix would involve teaching setref and friends that hashes before the ',' in the URI don't count... but I don't know that that's feasible/correct. Maybe we need to do both?

(Also, Boris, if you can stick this in core-dom-security or whatever it's called, instead of core-security, that'd be helpful, I imagine...)
Attachment #8683161 - Attachment description: , → Patch
Attachment #8683161 - Attachment filename: . → hashreplace.txt
(Assignee)

Comment 3

2 years ago
(In reply to :Gijs Kruitbosch from comment #2)
> An alternative fix would involve teaching setref and friends that hashes
> before the ',' in the URI don't count... but I don't know that that's
> feasible/correct. Maybe we need to do both?

... or we could just throw and not let you create a URI here in the first place, of course...
(Reporter)

Comment 4

2 years ago
(In reply to :Gijs Kruitbosch from comment #3)
> (In reply to :Gijs Kruitbosch from comment #2)
> > An alternative fix would involve teaching setref and friends that hashes
> > before the ',' in the URI don't count... but I don't know that that's
> > feasible/correct. Maybe we need to do both?
> 
> ... or we could just throw and not let you create a URI here in the first
> place, of course...

I agree, the goal should be to have 'data:#q;,q' be treated the same as 'data:' in terms of interpreting.
Now that I think about it, why not disable the hash from being parsed at all when it comes to data protocols? Let '#' be treated as a normal character and location.hash='' with no setter (similar to the Origin except this is for data urls only).
Comment on attachment 8683161 [details] [diff] [review]
Patch

> Now that I think about it, why not disable the hash from being parsed at all when it
> comes to data protocols?

1)  That violates the spec.
2)  That breaks some uses of data: (e.g. for SVG).

As far as the rest goes, what exactly is the security issue?  Yes, in an ideal world "data:#whatever" would be treated just like "data:" in terms of how it behaves and what it loads.  But what's the security bug as things stand?

In any case, seems to me like the patch should in fact implement the "ignore stuff after #" behavior.  Probably by, once you've found a comma, checking for a '#' before the comma and if found simply returning NS_ERROR_MALFORMED_URI, just like we do for "data:" without any commas in it.

And in general the data: parser needs to be tightened up in various ways.  But not in this bug; there are existing bugs on that.
Attachment #8683161 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 6

2 years ago
(In reply to Boris Zbarsky [:bz] from comment #5)
> As far as the rest goes, what exactly is the security issue?  Yes, in an
> ideal world "data:#whatever" would be treated just like "data:" in terms of
> how it behaves and what it loads.  But what's the security bug as things
> stand?

I think the hypothesis is that things that look a lot like http://www.google.com/ but aren't actually http(s)://www.google.com are harmful and could help phish people out of their credentials. See also, as a semi-random example, bug 1189082.

IMO sec-low at best, because the location bar doesn't *actually* say http://www.google.com/, but there we are. Still a good idea to get this fixed.

> In any case, seems to me like the patch should in fact implement the "ignore
> stuff after #" behavior.  Probably by, once you've found a comma, checking
> for a '#' before the comma and if found simply returning
> NS_ERROR_MALFORMED_URI, just like we do for "data:" without any commas in it.

Coming up.
(In reply to Abdulrahman Alqabandi from comment #4)
> Now that I think about it, why not disable the hash from being parsed at all
> when it comes to data protocols? Let '#' be treated as a normal character
> and location.hash='' with no setter (similar to the Origin except this is
> for data urls only).

we shouldn't break location.hash: to the extent people use data: documents anchor navigation can be an important feature.

We should really just reject this malformed data: url and not try to recover--"#;" is not a valid media type. nor should "text/html;base64" be parsed as "base64". And we're asking for trouble assuming invalid media types are "text/html" instead of the "text/plain" that's in the spec.
Group: core-security → network-core-security
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: csectype-spoof, sec-low
> I think the hypothesis is that things that look a lot like http://www.google.com/ but
> aren't actually http(s)://www.google.com are harmful

Sure, but in this case nothing says "http://www.google.com" in a way relevant to the "data:#" bit.  In other words, why does it matter that the testcase uses "data:#" at all?

I mean, it matters because location.href looks like "data:#//secure.google.com/login" but users aren't seeing that string anyway.
(Assignee)

Comment 9

2 years ago
(In reply to Boris Zbarsky [:bz] from comment #8)
> > I think the hypothesis is that things that look a lot like http://www.google.com/ but
> > aren't actually http(s)://www.google.com are harmful
> 
> Sure, but in this case nothing says "http://www.google.com" in a way
> relevant to the "data:#" bit.  In other words, why does it matter that the
> testcase uses "data:#" at all?
> 
> I mean, it matters because location.href looks like
> "data:#//secure.google.com/login" but users aren't seeing that string anyway.

... they are? It's in the location bar (tested with the attached testcase on 42rc).

Are you seeing something else? It's possible e10s somehow behaves differently here...
> It's possible e10s somehow behaves differently here...

Looks like it does.  Seems like a bug we need to file on e10s.
(Assignee)

Comment 11

2 years ago
Created attachment 8683258 [details] [diff] [review]
Patch v2

So... is it OK to remove the second strchr here? I *think* so, but it would be really really good to doublecheck that I'm not missing something stupid here. I'm assuming nobody actually modifies the length of buffer, except for the fudging of comma to nil and back (which shouldn't change anything for the purpose of that strchr). Anywho. This throws as requested, I believe.
Attachment #8683258 - Flags: review?(bzbarsky)
(Assignee)

Updated

2 years ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
(Assignee)

Updated

2 years ago
See Also: → bug 1221713
Comment on attachment 8683258 [details] [diff] [review]
Patch v2

r=me.  No one modifies the length of the buffer.
Attachment #8683258 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 13

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7957aa36fbd
https://hg.mozilla.org/mozilla-central/rev/b7957aa36fbd
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
(Assignee)

Comment 15

2 years ago
Comment on attachment 8683258 [details] [diff] [review]
Patch v2

This seems like a safe fix that we could uplift.

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: spoofing attacks
[Describe test coverage new/current, TreeHerder]: nope
[Risks and why]: low. It's theoretically possible this would break web compat, but that seems so unlikely (apart from fixing this security issue, which arguably breaks "compat" in an intentional fashion) that I think we should just take it.
[String/UUID change made/needed]: no
Attachment #8683258 - Flags: approval-mozilla-beta?
Attachment #8683258 - Flags: approval-mozilla-aurora?
Group: network-core-security → core-security-release
status-firefox43: --- → affected
status-firefox44: --- → affected
Comment on attachment 8683258 [details] [diff] [review]
Patch v2

OK, let's take it. Should be in 43 beta 2.
Attachment #8683258 - Flags: approval-mozilla-beta?
Attachment #8683258 - Flags: approval-mozilla-beta+
Attachment #8683258 - Flags: approval-mozilla-aurora?
Attachment #8683258 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/e96670656f90
status-firefox44: affected → fixed
https://hg.mozilla.org/releases/mozilla-beta/rev/1692a311615d
status-firefox43: affected → fixed
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main43+]
Alias: CVE-2015-7211
status-firefox-esr38: --- → affected
tracking-firefox43: --- → +
tracking-firefox44: --- → +
tracking-firefox45: --- → +
tracking-firefox-esr38: --- → -
status-firefox-esr38: affected → wontfix
Group: core-security-release
See Also: → bug 1255570
You need to log in before you can comment on or make changes to this bug.