I think you mean AddHeaderFromString (No 's' after Header).  It calls SetHeader, so should run into the CHECK you added in that case, right?

Could you say a little bit more about the security consequences and contexts in which this could happen?  I'm specifically confused by your statement on the CL (https://codereview.chromium.org/2212883002/) that no code calls this function with single newline or CR, so what use case are we protecting against?

Looks like https://cs.chromium.org/chromium/src/content/browser/loader/resource_dispatcher_host_impl.cc?q=ResourceDispatcherHostimpl::BeginRequest&sq=package:chromium&dr=CSs&l=1360 takes an unvalidated header string from the renderer, and adds them to the URLRequest.

Hrm...I wonder if we should remove AddHeadersFromString/AddHeaderFromString.  Seems like it's used in a couple places:

1)  Consumers of URLFetcher.  These generally have hard-coded "name: value" strings.  These should be trivial to just split apart and switch to using SetHeader.
2)  Tests.  These are presumably as easy to fix as 1).
3)  Code that gets headers from the renderer.  Presumably we should be doing extra validation on there?  Ideally, seems like the renderer should really be sending us name/value pairs as well, which we can validate in the browser individually, instead of using a net/ method to handle them en masse.

For tests I assume it is a convenience issue. AddHeadersFromString could be moved to a test_util file as a standalone function.

The renderer is not supposed to send invalid headers. Perhaps content should have its own variant of AddHeadersFromString() which kills the renderer on bad input?

There's some boring performance consequences of switching from a string representation to a vector<pair> representation for IPC and considering the cost/benefit of that is a separate issue in my opinion. Also the mojo-ification of fetch changes the trade-off.

I'm not really concerned about tests - perfectly fine with a test_util method, or even a ForTests setter.

Anything doing AddHeadersFromString on untrusted input presumably has some specced out rules on what it allows, so implicitly has to convert things into string/value pairs anyways.  I just don't see why we need another version of AddHeadersFromString's logic, just with bonus validation.

How many headers do we generally get from the renderer?  I assume they're a fairly limited set, unless explicitly added to XHRs, but maybe I'm wrong.

For the majority of requests, there is just one header from the renderer - User-Agent. I found that the string comes from just one place -- content::GetWebURLRequestHeaders(). So if we decide to switch to a vector representation, the changes required are minimal.

I CC'd a bunch of Blink network folks to get their perspective on the possible IPC change.

Hrm...  Why do we set the user-agent in the renderer?  We do that Chrome-side anyways.

devtools does set it, I think.

Whether headers are sent as a something-delimited string or as a vector serialized by the IPC library doesn't make any difference in protection against exploited renderer, I think. They'll just send a data for attack to the browser process in the serialization format in use.

At the point where the headers are deserialized, what we should do is just to make sure we store the result into the data structure (in this case, std::vector<HeaderKeyValuePair>) correctly so that any protection against bad data are performed (e.g. banning request with certain set of header names) correctly in terms of how the ultimate server code understands the data. This is achieved by the CHECK introduced by ricea@ in https://codereview.chromium.org/2143903002. I think that's enough.

The additional CR, LF splitting in https://codereview.chromium.org/2212883002/ are not necessary because of the reason I said in the first paragraph, I guess. WDYT?

I rather wonder if DCHECK on the header name should also be changed to CHECK. It's ok when SetHeader() is invoked from AddHeadersFromString() as they don't leave CR/LF in the name part. but AddHeaderFromString() or direct call on SetHeader() may be able to do what ricea@ explained in the opening comment?

The CHECK() introduced in https://codereview.chromium.org/2143903002 is supposed to be temporary because it is unnecessary overhead in the case when the caller obeys the API contract for SetHeader() (which is the case for almost all callers). I should probably change the comments to SetHeader() to make the API contract explicit.

Since I tightened the checks in IsValidHeaderValue(), AddHeadersFromString() is no longer obeying the API contract.

As tyoshino mentioned, AddHeaderFromString() is also disobeying the API contract, so I will look at fixing that first.

Ah, OK. You're trying to move CHECK to the point where the browser process sees data received from the renderer for the first time, and change the rest to DCHECK? That makes sense if feasible.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0753a6f4ffdb5bcd1ff9c4b977f9052878112f42

commit 0753a6f4ffdb5bcd1ff9c4b977f9052878112f42
Author: ricea <ricea@chromium.org>
Date: Fri Aug 12 05:20:58 2016

Avoid adding invalid headers in AddHeaderFromString

In net::HttpRequestHeaders::AddHeaderFromString(), verify that the
header name and value are valid. If they are not, don't add them (and
log a fatal message on debug builds).

BUG= 634225 

Review-Url: https://codereview.chromium.org/2225933004
Cr-Commit-Position: refs/heads/master@{#411556}

[modify] https://crrev.com/0753a6f4ffdb5bcd1ff9c4b977f9052878112f42/net/http/http_content_disposition.cc
[modify] https://crrev.com/0753a6f4ffdb5bcd1ff9c4b977f9052878112f42/net/http/http_request_headers.cc
[modify] https://crrev.com/0753a6f4ffdb5bcd1ff9c4b977f9052878112f42/net/http/http_request_headers.h
[modify] https://crrev.com/0753a6f4ffdb5bcd1ff9c4b977f9052878112f42/net/http/http_util.cc
[modify] https://crrev.com/0753a6f4ffdb5bcd1ff9c4b977f9052878112f42/net/http/http_util.h
[modify] https://crrev.com/0753a6f4ffdb5bcd1ff9c4b977f9052878112f42/net/http/http_util_unittest.cc

The CL in Comment 14 fixes my original concern by preventing headers like the one in my original comment from being added in AddHeaderFromString().

With this change I don't think there's any compelling reason to change AddHeadersFromString() any more.

If no-one has any complaints, I will close this bug next week.