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)
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)
1.41 KB,
patch
|
Gijs
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•9 years ago
|
Component: General → DOM
Product: Firefox → Core
Reporter | ||
Comment 1•9 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.
Comment 2•9 years ago
|
||
Bobby is out this week, it looks like. Could you look at this, Blake? Thanks.
Flags: needinfo?(mrbkap)
Comment 3•9 years ago
|
||
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•9 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.
Comment 5•9 years ago
|
||
In this instance not that severe because the user is at least prompted.
Assignee | ||
Comment 6•9 years ago
|
||
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•9 years ago
|
Assignee: nobody → mrbkap
Flags: needinfo?(mrbkap)
Comment 7•9 years ago
|
||
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•9 years ago
|
Group: core-security → dom-core-security
Assignee | ||
Comment 9•8 years 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•8 years ago
|
||
Attachment #8720420 -
Flags: review?(dao)
Assignee | ||
Updated•8 years ago
|
Attachment #8653071 -
Attachment is obsolete: true
Comment 11•8 years 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•8 years ago
|
||
I *might* have some time this weekend or before even if someone needs to pass the ball to me so to speak.
Comment 13•8 years ago
|
||
(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•8 years 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•8 years 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•8 years 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•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f20995bd994c68fc554676ef92868be779eb312 Bug 1193093 - Tighten up these checks a little. r=Gijs
Assignee | ||
Comment 18•8 years 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•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f937f9e1b5fee1312ea6fc838f8915a4271903e Bug 1193093 - Tighten up these checks a little. r=Gijs
Comment 22•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2f937f9e1b5f
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Comment 23•8 years 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)
Updated•8 years ago
|
Group: dom-core-security → core-security-release
Updated•8 years ago
|
Whiteboard: [post-critsmash-triage]
Updated•8 years ago
|
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main47+]
Updated•8 years ago
|
Alias: CVE-2016-2825
Updated•7 years ago
|
Group: core-security-release
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•