fetch() doesn't do a preflight when doing same-origin to cross-origin redirect

RESOLVED FIXED in Firefox 43

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: sicking, Assigned: bkelly)

Tracking

({csectype-sop, regression, sec-high})

unspecified
mozilla44
Points:
---

Firefox Tracking Flags

(firefox42 unaffected, firefox43+ fixed, firefox44+ fixed, firefox-esr38 unaffected)

Details

(Whiteboard: [b2g-adv-main2.5-])

Attachments

(2 attachments, 1 obsolete attachment)

Posted patch Add test to show problem (obsolete) — 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.
The right fix here is to do a preflight and only follow the redirect when that's successful, right?
Flags: needinfo?(jonas)
Jonas tells me he already has patches to fix this.
Group: core-security → dom-core-security
Assignee: nobody → jonas
Flags: needinfo?(jonas)
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.
I told Jonas I would make a patch against beta to cause these fetch() calls fail.  NI to myself to remember.
Flags: needinfo?(bkelly)
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
Flags: needinfo?(bkelly)
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;
+    }
+  }
Tracking since this is a regression and sec-high.
Assignee: jonas → bkelly
Status: NEW → ASSIGNED
Attachment #8675872 - Flags: review?(jonas)
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+
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?
Attachment #8675872 - Flags: sec-approval?
Attachment #8675872 - Flags: sec-approval+
Attachment #8675872 - Flags: approval-mozilla-aurora?
Attachment #8675872 - Flags: approval-mozilla-aurora+
Thanks for the quick turnaround.  I will land in inbound and aurora directly.
https://hg.mozilla.org/mozilla-central/rev/e15344362a4b
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
(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
Group: dom-core-security → core-security-release
Group: core-security-release
Whiteboard: [b2g-adv-main2.5-]
You need to log in before you can comment on or make changes to this bug.