Bug 67655

Summary: AssociatedURLLoader should check HTTP method / headers for untrusted requests.
Product: WebKit Reporter: Bill Budge <bbudge>
Component: PlatformAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, fishd, levin, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Proposed Patch
levin: commit-queue-
Proposed Patch
none
Proposed Patch
fishd: review+, fishd: commit-queue-
Proposed Patch none

Description Bill Budge 2011-09-06 10:51:51 PDT
Perform HTTP method and header validation in AssociatedURLLoader for requests coming from untrusted code (eg. Native Client in Chrome). Use the same code as XMLHttpRequest to reduce code duplication and have behavior identical to XHR in Javascript. Add an 'untrustedHTTP' option to WebURLLoaderOptions, which AssociatedURLLoader can use to determine if it should check the request method and headers.
Comment 1 Bill Budge 2011-09-06 11:02:51 PDT
Created attachment 106444 [details]
Proposed Patch
Comment 2 David Levin 2011-09-06 12:54:53 PDT
Comment on attachment 106444 [details]
Proposed Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=106444&action=review

Looks good to me.

Since there is a chromium/public change, I'll leave the final r+ for Darin.

> Source/WebKit/chromium/public/WebURLLoaderOptions.h:49
> +        , crossOriginRequestPolicy(CrossOriginRequestPolicyDeny) { }

Ideally put the {} on separate lines at this point.
Comment 3 Bill Budge 2011-09-07 11:30:52 PDT
Created attachment 106607 [details]
Proposed Patch
Comment 4 Bill Budge 2011-09-07 11:56:14 PDT
*** Bug 66963 has been marked as a duplicate of this bug. ***
Comment 5 Bill Budge 2011-09-07 11:57:01 PDT
Comment on attachment 106607 [details]
Proposed Patch

Adding some tests for invalid header values.
Comment 6 Bill Budge 2011-09-07 13:55:28 PDT
Created attachment 106631 [details]
Proposed Patch

Improved the tests to check for invalid header values. Improved helper method names.
Comment 7 WebKit Review Bot 2011-09-08 10:05:02 PDT
Comment on attachment 106631 [details]
Proposed Patch

Attachment 106631 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9616952
Comment 8 Bill Budge 2011-09-08 16:27:09 PDT
Comment on attachment 106631 [details]
Proposed Patch

I believe this build failure is spurious.
Comment 9 Darin Fisher (:fishd, Google) 2011-09-09 09:38:50 PDT
Comment on attachment 106631 [details]
Proposed Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=106631&action=review

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:56
> +namespace {

nit: put the anonymous namespace inside the WebKit namespace.  then we
shoudn't need the 'using namespace WebKit' up above.

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:289
> +        m_clientAdapter->setDelayedError(ResourceError());

what are your plans for this?
Comment 10 Bill Budge 2011-09-09 12:28:59 PDT
Once this lands, I can redo PPAPI and WebKit glue code that does all of these checks. As part of that CL I will have to modify URLRequest and URLLoader tests since these errors are reported asynchronously now.

I also have FIXMEs to improve error reporting in the glue code. I will need to define a new error domain in WebKit so AssociatedURLLoader can report syntax vs. security errors in a way that WebKit glue understands. Currently it doesn't know about WebKit error domains, only our net:: one, and reports the former as PP_ERROR. It might make sense to modify this patch to generate ResourceErrors with enough information to make this possible.
Comment 11 Bill Budge 2011-09-09 12:29:56 PDT
Quick question about the red bot. This is the first time I've seen a failure that isn't caused by the patch. Is there a way to re-run the bot?
Comment 12 David Levin 2011-09-09 12:43:34 PDT
(In reply to comment #11)
> Quick question about the red bot. This is the first time I've seen a failure that isn't caused by the patch. Is there a way to re-run the bot?

Reupload the patch is the only way that I know of. (New attachment).
Comment 13 Bill Budge 2011-09-13 14:01:11 PDT
Created attachment 107222 [details]
Proposed Patch

Moved header visitor into WebKit namespace. Added information on plans for fixing error reporting from this layer.
Comment 14 Darin Fisher (:fishd, Google) 2011-09-19 14:58:49 PDT
Comment on attachment 107222 [details]
Proposed Patch

R=me, sorry for the delayed response!
Comment 15 WebKit Review Bot 2011-09-19 16:01:59 PDT
Comment on attachment 107222 [details]
Proposed Patch

Clearing flags on attachment: 107222

Committed r95490: <http://trac.webkit.org/changeset/95490>
Comment 16 WebKit Review Bot 2011-09-19 16:02:04 PDT
All reviewed patches have been landed.  Closing bug.