In bug 1184607 I found and fixed a fetch() redirect problem: When fetch() is redirected from same-origin to cross-origin, it did not perform response tainting. At the time I just fixed this in order to make progress on my current task, but Jonas points out this is a serious security flaw. It means any attacker can do this: fetch(sameOriginRedirectorURL, { mode: 'no-cors', credentials: 'include' }); And get a basic Response with full access to the body. If its redirecting to your bank for instance, this is quite bad. We should uplift a modified version of bug 1184607 P6 to at least beta. We should also consider a chemspill.

To put it simple: This allows reading arbitrary cross-site resources. These resources would be loaded with the user's cookies and so personalized with user information.

This patch is essentially this: https://hg.mozilla.org/integration/mozilla-inbound/rev/42abe24004fd With the following changes: - Remove the RequestRedirect mode bits out as I did not backport those bits - Add back in the preflight-on-redirect security check that was erroneously removed in the linked commit above. If necessary I can make an interdiff. I also need to write a mochitest to verify the opaque tainting for the backport. The tests in bug 1184607 are wpt tests and we don't have the infrastructure necessary to run them on release/beta. https://treeherder.mozilla.org/#/jobs?repo=try&revision=d2320877fc04

Could you do an interdiff compared to the already-landed patch?

If this is caused by bug 1184607, then this only affects 43 and 44, right?

Or does the fix in bug 1184607 need to be backported?

This was *fixed* by bug 1184607, but that bug caused other security regressions so this is not a straight uplift.

Is this the same as bug 1208339, which I established was fixed by bug 1184607?

Yes, seems like it.

(In reply to Jonas Sicking (:sicking) from comment #6) > This was *fixed* by bug 1184607, but that bug caused other security > regressions so this is not a straight uplift. Its also not a straight uplift because bug 1184607 was adding a new feature to fetch that is too large to uplift directly.

This interdiff just removes the parts of the trunk patch related to RequestRedirect and OpaqueRedirect tainting.

This interdiff adds the check for redirect-with-preflight back in. The trunk patch erroneously removed it.

Here are some mochitests that verify the final response type after redirects. I intend to land these on central and all branches. Note, the tests are exempted if the service worker is present until bug 1212904 and bug 1173811 can be fixed. I didn't reference them in the code comments since these are security bugs.

Comment on attachment 8671384 [details] [diff] [review] interdiff 001 Remove RequestRedirect and OpaqueRedirect bits Review of attachment 8671384 [details] [diff] [review]: ----------------------------------------------------------------- I don't know this code well enough to understand what this does. Is it really needed though? Doesn't the other patch solve things? If we really have to land this please ask someone else that knows the code better for review. Unfortunately I'm on PTO on friday and monday, so I can't ask you to walk me through the patch.

Comment on attachment 8671117 [details] [diff] [review] redirect_stuff.patch Review of attachment 8671117 [details] [diff] [review]: ----------------------------------------------------------------- Same with this one.

Comment on attachment 8671435 [details] [diff] [review] P2 Test final response types after redirect. r=sicking Review of attachment 8671435 [details] [diff] [review]: ----------------------------------------------------------------- I assume you'll also land the test patch I attached?

(In reply to Jonas Sicking (:sicking) from comment #15) > I assume you'll also land the test patch I attached? That patch is already in 41 and 42 effectively. As I explained in another bug the tests were marked expected fail and I changed them to expected pass in 43.

(In reply to Jonas Sicking (:sicking) from comment #13) > I don't know this code well enough to understand what this does. Is it > really needed though? Doesn't the other patch solve things? If we really > have to land this please ask someone else that knows the code better for > review. Unfortunately I'm on PTO on friday and monday, so I can't ask you to > walk me through the patch. This is an inter diff to the patch applied to central. It's removing things that were added to central the should not be uplifted. I'll ask ehsan for review.

Comment on attachment 8671384 [details] [diff] [review] interdiff 001 Remove RequestRedirect and OpaqueRedirect bits Ehsan, can you review this interdiff. It strips RequestRedirect stuff we don't want to uplift out of this central commit: https://hg.mozilla.org/integration/mozilla-inbound/rev/42abe24004fd Also, see the second interdiff already r+'d by Jonas.

I need to backport a few more things to these older branches in order for all the tests to pass.

Comment on attachment 8671919 [details] [diff] [review] P3 Backport fetch test isSWPresent support. r=ehsan Review of attachment 8671919 [details] [diff] [review]: ----------------------------------------------------------------- Not sure why you need this since nothing accesses isSWPresent, but r=me.

(In reply to Ehsan Akhgari (don't ask for review please) from comment #21) > Not sure why you need this since nothing accesses isSWPresent, but r=me. P2 depends on it to not break test_fetch_cors_sw_reroute.html.

We need to disable some service worker tests. These attempt to intercept navigations with what is now an opaque response. This fails. On central/aurora this doesn't happen because we have proper "manual" redirect mode support. Since SW is disable on release branches, just disable these tests.

Combine interdiffs into a single patch that can be committed.

Comment on attachment 8671939 [details] [diff] [review] P1 Return the correct Response type from redirected fetch() requests. r=nsm r=sicking r=ehsan For patches P1 to P5 Approval Request Comment [Feature/regressing bug #]: fetch() released in FF39 [User impact if declined]: See comment 1. [Describe test coverage new/current, TreeHerder]: mochitests included in these patches [Risks and why]: Risk limited to fetch() which is a relatively new feature on the web. [String/UUID change made/needed]: none

Comment on attachment 8671939 [details] [diff] [review] P1 Return the correct Response type from redirected fetch() requests. r=nsm r=sicking r=ehsan For patches P1 to P5 Approval Request Comment [Feature/regressing bug #]: fetch() released in FF39 [User impact if declined]: See comment 1. [Describe test coverage new/current, TreeHerder]: mochitests included in these patches [Risks and why]: Risk limited to fetch() which is a relatively new feature on the web. [String/UUID change made/needed]: none

Note, I have done local testing, but not pushed this patch queue to try. What is the process for actually pushing these patches if they are approved? Since its a release branch do I need to leave it to the sheriffs?

Comment on attachment 8671939 [details] [diff] [review] P1 Return the correct Response type from redirected fetch() requests. r=nsm r=sicking r=ehsan For P1 to P5. Sorry, should have flagged this first. [Security approval request comment] How easily could an exploit be constructed based on the patch? Probably fairly easily. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? The tests clearly do. The code itself is a bit harder, but its clearly dealing with tainting and redirects. It would probably not take long for someone to figure out. Which older supported branches are affected by this flaw? 39 to 42 If not all supported branches, which bug introduced the flaw? It was introduced when fetch() was released in 39 and fixed in 43 by bug 1184607. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? These patches apply cleanly to beta. P3 requires a very small and easy rebase to apply to release. How likely is this patch to cause regressions; how much testing does it need? Regressions would be limited to fetch() as its the only affected released feature on these branches. We have decent mochitest coverage, though, so I am optimistic nothing has regressed.

(In reply to Ben Kelly [:bkelly] from comment #27) > Note, I have done local testing, but not pushed this patch queue to try. > > What is the process for actually pushing these patches if they are approved? > Since its a release branch do I need to leave it to the sheriffs? There is no process, go ahead and push. It's better to not include the bug# if possible.

(In reply to Ehsan Akhgari (don't ask for review please) from comment #29) > (In reply to Ben Kelly [:bkelly] from comment #27) > > Note, I have done local testing, but not pushed this patch queue to try. > > > > What is the process for actually pushing these patches if they are approved? > > Since its a release branch do I need to leave it to the sheriffs? > > There is no process, go ahead and push. It's better to not include the bug# > if possible. (I meant for the try server)

Comment on attachment 8671939 [details] [diff] [review] P1 Return the correct Response type from redirected fetch() requests. r=nsm r=sicking r=ehsan I've been told not to request sec-approval since this isn't landing on trunk.

After this lands on the release branches I want to land P2 on trunk/aurora. Marking this leave-open until I can do that.

Let's use in-testsuite? to indicate that the test will need to land later.

Actually, I think we can reasonable land the P2 patch in separate bugs without drawing attention to this bug. I can land the one-line CORS type check in bug 1212904. I asked Josh to land the no-cors type check in bug 1173811. So I won't try to directly land any patches here on trunk/aurora.

Comment on attachment 8671939 [details] [diff] [review] P1 Return the correct Response type from redirected fetch() requests. r=nsm r=sicking r=ehsan Now I'm told sec-approval *is* needed. For P1 to P5. [Security approval request comment] How easily could an exploit be constructed based on the patch? Probably fairly easily. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? The tests clearly do. The code itself is a bit harder, but its clearly dealing with tainting and redirects. It would probably not take long for someone to figure out. Which older supported branches are affected by this flaw? 39 to 42 If not all supported branches, which bug introduced the flaw? It was introduced when fetch() was released in 39 and fixed in 43 by bug 1184607. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? These patches apply cleanly to beta. P3 requires a very small and easy rebase to apply to release. How likely is this patch to cause regressions; how much testing does it need? Regressions would be limited to fetch() as its the only affected released feature on these branches. We have decent mochitest coverage, though, so I am optimistic nothing has regressed.

(In reply to Ehsan Akhgari (don't ask for review please) from comment #29) > (In reply to Ben Kelly [:bkelly] from comment #27) > > Note, I have done local testing, but not pushed this patch queue to try. > > > > What is the process for actually pushing these patches if they are approved? > > Since its a release branch do I need to leave it to the sheriffs? > > There is no process, go ahead and push. It's better to not include the bug# > if possible. For "Try" just be careful. Don't use a revealing check-in comment, try not to have obvious comments in the code, and avoid using a bug number that people can use to test whether the bug is accessible or a hidden security bugs. For the actual check-in of course we need the bug number for future code inspection, and part of the sec-approval decision is weighing the appropriate time to land vs. the discoverability. (In reply to Ben Kelly [:bkelly] from comment #36) > Now I'm told sec-approval *is* needed. Absolutely! Even _more_ important on non-trunk because there's a smaller haystack making the the security check-in needles much easier to find. The closer to Release a branch is the more likely a check-in is "interesting" to potential attackers. > Do comments in the patch, the check-in comment, or tests included in the > patch paint a bulls-eye on the security problem? > > The tests clearly do. The code itself is a bit harder, but its clearly > dealing with tainting and redirects. It would probably not take long for > someone to figure out. We usually avoid checking in tests for easily exploitable bugs. Either file a follow-up bug for landing the tests (often the clearest option), or if you think you can remember to check you can set the in-testsuite flag to '?' and later on--after we ship--you can set it to in-testsuite+. The patch you're requesting approval on has no tests. Is that the only attachment you were going to check in? We need a sec-approval request on each attachment you intend to check in so we can have the best shot at judging the risk and impact of the landing. Makes it less likely for misunderstandings to happen when we try to arrange backports and such. > Which older supported branches are affected by this flaw? > > 39 to 42 I thought someone said this regressed in Firefox 41?

Comment on attachment 8671435 [details] [diff] [review] P2 Test final response types after redirect. r=sicking Please see P1 for flag details.

Comment on attachment 8671919 [details] [diff] [review] P3 Backport fetch test isSWPresent support. r=ehsan Please see P1 for flag details. This patch needs a minor re-base to apply on release branch.

Comment on attachment 8671920 [details] [diff] [review] P4 Backport hiding URL on opaque Responses. r=ehsan Please see P1 for flag details.

Comment on attachment 8671937 [details] [diff] [review] P5 Disable service worker tests that require "manual" redirect mode to work properly. r=ehsan Please see P1 for flag details.

(In reply to Daniel Veditz [:dveditz] from comment #37) > I thought someone said this regressed in Firefox 41? It regressed in 39. The outside reported found it in 41.

If we don't want to land tests for this bug, I think all we need are P1, P4, and P5. P5 is a test change, but its disabling tests that will break in automation once we land P1.

I'm going to move P2 off into a separate bug. I'll then re-number P1, P4, and P5 as the only patches to land here.

Comment on attachment 8671435 [details] [diff] [review] P2 Test final response types after redirect. r=sicking Moved to bug 1214361.

Comment on attachment 8671919 [details] [diff] [review] P3 Backport fetch test isSWPresent support. r=ehsan This patch is unnecessary if we are not landing P2.

Renumber. See P1 for flag information.

Renumber. See P1 for flag information.

This is a rebase of the P3 patch to apply to the release branch.

Please let me know if there is anything else to do here. Otherwise I will leave it to the sheriffs and release-drivers. Thanks.

Dan, Al: Could you please sec+ the patches? I would like to get the Sheriffs to uplift this to moz-release branch so I can gtb 41.0.2 tonight (if possible).

Comment on attachment 8671939 [details] [diff] [review] P1 Return the correct Response type from redirected fetch() requests. r=nsm r=sicking r=ehsan sec-approval=dveditz (you don't put this on the check-in comment though) a=dveditz for beta and release branches (this you do, as usual)

remote: https://hg.mozilla.org/releases/mozilla-beta/rev/63092955e4b7 remote: https://hg.mozilla.org/releases/mozilla-beta/rev/e8bb3ab47bdd remote: https://hg.mozilla.org/releases/mozilla-beta/rev/7c95e499110e remote: https://hg.mozilla.org/releases/mozilla-release/rev/e7f12ada1fa9 remote: https://hg.mozilla.org/releases/mozilla-release/rev/db1749ec5446 remote: https://hg.mozilla.org/releases/mozilla-release/rev/9d6f62a976c3

As far as I can tell, this issue can not be verified on Firefox for Android

(In reply to Mihai Pop from comment #54) > As far as I can tell, this issue can not be verified on Firefox for Android Do you mean its not fixed or that we lack steps to verify its fixed? Do we need a test server to verify this locally on a flashed device? I could try to write something up. Unfortunately the requirement for a same-origin URL that redirects to a cross-origin URL makes it hard to test this just in the console.

(In reply to Ben Kelly [:bkelly] from comment #55) > (In reply to Mihai Pop from comment #54) > > As far as I can tell, this issue can not be verified on Firefox for Android > > Do you mean its not fixed or that we lack steps to verify its fixed? > > Do we need a test server to verify this locally on a flashed device? I > could try to write something up. > > Unfortunately the requirement for a same-origin URL that redirects to a > cross-origin URL makes it hard to test this just in the console. Sorry for not being clear. I don't know what steps should I perform to verify this issue on Firefox for Android.

Ok. I will attempt to create a server for local testing.

Here is a small python server and html file for local testing. Please, do not put this on people.mozilla.org or any other publicly accessible server. To use: 1) unzip localtest.zip 2) cd localtest 3) python redirect.py 4) open "http://localhost:8000/index.html" Existing versions of release will show "basic" on the screen. If fixed, the screen should show "opaque" instead. For android you should be able to use the IP of a machine to reach a local machine running this server over wifi.

Thanks for the quick test. Verified this on Nexus 6 (Nexus 5.1.1) on Firefox 42 Beta 6(affected) and Firefox 42.0.1(fixed) On Firefox 42 Beta 6 "basic" is displayed in browser On Firefox 42.0.1 "opaque" is displayed in browser So based on the previous comment, this is verified as fixed on Firefox 42.0.1

Followups to truly skip tests on release, since apparently release_build won't be defined until 42: https://hg.mozilla.org/releases/mozilla-release/rev/c6fe9707843a https://hg.mozilla.org/releases/mozilla-release/rev/2c279baf0907