Closed Bug 1199430 (CVE-2015-7188) Opened 10 years ago Closed 9 years ago

White-spaces in host IP address, leading to same origin policy bypass

Categories

(Core :: Networking, defect)

Product:
Core
Component:
Networking
Version:
40 Branch
Type:
defect
Priority:
Not set
Severity:
major

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla44
Tracking Flags:
Tracking Status
firefox41 --- wontfix
firefox42 + verified
firefox43 + verified
firefox44 --- verified
firefox-esr31 --- wontfix
firefox-esr38 42+ verified
thunderbird_esr38 --- fixed

People

(Reporter: michal.bentkowski, Assigned: valentin)

References

(
URL
)

Details

(Keywords: reporter-external, sec-high, Whiteboard: [adv-main42+][adv-esr38.4+])

Attachments

(6 files, 5 obsolete files)

Reject hostnames containing @
4.47 KB, patch
mcmanus
: review+
abillings
: sec-approval+
Details | Diff | Splinter Review
Update tests for more strict hostname parsing
4.42 KB, patch
Details | Diff | Splinter Review
wpt-test-url.patch
925 bytes, patch
Details | Diff | Splinter Review
[aurora] Bug 1199430 - Reject invalid characters from URLs
8.61 KB, patch
abillings
: approval-mozilla-aurora+
abillings
: approval-mozilla-beta+
abillings
: approval-mozilla-esr38+
Details | Diff | Splinter Review
[beta] Bug 1199430 - Reject invalid characters from URLs
8.66 KB, patch
Details | Diff | Splinter Review
[esr38] Bug 1199430 - Reject invalid characters from URLs
6.05 KB, patch
Details | Diff | Splinter Review
It is possible to add white-space characters in host names being IP addresses, which can lead to some security issues, including same origin policy bypass. Here's an example: 1. 176.58.110.151 is an IP address of host sekurak.securitum.com. 2. Enter the following line in JS console (you can replace \x0b with \x0c as well): location = 'http://176.58.110.151\x0bRANDOM_STUFF_HERE<html>' 3. Host name is still resolved to 176.58.110.151, while the host header will contain the string "176.58.110.151\x0bRANDOM_STUFF_HERE<html>". The same applies to document.domain and location.origin. Furthermore, using location.host or location.hostname, it is also possible to include new-line (0x0A) and tab (0x09) in host names. The former allows to request header injection attack. location.host = '176.58.110.151\nApache_will_complain_about_bad_request' Using unicode transliteration in DNS names, you can also put "@" character there. Interestingly, in the case below, a favicon will be set to the one of youtube.com: location.host = '176.58.110.151\x0b\uff20www.youtube.com' I can think of a few scenarios abusing this behaviour: 1. Phishing attempts: for less cautious users domains like: "176.58.110.151\x0b.accouts.google.com" might look genuine. The address bar even highlights "google.com" 2. Some postMessage implementations verify origin by checking its last characters. So any checks based on the scheme: `if (origin.endsWith(".example.com"))` will fail with host "176.58.110.151\x0bx.example.com". 3. Request header injection might be abused to perform some attacks, not usually possible with web browsers, like shellshock. 4. The most interesting one: bypass same origin policy - with help of Flash Plugin (which is a pretty common scenario). The notable exception here is, however, that it only works for http requests; it is not possible to do that in HTTPS because you can't have a valid certificate to any <ip_address><white_space><stuff> domain name. I have created a PoC here: http://sekurak.securitum.com/fx_sop_bypass942958/ You just enter an URL and it is fetched into an <pre> element. The idea is as follows: 1. There's a flash file using URLLoader and URLRequest classes. Using these, you can download any URL given that it is in the same origin as the flash file. 2. The URL of the flash file is abusing the "@" trick shown earlier. For example: when you try to fetch a resource from http://translate.google.com, the flash file is fetched from domain "176.58.110.151\x0b\uff20translate.google.com" that gets transliterated to "176.58.110.151\x0b@translate.google.com". 3. Firefox fetches the file from 176.58.110.151 but Flash assumes it is run in http://translate.google.com origin hence allowing to get any resource from http://translate.google.com. Please let me know if you have any further questions.
Flags: sec-bounty?
Depends on: 1199601
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: sec-high
something you can tackle valentin?
Flags: needinfo?(valentin.gosu)
Obviously we need to fix this on our end, but - as the reporter mentioned already - this has a special impact on Flash. For whatever legacy reasons, the Flash Player gets its location information from this string. I've confirmed via further testing that indeed, Flash will put its content in whatever origin is included in the trailing characters following the whitespace. This is bad for them, obviously, as it violates SOP. However, it is made worse due to the way Flash manages global camera/mic permissions. Spoof the Flash Player's magic domain/URL and you can control those settings also. Adding Jeromie from Adobe so that they are aware.
On it.
Assignee: nobody → valentin.gosu
Flags: needinfo?(valentin.gosu)
Attached patch Reject hostnames containing @ (obsolete) — — Splinter Review
Attachment #8656377 - Flags: review?(mcmanus)
This was quite a clever exploit. It worked because pr_inet_aton allowed one (or more) trailing whitespace characters when parsing an IP address. Also it uses the \uff20 which gets normalized to the regular @ sign in SetHost.
Group: firefox-core-security → core-security
Component: Security → Networking
Product: Firefox → Core
Comment on attachment 8656377 [details] [diff] [review] Reject hostnames containing @ Review of attachment 8656377 [details] [diff] [review]: ----------------------------------------------------------------- if we're going to release a new nspr, then you'll need to get a review from ted and start that process. But it would be easier certainly for at least backports if we could at least work around the issue in gecko. In any event, I don't think inet_aton is doing anything wrong - inet_addr must have a valid address terminated either by a space or by null https://www-01.ibm.com/support/knowledgecenter/ssw_aix_71/com.ibm.aix.commtrf2/inet_addr.htm .. so this is probably a gecko level fix needed, right?
Attachment #8656377 - Flags: review?(mcmanus)
the other thing that's making this hard is that nspr don't always use pr_inet_aton - https://mxr.mozilla.org/mozilla-central/source/nsprpub/pr/src/misc/prnetdb.c#2218 - sometimes it just uses getaddrinfo from the system. and getaddrinfo seems to have a few different behaviors wrt spaces http://comments.gmane.org/gmane.comp.gnu.inetutils.bugs/3718 so SetHost() already has a check for spaces (line 1597) - does it work to just make that logic more robust? (it only checks for ' ' right now).
(In reply to Patrick McManus [:mcmanus] from comment #6) > Comment on attachment 8656377 [details] [diff] [review] > Reject hostnames containing @ > > Review of attachment 8656377 [details] [diff] [review]: > ----------------------------------------------------------------- > > if we're going to release a new nspr, then you'll need to get a review from > ted and start that process. But it would be easier certainly for at least > backports if we could at least work around the issue in gecko. > > In any event, I don't think inet_aton is doing anything wrong - inet_addr > must have a valid address terminated either by a space or by null > https://www-01.ibm.com/support/knowledgecenter/ssw_aix_71/com.ibm.aix. > commtrf2/inet_addr.htm .. so this is probably a gecko level fix needed, > right? If we go by the docs, inet_aton should accept a null or a space terminator, not whitespace \x09 .. \x0D We could strip \r\n\t from the hostname as the spec suggests, but that still leaves \x0B and \x0C. Although this issue is concerning the URL parser, there might be other codepaths that could to PR_StringToNetAddr and cause similar issues. I will focus on fixing the URL bits, and file a different bug to investigate if there are any other attack vectors.
Attachment #8656762 - Flags: review?(mcmanus)
Attachment #8656377 - Attachment is obsolete: true
The patch improves our spec compat a bit. Some of the EXPECTED-FAIL tests now pass. The failing test_nsDefaultURIFixup_info.js needs to be removed as it no longer applies. shared-worker-connect-src-allowed.sub.html fails, because http://www1.{{host}}:{{ports[http][0]}} is not a valid URL. https://treeherder.mozilla.org/#/jobs?repo=try&revision=303f17cf8c52
Attachment #8656762 - Flags: review?(mcmanus) → review+
Gijs, I've got one unit test that is failing and might pose a problem. https://treeherder.mozilla.org/logviewer.html#?job_id=11190563&repo=try We can probably fix it in nsDefaultURIFixup.cpp by making the backslash replacement unconditional - currently depends on defined(XP_WIN). I've hit this wall before in bug 1023468 comment 13. Or we could allow \ in the hostname for now, and fix it later. What do you think?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Valentin Gosu [:valentin] from comment #12) > Gijs, I've got one unit test that is failing and might pose a problem. > https://treeherder.mozilla.org/logviewer.html#?job_id=11190563&repo=try > We can probably fix it in nsDefaultURIFixup.cpp by making the backslash > replacement unconditional - currently depends on defined(XP_WIN). I've hit > this wall before in bug 1023468 comment 13. Or we could allow \ in the > hostname for now, and fix it later. What do you think? I'm not sure I understand. The test is still passing on Windows. It seems we can just update the test expectation on non-Windows to expect a |null| fixedURI. That makes sense to me - the hostname specified does not make sense. We could follow up to make the fixup not be conditional on windows-ness, or make it result in <hostname>/%5C on non-Windows. For this bug, I think that the fixup URI being null is acceptable. I also think the test gives us a reasonable (though not perfect, because it's a test and confirmation bias and we are never perfect...) guarantee that the code isn't breaking other (more serious) assumptions. Hopefully. Does that make sense?
Flags: needinfo?(gijskruitbosch+bugs)
Group: core-security → network-core-security
(In reply to :Gijs Kruitbosch from comment #13) > Does that make sense? +needinfo
Flags: needinfo?(valentin.gosu)
(In reply to :Gijs Kruitbosch from comment #14) > (In reply to :Gijs Kruitbosch from comment #13) > > Does that make sense? > > +needinfo Of course. Sorry for the late reply. I was on PTO for the past few days and unable to answer bugmail. I will attach the modified tests ASAP. I will probably need to create patches for aurora, beta, etc as well, and will do so in about 24 hrs. Thanks
Flags: needinfo?(valentin.gosu)
Attachment #8660402 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8660402 [details] [diff] [review] Tests for more strict url parsing Review of attachment 8660402 [details] [diff] [review]: ----------------------------------------------------------------- r=me on the docshell test change with the nits below - not entirely sure about the location assignment wp test. ::: docshell/test/unit/test_nsDefaultURIFixup_info.js @@ -212,5 @@ > protocolChange: true > }, { > - input: "[::1][100", > - fixedURI: "http://[::1][100/", > - alternateURI: "http://[::1][100/", Seems like this, too, can just have null for both of these, so we can continue to verify this results in a keyword lookup where allowed. @@ +500,4 @@ > testcases.push({ > input: "mozilla\\", > + // fixedURI: "http://mozilla\\/", > + // alternateURI: "http://www.mozilla/", r=me assuming you've checked that omitting these works when the result is null (rather than undefined). ::: testing/web-platform/meta/html/browsers/history/the-location-interface/location_assign.html.ini @@ +1,4 @@ > [location_assign.html] > type: testharness > + [URL that fails to parse] > + expected: FAIL Not sure I'm the best reviewer for this bit - why does this test still fail?
Attachment #8660402 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs Kruitbosch from comment #17) > Comment on attachment 8660402 [details] [diff] [review] > Tests for more strict url parsing > > - input: "[::1][100", > > - fixedURI: "http://[::1][100/", > > - alternateURI: "http://[::1][100/", > > Seems like this, too, can just have null for both of these, so we can > continue to verify this results in a keyword lookup where allowed. Seems fair. > @@ +500,4 @@ > > testcases.push({ > > input: "mozilla\\", > > + // fixedURI: "http://mozilla\\/", > > + // alternateURI: "http://www.mozilla/", > > r=me assuming you've checked that omitting these works when the result is > null (rather than undefined). > Checked. It works if I make the expected result be null. > ::: > testing/web-platform/meta/html/browsers/history/the-location-interface/ > location_assign.html.ini > @@ +1,4 @@ > > [location_assign.html] > > type: testharness > > + [URL that fails to parse] > > + expected: FAIL > > Not sure I'm the best reviewer for this bit - why does this test still fail? This fails because location.assign() throws when parsing and invalid URI. I will file a bug on the WP tests upstream to add a try - catch block.
Just catching up on the post-vacation email queue. Thanks for the heads up. I've opened ADBE 4055745 to get this resolved on our end.
Carrying over the r+ from comment 17
Attachment #8660402 - Attachment is obsolete: true
Attachment #8660968 - Flags: review+
Comment on attachment 8656762 [details] [diff] [review] Reject hostnames containing @ [Security approval request comment] How easily could an exploit be constructed based on the patch? - Not easily. The exploit involves abusing missing safegards for invalid characters, the transformation of a unicode character into @ and the fact that pr_inet_aton ignores characters following an IP address. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? - No. See previous question. Which older supported branches are affected by this flaw? - All. If not all supported branches, which bug introduced the flaw? - Preexisting condition. It goes back a long way. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? - I've got patches for aurora and beta on running on try. How likely is this patch to cause regressions; how much testing does it need? - There is a certain risk that URLs that previously worked would stop working because they contain invalid characters.
Attachment #8656762 - Flags: sec-approval?
sec-approval+ for checkin on October 6, two weeks into the new cycle, as we've got a release candidate for 41 already and are shipping soon.
status-firefox41: --- → wontfix
status-firefox42: --- → affected
status-firefox43: --- → affected
status-firefox-esr31: --- → affected
tracking-firefox42: --- → +
tracking-firefox43: --- → +
tracking-firefox-esr38: --- → 42+
Whiteboard: [checkin on 10/6]
Comment on attachment 8656762 [details] [diff] [review] Reject hostnames containing @ When you do checkin, please do *not* checkin tests until after we've shipped the fix in a public release.
Attachment #8656762 - Flags: sec-approval? → sec-approval+
BTW, the patch with the tests is just to change the expectations for web-platform-tests. It does not include the testcase for this issue.
Firefox 40.0.3 on Mac, Firefox 39.0 and 40.0 on Win7, when viewing the POC http://sekurak.securitum.com/fx_sop_bypass942958/, I consistently fail to load the SWF : GET http://176.58.110.151@translate.google.com/fx_sop_bypass942958/FlashTest.swf [HTTP/1.1 504 Unknown Host 165ms] Are there any platform/version instructions I'm missing, or steps I should have taken ? I'd like to fix the Flash Player, but I'd need a working test case to base any changes on.
(In reply to Peter Grandmaison from comment #25) > Firefox 40.0.3 on Mac, Firefox 39.0 and 40.0 on Win7, when viewing the POC > http://sekurak.securitum.com/fx_sop_bypass942958/, I consistently fail to > load the SWF : > > > GET > http://176.58.110.151@translate.google.com/fx_sop_bypass942958/FlashTest.swf > [HTTP/1.1 504 Unknown Host 165ms] > > Are there any platform/version instructions I'm missing, or steps I should > have taken ? I'd like to fix the Flash Player, but I'd need a working test > case to base any changes on. Peter - just emailed you a ZIP file with samples and instructions. Let me know if you need help.
This is okay to check in now. Thanks.
Flags: needinfo?(valentin.gosu)
Flags: needinfo?(valentin.gosu)
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/657c9f974bdf for the fix the tests failed to apply: adding at_url_test.patch to series file applying at_url_test.patch patching file testing/web-platform/meta/url/url-constructor.html.ini Hunk #1 FAILED at 54 1 out of 2 hunks FAILED -- saving rejects to file testing/web-platform/meta/url/url-constructor.html.ini.rej also per Comment #23 can we apply this test patch now or do we need to wait ?
Flags: needinfo?(valentin.gosu)
Keywords: leave-open
The test need to be applied in order to avoid failing xpcshell and web-platform tests.
Flags: needinfo?(valentin.gosu)
This EXPECT-FAIL also needs to be removed. Carsten, could you check it in please? https://treeherder.mozilla.org/logviewer.html#?job_id=15378224&repo=mozilla-inbound
Flags: needinfo?(cbook)
(In reply to Valentin Gosu [:valentin] from comment #34) > Created attachment 8671314 [details] [diff] [review] > wpt-test-url.patch > > This EXPECT-FAIL also needs to be removed. > Carsten, could you check it in please? > > https://treeherder.mozilla.org/logviewer.html#?job_id=15378224&repo=mozilla- > inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/b641ebf97eec
Flags: needinfo?(cbook)
https://hg.mozilla.org/mozilla-central/rev/657c9f974bdf
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Valentin, I think we want that in esr38, 42 & 43, could you fill the uplift requests? Thanks
status-firefox-esr31: affected → wontfix
status-firefox-esr38: --- → affected
Flags: needinfo?(valentin.gosu)
Group: network-core-security → core-security-release
Attachment #8671002 - Attachment is obsolete: true
Attachment #8671004 - Attachment is obsolete: true
Comment on attachment 8672626 [details] [diff] [review] [aurora] Bug 1199430 - Reject invalid characters from URLs Approval Request Comment [Feature/regressing bug #]: Not a regression. The parsing issue has existed for a very long time. [User impact if declined]: An attacker is able to redirect the user to a special URI which resolves to an attacker-controlled IP address, but displays a different domain such as google.com and maybe bypass same-origin policies. [Describe test coverage new/current, TreeHerder]: Tested manually. Will add xpcshell tests to avoid regressions. [Risks and why]: There is a possibility for web-compat issues if websites or addons are abusing the URL parser for various reasons. However, this change follows the URL spec for host parsing, rejecting invalid characters. https://url.spec.whatwg.org/#concept-host-parser [String/UUID change made/needed]: None.
Flags: needinfo?(valentin.gosu)
Attachment #8672626 - Flags: approval-mozilla-esr38?
Attachment #8672626 - Flags: approval-mozilla-beta?
Attachment #8672626 - Flags: approval-mozilla-aurora?
Attachment #8672626 - Flags: approval-mozilla-esr38?
Attachment #8672626 - Flags: approval-mozilla-esr38+
Attachment #8672626 - Flags: approval-mozilla-beta?
Attachment #8672626 - Flags: approval-mozilla-beta+
Attachment #8672626 - Flags: approval-mozilla-aurora?
Attachment #8672626 - Flags: approval-mozilla-aurora+
Comment on attachment 8672626 [details] [diff] [review] [aurora] Bug 1199430 - Reject invalid characters from URLs Please don't add the tests until after we ship the fix publicly.
URL: http://sekurak.securitum.com/fx_sop_b...
Flags: sec-bounty? → sec-bounty+
Whiteboard: [checkin on 10/6]
on beta this caused unexpected pass results like https://treeherder.mozilla.org/logviewer.html#?job_id=572461&repo=mozilla-beta valentin, could you take a look, thanks!
Flags: needinfo?(valentin.gosu)
That seems very odd. I removed the EXPECTED-FAIL file at testing/web-platform/meta/content-security-policy/blink-contrib/shared-worker-connect-src-blocked.sub.html.ini so that shouldn't have happened. Unless I'm missing something. Also weird that it's only happening on windows. Could this be a bug in the test framework? Or maybe the patch failed to apply on windows?
(In reply to Valentin Gosu [:valentin] from comment #48) > That seems very odd. > I removed the EXPECTED-FAIL file at > testing/web-platform/meta/content-security-policy/blink-contrib/shared- > worker-connect-src-blocked.sub.html.ini so that shouldn't have happened. > Unless I'm missing something. > Also weird that it's only happening on windows. Could this be a bug in the > test framework? Or maybe the patch failed to apply on windows? what i used here was the [beta] Bug 1199430 patch - aurora seems fine. shall we try to apply the patch again ?
Sorry I didn't answer sooner, I was trying to debug the problem locally. I don't see any reason for the UNEXPECTED-PASS in the patch. I suspect a bug in the WPT framework, but I can't confirm it just yet. Still investigating.
Relanded in https://hg.mozilla.org/releases/mozilla-beta/rev/cdc65a3f9b02, with a touch of /CLOBBER in https://hg.mozilla.org/releases/mozilla-beta/rev/c5ef20f11b44 - that failure pattern, where every test run on a particular build (both WinXP and Win7 opt on one push, both WinXP and Win7 debug on another push) reeks of some long-forgotten and long-fixed needs-clobber bug.
Thanks Phil. That might explain why I haven't been able to reproduce it locally. Hope it sticks.
Flags: needinfo?(valentin.gosu)
QA Contact: mwobensmith
Depends on: 1215944
Verified fixed on all latest builds of Fx42, 43, 44 and ESR38.
Status: RESOLVED → VERIFIED
status-firefox42: fixed → verified
status-firefox43: fixed → verified
status-firefox44: fixed → verified
status-firefox-esr38: fixed → verified
Whiteboard: [adv-main42+][adv-esr38.4+]
Alias: CVE-2015-7188
Blocks: 1013615
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: