1645204 - (CVE-2020-15655) bypassCORSChecks is not cleared after a redirect

Status: VERIFIED FIXED

(Core :: Networking: HTTP, defect)

Description

[Rob Wu [:robwu]]

I came across the bypassCORSChecks flag and given its omnious name, I wondered whether the flag is unset when the channel is redirected. Apparently it is not, and as a result, the response of the redirect can be read by any website!
By "response", I don't just mean the immediate response; if an attacker is able to change any URL in the redirect chain, then they can read data from any other website, with cookies if desired.

This flag is only used by extension code that handles redirects, so in order to be vulnerable to the bug, a user should install an extension that performs a redirect.

STR:

1. Install HTTPS Everywhere - https://addons.mozilla.org/en-US/firefox/addon/https-everywhere/
   (or the attached minimal extension, in case HTTPS Everywhere version 2020.5.20 becomes unavailable)
2. Visit example.com and open the developer tools
3. Run await fetch("http://talkback-public.mozilla.org")
   (or http://live.com if you want to see that the flag persists across server-side redirects).

Expected:

• Request should fail, because the remote resource doesn't respond with CORS headers:
  Cross-Origin Request Blocked: The Same Origin Policy disallows reading the remote resource at http://talkback-public.mozilla.org/. (Reason: CORS request did not succeed).

Actual:

• Request succeeds, I am getting the response from https://crash-stats.mozilla.org/ because of this bug, triggered by the fact that HTTPS Everywhere has defined a redirect rule from talkback-public.mozilla.org to crash-stats.mozilla.org.

Regressed by bug 1450965. Fortunately this was in 69, so ESR68 is not affected. But the upcoming ESR78 will be affected unless this bug is fixed and uplifted. Since HTTPS Everywhere is installed by default in the Tor browser (based on ESR), this bug should be fixed ASAP.

Comment 1

[Rob Wu [:robwu]]

Attachment: webRequest-redirect-once.zip

This is a minimal extension that triggers the bug (to reproduce/verify the bug in case HTTPS Everywhere changes).

Comment 2

[Rob Wu [:robwu]]

Given the security impact I'd rate it P1, S1, sec-high, but I'll leave that to the network and security people.

Comment 3

[Shane Caraveo (:mixedpuppy)]

bug 1450965 introduced this, could you look into it?

Comment 4

[Honza Bambas (:mayhemer)]

To resummarize the original problem we wanted to fix with bug 1450965:

• example.com has a button to fetch() data from foo.com
• there is an extension that redirects any request to foo.com to example.com
  • internally it means that there is a request to foo.com which we redirect to example.com before even making the request to foo.com; this goes through a cors check and fails it because of missing Access-Control-Allow-Origin on (the never happening) response to foo.com.

Hence, we added a default trust for extensions to allow redirect of any request to any other cross-origin.

I think dropping the flag on next redirect hop is not enough to fix this.

Possible options:

1. do the cross-origin request (to foo.com in the above example), perhaps just a HEAD, and use the Access-Control-Allow-Origin. But, some extensions may use a redirect like this block the request from even happening to e.g. not leak private data. We would break it.
2. some other more explicit mechanism (e.g. a cross-origin check API that extensions may hook into to overrule cors checks for trusted set of URIs), not backward compatible.
3. we allow the bypassCORSChecks to have its effect only when the target is subsumed by the triggering principal.

Comment 5

[Rob Wu [:robwu]]

Is it possible to fake a response without reaching the server? Something like this (intentionally 307 instead of 302/303 to preserve the request method).

HTTP/1.0 307 Internal Redirect
Access-Control-Allow-Origin: <origin here>
Access-Control-Allow-Credentials: true
Location: <redirectUrl here>

Edit: include CORS headers of course.

Comment 6

[Honza Bambas (:mayhemer)]

(In reply to Rob Wu [:robwu] from comment #5)

Is it possible to fake a response without reaching the server? Something like this (intentionally 307 instead of 302/303 to preserve the request method).

HTTP/1.0 307 Internal Redirect
Access-Control-Allow-Origin: <origin here>
Access-Control-Allow-Credentials: true
Location: <redirectUrl here>

Edit: include CORS headers of course.

That is what the redirectTo() API exactly does. And my first patch to fix this was about adding the Access-Control-Allow-Origin: <origin here> "response" header. We may rather go that way and back the current solution out.

Comment 7

[Sebastian Streich [:sstreich]]

Attachment: Bug 1645204 - Replace bypassCors for Extention-redirects r=ckerschb,mayhemer

• Backed out changeset 308ef98e6e4b
• Re-Added the old Unit Test
• Add Honza Bambas (:mayhemer) solution

Comment 8

[Sebastian Streich [:sstreich]]

Attachment: Bug 1645204 - Patch 3: Add Test for Cors Bypass r=ckerschb

Depends on D79859

Comment 9

[Rob Wu [:robwu]]

[Tracking Requested - why for this release]:

This bug is easily exploitable once the conditions are met, which is the case on the Tor browser.

Tom - could you assign a security rating (probably sec-high) and make sure that an ESR78-based Tor browser does NOT ship with this bug?

Comment 10

[Tom Ritter [:tjr]]

Yeah, this is sec-high because it's a cross-origin data leak requiring no more than usual browsing.

Comment 11

[Sebastian Streich [:sstreich]]

Attachment: Bug 1645204 - Fix WebRequest.jsm r=robwu

Comment 12

[Sebastian Streich [:sstreich]]

Attachment: Bug 1645204 - Add Unit Test for CORS redirect headers r=robwu

Depends on D80953

Comment 13

[Sebastian Streich [:sstreich]]

Attachment: Bug 1645204 - Remove bypassCORSChecks flags r=robwu

Depends on D79860

Comment 14

[Rob Wu [:robwu]]

Please rename the patches by removing "Part x", in case they don't land together.

• D80953 "Patch 1" contains the fix and is the minimum that needs to land.
• D80954 "Patch 2" contains a regression test and may or may not land with the first patch (I recommend yes, to avoid regressions).
• D79860 "Patch 3" contains the PoC from my bug report. This should not land until the fix has become generally available to users.
• D80956 "Patch 4" contains a cleanup. This can land after patch 1 or 2, but at the very least before "Patch 3".

D79859 is obsolete now, please visit the revision and choose "Abandon" in the dropdown at the comment box. Make sure to remove the dependencies between the patch stack involving "patch 3" and "patch 4".

Comment 15

[Phabricator Automation]

Comment on attachment 9159059 [details]
Bug 1645204 - Remove bypassCORSChecks flags r=robwu

Revision D80956 was moved to bug 1648445. Setting attachment 9159059 [details] to obsolete.

Comment 16

[Julien Cristau [:jcristau]]

Let's aim to get this into 79.

Comment 17

[Rob Wu [:robwu]]

(copied csectype from bug 1648445)

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Is this ready for a sec-approval request?

Comment 19

[Rob Wu [:robwu]]

dragana asked a question in https://phabricator.services.mozilla.com/D80953#2485909 , but it is not clear whether that is blocking. The network changes were copied from the original patch in D79859, all of which were approved, including r=necko-reviewers.

IMO the fix itself (D80953) is ready, but an explicit sign-off from a necko peer wouldn't hurt (even if only to confirm that the r+ from the original patch still stands).

Edit: and the functional tests from D80954 should have a small fixup following my review comments, and then land together with the actual fix, to make sure that there are no functional regressions.

Comment 20

[:Gijs (he/him)]

(In reply to Rob Wu [:robwu] from comment #19)

Edit: and the functional tests from D80954 should have a small fixup following my review comments, and then land together with the actual fix, to make sure that there are no functional regressions.

This is a sec-high, we don't normally land tests together with the change unless they're required to keep the tree green...

Comment 21

[Rob Wu [:robwu]]

Comment on attachment 9159056 [details]
Bug 1645204 - Fix WebRequest.jsm r=robwu

Security Approval Request

• How easily could an exploit be constructed based on the patch?: While the patch itself does not show any code snippet that can be used to construct an exploit, the diff itself inevitably reveals that there is a vulnerability in the handling of the redirectUrl part of the webRequest API, and that it relates to the bypassCORSChecks member.

  To exploit the vulnerability, an attacker needs to figure out that they have to control the redirect target. This part is not blatantly obvious from the patch.
  But anyone who is familiar with these concepts (webRequest extension API, redirectUrl and "bypass CORS checks") can put the pieces together and create an exploit themselves.

• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No

• Which older supported branches are affected by this flaw?: ESR78, release, beta

• If not all supported branches, which bug introduced the flaw?: bug 1450965

• Do you have backports for the affected branches?: Yes (patch is expectedly to apply cleanly)

• If not, how different, hard to create, and risky will they be?:

• How likely is this patch to cause regressions; how much testing does it need?: For manual verification of the fix, run the STR from the bug report.

To test for regressions, run unit tests. Apply the following test-only patches:

• D80954 - verifies that existing functionality continues to work as intended (I still have a pending request for slightly improved test coverage, but it doesn't block the fix from landing).
• D79860 - verifies that this security bug has been fixed.

... and run all unit tests that involve this feature:

./mach test toolkit/components/extensions/test/browser/browser_ext_webRequest_redirect_mozextension.js toolkit/components/extensions/test/mochitest/test_ext_redirect_jar.html toolkit/components/extensions/test/mochitest/test_ext_webrequest_basic.html toolkit/components/extensions/test/mochitest/test_ext_webrequest_redirect_bypass_cors.html toolkit/components/extensions/test/mochitest/test_ext_webrequest_redirect_data_uri.html toolkit/components/extensions/test/mochitest/test_ext_webrequest_upgrade.html toolkit/components/extensions/test/xpcshell/test_ext_file_access.js toolkit/components/extensions/test/xpcshell/test_ext_redirects.js toolkit/components/extensions/test/xpcshell/test_ext_webRequest_permission.js

This should provide all coverage that we need.

Comment 22

[Tom Ritter [:tjr]]

Comment on attachment 9159056 [details]
Bug 1645204 - Fix WebRequest.jsm r=robwu

Approved to land July 16th. We'll hold the tests until October 1.

Comment 24

[Sebastian Streich [:sstreich]]

I've rebased the patch to central and verified we pass the related tests. Since we have Sec Approval to land today, can i just mark the patch "checkin-needed" or are there additional steps i need to do to get this landed today?

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Go ahead and land.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Or I will I guess.
https://hg.mozilla.org/integration/autoland/rev/5d5fb35f83ed07f16b4ed242e7dd7d091af16748

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ba95dba0df2e

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/0fa2e6171e7d

Comment 29

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/5d5fb35f83ed

Comment 30

[Catalin Sasca, Desktop Test Engineering [:csasca]]

I was able to reproduce the issue on Firefox Nightly (2020-06-11) under macOS 10.15.6 by using the STR from Comment 0.

This issue is fixed and the correct blocking strings are thrown. Tests were performed on Firefox 78.1.0esr (treeherder build), Firefox 79.0b9 and Firefox 80.0a1 (2020-07-19) under macOS 10.15.6, Windows 10 and Ubuntu 20.04.

Comment 31

[BugBot [:suhaib / :marco/ :calixte]]

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

Comment 32

[Sebastian Streich [:sstreich]]

filled the form :)

Comment 33

[Frederik Braun [:freddy]]

Attachment: advisory.txt

Comment 34

[Rob Wu [:robwu]]

Sebastian, could you land the cleanup and unit tests? Might have had some bit rot by now.