1542194 - (CVE-2021-23969) CSP violation information contains URL of redirect started from client-side code

Status: VERIFIED FIXED

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

Description

[Masato Kinugawa]

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

2. 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 )

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

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

Comment 1

[Daniel Veditz [:dveditz]]

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?

Comment 2

[Andrea Marchesini [:baku]]

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

Comment 3

[Christoph Kerschbaumer [: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!

Comment 4

[Daniel Veditz [: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.

Comment 5

[Daniel Veditz [:dveditz]]

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

Comment 6

[Daniel Veditz [:dveditz]]

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.

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

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Attachment: Bug 1542194: Test blockedURI in CSP violation events in certain redirect scenarios

Comment 8

[Daniel Veditz [:dveditz]]

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.

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

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Attachment: Bug 1542194: Test blockedURI in CSP violation events in certain redirect scenarios

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

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

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.

Comment 12

[Tom Ritter [:tjr]]

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

Comment 13

[Tom Ritter [:tjr]]

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

Test should land after 4/1

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

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:

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/autoland/rev/0abd871c49201221e80bbbfb62043219f796d4e5

Comment 16

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

Backed out for xpcshell failures on test_ext_contentscript_triggeringPrincipal.js.

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=0abd871c49201221e80bbbfb62043219f796d4e5&searchStr=xpc&selectedTaskRun=QYs1jWtjSl6OQSWsH6OeYw.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=329396435&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/f9fe7a3b62048942a935df05d0c246b43ec094cf

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 18

[Christoph Kerschbaumer [: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:

• http://example.com/iframe.html?origin=page&source=contentScript-content-attr-after-inject
• http://example.com/iframe.html?origin=page&source=pageScript-attr-after-inject
• http://example.com/iframe.html?origin=page&source=pageScript-prop-after-inject

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.

Comment 19

[Kris Maglione [:kmag]]

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

Comment 20

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

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

Comment 21

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

https://hg.mozilla.org/mozilla-central/rev/3d8e81530af6

Comment 22

[Pascal Chevrel:pascalc]

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.

Comment 23

[Julien Cristau [:jcristau]]

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

Comment 24

[Timea Cernea [:tbabos][inactive]]

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:

• Test1: "https://vulnerabledoma.in/fx_csp_violation_leak_302.php" is popped up.
• Test2: "https://www.vulnerabledoma.in/" is popped up
• Test3: "https://www.vulnerabledoma.in/" is popped up

Waiting for fix to land in ESR for further verification.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/748f749fda1d
https://hg.mozilla.org/releases/mozilla-esr78/rev/70b9086b702c

Comment 26

[Timea Cernea [:tbabos][inactive]]

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

Comment 27

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

[Christoph Kerschbaumer [:ckerschb]]

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

Please visit this google form to reply.

Done!

Comment 29

[Tom Ritter [:tjr]]

Attachment: advisory.txt

Comment 30

[Tom Ritter [:tjr]]

Attachment: advisory.txt

Comment 31

[Tom Ritter [:tjr]]

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.

Comment 32

[Julien Cristau [:jcristau]]

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