Sanitize headers in Proxy Authentication Required responses

net/http/proxy_client_socket.cc

Index: net/http/proxy_client_socket.cc
diff --git a/net/http/proxy_client_socket.cc b/net/http/proxy_client_socket.cc
index dcfae037ce721c92bd95dfa3aeded77bd2f6bf62..1bbb7000173806460706f8751ff09d5c819312ba 100644
--- a/net/http/proxy_client_socket.cc
+++ b/net/http/proxy_client_socket.cc
@@ -72,22 +72,48 @@ void ProxyClientSocket::LogBlockedTunnelResponse(int http_status_code,
 }
 
 // static
-bool ProxyClientSocket::SanitizeProxyRedirect(HttpResponseInfo* response,
-                                              const GURL& url) {
+bool ProxyClientSocket::SanitizeProxyAuth(HttpResponseInfo* response) {
+  DCHECK(response && response->headers.get());
+
+  const char* kHeaderName = "Proxy-Authenticate";
+  void* iter = NULL;
+  std::string auth, ignored;
+  // Make sure there is one Proxy-Authenticate header...
+  if (!response->headers->EnumerateHeader(&iter, kHeaderName, &auth))
+    return false;
+  // ...but not more than one.

[Ryan Sleevi, 2014/12/08 22:04:39]

BUG: This is NOT required by the spec. Per RFC 7235, Proxy-Authenticate follows
the <n><m>#list form
(see https://tools.ietf.org/html/rfc7235#section-4.3 )

See also https://tools.ietf.org/html/rfc7230#section-7 and
https://tools.ietf.org/html/rfc7230#section-3.2.2

Namely, a sender MAY take a #list and send across multiple headers. And the
client MAY coalesce.

So the right way to do it is to enumerate all of the header values, join them
into a single comma-separated value, and use that as the synthetic
Proxy-Authenticate response.

+  if (response->headers->EnumerateHeader(&iter, kHeaderName, &ignored))
+    return false;
+
+  std::string fake_response_headers = base::StringPrintf(
+      "HTTP/1.1 407 Proxy Authentication Required\n"
+      "Proxy-Authenticate: %s\n"
+      "Content-Length: 0\n"

[Ryan Hamilton, 2014/12/08 22:51:59]

Looks like the content-length should not be 0, since there is actually a
response body.

[Deprecated (see juliatuttle), 2014/12/09 15:31:15]

The goal was to avoid providing the response body, since it can contain scripts
that would execute as the origin we were trying to CONNECT to.

[Ryan Hamilton, 2014/12/10 19:53:42]

Oh! I'm sorry. I misread line 94 to be appending a response body via
auth.c_str() but in re-reading it's obvious that I was confused. Sorry!

[Deprecated (see juliatuttle), 2014/12/10 20:38:40]

I've removed the content-length header entirely, since we error out if the
caller reads the body anyway.

+      "Connection: close\n"

[Ryan Hamilton, 2014/12/08 22:51:59]

I'm not sure that you want to make this connection close because that prevents
the socket from being reused, which I think you need for Negotiate/NTLM auth.

[Deprecated (see juliatuttle), 2014/12/09 15:31:15]

Sigh, okay.

[Deprecated (see juliatuttle), 2014/12/10 20:38:40]

I've passed along Connection headers along with the Proxy-Authenticate ones, so
keep-alive (or close, if the proxy wants) should work.

+      "\n",
+      auth.c_str());
+  std::string raw_headers = HttpUtil::AssembleRawHeaders(
+      fake_response_headers.data(), fake_response_headers.length());
+  response->headers = new HttpResponseHeaders(raw_headers);
+  return true;
+}
+
+// static
+bool ProxyClientSocket::SanitizeProxyRedirect(HttpResponseInfo* response) {
   DCHECK(response && response->headers.get());
 
   std::string location;
   if (!response->headers->IsRedirect(&location))
     return false;
 
-  // Return minimal headers; set "Content-length: 0" to ignore response body.
-  std::string fake_response_headers =
-      base::StringPrintf("HTTP/1.0 302 Found\n"
-                         "Location: %s\n"
-                         "Content-length: 0\n"
-                         "Connection: close\n"
-                         "\n",
-                         location.c_str());
+  // Return minimal headers; set "Content-Length: 0" to ignore response body.
+  std::string fake_response_headers = base::StringPrintf(
+      "HTTP/1.1 302 Found\n"

[Ryan Sleevi, 2014/12/08 22:04:40]

Note: While this may seem trivial, it's going to require someone to go back to
the HTTP/1.1 spec and ensure our response is a fully conformant 1.1 response.
There are difference in requirements for requests (e.g. 1.1 requests MUST have a
Host header), and I haven't checked if 1.1 responses have similar requirements.

This may be the correct thing to do, but a reviewer needs to vet this is true.

[Ryan Hamilton, 2014/12/08 22:53:48]

From my reading, I think there are no required response headers:

       Response      = Status-Line               ; Section 6.1
                       *(( general-header        ; Section 4.5
                        | response-header        ; Section 6.2
                        | entity-header ) CRLF)  ; Section 7.1
                       CRLF
                       [ message-body ]          ; Section 7.2

[Deprecated (see juliatuttle), 2014/12/09 15:31:15]

I can make them both 1.0, but that would break keep-alive, which we need for
auth.

If you want, I can just leave *this* one 1.0.

+      "Location: %s\n"
+      "Content-Length: 0\n"
+      "Connection: close\n"
+      "\n",
+      location.c_str());
   std::string raw_headers =
       HttpUtil::AssembleRawHeaders(fake_response_headers.data(),
                                    fake_response_headers.length());