Commit de398a55 authored by David Van Cleve's avatar David Van Cleve Committed by Chromium LUCI CQ

Reland a couple CHECKs on request headers / UA validity

This relands:
 - http://crrev/e98b84e "net: Change HttpRequestHeaders::SetHeader's DCHECKs to CHECK"
 - http://crrev/f998816347be "content: Enforce precondition that UA overrides must..."

The first change upgraded a security-relevant network stack DCHECK, ensuring no invalid characters (\r, \n, \0) in headers, to a CHECK; the second bubbled this invariant up to the //content API so that its "override the UA" method wouldn't result in crashes when the CHECKs failed.

We ended up reverting these changes out of Android WebView compat concerns. Torne (thanks!) has since then landed and analyzed some metrics and verified (see the linked bug) that there won't be too big a compat impact, so we're relanding these initial changes. Rather than reintroducing the second CHECK, in web_contents_impl.cc, this CL just adds an early return to WebContentsImpl::SetUserAgentOverride in the case that the given override is not a valid HTTP header value.

Bug: 1105745
Change-Id: Ie2924bdbfbfd092c12e52fda8eaa52aa40e5425d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2589666Reviewed-by: default avatarDevlin <rdevlin.cronin@chromium.org>
Reviewed-by: default avatarMatt Menke <mmenke@chromium.org>
Reviewed-by: default avatarCharlie Reis <creis@chromium.org>
Commit-Queue: David Van Cleve <davidvc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#839485}
parent a11b9535
......@@ -1576,6 +1576,11 @@ void WebContentsImpl::SetUserAgentOverride(
if (GetUserAgentOverride() == ua_override)
return;
if (!ua_override.ua_string_override.empty() &&
!net::HttpUtil::IsValidHeaderValue(ua_override.ua_string_override)) {
return;
}
should_override_user_agent_in_new_tabs_ = override_in_new_tabs;
renderer_preferences_.user_agent_override = ua_override;
......
......@@ -403,6 +403,9 @@ class WebContents : public PageNavigator,
// renderer initiated, then is-overriding-user-agent is set to true for the
// NavigationEntry. See SetRendererInitiatedUserAgentOverrideOption() for
// details on how renderer initiated navigations are configured.
//
// If nonempty, |ua_override|'s value must not contain '\0', '\r', or '\n' (in
// other words, it must be a valid HTTP header value).
virtual void SetUserAgentOverride(const blink::UserAgentOverride& ua_override,
bool override_in_new_tabs) = 0;
......
......@@ -108,8 +108,8 @@ void HttpRequestHeaders::SetHeader(const base::StringPiece& key,
const base::StringPiece& value) {
// Invalid header names or values could mean clients can attach
// browser-internal headers.
DCHECK(HttpUtil::IsValidHeaderName(key)) << key;
DCHECK(HttpUtil::IsValidHeaderValue(value)) << key << ":" << value;
CHECK(HttpUtil::IsValidHeaderName(key)) << key;
CHECK(HttpUtil::IsValidHeaderValue(value)) << key << ":" << value;
SetHeaderInternal(key, value);
}
......@@ -117,8 +117,8 @@ void HttpRequestHeaders::SetHeaderIfMissing(const base::StringPiece& key,
const base::StringPiece& value) {
// Invalid header names or values could mean clients can attach
// browser-internal headers.
DCHECK(HttpUtil::IsValidHeaderName(key));
DCHECK(HttpUtil::IsValidHeaderValue(value));
CHECK(HttpUtil::IsValidHeaderName(key));
CHECK(HttpUtil::IsValidHeaderValue(value));
auto it = FindHeader(key);
if (it == headers_.end())
headers_.push_back(HeaderKeyValuePair(key, value));
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment