1746783 - document.then gets called

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P1)

Description

[BZZZZ]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:95.0) Gecko/20100101 Firefox/95.0

Steps to reproduce:

1. host this as html on http server: "<script>document.then=document.write;</script>"
2. open that in firefox

Actual results:

document.then gets called with 2 native functions as arguments and prints "function () { [native code] }function () { [native code] }"

Expected results:

document.then doesn't get called

Comment 1

[Tooru Fujisawa [:arai]]

Thank you for reporting!

Confirmed the behavior.
This doesn't happen on Chrome and Safari.

The issue is that, document is passed to Promise.resolve-equivalent function here:
https://searchfox.org/mozilla-central/rev/7d17fd1fe9f0005a2fb19e5d53da4741b06a98ba/dom/base/Document.cpp#13680-13686

void Document::MaybeResolveReadyForIdle() {
  IgnoredErrorResult rv;
  Promise* readyPromise = GetDocumentReadyForIdle(rv);
  if (readyPromise) {
    readyPromise->MaybeResolve(this);
  }
}

the promise is used by WebExtensions.

https://searchfox.org/mozilla-central/rev/7d17fd1fe9f0005a2fb19e5d53da4741b06a98ba/toolkit/components/extensions/ExtensionUtils.jsm#206

function promiseDocumentIdle(window) {
  return window.document.documentReadyForIdle.then(() => {
    return new Promise(resolve =>
      window.requestIdleCallback(resolve, { timeout: idleTimeout })
    );
  });
}

So, the resolved value is not used, and it doesn't have to resolve the promise with the document, but undefined should be sufficient.

In term of exploitability, the resolved value is not used, so the user code cannot inject any value to the code,
but still can control whether to or not to call onFulfilled/onRejected functions passed to the monkey-patched then value.
So, a webpage can stop running WebExtension content script with "runAt": "document_idle", that might break some extensions that's security-related.
So, marking as security-sensitive for now.

This is regression from bug 1437921, but I'm not setting "regressed by" field, to avoid leaking information.

Comment 2

[Rob Wu [:robwu]]

I'll take a look.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Resolving with undefined is probably fine, if it makes sense for the consumer.

But it's a little disturbing that this is all ending up seeing content-controllable .then on the document; I would have expected Xrays to kick in here.

Comment 4

[Tooru Fujisawa [:arai]]

In Promise world, a "thenable" is basically a random object that has "then" property, and Xray doesn't handle properties that do not exist by default.
document doesn't have "then" by default, and resolving a promise with it doesn't do anything special, just resolves the promise with document.
Adding "then" property makes the document a "thenable", that screws things up and makes the MaybeResolve call there handles "then" property, instead of just resolving with document object.

maybe we can add special case for "then" property in Xray, in a followup bug.

Comment 5

[Boris Zbarsky [:bzbarsky]]

I guess the real issue is that the Promise in question is being created in the content context, so it does not see an Xray to the document, but rather the document itself...

Comment 6

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bzbarsky] from comment #5)

I guess the real issue is that the Promise in question is being created in the content context, so it does not see an Xray to the document, but rather the document itself...

This sounds right to me. Auditing all of https://searchfox.org/mozilla-central/search?q=promise%3A%3Acreate%28&path=&case=false&regexp=false for similar issues doesn't sound like fun, plus we're bound to introduce some more at a later point. Could we make the webidl binding code assert if a ChromeOnly API returns a promise with a content privileged global, or would that hit false-positives? An extremely naive search produces:

dom/webidl/RTCIdentityProvider.webidl-24-  [ChromeOnly, Throws]
dom/webidl/RTCIdentityProvider.webidl:25:  Promise<RTCIdentityAssertionResult>
--
dom/webidl/RTCIdentityProvider.webidl-29-  [ChromeOnly, Throws]
dom/webidl/RTCIdentityProvider.webidl:30:  Promise<RTCIdentityValidationResult>
--
dom/webidl/Navigator.webidl-138-  [Throws, ChromeOnly, Pref="dom.battery.enabled"]
dom/webidl/Navigator.webidl:139:  Promise<BatteryManager> getBattery();
--
dom/webidl/File.webidl-53-  [ChromeOnly, Throws, NeedsCallerType]
dom/webidl/File.webidl:54:  static Promise<File> createFromNsIFile(nsIFile file,
--
dom/webidl/File.webidl-57-  [ChromeOnly, Throws, NeedsCallerType]
dom/webidl/File.webidl:58:  static Promise<File> createFromFileName(USVString fileName,
--
dom/webidl/MediaStream.webidl-52-    [ChromeOnly, Throws]
dom/webidl/MediaStream.webidl:53:    static Promise<long> countUnderlyingStreams();
--
dom/webidl/MediaSource.webidl-49-  [Throws, ChromeOnly]
dom/webidl/MediaSource.webidl:50:  Promise<MediaSourceDecoderDebugInfo> mozDebugReaderData();
--
dom/webidl/Document.webidl-451-  [ChromeOnly, Throws]
dom/webidl/Document.webidl:452:  Promise<any> blockParsing(Promise<any> promise,
--
dom/webidl/Document.webidl-465-  [ChromeOnly, Throws]
dom/webidl/Document.webidl:466:  readonly attribute Promise<Document> documentReadyForIdle;
--
dom/chrome-webidl/FrameLoader.webidl-119-  [ChromeOnly, Throws]
dom/chrome-webidl/FrameLoader.webidl:120:  Promise<unsigned long> printPreview(nsIPrintSettings aPrintSettings,

I don't know off-hand if any of them have similar issues already, but at least from a casual scan it would seem that adding such an assert would be correct? Arai, wdyt?

Comment 7

[Tooru Fujisawa [:arai]]

Thank you for pointing out that Xray isn't used here.

Using system compartment for the ChromeOnly Promise attributes makes sense.

As long as it's chrome-only, forcing the system compartment won't cause any security issue. so adding an assertion would work.

There's good side effect that we could workaround the lifetime issue of the global (example case: bug 1674505), by using system global/compartment.
If the Promise is created in document's global, and the global is discarded, the reaction job is not called for promise.finally.
There's ongoing spec issue in https://github.com/tc39/ecma262/issues/2222 , that which global to use for the reaction job.
Then, if we use system global for the Promise, and the onFulfilled/onRejected handlers are also from system global,
we don't see the effect (as long as system global isn't discarded), and reaction jobs are always called.

But anyway, addressing that may take some time.
For this bug, IMO fixing the consumer of the promise (Document::MaybeResolveReadyForIdle) not to pass document would be simpler.
we can look into the underlying issue in followup bug.

Comment 8

[Rob Wu [:robwu]]

Attachment: Bug 1746783 - Remove unused resolution value from documentReadyFromIdle

Comment 9

[Rob Wu [:robwu]]

I've submitted a patch above that fixes this specific case; I suggest to file a new bug to cover the general issue.

Comment 10

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

Remove unused resolution value from documentReadyFromIdle r=arai,emilio
https://hg.mozilla.org/integration/autoland/rev/fdeb66de0dd504ac8b2f0a784d206679cbd24aa4
https://hg.mozilla.org/mozilla-central/rev/fdeb66de0dd5

Comment 11

[Ryan VanderMeulen [:RyanVM]]

What's the security impact of this bug? Can you suggest a severity rating?

Comment 12

[Rob Wu [:robwu]]

Web pages can prevent content scripts at document_idle from executing.
In theory web pages also have greater control over when a document_idle script executes, but that's something that they could already control before.

So at worst I'd rate this a DOS kind of issue. sec-low.

Comment 13

[Daniel Veditz [:dveditz]]

This is regression from bug 1437921, but I'm not setting "regressed by" field, to avoid leaking information.

the regression/blocks/depends-on relations are fine to use: people who can't see the security bug won't see the relation (bug 1591549). There's currently a leak of the bug number (only) in bugmail to folks CC'd on the public end of the relation, but that's minor so don't let that discourage you (the fix for it will soon be live: bug 1632650).

"See Also" is currently always visible so take a little care there, but the fix for that, too, will soon be live: bug 1628759

There's no good way to hide a Duplicate resolution so take care making a public bug a dupe of a security bug. Sometimes it's better to make it "depend on" (which is private) and then it will eventually get FIXED.