| Summary: | XMLHttpRequests blocked by CSP should throw a more descriptive exception. | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Mike West <mkwst> | ||||||||||
| Component: | WebCore JavaScript | Assignee: | Nobody <webkit-unassigned> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | abarth, bfulgham, dbates, dglazkov, felipe, haraken, japhet, jochen, mjs, ojan, webkit.review.bot | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=155777 | ||||||||||||
| Bug Depends on: | |||||||||||||
| Bug Blocks: | 97693 | ||||||||||||
| Attachments: |
|
||||||||||||
Created attachment 165802 [details]
WIP
WIP patch that I fully expect to see explode. I'll drop a note to webkit-dev@ before proceeding much further, as this is the tip of an iceburg. :/ Hey Jochen, before I get too far along with this, can you suggest someone who can evaluate the approach? The goal is to improve the messaging for a laaaarge number of DOM exceptions, and (assuming webkit-dev doesn't reject the concept) the series of patches would end up touching some generation scripts, V8/JSC bindings, and probably lots of exception types. Thanks! Comment on attachment 165802 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=165802&action=review > Source/WebCore/bindings/v8/V8Binding.cpp:72 > + return V8ThrowException::setDOMException(exceptionCode, exceptionDescription.utf8().data(), isolate); Is data always NUL terminated? Usually we need to call a function like "charactersWithNullTermination" to get that. > Source/WebCore/dom/make_dom_exceptions.pl:119 > + print F " const char* context;\n"; I wonder if this should be a String. The values above are always constants, which is why they're const char*s. For context, it looks like you're going to generate the string at runtime (e.g., to include a URL). In that case, we need to make sure that we retain the underlying storage for the string. That's what String helps us do. Comment on attachment 165802 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=165802&action=review > LayoutTests/http/tests/security/contentSecurityPolicy/connect-src-xmlhttprequest-blocked-expected.txt:4 > +{"message":"SECURITY_ERR: DOM Exception 18. XMLHttpRequest connections to 'http://localhost:8000/xmlhttprequest/resources/get.txt' are blocked due to this page's Content Security Policy.","name":"SECURITY_ERR","code":18,"stack":"Error: An attempt was made to break through the security policy of the user agent.\n at http://127.0.0.1:8000/security/contentSecurityPolicy/connect-src-xmlhttprequest-blocked.html:20:9"} We need to be careful not to leak security-sensitive information in these error messages. For example, we're not allowed to leak where redirects lead. This example is ok because this URL was supplied by the web page. However, if the URL was as result of a redirect, we wouldn't be able to include it. Thanks, Adam! (In reply to comment #4) > (From update of attachment 165802 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=165802&action=review > > > Source/WebCore/bindings/v8/V8Binding.cpp:72 > > + return V8ThrowException::setDOMException(exceptionCode, exceptionDescription.utf8().data(), isolate); > > Is data always NUL terminated? Usually we need to call a function like "charactersWithNullTermination" to get that. Almost certainly not. This patch is pretty much at the "OMG it compiled!" stage, so I expect a lot of this sort of cleanup. :) > > Source/WebCore/dom/make_dom_exceptions.pl:119 > > + print F " const char* context;\n"; > > I wonder if this should be a String. The values above are always constants, which is why they're const char*s. For context, it looks like you're going to generate the string at runtime (e.g., to include a URL). In that case, we need to make sure that we retain the underlying storage for the string. That's what String helps us do. I started with a String, but it stood out like a sore thumb in the V8 bindings, where it looks like we try to get away from String as soon as possible. Doing this sort of work with a String would be simpler, and less error prone. I'd be happy to make that change. :) (In reply to comment #5) > We need to be careful not to leak security-sensitive information in these error messages. For example, we're not allowed to leak where redirects lead. This example is ok because this URL was supplied by the web page. However, if the URL was as result of a redirect, we wouldn't be able to include it. Right. I didn't think about that. As you say, in this particular case it doesn't matter, but in others it certainly will. Generally, I'd be fine with generic error messages in security-sensitive areas. I'd just like them to be a bit less generic than they are now. :) (In reply to comment #4) > (From update of attachment 165802 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=165802&action=review > > > Source/WebCore/bindings/v8/V8Binding.cpp:72 > > + return V8ThrowException::setDOMException(exceptionCode, exceptionDescription.utf8().data(), isolate); > > Is data always NUL terminated? Usually we need to call a function like "charactersWithNullTermination" to get that. Based on the comments in CString.h, it looks like it is null terminated: http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WTF/wtf/text/CString.h&exact_package=chromium&q=CString&type=cs&l=36 Created attachment 165972 [details]
WIP: First pass at JSC.
Comment on attachment 165972 [details] WIP: First pass at JSC. Attachment 165972 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14032861 New failing tests: http/tests/security/contentSecurityPolicy/connect-src-xmlhttprequest-blocked.html Comment on attachment 165972 [details] WIP: First pass at JSC. View in context: https://bugs.webkit.org/attachment.cgi?id=165972&action=review Just some random notes I had while looking at this patch. > Source/WebCore/bindings/v8/V8ThrowException.cpp:51 > +v8::Handle<v8::Value> V8ThrowException::setDOMException(int ec, const WTF::String& exceptionContext, v8::Isolate* isolate) Two spaces here. > Source/WebCore/inspector/InspectorResourceAgent.cpp:614 > ExceptionCode code; > + String errorDescription; Why are these at the top of the function? It's not a plain C file. Created attachment 166042 [details]
Patch
Thanks, Alexey and Adam. I've addressed the pre-review comments, and fleshed out the JSC side. Would one of you mind taking a closer look when you have a few minutes? This patch really combines two separate things: plumbing through bindings on the one hand, and the first consumer of the new plumbing on the other. Do you think it's worth splitting this into two smaller patches? Comment on attachment 166042 [details]
Patch
After a conversation on IRC with Adam and Maciej, I'm dropping the review flag for the moment. The current suggestion is not to expose the additional context to JavaScript, but to store it on ExceptionBase, and teach the Inspector how to display it.
This give us the best of both worlds: developers get good error messages, and attackers don't get any additional information than they're getting already.
Created attachment 166300 [details]
WIP: first pass at JSC Inspector-only context.
The JSC side looks pretty trivial, but V8's going to be a pain. Maciej, Adam: does the approach in the most recent patch make sense? Comment on attachment 166300 [details] WIP: first pass at JSC Inspector-only context. Attachment 166300 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14078060 New failing tests: http/tests/security/contentSecurityPolicy/connect-src-xmlhttprequest-blocked.html (In reply to comment #14) > (From update of attachment 166042 [details]) > After a conversation on IRC with Adam and Maciej, I'm dropping the review flag for the moment. The current suggestion is not to expose the additional context to JavaScript, but to store it on ExceptionBase, and teach the Inspector how to display it. > > This give us the best of both worlds: developers get good error messages, and attackers don't get any additional information than they're getting already. Kind of. It's becoming common practice for web apps to send back JS stacks to the server so they can diagnose errors in the wild. In the same way that better error messages would help in the console, they would help here. Could we instead expose a safe version of the better error message to JS (i.e. don't include the URL at all)? (In reply to comment #18) > Kind of. It's becoming common practice for web apps to send back JS stacks to the server so they can diagnose errors in the wild. In the same way that better error messages would help in the console, they would help here. Could we instead expose a safe version of the better error message to JS (i.e. don't include the URL at all)? That's a good point. My worry is that we'd end up asking folks to pass in a "safeContext" and "descriptiveContext". If we did that, I think we'd probably not make much progress. Doing nothing is simpler than carefully considering what should go where; I suspect that's what many developers would do. Do you have a suggestion for an implementation that would avoid that sort of issue? Unassigning myself; let's be realistic about what I'm actually working on. :/ I think this is resolved by Bug 155777 and related updates since then. |
The following code throws "SECURITY_ERR DOM Exception 18". It would be ideal if that exception contained a message with actionable detail, as opposed to being the exact same text that's used for every other thrown SECURITY_ERR exception.: <!doctype html> <html> <head> <title>Bug 152212</title> <meta http-equiv="X-WebKit-CSP" content="connect-src 'none'"> </head> <body> <script> var xhr = new XMLHttpRequest(); var url = 'https://api.twitter.com/1/trends/daily.json?exclude=hashtags'; xhr.open('GET', url, true); xhr.send(); </script> </body> </html> This will involve piping a descriptive error message down into the V8/JSC bindings (V8ThrowException::setDOMException and WebCore::setDOMException respectively), which probably will be done in a separate bug.