Bug 66909 – XMLHttpRequest method/header validation is not available for other untrusted HTTP requests.

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

[Bill Budge]

Created attachment 105104 [details]
Preliminary patch, to get guidance from domain experts

It would be convenient if we could reuse the HTTP method / header restriction checks in XMLHttpRequest for other uses. Currently this logic is private. The obvious way to do this is to expose the validation checks as static methods on the class.

A problem with this is that a static HashSet is used to speed header field checks, and this is initialized in a complex way in the constructor. The attachment uses a static sorted array of c-strings instead, and uses binary search to determine if the field is forbidden. This requires a conversion of the UTF16 String to a UTF-8 c-string before checking.

[Bill Budge]

I need to modify this to do a case-insensitive compare.

[David Levin]

Comment on attachment 105104 [details]
Preliminary patch, to get guidance from domain experts

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

I like the overall approach. I have a few misc comments that I happened to see.

I do wonder if these should be in XMLHttpRequest since the calls are for other untrusted HTTP requests.

> Source/WebCore/xml/XMLHttpRequest.cpp:5
>   *  Copyright (C) 2008 David Levin <[e-mail]>

You can remove this line. I put it there before I understood that we were to use "Google Inc." so the next line is better.

> Source/WebCore/xml/XMLHttpRequest.cpp:361
> +// This array must remain in lexicographical order for the binary search to work.

It would be good to write some debug code somewhere to verify that it is in order.

> Source/WebCore/xml/XMLHttpRequest.cpp:388
> +    return strcmp((const char*) a, *(const char**) b);

Use C++ style casts.

> Source/WebCore/xml/XMLHttpRequest.cpp:393
> +    void* forbidden = bsearch(name.utf8().data(), forbiddenHeaders, ARRAYSIZE_UNSAFE(forbiddenHeaders), sizeof(const char*), compareHeader);

It would feel better to me to express the sizeof in terms of forbiddenHeaders.

> Source/WebCore/xml/XMLHttpRequest.h:4
> + *  Copyright (C) 2008, 2011 Google Inc. All rights reserved.

I don't know where the "2008., " comes from for this one.

[Alexey Proskuryakov]

Can you tell what the other expected uses of this list are? I don't think that it appears anywhere besides the XMLHttpRequest spec.

[Bill Budge]

The reason for this is to implement untrusted HTTP requests in Chromium's Pepper API (PPAPI). We're trying to provide the same restricted URL requests to untrusted code as in Javascript. These methods will be used in an upcoming patch to Source/Webkit/chromium/src/AssociatedURLLoader. We're adding a new 'untrustedHttp' field to WebURLLoaderOptions, which will cause AssociatedURLLoader to check the method and headers in exactly the same way as XMLHttpRequest.

[Alexey Proskuryakov]

OK, thanks! I'm a little surprised that this level of functionality is going to be in WebCore, not in plug-ins. Isn't it plug-ins responsibility to provide network loading mechanisms with custom amount of trust?

Speaking about implementation, I don't understand why it was changed from Strings to char arrays. What makes the HashSet implementation unsuitable for being called from a plug-in? You could put initializeXMLHttpRequestStaticData() in any other function.

[Adam Barth]

> OK, thanks! I'm a little surprised that this level of functionality is going to be in WebCore, not in plug-ins. Isn't it plug-ins responsibility to provide network loading mechanisms with custom amount of trust?

PPAPI supports both trusted and untrusted plugins.  For untrusted plugs, we need to provide the security.  We could duplicate this logic outside of WebCore, but that seems inferior to using the same code for XMLHttpRequest and URL requests from untrusted plugins.

[Bill Budge]

I would prefer to use the HashSet implementation - it's much slicker than using char arrays. If you don't feel that calling initializeXMLHttpRequestStaticData (with it's lock/unlock) for every header field check is a performance concern, I will do that. The cost could be reduced by adding a public static method to perform this initialization, but I felt that design was more error prone.

[Bill Budge]

Created attachment 105223 [details]
Proposed Patch

Reworked the patch to use HashSet as before. My understanding is that uncontested mutex lock/unlock is negligible in terms of cost.

[Alexey Proskuryakov]

Comment on attachment 105223 [details]
Proposed Patch

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

Seems fine in general. However, since this patch only exposes existing functionality, the bar should be pretty high on how well it's exposed, particularly in regards to naming.

> Source/WebCore/xml/XMLHttpRequest.h:115
> +    static bool isValidToken(const String&);

This method has nothing to do with XHR, it's pure HTTP. If it needs to be exposed, it should go into a new header in platform/network.

> Source/WebCore/xml/XMLHttpRequest.h:116
> +    static bool isSafeMethod(const String&);

This needs a better name. Safe in what sense? There is nothing safe on the Internet.

> Source/WebCore/xml/XMLHttpRequest.h:117
> +    static String canonicalizeMethod(const String&);

This really needs a better name - "canonicalize" does not tell the reader anything about what's going to happen when it's not defined in relevant specs. It could as well be "transmogrify"!

> Source/WebCore/xml/XMLHttpRequest.h:118
> +    static bool isSafeRequestHeader(const String&);

Same comment about "safe".

> Source/WebCore/xml/XMLHttpRequest.h:119
> +    static bool isValidHeaderValue(const String&);

This method also has nothing to do with XHR, and shouldn't be exposed from here.

[Bill Budge]

Looking at platform/network/HTTPParsers.h, this seems like it might be a good place to move these static methods. What do you think?

[Alexey Proskuryakov]

Since this is not about parsing, I would have added a new header.

[Bill Budge]

Created attachment 105247 [details]
Proposed Patch

Broke out HTTP validation into platform/network/HTTPValidation.h / .cpp

[Bill Budge]

Comment on attachment 105247 [details]
Proposed Patch

Bad patch. Fixing it now.

[Bill Budge]

Created attachment 105250 [details]
Proposed Patch

[Bill Budge]

Created attachment 105251 [details]
Proposed Patch

[Bill Budge]

Created attachment 105269 [details]
Proposed Patch

Added HTTPValidation to build files. Please let me know if I missed any.

[Bill Budge]

Created attachment 105283 [details]
Proposed Patch

Missed a Windows build file.

[David Levin]

Comment on attachment 105283 [details]
Proposed Patch

This looks good to me. I'll wait a bit to see if ap has any other comments.

[Alexey Proskuryakov]

Comment on attachment 105283 [details]
Proposed Patch

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

Looks good to me. I'm not fond of "HTTPValidation" file name, but I don't have a better suggestion.

> Source/WebCore/platform/network/HTTPValidation.cpp:36
> +using namespace WTF;

There should never be a reason to do "using namespace WTF" - all public WTF symbols should be exported into global namespace by their headers.

[Bill Budge]

Created attachment 105688 [details]
Proposed Patch

[WebKit Review Bot]

Comment on attachment 105688 [details]
Proposed Patch

Clearing flags on attachment: 105688

Committed r94128: <http://trac.webkit.org/changeset/94128>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

This patch appears to have broken Leopard build:

http://build.webkit.org/builders/Leopard%20Intel%20Debug%20%28Build%29/builds/38818/steps/compile-webkit/logs/stdio
Undefined symbols:
  "__ZN7WebCore22isValidHTTPHeaderValueERKN3WTF6StringE", referenced from:
      __ZN7WebCore14XMLHttpRequest16setRequestHeaderERKN3WTF12AtomicStringERKNS1_6StringERi in XMLHttpRequest.o
  "__ZN7WebCore16isValidHTTPTokenERKN3WTF6StringE", referenced from:
      __ZN7WebCore14XMLHttpRequest16setRequestHeaderERKN3WTF12AtomicStringERKNS1_6StringERi in XMLHttpRequest.o
      __ZN7WebCore14XMLHttpRequest4openERKN3WTF6StringERKNS_4KURLEbRi in XMLHttpRequest.o
ld: symbol(s) not found

[Ryosuke Niwa]

So you apparently forgot to add your file to xcodeproj :(

[Ryosuke Niwa]

Fixed in http://trac.webkit.org/changeset/94136.

[Bill Budge]

You fixed it? Thanks!

This is the first time I've had to add a file and I couldn't find any docs about what needed changing. I asked around a bit but obviously missed this. By the way, is the best way to fix the .xcodeproj file to load it into XCode?

[David Levin]

(In reply to comment #26)
> You fixed it? Thanks!
> 
> This is the first time I've had to add a file and I couldn't find any docs about what needed changing. I asked around a bit but obviously missed this. By the way, is the best way to fix the .xcodeproj file to load it into XCode?

No docs. You just need to find another change which added some files 

I should have caught this and told you this before.

[David Levin]

btw, consider hanging out in irc. Also, I'm going to nominate you to be a committer so that you can get mac EWS results :).

[Ryosuke Niwa]

(In reply to comment #26)
> This is the first time I've had to add a file and I couldn't find any docs about what needed changing. I asked around a bit but obviously missed this. By the way, is the best way to fix the .xcodeproj file to load it into XCode?

Unfortunately, there's no tool or documentation on how to do this other than to say open it up in XCode and add them using GUI :(

Also, if you're using WebCore files you're adding in WebKit layer, then you'd have to right click on those files and set the role to private.