Closed Bug 1645204 (CVE-2020-15655) Opened 1 year ago Closed 1 year ago

bypassCORSChecks is not cleared after a redirect

Categories

(Core :: Networking: HTTP, defect)

defect

Tracking

()

VERIFIED FIXED
mozilla80
Tracking Status
firefox-esr68 --- unaffected
firefox-esr78 79+ verified
firefox77 --- wontfix
firefox78 - wontfix
firefox79 + verified
firefox80 + verified

People

(Reporter: robwu, Assigned: sstreich, NeedInfo)

References

(Regression)

Details

(Keywords: csectype-sop, regression, sec-high, Whiteboard: [post-critsmash-triage][sec-survey][adv-main79+][adv-ESR78.1+])

Attachments

(6 files, 1 obsolete file)

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:

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.

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

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

Group: core-security → network-core-security

bug 1450965 introduced this, could you look into it?

Flags: needinfo?(sstreich)
Flags: needinfo?(honzab.moz)

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.
Flags: needinfo?(honzab.moz)

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.

(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.

  • Backed out changeset 308ef98e6e4b
  • Re-Added the old Unit Test
  • Add Honza Bambas (:mayhemer) solution
Assignee: nobody → sstreich
Status: NEW → ASSIGNED

[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?

Flags: needinfo?(tom)

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

Flags: needinfo?(tom)
Keywords: sec-high
Attachment #9157027 - Attachment description: Bug 1645204 - Add Test r=ckerschb → Bug 1645204 - Patch 3: Add Test for Cors Bypass r=ckerschb

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".

Flags: needinfo?(sstreich)
Attachment #9159056 - Attachment description: Bug 1645204 - Patch 1: Fix WebRequest.jsm r=robwu → Bug 1645204 - Fix WebRequest.jsm r=robwu
Attachment #9159057 - Attachment description: Bug 1645204 -Patch 2: Add Unit Test for CORS redirect headers r=robwu → Bug 1645204 - Add Unit Test for CORS redirect headers r=robwu

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.

Attachment #9159059 - Attachment is obsolete: true
Attachment #9157026 - Attachment is obsolete: true

(copied csectype from bug 1648445)

Keywords: csectype-sop

Is this ready for a sec-approval request?

Flags: needinfo?(sstreich)

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.

(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 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.

Attachment #9159056 - Flags: sec-approval?

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.

Attachment #9159056 - Flags: sec-approval?
Attachment #9159056 - Flags: sec-approval+
Attachment #9159056 - Flags: approval-mozilla-esr78+
Attachment #9159059 - Attachment description: Bug 1645204 - Patch 4: Remove unused bypassCORSChecks flags r=robwu → Bug 1645204 - Remove bypassCORSChecks flags r=robwu
Attachment #9159059 - Attachment is obsolete: false
Duplicate of this bug: 1648445

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?

Flags: needinfo?(sstreich) → needinfo?(ryanvm)

Go ahead and land.

Flags: needinfo?(ryanvm) → needinfo?(sstreich)
Group: network-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla80
Flags: qe-verify+
Whiteboard: [post-critsmash-triage]

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.

Status: RESOLVED → VERIFIED
Flags: qe-verify+

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.

Flags: needinfo?(sstreich)
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][sec-survey]

filled the form :)

Flags: needinfo?(sstreich)
Whiteboard: [post-critsmash-triage][sec-survey] → [post-critsmash-triage][sec-survey][adv-main79+]
Whiteboard: [post-critsmash-triage][sec-survey][adv-main79+] → [post-critsmash-triage][sec-survey][adv-main79+][adv-ESR78.1+]
Alias: CVE-2020-15655
Group: core-security-release

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

Flags: needinfo?(sstreich)
Regressions: 1694679
Regressed by: 1450965
See Also: → 1694711
You need to log in before you can comment on or make changes to this bug.