1959298 - Possible DOMXSS in the _getEntryPoint() method of browser/components/preferences/sync.js

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect)

Description

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/135.0.0.0 Safari/537.36

Steps to reproduce:

The file browser/components/preferences/sync.js and identified that the _getEntryPoint() method parses document.URL directly to extract the entrypoint query parameter, which is then passed to FxAccounts.config.promiseConnectAccountURI() and promiseManageURI(). The resulting URL is inserted into the DOM via element.setAttribute("href", ...) without validation or sanitization.

Key code:

   let params = new URLSearchParams(document.URL.split("#")[0].split("?")[1] || "");
   return params.get("entrypoint") || "preferences";

This value is later used in:

   .setAttribute("href", accountsManageURI);
   .setAttribute("href", connectURI);

example url but it does not display an alert because it is about:// and it does not allow the javascript code to be executed
about:preferences?entrypoint=javascript:alert(1)#sync

In simple words, you should just remove the domxss vulnerability, even though it is not feasible, but the code itself is vulnerable to it.

Actual results:

The parameter entrypoint is inserted directly into the DOM without being validated or sanitized. While Firefox currently blocks javascript: URLs in most privileged contexts like about:preferences, the flow still qualifies as DOM-based XSS because:

Untrusted user-controlled input flows to a dangerous DOM sink.

This creates a security smell and future risk.

Expected results:

Input such as entrypoint should be strictly validated against a whitelist of expected values (e.g., "preferences", "sync", etc.) before being used in any DOM insertion, especially in sensitive attributes like href.

Comment 1

[Daniel Veditz [:dveditz]]

Tangent

document.URL.split("#")[0].split("?")[1] is pretty horrifying. Let's stick to a common URL parser, please, so we don't create security bugs when different parts of the product interpret the same string as meaning different URLs. To get the search params this should be

   let params = new URL(document.URL).searchParams;

NB: document.location.search returns nothing for an about: URL so you can't use that in sync. Is that a separate bug, or just "historical behavior" we have to live with? I know Location is not a URL, but it has a lot of same-named properties that sure seem like they ought to agree with how the URL parser handles them.

This kind of URL splitting seems to be in many places in front-end code -- we need to fix that!

This bug

It's true there's no sanity checking on the entrypoint, but it's not true that parameter is stuffed directly into the "href" attribute. The entrypoint value is an argument to functions like FxAccounts.config.promiseConnectDeviceURI(this._getEntryPoint()) which builds an entire URL and limits what a hacked entrypoint argument could do. Basically the parameter gets reproduced as a parameter in the new URL.
https://searchfox.org/mozilla-central/rev/27bb03eebacd679df419dac52a3b99c142cdb5db/services/fxaccounts/FxAccountsConfig.sys.mjs#102

These URLs are all pages loaded from https://accounts.firefox.com/ so I don't know for sure what they use entrypoint for, but I think it is just basically for analytics of where site visitors are coming from.

In addition, web pages can't navigate to about: URLs so it would be hard to deliver a hacked entrypoint parameter. Maybe trick a user into copying a string and pasting it into the address bar?

I don't believe there's any possible DOMXSS here.

Next steps

1. it would still be good practice to sanitize the entrypoint param. For example, maybe it should match /^[-.\w]+$/
2. we need to audit the places where we do similar url chopping in front-end code. Here's some potential places to look: https://searchfox.org/mozilla-central/search?q=url.split

Comment 2

[:Gijs (he/him)]

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

Tangent

document.URL.split("#")[0].split("?")[1] is pretty horrifying. Let's stick to a common URL parser, please, so we don't create security bugs when different parts of the product interpret the same string as meaning different URLs. To get the search params this should be

   let params = new URL(document.URL).searchParams;

Tangent tangent: document.documentURIObject already has an nsIURI so we can use that with URL.fromURI to avoid re-parsing the URL.

NB: document.location.search returns nothing for an about: URL so you can't use that in sync. Is that a separate bug, or just "historical behavior" we have to live with? I know Location is not a URL, but it has a lot of same-named properties that sure seem like they ought to agree with how the URL parser handles them.

Yeah about: URIs are special (not nsStandardURI) and IIRC this means hash/ref handling is weird. Mostly doesn't matter for the web but does in some cases... See also bug 1767001 .

Anyway apparently URL.searchParams does the right thing so that is nice.

Comment 3

[:Gijs (he/him)]

Attachment: Bug 1959298 - improve sync entrypoint handling, r?markh

Comment 4

[:Gijs (he/him)]

Attachment: Bug 1959298 - improve SERP search params handling, r?Standard8

Comment 5

[:Gijs (he/him)]

Attachment: Bug 1959298 - improve web app manifest url handling, r?baku

Comment 6

[:Gijs (he/him)]

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

Next steps

1. it would still be good practice to sanitize the entrypoint param. For example, maybe it should match /^[-.\w]+$/
2. we need to audit the places where we do similar url chopping in front-end code. Here's some potential places to look: https://searchfox.org/mozilla-central/search?q=url.split

The patches cover this. I also looked at https://searchfox.org/mozilla-central/search?q=.split(%22%23%22 and https://searchfox.org/mozilla-central/search?q=.split(%2F%23 and https://searchfox.org/mozilla-central/search?q=.split%28%22%3F%22&path=&case=false&regexp=false and https://searchfox.org/mozilla-central/search?q=.split%28%2F%5C%3F&path=&case=false&regexp=false .

These queries show some more places in devtools and pdf.js that do things that perhaps we should change. I think we can file follow-ups for that; PDF.js will need changing in PDF.js , and devtools I am assuming/hoping know what they're doing in terms of handling urls and url portions.

The only other cases I noticed are:

https://searchfox.org/mozilla-central/rev/601256c3cc6f397b018995810fd3f586570f50ee/browser/actors/SearchSERPTelemetryChild.sys.mjs#539-547 which has comments indicating the avoidance of new URL / Services.io.newURI is deliberate, but more generally seems wrong in other ways (e.g. the https prefixing for relative URIs slightly higher up, I think it should use uri.resolve() ). Mark (Banner), would you mind filing a follow-up for that?

https://searchfox.org/mozilla-central/rev/601256c3cc6f397b018995810fd3f586570f50ee/browser/base/content/browser.js#4795-4808 which strips out fragments/search parts. This should use uri.mutate() but I literally earlier this morning filed bugs for some outside contributors so they could work on refactoring this code into a different file, and because the code is only removing bits and normally (I think only ? ) used with internal URLs from browser code (not attacker-controlled), I think this could be a non-security follow-up.

https://searchfox.org/mozilla-central/rev/601256c3cc6f397b018995810fd3f586570f50ee/toolkit/components/aboutmemory/content/aboutMemory.js#526-529 - I'll write a patch for this on top of the stack I attached.

Someone should probably doublecheck I didn't miss anything and/or if that sounds reasonable?

Comment 7

[:Gijs (he/him)]

Attachment: Bug 1959298 - use search params in about:memory, r?mccr8

Comment 8

[Daniel Veditz [:dveditz]]

Tangent tangent: document.documentURIObject already has an nsIURI so we can use that with URL.fromURI to avoid re-parsing the URL.

very nice! I knew about document.documentURI but that's just another string. (I don't know why the web needs both .URL and .documentURI. "Too much typing" vs "backwards compatibility" I suppose)

Someone should probably doublecheck I didn't miss anything and/or if that sounds reasonable?

Sounds reasonable and the patches look good.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d9c8211f83fe
improve sync entrypoint handling, r=markh
https://hg.mozilla.org/integration/autoland/rev/cb96c92ac472
improve SERP search params handling, r=Standard8
https://hg.mozilla.org/integration/autoland/rev/37d6a257d32b
improve web app manifest url handling, r=baku
https://hg.mozilla.org/integration/autoland/rev/ef842ad5ff0c
use search params in about:memory, r=mccr8

Comment 10

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

Backed out for causing bc failures on browser_Manifest_install.js.

• Push with failures
• Failure log
• Backout link

Comment 11

[:Gijs (he/him)]

Well, that is embarrassing. Should be easy to fix, but unfortunately I'm out of time to come back to this this week. Hopefully on Tuesday, post-public-holiday here. Leaving ni for that.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/38d641178f9e
improve sync entrypoint handling, r=markh
https://hg.mozilla.org/integration/autoland/rev/14f515d9ff2c
improve SERP search params handling, r=Standard8
https://hg.mozilla.org/integration/autoland/rev/7302fb0c2bad
improve web app manifest url handling, r=baku
https://hg.mozilla.org/integration/autoland/rev/e955cae13a0a
use search params in about:memory, r=mccr8

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/38d641178f9e
https://hg.mozilla.org/mozilla-central/rev/14f515d9ff2c
https://hg.mozilla.org/mozilla-central/rev/7302fb0c2bad
https://hg.mozilla.org/mozilla-central/rev/e955cae13a0a

Comment 14

[:Gijs (he/him)]

I filed bug 1962672 for the PDF.js instances and bug 1962668 for the devtools ones. Kept as sec-sensitive out of caution though off-hand I doubt they are exploitable in any way.

Comment 15

[Mark Banner (:standard8)]

(In reply to :Gijs (he/him) from comment #6)

https://searchfox.org/mozilla-central/rev/601256c3cc6f397b018995810fd3f586570f50ee/browser/actors/SearchSERPTelemetryChild.sys.mjs#539-547 which has comments indicating the avoidance of new URL / Services.io.newURI is deliberate, but more generally seems wrong in other ways (e.g. the https prefixing for relative URIs slightly higher up, I think it should use uri.resolve() ). Mark (Banner), would you mind filing a follow-up for that?

Filed as bug 1962681.

I also filed bug 1962684 as a follow-up to audit cases of .split("&").