1193093 - (CVE-2016-2825) Partial SOP violation via forged location.host

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Armin Ebert [:arminius]]

The location.host property can be set to an arbitrary string after creating an invalid data: URI via location.protocol='data'.

Security impact:

An origin security check relying on location.host / location.hostname can potentially be bypassed. The attack surface is limited, though: Since afterwards the scheme is data:, a forged location will not pass any checks involving a http/https location.protocol value. So cookie stealing, etc. would still not be possible.

Proof of Concept:

- Visit http://example.com/.
- location.protocol = 'data';
- location.hostname = 'mozilla.org';
- alert(location.host);

You can encounter some host-only origin validations in the FF source, for instance in the Navigator.registerContentHandler() routine. The function will normally refuse to register a handler for a different host:

navigator.registerContentHandler('application/vnd.mozilla.maybe.feed', 'http://mozilla.org/%s', 'test');
"Permission denied to add http://mozilla.org/%s as a content or protocol handler"

However, after setting the host to mozilla.org as described above, the handler is permitted. Although this particular case is not a severe security problem, there might be similar flaws in other components and addons. Finally, such a host string could now also contain quotes and angle brackets, while some optimistic piece of chrome code might falsely assume that a host does not need to be sanitized for a SQL query / HTML output.

Comment 1

[Armin Ebert [:arminius]]

Note, that this does not affect document.domain, so the origin will stay intact. This bug is really just about the hostname property.

Comment 2

[Andrew McCreight [:mccr8]]

Bobby is out this week, it looks like. Could you look at this, Blake? Thanks.

Comment 3

[Bobby Holley (:bholley)]

This doesn't look like a bug in our origin handling, though potentially registerContentHandler is misbehaving. If there's a bug here, it's there. The host of a data URI is incomparable with the host of an http URI.

I don't have time to investigate myself this week, so having Blake take a look would be good.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

I looked at this briefly today and it looks like comment 3 is right on. In addition, I looked at all of the other (non-test) uses of location.hostname and registerContentHandler is the only one that relies on it for any sort of security check. I have a patch that fixes registerContentHandler that I'll attach on Monday once I've written a test.

Comment 5

[Daniel Veditz [:dveditz]]

In this instance not that severe because the user is at least prompted.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch v1

I had a lot of trouble writing a mochitest for this. Doing location.protocol = "data"; location.hostname = "example.org" (for example) seems to bypass the PAC policy we have while resolving the hostname and causes us to abort because mochitests are never supposed to touch a non-local network address.

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 8653071 [details] [diff] [review]
Patch v1

>     if ((!pb.prefHasUserValue(PREF_ALLOW_DIFFERENT_HOST) ||
>          !pb.getBoolPref(PREF_ALLOW_DIFFERENT_HOST)) &&
>-        aContentWindow.location.hostname != uri.host)
>+        (aContentWindow.location.protocol != "http" &&
>+         aContentWindow.location.protocol != "https")) ||
>+        aContentWindow.location.hostname != uri.host) {

This is really hard to read, and in fact I think it's broken... 5 opening parentheses and 6 closing ones.

You could probably make this more readable by moving the pref calls on one line (or even removing the prefHasUserValue call; I don't see what it's good for) and replacing the two protocol checks with ["http", "https"].includes(aContentWindow.location.protocol).

Comment 8

[Boris Zbarsky [:bzbarsky]]

Blake, are you planning to pick this up again?

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

In the time between when I wrote the patch and now I lost my testcase. I'll attach a new patch anyway, though.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Patch v2

Comment 11

[:Gijs (he/him)]

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

Dão is on holiday, so taking over the review. Will try to look at this today or tomorrow.

Comment 12

[Cody Crews]

I *might* have some time this weekend or before even if someone needs to pass the ball to me so to speak.

Comment 13

[Bobby Holley (:bholley)]

(In reply to Cody Crews from comment #12)
> I *might* have some time this weekend or before even if someone needs to
> pass the ball to me so to speak.

Naw, I'm just CCing you on things that I think you might be interested in. :-)

Comment 14

[:Gijs (he/him)]

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

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

r=me

I hesitate to ask, but has anyone audited the rest of the code for checks like this?

Note that we're also fixing the data protocol assign stuff in bug 1245264.

Comment 15

[:Gijs (he/him)]

(In reply to Blake Kaplan (:mrbkap) (please use needinfo!) from comment #6)
> Created attachment 8653071 [details] [diff] [review]
> Patch v1
> 
> I had a lot of trouble writing a mochitest for this. Doing location.protocol
> = "data"; location.hostname = "example.org" (for example) seems to bypass
> the PAC policy we have while resolving the hostname and causes us to abort
> because mochitests are never supposed to touch a non-local network address.

Is there a bug on file for this bypassing the proxy/pac? :-\

Comment 16

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #14)
> Comment on attachment 8720420 [details] [diff] [review]
> Patch v2
> 
> Review of attachment 8720420 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me
> 
> I hesitate to ask, but has anyone audited the rest of the code for checks
> like this?

Ah, just noticed comment #4.

(In reply to Bobby Holley (busy) from comment #3)
> This doesn't look like a bug in our origin handling, though potentially
> registerContentHandler is misbehaving. If there's a bug here, it's there.
> The host of a data URI is incomparable with the host of an http URI.

Indeed, but a regular data URI doesn't have a host. If you do:

location.href = "data:text/plain,foo"

then the hostname and host properties are both "", and attempting to change those from the console using location.hostname = "whatever" produces an exception.

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f20995bd994c68fc554676ef92868be779eb312
Bug 1193093 - Tighten up these checks a little. r=Gijs

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=138d384f3f93

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out in https://hg.mozilla.org/integration/mozilla-inbound/rev/3858cae0e822 for various m(bc) test failures:

https://treeherder.mozilla.org/logviewer.html#?job_id=22148319&repo=mozilla-inbound

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

You also seem to have broken some other tests:

Mochitest: https://treeherder.mozilla.org/logviewer.html#?job_id=22148722&repo=mozilla-inbound
Web Platform Test: https://treeherder.mozilla.org/logviewer.html#?job_id=22146676&repo=mozilla-inbound

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f937f9e1b5fee1312ea6fc838f8915a4271903e
Bug 1193093 - Tighten up these checks a little. r=Gijs

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2f937f9e1b5f

Comment 23

[:Gijs (he/him)]

(In reply to Blake Kaplan (:mrbkap) (please use needinfo!) from comment #21)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 2f937f9e1b5fee1312ea6fc838f8915a4271903e
> Bug 1193093 - Tighten up these checks a little. r=Gijs

Ugh, sorry for not catching that in review. Thanks!