Bug 67655 – AssociatedURLLoader should check HTTP method / headers for untrusted requests.

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Bill Budge]

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.

[Bill Budge]

Created attachment 106444 [details]
Proposed Patch

[David Levin]

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.

[Bill Budge]

Created attachment 106607 [details]
Proposed Patch

[Bill Budge]

*** Bug 66963 has been marked as a duplicate of this bug. ***

[Bill Budge]

Comment on attachment 106607 [details]
Proposed Patch

Adding some tests for invalid header values.

[Bill Budge]

Created attachment 106631 [details]
Proposed Patch

Improved the tests to check for invalid header values. Improved helper method names.

[WebKit Review Bot]

Comment on attachment 106631 [details]
Proposed Patch

Attachment 106631 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9616952

[Bill Budge]

Comment on attachment 106631 [details]
Proposed Patch

I believe this build failure is spurious.

[Darin Fisher (:fishd, Google)]

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?

[Bill Budge]

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.

[Bill Budge]

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?

[David Levin]

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

[Bill Budge]

Created attachment 107222 [details]
Proposed Patch

Moved header visitor into WebKit namespace. Added information on plans for fixing error reporting from this layer.

[Darin Fisher (:fishd, Google)]

Comment on attachment 107222 [details]
Proposed Patch

R=me, sorry for the delayed response!

[WebKit Review Bot]

Comment on attachment 107222 [details]
Proposed Patch

Clearing flags on attachment: 107222

Committed r95490: <http://trac.webkit.org/changeset/95490>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.