whatwg / html

HTML Standard
https://html.spec.whatwg.org/multipage/
Other
8.03k stars 2.63k forks source link

"Script error." message in window.onerror makes bad DevExp trade off #2440

Closed cramforce closed 2 months ago

cramforce commented 7 years ago

When a script that isn't served from the same origin as the document throws an error, then the error message exposed to window.onerror is "Script error." and the stack trace is empty.

This is to spec and it can be worked around in some browsers by

While the behavior is understandable, it leads to bad developer experience in practice and is overzealous with respect to the threat model.

My understanding of the threat model is that it is worried about attackers circumventing CORS restrictions by e.g. script src-ing a privileged file from a cross origin and then gaining insights from the error message that happens because the file is not valid JavaScript.

I think browsers could successfully prevent the threat while maintaining most of the developer experience:

Since SyntaxErrors are essentially irrelevant in JavaScript production monitoring, these changes would reinstate the ideal developer experience while maintaining the desired protection against data leaks.

As an additional data point: Safari& Chrome seem to follow the spec but Edge & IE do not handle cross origin errors specifically at all and provide full exception info.

annevk commented 7 years ago

I recommend filing a (security) bug against Edge.

I don't see why we would potentially compromise security of users or complicate JavaScript's processing model when the alternative is rather easy to deploy and works everywhere.

cramforce commented 7 years ago

What is the added benefit of the error obfuscation for non-initial execution errors?

Since JS can instrument all other stack entry points this protection effectively only applies to initial execution.

Try even googling for "Script error.". There are 2 obscure blog posts explaining what is going on and a lot of confused developers that wonder why their stuff just doesn't work after they deployed using a CDN in production. Meanwhile people use the mentioned above stack entry point instrumentation to fix things mostly for them (See Angular Zones, goog.debug.entryPointRegistry, and dozens of commercial error monitoring services that could be one line of JS if it wasn't for the above). So, what happens is that the "complicated processing model" you mention was shifted into user space, making JS slower for everyone everywhere.

annevk commented 7 years ago

What is the added benefit of the error obfuscation for non-initial execution errors?

I'm not entirely sure, but I don't think the burden is on me to proof that it doesn't reveal any new data.

I understand that running into this is frustrating, but the answer here is CORS and it doesn't make JS slower for everyone everywhere. Poking more holes in the same-origin policy is not a good solution.

cramforce commented 7 years ago

The answer to the question is that there is no benefit as proven in subsequent paragraph.

If spec simplicity is more important than developer experience, sure I understand your point, but some developers may disagree.

annevk commented 7 years ago

The answer to the question is that there is no benefit as proven in subsequent paragraph.

I don't see how that's true though, e.g., with closures.

annevk commented 7 years ago

If spec simplicity is more important than developer experience, sure I understand your point, but some developers may disagree.

I don't think this is warranted.

cramforce commented 7 years ago

@annevk Do you have an example for how to run JS in a browser outside of script tag injection's immediate side effect that cannot be wrapped as an entry point as in

const orig = window.setTimeout;
window.setTimeout = function(fn, timeout) {
  orig(function() {
    try { return fn.apply(this, arguments) } catch (e) { console.log(e.stack) /* full stack trace */ }
  }, timeout)
};

I'm not aware of a way that JS gets run outside of initial execution where a sufficiently motivated attacker couldn't change it so that they get to wrap that execution in a try block. Do you have an example?

annevk commented 7 years ago

If the foreign script uses Promise.then() or registers event listeners?

cramforce commented 7 years ago

You can (and this is what all the tools do), just override Promise, Promise.prototype.then and Foo.prototype.addEventListener just before running the foreign script.

Of course, this makes execution slower since every API call is going through an extra closure and try-block.

annevk commented 7 years ago

If that leaves no holes and the exception you get is otherwise identical that is somewhat compelling. Did you talk to Chrome's security team about this?

domenic commented 7 years ago

We're certainly not going to poke more holes in the same-origin policy. Please use CORS.

cramforce commented 7 years ago

@annevk Yes, talked to @mikewest. https://bugs.chromium.org/p/chromium/issues/detail?id=701371#c1

@domenic Not proposing to poke a new hole. Just make an existing hole benefit developers instead of only attackers.

bzbarsky commented 7 years ago

So a few thoughts:

1) There is no spec that requires exceptions to have any location information, nor any specifically useful error messages. Of course in practice in UAs they have both, though not in interoperable ways.

2) I agree that by the time you're running script you're more or less SOL because an attacker can hook all the global objects and intercept most of what you're doing. Thus, while we could extend the muted error protections into what's available from JS exception objects in general, in practice it probably doesn't help much.

3) We have added restrictions on what could be hooked in the past. For example see the changes to disallow adding a proxy on the proto chain of the global (to prevent interception of arbitrary barewords in the script text). There's also still some thought going on wrt what can be done with getters/setters on the global, if anything, which allow targeted interception of specific barewords.

4) Given item 2, I expect that in practice restricting error muting to invalid syntax and "early errors" in ES parlance is probably reasonable. Given item 3 I'm not 100% sure of it, and it could change in the future. So we need to be a bit careful here. :(

5) Not all early errors are SyntaxErrors, so restricting error muting to SyntaxError is definitely wrong. Simple example from a few minutes of reading the ES spec: <script>(()=>{})++</script> produces an unhookable ReferenceError, not SyntaxError. Not all UAs get this right, of course (e.g. Firefox produces a SyntaxError here).

6) "Initial execution" is a bit hard to define rigorously (unlike early errors). But early errors are somewhat orthogonal to "initial execution", given eval. And eval is hookable, and both syntax errors and early errors in it are catchable too. So for maximum usefulness you would want to do something like mute early errors coming from non-eval executions or something...

7) You really need to talk to some ES experts to make sure they're not planning to add features that will blow up some or all of the assumptions about this.

Paging @syg and @jswalden for this last bit and more hard data on what the state of mitigations I mention in item 3 is.

sebmarkbage commented 7 years ago

FWIW we use onerror handling in React and it's very confusing for people that it doesn't work cross-origin. Especially on sites like jsfiddle style sites that might need cross-origin in place without CORS.

However, the main reason we use this instead of try/catch everywhere is because we don't want to force people to use "Break On Caught Exceptions" in debuggers. Because try/catch is also used by libraries to do feature detection and "Break On Caught Exception" is too noisy because of it.

domenic commented 7 years ago

sites like jsfiddle style sites that might need cross-origin in place without CORS.

Can you explain why jsfiddle cannot use the basic safe CORS protocol setup?

sebmarkbage commented 7 years ago

I don't know why TBH. The last one we saw was codesandbox.io. I haven't evangelized all of them to update so I just gave up.

cramforce commented 7 years ago

@domenic CORS also requires a crossorigin attribute on the script tag, no? If projects start putting that into their default docs they need to add stuff like "it will only work using a custom web server config" as a caveat. At which point people would probably stop reading the docs and go away.

sebmarkbage commented 7 years ago

The core of the problem is that it'll just work in the normal case and you won't notice until you get an error and then it's a broken experience. You can tell people to put stuff in all their fiddle hosts, CDNs, fiddle examples and tutorial examples but they'll keep getting dropped because they do nothing to enable the primary functionality.

domenic commented 7 years ago

No, that's only used if you want to enforce disallowing opaque cross-origin scripts. If the correct CORS headers are set, the response is still CORS-same-origin, and so errors are not muted, regardless of the crossorigin="" attribute (which only affects the request, not the response).

domenic commented 7 years ago

Hmm, I might be wrong, I'm having a hard time following the state mutations throughout the Fetch algorithm. Regardless, we've at least fixed this mess for module scripts, where CORS is always required, so we won't run into the problem @sebmarkbage mentions. I think that's our long-term solution here.

cramforce commented 7 years ago

@domenic Maybe then we could clarify the spec here to not require the crossorigin attribute for the response header to opt into showing clear text errors?

annevk commented 7 years ago

If the correct CORS headers are set, the response is still CORS-same-origin, and so errors are not muted, regardless of the crossorigin="" attribute (which only affects the request, not the response).

No, we require explicit client-side opt-in (be it crossorigin or type=module). There's no "maybe CORS" and I don't think we should add that. It's already confusing enough.

cramforce commented 7 years ago

I already outlined my preferred solution. Right now we are are hurting developers with a s solution that is overzealous given the thread model. Module scripts are years from reality. Fixing the status quo is well worth the effort.

On Jul 29, 2017 12:14 AM, "Anne van Kesteren" notifications@github.com wrote:

If the correct CORS headers are set, the response is still CORS-same-origin, and so errors are not muted, regardless of the crossorigin="" attribute (which only affects the request, not the response).

No, we require explicit client-side opt-in (be it crossorigin or type=module). There's no "maybe CORS" and I don't think we should add that. It's already confusing enough.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/whatwg/html/issues/2440#issuecomment-318810292, or mute the thread https://github.com/notifications/unsubscribe-auth/AAFeT_stCfgq-sxwlPlOFoU3OnT6Tf1Zks5sStvUgaJpZM4MfVuZ .

cramforce commented 7 years ago

Just for the record: Due to https://github.com/whatwg/fetch/issues/341 one has to be extremely careful about the performance impact of adding the crossorigin attribute. Unless one picks the value use-credentials the script request would be forced into a separate connection which for 3G regresses download time by about 1 second. In many CDN cases this won't make a difference (because a connection has to be created anyway), but in carefully optimized scenarios it can have major impact.

Module scripts would have the same issue even more same origin requests.

domenic commented 2 months ago

Merging into #10514. It's possible one direction for resolving that issue is to take the approach mentioned here, but as I discuss there, I suspect loosening security boundaries won't be a high priority for anyone.