Closed
Bug 1212433
Opened 9 years ago
Closed 9 years ago
fetch() doesn't do a preflight when doing same-origin to cross-origin redirect
Categories
(Core :: DOM: Security, defect)
Core
DOM: Security
Tracking
()
RESOLVED
FIXED
mozilla44
Tracking | Status | |
---|---|---|
firefox42 | --- | unaffected |
firefox43 | + | fixed |
firefox44 | + | fixed |
firefox-esr38 | --- | unaffected |
People
(Reporter: sicking, Assigned: bkelly)
Details
(Keywords: csectype-sop, regression, sec-high, Whiteboard: [b2g-adv-main2.5-])
Attachments
(2 files, 1 obsolete file)
1.55 KB,
patch
|
Details | Diff | Splinter Review | |
3.62 KB,
patch
|
sicking
:
review+
abillings
:
approval-mozilla-aurora+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
Easiest way to reproduce is to apply the attached patch which adds a test for this case.
If fetch() does a same-origin "complex" request (for example with method DELETE), which is then redirected to a cross-origin URL, then the fetch() code follows that redirect.
The bad part is that it follows this redirect without doing a preflight first. This makes it trivial to do an arbitrary request to an arbitrary cross-origin URL using the users cookies.
We do do a CORS check on the returned response, but at this time any side effects on the server might have already happened.
Assignee | ||
Comment 1•9 years ago
|
||
I believe we just need to add this code:
https://dxr.mozilla.org/mozilla-central/rev/727d765a5ed81656d3a30724f5b70ba0bd17b9c5/dom/fetch/FetchDriver.cpp#618
Here:
https://dxr.mozilla.org/mozilla-central/rev/727d765a5ed81656d3a30724f5b70ba0bd17b9c5/dom/fetch/FetchDriver.cpp#1092
If nextOp.mCORSPreflightFlag is true.
Updated•9 years ago
|
status-firefox42:
--- → affected
status-firefox43:
--- → affected
status-firefox44:
--- → affected
status-firefox-esr38:
--- → unaffected
tracking-firefox42:
--- → ?
tracking-firefox43:
--- → ?
tracking-firefox44:
--- → ?
tracking-firefox-esr38:
--- → -
Keywords: csectype-sop,
sec-high
Comment 2•9 years ago
|
||
The right fix here is to do a preflight and only follow the redirect when that's successful, right?
Flags: needinfo?(jonas)
Assignee | ||
Comment 3•9 years ago
|
||
Jonas tells me he already has patches to fix this.
Updated•9 years ago
|
Group: core-security → dom-core-security
Updated•9 years ago
|
Assignee: nobody → jonas
Flags: needinfo?(jonas)
Reporter | ||
Comment 4•9 years ago
|
||
I have patches for *trunk*. But they are much too large to be applied to branches.
I think the simplest solution here would be to block the redirect to cross-origin. That's what we do for XHR.
The fact that fetch() is trying to be fancy and do things that our CORS implementation wasn't written to handle is why we have this security bug in the first place. So I'd rather not add more fancyness to the fetch() code.
Reporter | ||
Comment 5•9 years ago
|
||
Reporter | ||
Updated•9 years ago
|
Attachment #8670863 -
Attachment is obsolete: true
Assignee | ||
Comment 6•9 years ago
|
||
I told Jonas I would make a patch against beta to cause these fetch() calls fail. NI to myself to remember.
Flags: needinfo?(bkelly)
Assignee | ||
Comment 7•9 years ago
|
||
Actually, this only affects aurora and nightly. I introduced this problem in bug 1184607 as you can see from me changing these exact tests here:
https://hg.mozilla.org/mozilla-central/rev/a01c5c8209a8
status-firefox42:
affected → ---
Flags: needinfo?(bkelly)
Assignee | ||
Updated•9 years ago
|
status-firefox42:
--- → unaffected
tracking-firefox42:
? → ---
Assignee | ||
Comment 8•9 years ago
|
||
Specifically, I introduced this bug by removing this block from FetchDriver::AsyncOnChannelRedirect():
+ //
+ // Requests that require preflight are not permitted to redirect.
+ // Fetch spec section 4.2 "HTTP Fetch", step 4.9 just uses the manual
+ // redirect flag to decide whether to execute step 4.10 or not. We do not
+ // represent it in our implementation.
+ // The only thing we do is to check if the request requires a preflight (part
+ // of step 4.9), in which case we abort. This part cannot be done by
+ // nsCORSListenerProxy since it does not have access to mRequest.
+ // which case. Step 4.10.3 is handled by OnRedirectVerifyCallback(), and all
+ // the other steps are handled by nsCORSListenerProxy.
+ if (!NS_IsInternalSameURIRedirect(aOldChannel, aNewChannel, aFlags)) {
+ rv = DoesNotRequirePreflight(aNewChannel);
+ if (NS_FAILED(rv)) {
+ NS_WARNING("FetchDriver::OnChannelRedirect: "
+ "DoesNotRequirePreflight returned failure");
+ return rv;
+ }
+ }
Comment 9•9 years ago
|
||
Tracking since this is a regression and sec-high.
Assignee | ||
Comment 10•9 years ago
|
||
Reporter | ||
Comment 11•9 years ago
|
||
Comment on attachment 8675872 [details] [diff] [review]
Fail fetch() calls that require preflight and also redirect. r=sicking
Review of attachment 8675872 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/fetch/FetchDriver.cpp
@@ +954,5 @@
> + nsresult rv = DoesNotRequirePreflight(aNewChannel);
> + if (NS_FAILED(rv)) {
> + NS_WARNING("FetchDriver::OnChannelRedirect: "
> + "DoesNotRequirePreflight returned failure");
> + return rv;
Nit: A simple NS_ENSURE_SUCCESS(rv, rv) will also warn. And it'll print a line number so you can find where we're bailing.
Attachment #8675872 -
Flags: review?(jonas) → review+
Assignee | ||
Comment 12•9 years ago
|
||
Comment on attachment 8675872 [details] [diff] [review]
Fail fetch() calls that require preflight and also redirect. r=sicking
How easily could an exploit be constructed based on the patch?
Fairly easily since the patch is clearly about preflight and redirects. You would just need to trick someone into visiting your site in order to do a potentially unsafe request.
Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
I think its easy to see the problem even without the tests, so I did include tests here.
Note, these test changes are currently in an open bug (bug 1195167) which is why I'd like to get this bug landed and uplifted ASAP.
Which older supported branches are affected by this flaw?
This only affects 43 and 44. Beta 42 is not affected.
If not all supported branches, which bug introduced the flaw?
Introduced in bug 1184607.
Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
It should be a clean uplift.
How likely is this patch to cause regressions; how much testing does it need?
Minimal risk. Included mochitests cover this condition.
Approval Request Comment
[Feature/regressing bug #]: fetch(), bug 1184607
[User impact if declined]: security issue allowing attacker to perform unsafe cross-origin requests without a preflight
[Describe test coverage new/current, TreeHerder]: included mochitests
[Risks and why]: minimal
[String/UUID change made/needed]: none
[Security approval request comment]
Attachment #8675872 -
Flags: sec-approval?
Attachment #8675872 -
Flags: approval-mozilla-aurora?
Updated•9 years ago
|
Attachment #8675872 -
Flags: sec-approval?
Attachment #8675872 -
Flags: sec-approval+
Attachment #8675872 -
Flags: approval-mozilla-aurora?
Attachment #8675872 -
Flags: approval-mozilla-aurora+
Updated•9 years ago
|
tracking-firefox-esr38:
- → ---
Assignee | ||
Comment 13•9 years ago
|
||
Thanks for the quick turnaround. I will land in inbound and aurora directly.
Assignee | ||
Comment 14•9 years ago
|
||
mozilla-inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/e15344362a4b
mozilla-aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/8cd5ca225e43
Comment 15•9 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Comment 16•9 years ago
|
||
(In reply to Ben Kelly [:bkelly] from comment #14)
> mozilla-inbound:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/e15344362a4b
> mozilla-aurora:
> https://hg.mozilla.org/releases/mozilla-aurora/rev/8cd5ca225e43
setting flags for the aurora checkin
Updated•9 years ago
|
Group: dom-core-security → core-security-release
Updated•9 years ago
|
Group: core-security-release
Updated•9 years ago
|
Whiteboard: [b2g-adv-main2.5-]
You need to log in
before you can comment on or make changes to this bug.
Description
•