Closed Bug 1542194 (CVE-2021-23969) Opened 3 years ago Closed 10 months ago

CSP violation information contains URL of redirect started from client-side code

Categories

(Core :: DOM: Security, defect, P1)

68 Branch
defect

Tracking

()

VERIFIED FIXED
87 Branch
Tracking Status
firefox-esr78 86+ verified
firefox85 --- wontfix
firefox86 + verified
firefox87 + verified

People

(Reporter: masatokinugawa, Assigned: ckerschb)

References

Details

(Keywords: csectype-sop, sec-high, Whiteboard: [domsecurity-active][sec-survey][adv-main86+][adv-esr78.8+])

Attachments

(3 files, 2 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:68.0) Gecko/20100101 Firefox/68.0

Steps to reproduce:

  1. Navigate to https://l0.cm/fx_csp_violation_leak.html . This page has the following CSP and CSP violation event listener:
<meta http-equiv="Content-Security-Policy" content="default-src 'self' 'unsafe-inline' vulnerabledoma.in">
document.addEventListener('securitypolicyviolation', e => {
        console.dir(e);
        alert(e.blockedURI);
    });

In addition, there are three buttons to embed other origin's pages into an iframe. These buttons are for testing the following cases.

TEST 1: Navigate to domain not allowed by CSP via 302 redirect
TEST 2: Navigate to domain not allowed by CSP via JavaScript
TEST 3: Navigate to domain not allowed by CSP via link

  1. Click on the "TEST 1" button. "https://vulnerabledoma.in/fx_csp_violation_leak_302.php" is popped up. (= Safe. I think this behavior comes from a fix of bug 1069762 )

  2. Click on the "TEST 2" button. "https://www.vulnerabledoma.in/path?query" is popped up. (= leak of redirect URL )

  3. Click on the "TEST 3" button and then click on the displayed link. "https://www.vulnerabledoma.in/path?query" is popped up. (= leak of redirect URL )

For your information, also the behavior of "TEST 1" is different from other browsers. Other browsers return "https://www.vulnerabledoma.in".

Actual results:

The CSP violation event object ( and CSP violation report ) contains the full redirect URL if the violation happens via a client-side navigation.

Expected results:

The CSP violation event object ( and CSP violation report ) should not contain the full redirect URL in the case of navigation started from a client-side code also. This is because the redirect URL might have sensitive information.

Group: firefox-core-security → dom-core-security
Component: Untriaged → DOM: Security
Product: Firefox → Core

I assume the information sent to report-uri is the same.

We seem to be calling StripURIForReporting() so not sure how these are leaking through.

Actually, though, if we navigate inside the frame, shouldn't it be the frame's CSP that matters, not the parent's? TEST1 is fine either way (maybe we should strip more?) but TEST2 and TEST3 are a different kind of thing. I guess it must be correct because our blocking behavior does do what Chrome does and the reporter expects, it's just the reporting that's wrong.

Baku: you did the violation events -- can you take a look?

Assignee: nobody → amarchesini
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(amarchesini)
Priority: -- → P1
Whiteboard: [domsecurity-active]

I don't see where the spec says that we should do more than stripping the URI ref.
ckerschb, do you know more?

Flags: needinfo?(amarchesini) → needinfo?(ckerschb)

(In reply to Andrea Marchesini [:baku] from comment #2)

I don't see where the spec says that we should do more than stripping the URI ref.
ckerschb, do you know more?

I chatted with Dan, he offered to provide some guidance for this bug - 302 the ni? to him!

Flags: needinfo?(ckerschb) → needinfo?(dveditz)

I don't see where the spec says that we should do more than stripping the URI ref.

For a URL the page already knows about that's all we need to do.

In other cases https://w3c.github.io/webappsec-csp/#create-violation-for-global says

Note: User agents need to ensure that the source file is the URL requested by the page, pre-redirects. If that’s not possible, user agents need to strip the URL down to an origin to avoid unintentional leakage.

So there are two valid choices: the URL of the frame (as we do in case 1) or "https://www.vulnerabledoma.in" stripped to just the origin (as Chrome does in all 3 cases). Given a choice I'd prefer the first option because then the page author at least has some idea where the violation came from, but if necessary the stripped origin is acceptable.

Valid according to the current not-final version of the spec, anyway. I'm not sure I'm happy about that option because it's still leaking an origin. So maybe it's only a small bit of information, but for example imagine a frame that asks the user to log in using their choice of several SSO sites, a violation event could leak which service the user prefers.

If you do try to do the first option make sure to use the src attribute from the iframe element rather than the source URL of the current non-blocked frame document. We have to be careful of the case where a whitelisted frame src A navigates to another whitelisted src B (unknown to the parent page), which then tries to load something C that is blocked -- don't want to report B back to the parent because that leaks knowledge of where the frame navigated. If we report only the origin then B isn't technically worse than C (both leak an origin), but more confusing because B wasn't what got blocked.

Flags: needinfo?(dveditz)
See Also: → CVE-2021-23968

I don't know why I called it sec-moderate -- this can leak highly sensitive cross-origin information like OAuth tokens

Keywords: sec-moderatesec-high

Here's what tipped it from moderate to high for me: TEST1 would clearly be sec-high if we failed it. TEST3 seems pretty clearly sec-moderate because you've loaded an allowed domain and it doesn't leak origin until the user interacts (still bad, but somewhat mitigated!). TEST2 is the tipping point. I think I considered it more like TEST3 originally, but I can imagine scripted "redirects" might be more common on today's script-heavy sites, rather than using a 302.

(In reply to Andrea Marchesini [:baku] from comment #2)

I don't see where the spec says that we should do more than stripping the URI ref.

See step 3 and note of §2.4.2. Create a violation object for request, and policy.

  1. Set violation’s resource to request’s url.

Note: We use request’s url, and not its current url, as the latter might contain information about redirect targets to which the page MUST NOT be given access.

Assignee: amarchesini → ckerschb
Status: NEW → ASSIGNED

Chrome and Safari consistently show the destination origin and strip the path which is wrong; §2.4.2 seems pretty clear on the point. This wrongness has gotten baked into the WPT tests. Rather, Near as I can figure the Chrome and Safari behavior was influenced by $2.4.1, which is about script violations, and is supposed to apply to the reported source file, not the reported resource. And even then it's in a dodgy note.

  1. If the user agent is currently executing script, and can extract a source file’s URL, line number, and column number from the global, set violation’s source file, line number, and column number accordingly.

Note: User agents need to ensure that the source file is the URL requested by the page, pre-redirects. If that’s not possible, user agents need to strip the URL down to an origin to avoid unintentional leakage.

Elsewhere the spec is careful to forbid comparing whitelisted paths on redirects because some services redirect logged-in users to their home directory (e.g. facebook.com -> www.facebook.com/bobbytables). But many services are using subdomains for added isolation (e.g. github.io) so it's easy to imagine a redirect that still leaks interesting bits if you follow the Chrome and Safari behavior, e.g. facebook.com -> bobbytables.facebook.com).

In other words

  1. Chrome and Safari are not following §2.4.2 correctly
  2. The WPT test for this is testing for the broken behavior
  3. The spec's "if that's not possible" bit in §2.4.1 is dangerous and should be reconsidered.
  4. [this bug] Firefox is also wrong, but in a completely different--and arguably worse--way

Bug 1542194: Update blocked-uri in CSP reporting by treating frame naviations as redirects

Attachment #9200480 - Attachment is obsolete: true

Comment on attachment 9200481 [details]
Bug 1542194: Update blocked-uri in CSP reporting by treating frame naviations as redirects

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: I think it's fairly straight forward for someone familiar with CSP to identify the problem by looking at the patch.

FWIW, this bug (similar to Bug 1687342) was introduced with the CSP reporting mechanism back in the days.

  • Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
  • Which older supported branches are affected by this flaw?:
  • If not all supported branches, which bug introduced the flaw?: None
  • Do you have backports for the affected branches?: Yes
  • 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?: Not very likely, we have automated tests. On a scale from 1-5 it would be 1. Since it really only affects the CSP reporting mechanism.
Attachment #9200481 - Flags: sec-approval?
Attachment #9200038 - Flags: sec-approval?

Comment on attachment 9200481 [details]
Bug 1542194: Update blocked-uri in CSP reporting by treating frame naviations as redirects

Approved to land and request uplift from Relman. Test should land after 4/1

Attachment #9200481 - Flags: sec-approval? → sec-approval+

Comment on attachment 9200038 [details]
Bug 1542194: Test blockedURI in CSP violation events in certain redirect scenarios

Test should land after 4/1

Attachment #9200038 - Flags: sec-approval? → sec-approval+
Flags: in-testsuite?

Comment on attachment 9200481 [details]
Bug 1542194: Update blocked-uri in CSP reporting by treating frame naviations as redirects

Beta/Release Uplift Approval Request

  • User impact if declined: See https://bugzilla.mozilla.org/show_bug.cgi?id=1542194#c11
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky):
  • String changes made/needed:

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: see https://bugzilla.mozilla.org/show_bug.cgi?id=1542194#c11
  • User impact if declined:
  • Fix Landed on Version:
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky):
  • String or UUID changes made by this patch:
Attachment #9200481 - Flags: approval-mozilla-esr78?
Attachment #9200481 - Flags: approval-mozilla-beta?
Attachment #9200038 - Flags: approval-mozilla-beta? approval-mozilla-esr78?

(In reply to Sebastian Hengst [:aryx] (needinfo on intermittent or backout) from comment #16)

Backed out for xpcshell failures on test_ext_contentscript_triggeringPrincipal.js.

Investigating ...

Flags: needinfo?(ckerschb)

Hey Kris, within this bug we are updating the value within the report-uri for frame navigations and that change causes problems for the test setup within test_ext_contentscript_triggeringPrincipal.js.

In particular, the patch causes us to truncate the blocked-uri to the prePath so the following loads:

get truncated to http://example.com.

The test setup is massive and a rewrite of the test setup seems complicated. Given that we want to uplift the change, I was wondering what your take is. One option would be to introduce a pref in the code which enforces the new behavior by default but would allow us to flip it explicitly for the test. I personally think that's the best way to go, but wanted to hear your opinion too.

Flags: needinfo?(kmaglione+bmo)

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #18)

The test setup is massive and a rewrite of the test setup seems complicated. Given that we want to uplift the change, I was wondering what your take is. One option would be to introduce a pref in the code which enforces the new behavior by default but would allow us to flip it explicitly for the test. I personally think that's the best way to go, but wanted to hear your opinion too.

I think that's probably the best option, yeah. It doesn't affect the integrity of the test, and it sounds like the simplest option. In any case, I can't think of anything better.

Flags: needinfo?(kmaglione+bmo)

Update blocked-uri in CSP reporting by treating frame naviations as redirects r=freddyb,dveditz,mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/3d8e81530af6a465731c4dbcc9aeb550da18396b

Group: dom-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → 87 Branch
Attachment #9200038 - Flags: approval-mozilla-esr78?
Attachment #9200038 - Flags: approval-mozilla-beta?

Comment on attachment 9200481 [details]
Bug 1542194: Update blocked-uri in CSP reporting by treating frame naviations as redirects

Approved for our last beta, thanks.

Attachment #9200481 - Flags: approval-mozilla-esr78?
Attachment #9200481 - Flags: approval-mozilla-esr78+
Attachment #9200481 - Flags: approval-mozilla-beta?
Attachment #9200481 - Flags: approval-mozilla-beta+
QA Whiteboard: [post-critsmash-triage]
Flags: qe-verify+
QA Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][qa-triaged]

Reproduced the issue on affected Firefox Release 85.0.2, "https://www.vulnerabledoma.in/path?query" is popped up for Test2 and Test 3.

Verified-fixed on latest Firefox Nightly 87.0a1 (2021-02-15) (64-bit) and Firefox Beta 86.0b9 on Windows 10 x64, MacOS 10.15 and Ubuntu 20.06 with the following results:

Waiting for fix to land in ESR for further verification.

Verified-fixed on the latest ESR 78.8.0esr (64-bit)(20210217034806) version on Windows 10 x64 and MacOS 10.15.

Status: RESOLVED → VERIFIED
QA Whiteboard: [post-critsmash-triage][qa-triaged] → [post-critsmash-triage]
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?(ckerschb)
Whiteboard: [domsecurity-active] → [domsecurity-active][sec-survey]
Whiteboard: [domsecurity-active][sec-survey] → [domsecurity-active][sec-survey][adv-main86+]

(In reply to Release mgmt bot [:sylvestre / :calixte / :marco for bugbug] from comment #27)

Please visit this google form to reply.

Done!

Flags: needinfo?(ckerschb)
Whiteboard: [domsecurity-active][sec-survey][adv-main86+] → [domsecurity-active][sec-survey][adv-main86+][adv-esr78.8+]
Flags: sec-bounty?
Flags: sec-bounty? → sec-bounty+

Note that we set the bounty flag on this bug; but in general if the flag is not set we may not notice it, so please ask to have the flag set when appropriate.

Test blockedURI in CSP violation events in certain redirect scenarios r=dveditz,freddyb
https://hg.mozilla.org/integration/autoland/rev/76d749938fd3
https://hg.mozilla.org/mozilla-central/rev/76d749938fd3

Flags: in-testsuite? → in-testsuite+
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.