The default bug view has changed. See this FAQ.
Bug 1193093 (CVE-2016-2825)

Partial SOP violation via forged location.host

RESOLVED FIXED in Firefox 47

Status

()

Core
DOM
RESOLVED FIXED
2 years ago
3 months ago

People

(Reporter: Armin Razmjou, Assigned: mrbkap)

Tracking

({sec-low})

39 Branch
mozilla47
sec-low
Points:
---

Firefox Tracking Flags

(firefox47 fixed)

Details

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

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
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
(Reporter)

Comment 1

2 years ago
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.
(Assignee)

Comment 4

2 years ago
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
(Assignee)

Comment 6

2 years ago
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.
Attachment #8653071 - Flags: review?(dao)
(Assignee)

Updated

2 years ago
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-

Updated

2 years ago
Group: core-security → dom-core-security
Blake, are you planning to pick this up again?
Flags: needinfo?(mrbkap)
(Assignee)

Comment 9

a year ago
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)
(Assignee)

Comment 10

a year ago
Created attachment 8720420 [details] [diff] [review]
Patch v2
Attachment #8720420 - Flags: review?(dao)
(Assignee)

Updated

a year ago
Attachment #8653071 - Attachment is obsolete: true

Comment 11

a year ago
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)

Comment 12

a year ago
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 14

a year ago
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+

Comment 15

a year ago
(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)

Comment 16

a year ago
(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.
(Assignee)

Comment 17

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

Comment 18

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=138d384f3f93
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
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
(Assignee)

Comment 21

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f937f9e1b5fee1312ea6fc838f8915a4271903e
Bug 1193093 - Tighten up these checks a little. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/2f937f9e1b5f
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47

Comment 23

a year ago
(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]

Updated

10 months ago
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main47+]

Updated

10 months ago
Alias: CVE-2016-2825
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.