1909298 - (CVE-2024-7525) webRequest.filterResponseData does not require origin permissions

Status: RESOLVED FIXED

(WebExtensions :: Request Handling, defect, P1)

Description

[Rob Wu [:robwu]]

Attachment: streamfilter-permission-check.zip

webRequest.filterResponseData creates a StreamFilter which can be used to read and modify a response body of a matching request. The only required input is a valid requestId. These requestIds are sequential numbers and thus predictable. requestId are also constant across redirects, so an extension can also just trigger an initial innocent request, intercept + redirect that to the desired destination and attach a StreamFilter.

This bug enables an extension with minimal permissions to run code in any website, including highly-privileged domains such as addons.mozilla.org.

STR:

1. Visit about:debugging
2. Load the attached zip file. It is an extension with the webRequest, webRequestBlocking and example.com permissions, that redirects https://example.com/redir/<url> to <url> and tries to attach a StreamFilter to <url>.
3. Click on the extension button. It navigates to https://example.com/redir/https://addons.mozilla.org/en-US/firefox/.
4. Wait for the page to load (the extension will redirect to https://addons.mozilla.org/en-US/firefox/ and try to attach a stream filter).

Expected:

• The extension should not be able to modify the response body of addons.mozilla.org, and in particular not be able to run scripts there.

Actual:

• I see a dialog "Script executed at origin https://addons.mozilla.org, at https://addons.mozilla.org/en-US/firefox/".

Comment 1

[Andrew McCreight [:mccr8]]

Normally we'd mark things requiring an addon as sec-moderate, but the level of exploitability seems high so I'll mark it sec-high for now. Maybe that's not right.

Comment 2

[Rob Wu [:robwu]]

Attachment: Bug 1909298 - Confirm permission in webRequest

Comment 3

[Rob Wu [:robwu]]

Attachment: Bug 1909298 - Test webRequest.getSecurityInfo() without permission

Comment 4

[Rob Wu [:robwu]]

I attached two patches: one with a fix, and one test case.

The patch introduces a permission check in ChannelWrapper::GetTraceableChannel, which is the common method with the following (exhaustive) list of callers:

• StreamFilterParent::Create - which is the security issue I reported.
• ChannelWrapper::GetRegisteredChannel - which has the following callers:
  • webRequest.getSecurityInfo implementation - this could leak some certificate information (also after a redirect).
  • Built-in extensions:
    • search-detection - the worst case is that a URL is not erased from an in-memory map for redirects from search engines.
    • webcompat (and webcompat in Fenix - already has null checks.

The unit test confirms that validation works, through the webRequest.getSecurityInfo method. This method enables a more deterministic way to trigger the permission check, than webRequest.filterResponseData. I think that it is acceptable to land that unit test together with the patch itself, because the test is written such that it does not offer any hints on how to exploit this issue in the worst way (i.e. the PoC from this report).

Comment 5

[Rob Wu [:robwu]]

Attachment: Bug 1909298 - Expand test coverage for StreamFilter

Comment 6

[Rob Wu [:robwu]]

The third patch above (D217578) has a test case that resembles the manual PoC (but rewritten to be fully deterministic). That patch should NOT land until several releases in the future because it is an effective PoC of this bug.

The first two patches (D217449 and D217450) are good to land soon in central (and uplift to beta, ESR128 and ESR115).

Comment 7

[Rob Wu [:robwu]]

Comment on attachment 9414383 [details]
Bug 1909298 - Confirm permission in webRequest

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Not easily. The key to exploitation is a redirect and attaching a StreamFilter after the redirect. The potential relevance of StreamFilter could be inferred by looking at the diff and/or tracing the callers of the patched method (e.g. as done in https://bugzilla.mozilla.org/show_bug.cgi?id=1909298#c4). The patches (D217449 and D217450) do not mention redirects at all.

The unit test (D217450) creates an artificial scenario that is enough to verify the fix, but without much security impact (other than information leakage of any website's TLS certificate information).

• 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 branches (beta, release, and/or ESR) are affected by this flaw, and do the release status flags reflect this affected/unaffected state correctly?: all branches
• If not all supported branches, which bug introduced the flaw?: bug 1255894 (all the way back to Firefox 57)
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: All patches apply cleanly to the Beta, ESR115 and ESR128 branches.
• How likely is this patch to cause regressions; how much testing does it need?: Unlikely. This introduced validation is a subset of the checks that are run before an extension can see the request at all, and the only change in behavior is the fix for the security bug.
• Is the patch ready to land after security approval is given?: Yes
• Is Android affected?: Yes

Comment 8

[Rob Wu [:robwu]]

Comment on attachment 9414384 [details]
Bug 1909298 - Test webRequest.getSecurityInfo() without permission

sec-approval form in comment 7.

Comment 9

[Rob Wu [:robwu]]

Comment on attachment 9414383 [details]
Bug 1909298 - Confirm permission in webRequest

Beta/Release Uplift Approval Request

• User impact if declined: Security bug, see https://bugzilla.mozilla.org/show_bug.cgi?id=1909298#c7
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce: comment 0
• List of other uplifts needed: bug 1910110 (D217822) after the ones from this bug.
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Adds missing access check to webRequest code with no side effects.
• String changes made/needed: None
• Is Android affected?: Yes

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Security bug, see https://bugzilla.mozilla.org/show_bug.cgi?id=1909298#c7
• User impact if declined: Security bug, comment 0
• Fix Landed on Version: (will land in 130, pending uplift to 129 beta)
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Adds missing access check to webRequest code with no side effects.

Comment 10

[Rob Wu [:robwu]]

Comment on attachment 9414384 [details]
Bug 1909298 - Test webRequest.getSecurityInfo() without permission

Beta/Release Uplift Approval Request

• User impact if declined: None. This is a test case to verify the fix to the security bug, without putting a bulls eye on the vulnerability. See https://bugzilla.mozilla.org/show_bug.cgi?id=1909298#c7
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: bug 1910110 (D217822) after the ones from this bug.
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Test-only.
• String changes made/needed: None
• Is Android affected?: Yes

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined: None. This is a test case to verify the fix to the security bug, without putting a bulls eye on the vulnerability. See https://bugzilla.mozilla.org/show_bug.cgi?id=1909298#c7
• Fix Landed on Version: (will land in 130, pending uplift to 129 beta)
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Test-only.

Comment 11

[Daniel Veditz [:dveditz]]

Comment on attachment 9414383 [details]
Bug 1909298 - Confirm permission in webRequest

Thank you for the thoughtful way you wrote and split up the tests

sec-approval+ = dveditz

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3624ccf6ed19
Confirm permission in webRequest r=rpl
https://hg.mozilla.org/integration/autoland/rev/478ac5e5aac1
Test webRequest.getSecurityInfo() without permission r=rpl

Comment 13

[Ryan VanderMeulen (PTO, back 6-April)]

https://hg.mozilla.org/mozilla-central/rev/3624ccf6ed19
https://hg.mozilla.org/mozilla-central/rev/478ac5e5aac1

Comment 14

[Rob Wu [:robwu]]

The patch caused a top Nightly crash (bug 1910110), so it will be backed out.

The cause of it is that under some circumstances, FinalURLInfo() can end up being nullptr (which is undesired, but theoretically possible, and as evidenced by the crashes also possible in practice). I'm going to update the patch to add a null check.

Comment 15

[Rob Wu [:robwu]]

Instead of backing out, the sheriffs accepted a patch to fix up the issue. It is attached in bug 1910110 and should also be included when merging with beta, ESR115 and ESR128.

Comment 16

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/51bfb678c1ef
https://hg.mozilla.org/releases/mozilla-beta/rev/4e1d05d3bcb4

Comment 17

[Donal Meehan [:dmeehan]]

Comment on attachment 9414383 [details]
Bug 1909298 - Confirm permission in webRequest

Approved for 128.1esr

Comment 18

[Donal Meehan [:dmeehan]]

Comment on attachment 9414384 [details]
Bug 1909298 - Test webRequest.getSecurityInfo() without permission

Approved for 128.1esr

Comment 19

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/9e6c92fef3a2
https://hg.mozilla.org/releases/mozilla-esr128/rev/5d15642e59a9

Comment 20

[Donal Meehan [:dmeehan]]

Comment on attachment 9414384 [details]
Bug 1909298 - Test webRequest.getSecurityInfo() without permission

Approved for 115.14esr

Comment 21

[Donal Meehan [:dmeehan]]

Comment on attachment 9414383 [details]
Bug 1909298 - Confirm permission in webRequest

Approved for 115.14esr

Comment 22

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/dc8255b8b3f8
https://hg.mozilla.org/releases/mozilla-esr115/rev/2a9de7cd569a

Comment 23

[Daniel Veditz [:dveditz]]

Rob: On the assumption that the [reminder-landing] whiteboard tag was about the tests (since you've already landed the fix) I've updated the tag to [reminder-test]. Please correct if that's not right and you meant something else. January 2025 seems a long way out though. Normally we'd only ask you to wait 6-8 weeks after we ship the fix (e.g. end of September in this case).

Comment 24

[Rob Wu [:robwu]]

(In reply to Daniel Veditz [:dveditz] from comment #23)

Rob: On the assumption that the [reminder-landing] whiteboard tag was about the tests (since you've already landed the fix)

Yes this was about landing the test (third patch). I set a date in the far future, with the intend to land the test when the bug becomes public or the deadline is reached, whichever comes first.

I've updated the tag to [reminder-test].

I did not know about reminder-test. I suppose that there is no automatic notification, but that it can be used to query bugs where the test has not landed yet. If it is something established, it would be nice to document it at https://firefox-source-docs.mozilla.org/bug-mgmt/processes/security-approval.html

Comment 25

[Tyson Smith [:tsmith]]

Attachment: advisory.txt

Comment 26

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

5 months ago, dveditz placed a reminder on the bug using the whiteboard tag [reminder-test 2025-01-08] .

robwu, please refer to the original comment to better understand the reason for the reminder.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c40882b5fac2
Expand test coverage for StreamFilter r=rpl

Comment 28

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

https://hg.mozilla.org/mozilla-central/rev/c40882b5fac2