1608027 - JS Errors from new Error are not correctly muted

Status: NEW

(Core :: JavaScript Engine, defect, P2)

Description

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

:bkelly reached out about a test chromium just landed that tests that opaque cross-origin JS resources properly have their errors muted.

• Live test: https://wpt.live/service-workers/service-worker/opaque-script.https.html
• Test file: https://github.com/web-platform-tests/wpt/blob/master/service-workers/service-worker/opaque-script.https.html
• Merged pull request: https://github.com/web-platform-tests/wpt/pull/21074

This has not yet been synchronized into our repo.

Pernosco trace up at:
https://pernos.co/debug/3v89Bg5bSre2P-9Fh1hIgw/index.html

The interesting errors being logged are the cross-origin "www1.wpt.live" opaque-script-small.js and opaque-script-large.js in the stdout/stderr.

Comment 1

[Jens Stutte [:jstutte]]

Captured also by WPT tests here: bug 1607540

Comment 2

[Andrew McCreight [:mccr8]]

These sorts of issues in the past have been sec-high, I think, because they can leak information cross-origin.

Comment 3

[Perry Jiang [:perry] [no longer employee, use ni?]]

Attachment: Bug 1608027 - copy+paste WPT from github b/c it hasn't synced yet, without a SW involved

Comment 4

[Perry Jiang [:perry] [no longer employee, use ni?]]

I can reproduce without using a ServiceWorker + Cache (WIP patch with the modified test to exclude the ServiceWorker), so this doesn't seem to be ServiceWorker/Cache specific. :bz, I was told you might have some input for cross-origin error muting?

Comment 5

[Perry Jiang [:perry] [no longer employee, use ni?]]

I added some entries to the pernosco notebook. I don't think the muting state is captured by the exception thrown that's later used to initialize the ErrorReport.

Comment 6

[Boris Zbarsky [:bzbarsky]]

So just so we're clear: Gecko's implementation of muting does NOT match the spec's definition of muting, last I checked.

In particular, we mute errors during initial script evaluation, but not really runtime errors due to calling a function of the sort this test is testing, because the latter is pointless: the test could just as easily try/catch to get all the same information, so muting gives no benefit. This test is not testing muting of script-evaluation errors, of course; it should.

(More precisely, I think we mute or not based on whether the entrypoint into the script had muted errors, because if it did not it could obviously try/catch things and is then same-origin with where we're reporting to, so all is lost anyway.)

I don't know of an existing bug that tracks the spec divergence here in the general case, though the XXX comment at https://searchfox.org/mozilla-central/rev/ba4fab1cc2f1c9c4e07cdb71542b8d441707c577/js/src/jsexn.cpp#630-632 is likely relevant) but if we fix the test to test the cases when the entrypoint into the script is not same-origin with the page and find that those errors are muted correctly, there's no security bug here.

Comment 7

[Boris Zbarsky [:bzbarsky]]

So to be clear, the page is doing this:

el.addEventListener('load', evt => {
  runScript();
});

where el is a <script> element that links to an off-site URL, which defines a runScript function that throws a new Error. So in this case, there really is no security issue: that listener can just catch the error.

More interestingly, per spec the right entrypoint here is https://dom.spec.whatwg.org/#concept-event-listener-inner-invoke when we call the event listener. This runs the function and an exception is thrown. Step 2.10.1 says to "report the exception" which goes to https://html.spec.whatwg.org/multipage/webappapis.html#report-the-exception and talks about "the relevant script". But what is "the relevant script" in this case anyway? Is it the script where the exception got thrown (whatever that means, given try/catch and rethrowing), or the script that the function we called is defined in, or something else? This isn't defined at all in the spec, through the rest of the verbiage (about line/column being the position of the exception) make it sound like ... I'm not quite sure what. So it's not clear to me that what this test is testing is even specced clearly. What's specced clearly is what happens when the execution entrypoint is "run a script", but this test is not testing that, now is it?

Comment 8

[Perry Jiang [:perry] [no longer employee, use ni?]]

Agreed that "relevant script" isn't exactly the clearest language, but my interpretation of "with the problematic position (line number and column number) in the resource containing the script" is that the "relevant script" is the script doing the most recent throw, the idea being that if there's a re-throw, the "problematic" thing is not being able to handle catching the initial throw. (Also not sure if "problematic" is the clearest word either...) Reading again, I guess that's not a really convincing argument because it could hide the "true" "problematic position".

Comment 9

[Boris Zbarsky [:bzbarsky]]

Bug if there's a rethrow, the line/column reported in practice would still be for the original script... maybe. Depending on what was rethrown.

Anyway, I strongly suspect there is no security issue here, but the test would need to be fixed to actually check for the security-relevant case to check that.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

I'm happy to see the test switched to evaluation time throwing if you prefer. It would still test what I need it to.

I will note, though, every other browser including old edge passes this test. So it might be worth considering if there is an interop issue here.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Sure, there could be an interop issue. I would welcome having spec text that clearly defines the behavior so we know what to implement...

And yes, I would prefer that we tested both cases.

Comment 12

[Andrew McCreight [:mccr8]]

Feel free to clear the keywords etc. if there's no actually sec issue.

Comment 13

[Perry Jiang [:perry] [no longer employee, use ni?]]

Changing the two throwing scripts from

function runScript() {
  throw new Error("Interntional error.");
}

to

throw new Error("Intentional error.");

still allows "Intentional error." to be observed by the cross-origin script from what I'm seeing.

Comment 14

[Boris Zbarsky [:bzbarsky]]

That could be an issue. Again, we do mute syntax errors (the real reason muting was added, because those expose part of the script text in interesting ways), but it looks like we do not mute the throw new Error() case, even at toplevel. Presumably fixing that requires either the Error constructor or something after that point to capture the "muted script" state.

Comment 15

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

(clearing assignee/priority for JS team triage)

Comment 16

[Jeff Walden [:Waldo]]

So...you really want the Error constructor to examine the stack, figure out the nearest scripted caller or something, and look whether its errors are muted? This sort of behavior is pretty frowned upon and undesirable, e.g. for an extraordinarily brief period of time we made parseInt behavior on octal strings depend on the caller's strictness in bug 577536 til we reverted back to same-in-all-cases behavior in bug 583925.

I guess this is something we could do if we really had to -- but stack-walking to find a scripted caller (and maybe one that isn't self-hosted code?) and other nonsense is just really not pleasant at all. If the spec is really currently requiring this, this is dumb and really ought be changed.

(And all that's before even getting to "script could just try/catch anyway", as noted in comment 6, which serves as additional reason this is dumb.)

Comment 17

[Boris Zbarsky [:bzbarsky]]

I personally think the spec is pretty silly here, yes, though it's also vague enough that who knows what it's really requiring. If someone has the time to come up with a spec proposal that is not silly and addresses the actual security threats, I would very much welcome that. So I do not want that thing that you are describing, at all, but at first glance it is what other browsers implement right now.

Comment 18

[Anne (:annevk)]

That differs from your last comment in https://github.com/whatwg/html/issues/958 I think and might be worth clarifying there?

Comment 19

[Boris Zbarsky [:bzbarsky]]

Will do.

Comment 20

[Boris Zbarsky [:bzbarsky]]

See https://github.com/whatwg/html/issues/958#issuecomment-582005343

Comment 21

[Jason Orendorff [:jorendorff]]

Doing triage today. This bug is weird:

• not clear if it's really sec-anything
• spec is vague and silly
• the real work is deciding what should happen
• bug lives in js/src, but nobody on JS team is qualified to decide

How about this: Let's either assign someone to make those decisions, or admit this is not a security bug and open it. Boris, what do you think?

Comment 22

[Boris Zbarsky [:bzbarsky]]

I don't think this is a security bug.

Comment 23

[Boris Zbarsky [:bzbarsky]]

That said, it's worth deciding what should happen here. I don't know why no one on the JS team is qualified to decide; the question is whether you can get nontrivial cross-site info leakage via errors being reported to the global error handler, and if so how to prevent that. Answering that first part requires understanding what sorts of information can end up in errors, no?

Comment 24

[Andrew McCreight [:mccr8]]

Removing the sec flags based on comment 22.

Comment 25

[Jason Orendorff [:jorendorff]]

bz patiently explained this to me.

The problem: User visits attacker.com. The target file target.com/file is CSV or something but happens to parse as JS. Attacker wants to get bits from target using <script src="https://target.com/file"> and scraping error messages.

So below, "same-origin" = attacker, "cross-origin" = target.

The solution: Mute errors in this case - just don't deliver them to window.onerror. (We mute SyntaxErrors in cross-site scripts this way, to protect against cases where the target file does not happen to parse as JS. But CSV can get past that and start running.)

The problem with the solution: Squelching errors is infuriating, so we want something tailored to the actual security problem.

We want rules for muting based on which domain is doing what. So let's enumerate all 4 cases.

1. Entry same-origin, thrower same-origin. The easy case: no cross-site, no muting.
2. Entry same-origin, thrower cross-origin. Current spec mutes, but that's silly.
3. Entry cross-origin, thrower same-origin. Current spec does not mute, and there's no point muting this. If this happens it's unlikely to be an attack, and we should try to report errors in such cases. But requires stack inspection to detect and not mute it.
4. Entry cross-origin, thrower cross-origin. Muted in spec; should be muted.

Loose threads:

• Spec bugs. See github discussion.
• Implementation bugs. Some situations under case 4 are not yet muted; these are not covered in wpt-tests (which mostly test case 2).

Comment 26

[Boris Zbarsky [:bzbarsky]]

Entry same-origin, thrower cross-origin. Current spec mutes, but that's silly.

To be clear, that's because the entry code can just try/catch everything and get the exceptions that way.

Comment 27

[Tom S. (please needinfo tschuster)]

I just came across this issue while working on bug 1623226, because I was trying to use html/semantics/scripting-1/the-script-element/muted-errors.sub.html to make sure we don't regress error muting.