Fix security bug that allowed invalid header fields to be injected by

Fix security bug that allowed invalid header fields to be injected by
setting the HTTP method to a multi-line string.
BUG= http://code.google.com/p/nativeclient/issues/detail?id=2024
TEST=TestShellTests, url_request_info_unittest.cc

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=96888

[bbudge, 2011-08-13 00:00:41]

http://codereview.chromium.org/7645010/diff/1/webkit/plugins/ppapi/ppb_url_re...
File webkit/plugins/ppapi/ppb_url_request_info_impl.cc (right):

http://codereview.chromium.org/7645010/diff/1/webkit/plugins/ppapi/ppb_url_re...
webkit/plugins/ppapi/ppb_url_request_info_impl.cc:74: // converted to
upper-case.
This isn't quite true; this list comes from Webkit code, which states that it is
following Firefox's precedent of upper-casing these.

[brettw, 2011-08-14 18:30:42]

http://codereview.chromium.org/7645010/diff/1/webkit/plugins/ppapi/ppb_url_re...
File webkit/plugins/ppapi/ppb_url_request_info_impl.cc (right):

http://codereview.chromium.org/7645010/diff/1/webkit/plugins/ppapi/ppb_url_re...
webkit/plugins/ppapi/ppb_url_request_info_impl.cc:74: // converted to
upper-case.
Can you expand on "they should be converted to upper-case when...". My first
thought was "if they should be converted to upper case, why don't you just do it
and make the constants upper case?"

http://codereview.chromium.org/7645010/diff/1/webkit/plugins/ppapi/ppb_url_re...
webkit/plugins/ppapi/ppb_url_request_info_impl.cc:400: if
(!AreValidHeaders(value))
Is there a similar attack if one of the other headers contains a newline?

[bbudge, 2011-08-15 15:03:52]

http://codereview.chromium.org/7645010/diff/1/webkit/plugins/ppapi/ppb_url_re...
File webkit/plugins/ppapi/ppb_url_request_info_impl.cc (right):

http://codereview.chromium.org/7645010/diff/1/webkit/plugins/ppapi/ppb_url_re...
webkit/plugins/ppapi/ppb_url_request_info_impl.cc:74: // converted to
upper-case.
On 2011/08/14 18:30:42, brettw wrote:
> Can you expand on "they should be converted to upper-case when...". My first
> thought was "if they should be converted to upper case, why don't you just do
it
> and make the constants upper case?"
I'll improve the comment. I would prefer to make the constants upper case but
didn't see any suitable compare method. Are there case insensitive
(locale-invariant) string compares in string_util?

http://codereview.chromium.org/7645010/diff/1/webkit/plugins/ppapi/ppb_url_re...
webkit/plugins/ppapi/ppb_url_request_info_impl.cc:400: if
(!AreValidHeaders(value))
On 2011/08/14 18:30:42, brettw wrote:
> Is there a similar attack if one of the other headers contains a newline?

We use HttpUtil::HeadersIterator to parse the header name-value pairs. It looks
pretty robust to me.

[brettw, 2011-08-15 15:54:56]

LGTM with improved comment.

http://codereview.chromium.org/7645010/diff/1/webkit/plugins/ppapi/ppb_url_re...
File webkit/plugins/ppapi/ppb_url_request_info_impl.cc (right):

http://codereview.chromium.org/7645010/diff/1/webkit/plugins/ppapi/ppb_url_re...
webkit/plugins/ppapi/ppb_url_request_info_impl.cc:74: // converted to
upper-case.
No, I think lower-case is fine for the constants.

[commit-bot: I haz the power, 2011-08-16 02:49:29]

Change committed as 96888

[darin (slow to review), 2011-08-16 05:23:47]

http://codereview.chromium.org/7645010/diff/6001/webkit/plugins/ppapi/ppb_url...
File webkit/plugins/ppapi/ppb_url_request_info_impl.cc (right):

http://codereview.chromium.org/7645010/diff/6001/webkit/plugins/ppapi/ppb_url...
webkit/plugins/ppapi/ppb_url_request_info_impl.cc:47: bool IsValidToken(const
std::string& token) {
It makes me a bit sad to see this validation code replicated here from webkit. 
an alternative would be to create a webkit API to invoke these validation
routines.

Or, we could even put this validation logic behind AssociatedURLLoader with an
option to disable it.

[bbudge, 2011-08-16 12:40:07]

On 2011/08/16 05:23:47, darin wrote:
>
http://codereview.chromium.org/7645010/diff/6001/webkit/plugins/ppapi/ppb_url...
> File webkit/plugins/ppapi/ppb_url_request_info_impl.cc (right):
> 
>
http://codereview.chromium.org/7645010/diff/6001/webkit/plugins/ppapi/ppb_url...
> webkit/plugins/ppapi/ppb_url_request_info_impl.cc:47: bool IsValidToken(const
> std::string& token) {
> It makes me a bit sad to see this validation code replicated here from webkit.

> an alternative would be to create a webkit API to invoke these validation
> routines.
> 
> Or, we could even put this validation logic behind AssociatedURLLoader with an
> option to disable it.

I agree. I'll look for a good place to put this.