Closed Bug 1193093 (CVE-2016-2825) Opened 9 years ago Closed 8 years ago

Partial SOP violation via forged location.host

Categories

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

39 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: arminius, Assigned: mrbkap)

Details

(Keywords: sec-low, Whiteboard: [post-critsmash-triage][adv-main47+])

Attachments

(1 file, 1 obsolete file)

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.
Component: General → DOM
Product: Firefox → Core
Note, that this does not affect document.domain, so the origin will stay intact. This bug is really just about the hostname property.
Bobby is out this week, it looks like. Could you look at this, Blake? Thanks.
Flags: needinfo?(mrbkap)
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.
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.
In this instance not that severe because the user is at least prompted.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: sec-low
Attached patch Patch v1 (obsolete) — Splinter Review
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.
Attachment #8653071 - Flags: review?(dao)
Assignee: nobody → mrbkap
Flags: needinfo?(mrbkap)
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).
Attachment #8653071 - Flags: review?(dao) → review-
Group: core-security → dom-core-security
Blake, are you planning to pick this up again?
Flags: needinfo?(mrbkap)
In the time between when I wrote the patch and now I lost my testcase. I'll attach a new patch anyway, though.
Flags: needinfo?(mrbkap)
Attached patch Patch v2Splinter Review
Attachment #8720420 - Flags: review?(dao)
Attachment #8653071 - Attachment is obsolete: true
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.
Attachment #8720420 - Flags: review?(dao) → review?(gijskruitbosch+bugs)
I *might* have some time this weekend or before even if someone needs to pass the ball to me so to speak.
(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 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.
Attachment #8720420 - Flags: review?(gijskruitbosch+bugs) → review+
(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? :-\
Flags: needinfo?(mrbkap)
(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.
https://hg.mozilla.org/mozilla-central/rev/2f937f9e1b5f
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
(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!
Flags: needinfo?(mrbkap)
Group: dom-core-security → core-security-release
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main47+]
Alias: CVE-2016-2825
Group: core-security-release
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: