1221444 - (CVE-2015-7211) Partial URL spoofing using the data URI scheme

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Abdulrahman Alqabandi]

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

Comment 1

[:Gijs (he/him)]

Attachment: Patch

Comment 2

[:Gijs (he/him)]

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

Comment 3

[:Gijs (he/him)]

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

Comment 4

[Abdulrahman Alqabandi]

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

[Boris Zbarsky [:bzbarsky]]

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.

Comment 6

[:Gijs (he/him)]

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

Comment 7

[Daniel Veditz [:dveditz]]

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

Comment 8

[Boris Zbarsky [:bzbarsky]]

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

Comment 9

[:Gijs (he/him)]

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

Comment 10

[Boris Zbarsky [:bzbarsky]]

> It's possible e10s somehow behaves differently here...

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

Comment 11

[:Gijs (he/him)]

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

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8683258 [details] [diff] [review]
Patch v2

r=me.  No one modifies the length of the buffer.

Comment 13

[:Gijs (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b7957aa36fbd

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b7957aa36fbd

Comment 15

[:Gijs (he/him)]

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

Comment 16

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8683258 [details] [diff] [review]
Patch v2

OK, let's take it. Should be in 43 beta 2.

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e96670656f90

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1692a311615d