Bug 97654

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:

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:

Description	Flags
WIP	none
WIP: First pass at JSC.	none
Patch	none
WIP: first pass at JSC Inspector-only context.	webkit.review.bot: commit-queue-

Mike West

Reported 2012-09-26 03:40:01 PDT

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.

Attachments
WIP (17.79 KB, patch) 2012-09-26 07:55 PDT, Mike West	no flags	Details Formatted Diff Diff
WIP: First pass at JSC. (21.95 KB, patch) 2012-09-27 04:18 PDT, Mike West	no flags	Details Formatted Diff Diff
Patch (24.71 KB, patch) 2012-09-27 12:03 PDT, Mike West	no flags	Details Formatted Diff Diff
WIP: first pass at JSC Inspector-only context. (26.72 KB, patch) 2012-09-28 13:04 PDT, Mike West	webkit.review.bot: commit-queue-	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Mike West

Comment 1 2012-09-26 07:55:26 PDT

Created attachment 165802 [details] WIP

Mike West

Comment 2 2012-09-26 07:56:19 PDT

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. :/

Mike West

Comment 3 2012-09-26 07:58:25 PDT

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!

Adam Barth

Comment 4 2012-09-26 08:54:17 PDT

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.

Adam Barth

Comment 5 2012-09-26 08:56:08 PDT

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.

Mike West

Comment 6 2012-09-26 09:38:44 PDT

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. :)

Mike West

Comment 7 2012-09-26 09:39:48 PDT

(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. :)

Mike West

Comment 8 2012-09-27 02:59:52 PDT

(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

Mike West

Comment 9 2012-09-27 04:18:34 PDT

Created attachment 165972 [details] WIP: First pass at JSC.

Build Bot

Comment 10 2012-09-27 08:34:22 PDT

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

Alexey Proskuryakov

Comment 11 2012-09-27 10:58:12 PDT

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.

Mike West

Comment 12 2012-09-27 12:03:56 PDT

Created attachment 166042 [details] Patch

Mike West

Comment 13 2012-09-27 12:06:27 PDT

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?

Mike West

Comment 14 2012-09-28 11:28:12 PDT

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.

Mike West

Comment 15 2012-09-28 13:04:14 PDT

Created attachment 166300 [details] WIP: first pass at JSC Inspector-only context.

Mike West

Comment 16 2012-09-28 13:05:44 PDT

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?

WebKit Review Bot

Comment 17 2012-09-28 18:39:53 PDT

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

Ojan Vafai

Comment 18 2012-09-29 09:17:54 PDT

(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)?

Mike West

Comment 19 2012-09-29 10:29:38 PDT

(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?

Mike West

Comment 20 2013-02-07 11:00:46 PST

Unassigning myself; let's be realistic about what I'm actually working on. :/

Brent Fulgham

Comment 21 2016-04-12 13:08:20 PDT

I think this is resolved by Bug 155777 and related updates since then.

Note You need to log in before you can comment on or make changes to this bug.