Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
cramforce opened this issue Mar 16, 2017 · 25 comments
Closed

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

cramforce opened this issue Mar 16, 2017 · 25 comments
Labels
addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest security/privacy There are security or privacy implications topic: error reporting topic: script

Comments

@cramforce
Copy link

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

  • serving the script with CORS headers
  • requesting the script as CORS.

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:

  • The "Script error." rewriting should be limited to initial execution of a script, because all other stack entry points can be wrapped in try-catch and hence aren't subject to the limitation anyway.
  • Potentially the rewriting of the error message could be limited to SyntaxError only.

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
Copy link
Member

annevk commented Mar 16, 2017

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
Copy link
Author

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
Copy link
Member

annevk commented Mar 16, 2017

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
Copy link
Author

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
Copy link
Member

annevk commented Mar 16, 2017

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
Copy link
Member

annevk commented Mar 16, 2017

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
Copy link
Author

cramforce commented Mar 16, 2017

@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
Copy link
Member

annevk commented Mar 16, 2017

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

@cramforce
Copy link
Author

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
Copy link
Member

annevk commented Mar 16, 2017

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
Copy link
Member

domenic commented Mar 16, 2017

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

@cramforce
Copy link
Author

@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
Copy link
Contributor

bzbarsky commented Mar 16, 2017

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.

@annevk annevk added topic: script addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest security/privacy There are security or privacy implications labels Mar 30, 2017
@sebmarkbage
Copy link

sebmarkbage commented Jul 29, 2017

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
Copy link
Member

domenic commented Jul 29, 2017

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
Copy link

sebmarkbage commented Jul 29, 2017

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
Copy link
Author

@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
Copy link

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
Copy link
Member

domenic commented Jul 29, 2017

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
Copy link
Member

domenic commented Jul 29, 2017

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
Copy link
Author

@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
Copy link
Member

annevk commented Jul 29, 2017

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
Copy link
Author

cramforce commented Jul 29, 2017 via email

@cramforce
Copy link
Author

Just for the record:
Due to whatwg/fetch#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
Copy link
Member

domenic commented Jul 25, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest security/privacy There are security or privacy implications topic: error reporting topic: script
Development

No branches or pull requests

5 participants