1886108 - (CVE-2024-4769) Detect Content Script of Cross-Origin using worker load-error messages

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect, P3)

Description

[Shaheen Fazim]

Attachment: poc.html

On Firefox browser, a site could still detect content across cross-origin whether it is application/javascript using error messages. This is because of different variations of error messages. For example, if application/javascript is detected, Firefox produces the error: NetworkError: WorkerGlobalScope.importScripts: Failed to load worker script at https://www.google-analytics.com/ (nsresult = 0x80530013). If not, it produces the error: NetworkError: A network error occurred.

Steps to Reproduce:

1. Download the file poc.html.
2. Open poc.html in a Firefox browser, then copy and paste https://www.google-analytics.com/analytics.js into the input field and click "check" to see if it is detected. Repeat with just https://www.google-analytics.com to see not detected.

Comment 1

[Shaheen Fazim]

Attachment: demo.png

This issue is the same as CVE-2022-22760.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Hmm, https://searchfox.org/mozilla-central/rev/b73676a106c1655030bb876fd5e0a6825aee6044/dom/workers/ScriptLoader.cpp#1851

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

Yeah, fetch a classic worker-imported script step 7 is clear we should be throwing a NetworkError and although it's okay for us to report something more detailed for devtools to surface to the user, the NetworkError should not be surfacing something differentiable to content.

https://phabricator.services.mozilla.com/D147314 introduced ReportErrorToConsole which somewhat obscures that it's propagating an ErrorResult which will be surfaced to content, but that didn't change the error handling itself.

By inspection, the problem here seems to be that if we have an NS_ERROR_DOM_NETWORK_ERR coming in we will take the default branch which produces an error string that tunnels the nsresult

default:
  // For lack of anything better, go ahead and throw a NetworkError here.
  // We don't want to throw a JS exception, because for toplevel script
  // loads that would get squelched.
  aRv.ThrowNetworkError(nsPrintfCString(
      "Failed to load worker script at %s (nsresult = 0x%" PRIx32 ")",
      NS_ConvertUTF16toUTF8(aScriptURL).get(),
      static_cast<uint32_t>(aLoadResult)));
  return;

But in the other cases where we return NS_ERROR_DOM_NETWORK_ERR we will let ErrorResult do its thing:

case NS_ERROR_FILE_NOT_FOUND:
case NS_ERROR_NOT_AVAILABLE:
case NS_ERROR_CORRUPTED_CONTENT:
  aRv.Throw(NS_ERROR_DOM_NETWORK_ERR);
  break;

In terms of the ErrorResult, I think the right course of action is to:

• Also explicitly name NS_ERROR_DOM_NETWORK_ERR for us throwing NS_ERROR_DOM_NETWORK_ERR.
• Turn the default branch also into us throwing NS_ERROR_DOM_NETWORK_ERR.

Considerations here would be:

• Should we be doing more to log the specific nsresult to the console for devtool purposes (but invisible to content). I think the answer is no. The network panel will provide significantly more useful data or console errors reported by earlier stages in the pipeline. We're also in the process of cleaning up the console error reporting for workers and potentially the script loader, so I don't think we want the complexity for an error code that's generally going to be opaque and useless to users.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

This is a variant of bug 1748503

Comment 5

[Daniel Veditz [:dveditz]]

(In reply to Olli Pettay [:smaug][[e-mail]] from comment #4)

This is a variant of bug 1748503

Is it? It seems to be almost the same testcase as bug 1740985. Did that get un-fixed at some point?

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Well, all these are related. Dealing with the errors.

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

Attachment: Bug 1886108 - Normalize error messages. r=#dom-workers!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/577863693061
Normalize error messages. r=dom-worker-reviewers,smaug

Comment 9

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

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

Comment 10

[Frederik Braun [:freddy]]

CCing Luan Herrera who found the original bug.

Comment 11

[Donal Meehan [:dmeehan]]

:asuth could you please add an esr115 uplift request?
It doesn't graft cleanly to esr115 if you could also attach a rebased patch

Comment 12

[Andrew Sutherland [:asuth] (he/him)]

Attachment: Bug 1886108 - Normalize error messages. ESR115

Original Revision: https://phabricator.services.mozilla.com/D207104

Comment 13

[Phabricator Automation]

esr115 Uplift Approval Request

• User impact if declined: sec-mod security side channel
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: n/a
• Risk associated with taking this patch: Minimal
• Explanation of risk level: Change in error message string
• String changes made/needed: The error messages are not localized
• Is Android affected?: yes

Comment 14

[Phabricator Automation]

esr115 Uplift Approval Request

• User impact if declined: sec-mod security side channel
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: n/a
• Risk associated with taking this patch: Minimal
• Explanation of risk level: Change in error message string
• String changes made/needed: The error messages are not localized
• Is Android affected?: yes

Comment 15

[Donal Meehan [:dmeehan]]

:asuth flagging that there's an issue with your uplift request.
Not sure how you created it, the patch is not the same as the revision that landed in central.

Comment 16

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Donal Meehan [:dmeehan] from comment #15)

:asuth flagging that there's an issue with your uplift request.
Not sure how you created it, the patch is not the same as the revision that landed in central.

Is the issue that it's not the same as what's on central, or did it fail to apply? As per the comments in https://phabricator.services.mozilla.com/D207920 it was clear the test changes would not cleanly uplift to ESR115 so I manually edited the diff to drop the test hunks, leaving only the fix hunk.

Comment 17

[Donal Meehan [:dmeehan]]

Diff does not have proper author information in Phabricator. See the Lando FAQ for help with this error.

This is the error I'm seeing on Lando when trying to land it.

A diff without commit information detected in revision D207920.

This is the error I see when I import the phabricator patch locally

Comment 18

[Donal Meehan [:dmeehan]]

:asuth some additional info on issues with the Phabricator revision.

The initial revision was created with https://phabricator.services.mozilla.com/differential/diff/create/
Please use moz-phab client to submit patches so we can land using Lando.

Internal Slack thread
https://mozilla.slack.com/archives/C04LGP7428J/p1713620269432339

Comment 19

[Andrew Sutherland [:asuth] (he/him)]

Interesting, thank you. It seems like there's an asymmetry where "download raw diff" strips the commit message (and requisite metadata) but "update diff" is expected to include that commit message / metadata. I'll roundtrip it through moz-phab; I was prepping for travel and the moz-phab process is rather more involved. (Leaving needinfo until I resolve this.)

Comment 20

[Andrew Sutherland [:asuth] (he/him)]

I cherry-picked the patch locally to an esr115 checkout handling the test conflicts, built, tested that the existing test did not get mad, and then altered the phab metadata in the commit-message to point it at the existing uplift phab request at https://phabricator.services.mozilla.com/D207920 (so moz-phab would know where to put it) and then used moz-phab to submit the update which should hopefully be okay now? That said, I did use "moz-phab submit" because I was concerned that using "moz-phab uplift" would create a new uplift request and differential revision; the docs at https://wiki.mozilla.org/index.php?title=Release_Management/Requesting_an_Uplift#Requesting_Uplift_using_moz-phab and https://github.com/mozilla-conduit/review?tab=readme-ov-file#submitting-an-uplift-request do not make it clear if the "uplift" command is idempotent or not and how one would update an already submitted uplift request.

Comment 21

[Phabricator Automation]

esr115 Uplift Approval Request

• User impact if declined: sec-mod security side channel
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: n/a
• Risk associated with taking this patch: Minimal
• Explanation of risk level: Change in error message string
• String changes made/needed: The error messages are not localized
• Is Android affected?: yes

Comment 22

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/98dee93cf461

Comment 23

[Jesse Schwartzentruber (:truber)]

Attachment: advisory.txt