diff options
author | ttuttle <ttuttle@chromium.org> | 2015-01-05 16:55:24 -0800 |
---|---|---|
committer | Commit bot <commit-bot@chromium.org> | 2015-01-06 00:56:04 +0000 |
commit | 7933c117fd16b192e70609c331641e9112af5e42 (patch) | |
tree | 961621167d5c99929849d7aa9f46cc5f71b22f97 /net | |
parent | 8249145b13b4893f0de507cbe26de72dd7ffbd79 (diff) | |
download | chromium_src-7933c117fd16b192e70609c331641e9112af5e42.zip chromium_src-7933c117fd16b192e70609c331641e9112af5e42.tar.gz chromium_src-7933c117fd16b192e70609c331641e9112af5e42.tar.bz2 |
Sanitize headers in Proxy Authentication Required responses
BUG=431504
Review URL: https://codereview.chromium.org/769043003
Cr-Commit-Position: refs/heads/master@{#310014}
Diffstat (limited to 'net')
-rw-r--r-- | net/http/http_network_transaction_unittest.cc | 87 | ||||
-rw-r--r-- | net/http/http_proxy_client_socket.cc | 24 | ||||
-rw-r--r-- | net/http/proxy_client_socket.cc | 51 | ||||
-rw-r--r-- | net/http/proxy_client_socket.h | 12 | ||||
-rw-r--r-- | net/spdy/spdy_proxy_client_socket.cc | 20 |
5 files changed, 150 insertions, 44 deletions
diff --git a/net/http/http_network_transaction_unittest.cc b/net/http/http_network_transaction_unittest.cc index 19dfeb7..21a8c00 100644 --- a/net/http/http_network_transaction_unittest.cc +++ b/net/http/http_network_transaction_unittest.cc @@ -2546,11 +2546,11 @@ TEST_P(HttpNetworkTransactionTest, BasicAuthProxyKeepAlive) { NetLog::PHASE_NONE); const HttpResponseInfo* response = trans->GetResponseInfo(); - ASSERT_TRUE(response != NULL); - ASSERT_FALSE(response->headers.get() == NULL); + ASSERT_TRUE(response); + ASSERT_TRUE(response->headers); EXPECT_TRUE(response->headers->IsKeepAlive()); EXPECT_EQ(407, response->headers->response_code()); - EXPECT_EQ(10, response->headers->GetContentLength()); + EXPECT_EQ(-1, response->headers->GetContentLength()); EXPECT_TRUE(HttpVersion(1, 1) == response->headers->GetHttpVersion()); EXPECT_TRUE(CheckBasicProxyAuth(response->auth_challenge.get())); @@ -2565,11 +2565,11 @@ TEST_P(HttpNetworkTransactionTest, BasicAuthProxyKeepAlive) { EXPECT_EQ(OK, rv); response = trans->GetResponseInfo(); - ASSERT_TRUE(response != NULL); - ASSERT_FALSE(response->headers.get() == NULL); + ASSERT_TRUE(response); + ASSERT_TRUE(response->headers); EXPECT_TRUE(response->headers->IsKeepAlive()); EXPECT_EQ(407, response->headers->response_code()); - EXPECT_EQ(10, response->headers->GetContentLength()); + EXPECT_EQ(-1, response->headers->GetContentLength()); EXPECT_TRUE(HttpVersion(1, 1) == response->headers->GetHttpVersion()); EXPECT_TRUE(CheckBasicProxyAuth(response->auth_challenge.get())); @@ -2603,10 +2603,11 @@ TEST_P(HttpNetworkTransactionTest, BasicAuthProxyCancelTunnel) { // The proxy responds to the connect with a 407. MockRead data_reads[] = { - MockRead("HTTP/1.1 407 Proxy Authentication Required\r\n"), - MockRead("Proxy-Authenticate: Basic realm=\"MyRealm1\"\r\n"), - MockRead("Content-Length: 10\r\n\r\n"), - MockRead(SYNCHRONOUS, ERR_UNEXPECTED), // Should not be reached. + MockRead("HTTP/1.1 407 Proxy Authentication Required\r\n"), + MockRead("Proxy-Authenticate: Basic realm=\"MyRealm1\"\r\n"), + MockRead("Content-Length: 10\r\n\r\n"), + MockRead("0123456789"), // Should not be reached. + MockRead(SYNCHRONOUS, ERR_UNEXPECTED), }; StaticSocketDataProvider data(data_reads, arraysize(data_reads), @@ -2622,12 +2623,74 @@ TEST_P(HttpNetworkTransactionTest, BasicAuthProxyCancelTunnel) { EXPECT_EQ(OK, rv); const HttpResponseInfo* response = trans->GetResponseInfo(); - ASSERT_TRUE(response != NULL); + ASSERT_TRUE(response); + ASSERT_TRUE(response->headers); + EXPECT_TRUE(response->headers->IsKeepAlive()); + EXPECT_EQ(407, response->headers->response_code()); + EXPECT_TRUE(HttpVersion(1, 1) == response->headers->GetHttpVersion()); + + std::string response_data; + rv = ReadTransaction(trans.get(), &response_data); + EXPECT_EQ(ERR_TUNNEL_CONNECTION_FAILED, rv); + + // Flush the idle socket before the HttpNetworkTransaction goes out of scope. + session->CloseAllConnections(); +} + +// Test that we don't pass extraneous headers from the proxy's response to the +// caller when the proxy responds to CONNECT with 407. +TEST_P(HttpNetworkTransactionTest, SanitizeProxyAuthHeaders) { + HttpRequestInfo request; + request.method = "GET"; + request.url = GURL("https://www.google.com/"); + request.load_flags = 0; + + // Configure against proxy server "myproxy:70". + session_deps_.proxy_service.reset(ProxyService::CreateFixed("myproxy:70")); + + scoped_refptr<HttpNetworkSession> session(CreateSession(&session_deps_)); + + scoped_ptr<HttpTransaction> trans( + new HttpNetworkTransaction(DEFAULT_PRIORITY, session.get())); + + // Since we have proxy, should try to establish tunnel. + MockWrite data_writes[] = { + MockWrite( + "CONNECT www.google.com:443 HTTP/1.1\r\n" + "Host: www.google.com\r\n" + "Proxy-Connection: keep-alive\r\n\r\n"), + }; + + // The proxy responds to the connect with a 407. + MockRead data_reads[] = { + MockRead("HTTP/1.1 407 Proxy Authentication Required\r\n"), + MockRead("X-Foo: bar\r\n"), + MockRead("Set-Cookie: foo=bar\r\n"), + MockRead("Proxy-Authenticate: Basic realm=\"MyRealm1\"\r\n"), + MockRead("Content-Length: 10\r\n\r\n"), + MockRead(SYNCHRONOUS, ERR_UNEXPECTED), // Should not be reached. + }; + + StaticSocketDataProvider data(data_reads, arraysize(data_reads), data_writes, + arraysize(data_writes)); + session_deps_.socket_factory->AddSocketDataProvider(&data); + + TestCompletionCallback callback; + int rv = trans->Start(&request, callback.callback(), BoundNetLog()); + EXPECT_EQ(ERR_IO_PENDING, rv); + + rv = callback.WaitForResult(); + EXPECT_EQ(OK, rv); + + const HttpResponseInfo* response = trans->GetResponseInfo(); + ASSERT_TRUE(response); + ASSERT_TRUE(response->headers); EXPECT_TRUE(response->headers->IsKeepAlive()); EXPECT_EQ(407, response->headers->response_code()); - EXPECT_EQ(10, response->headers->GetContentLength()); EXPECT_TRUE(HttpVersion(1, 1) == response->headers->GetHttpVersion()); + EXPECT_FALSE(response->headers->HasHeader("X-Foo")); + EXPECT_FALSE(response->headers->HasHeader("Set-Cookie")); std::string response_data; rv = ReadTransaction(trans.get(), &response_data); diff --git a/net/http/http_proxy_client_socket.cc b/net/http/http_proxy_client_socket.cc index 515301c..97945d9 100644 --- a/net/http/http_proxy_client_socket.cc +++ b/net/http/http_proxy_client_socket.cc @@ -483,25 +483,27 @@ int HttpProxyClientSocket::DoReadHeadersComplete(int result) { // sanitize the response. This still allows a rogue HTTPS proxy to // redirect an HTTPS site load to a similar-looking site, but no longer // allows it to impersonate the site the user requested. - if (is_https_proxy_ && SanitizeProxyRedirect(&response_, request_.url)) { - bool is_connection_reused = http_stream_parser_->IsConnectionReused(); - redirect_has_load_timing_info_ = - transport_->GetLoadTimingInfo( - is_connection_reused, &redirect_load_timing_info_); - transport_.reset(); - http_stream_parser_.reset(); - return ERR_HTTPS_PROXY_TUNNEL_RESPONSE; + if (!is_https_proxy_ || !SanitizeProxyRedirect(&response_)) { + LogBlockedTunnelResponse(); + return ERR_TUNNEL_CONNECTION_FAILED; } - // We're not using an HTTPS proxy, or we couldn't sanitize the redirect. - LogBlockedTunnelResponse(); - return ERR_TUNNEL_CONNECTION_FAILED; + redirect_has_load_timing_info_ = transport_->GetLoadTimingInfo( + http_stream_parser_->IsConnectionReused(), + &redirect_load_timing_info_); + transport_.reset(); + http_stream_parser_.reset(); + return ERR_HTTPS_PROXY_TUNNEL_RESPONSE; case 407: // Proxy Authentication Required // We need this status code to allow proxy authentication. Our // authentication code is smart enough to avoid being tricked by an // active network attacker. // The next state is intentionally not set as it should be STATE_NONE; + if (!SanitizeProxyAuth(&response_)) { + LogBlockedTunnelResponse(); + return ERR_TUNNEL_CONNECTION_FAILED; + } return HandleProxyAuthChallenge(auth_.get(), &response_, net_log_); default: diff --git a/net/http/proxy_client_socket.cc b/net/http/proxy_client_socket.cc index dcfae03..3c539c6 100644 --- a/net/http/proxy_client_socket.cc +++ b/net/http/proxy_client_socket.cc @@ -17,6 +17,20 @@ namespace net { +namespace { + +void CopyHeaderValues(scoped_refptr<HttpResponseHeaders> source, + scoped_refptr<HttpResponseHeaders> dest, + const std::string& header_name) { + void* iter = NULL; + std::string header_value; + + while (source->EnumerateHeader(&iter, header_name, &header_value)) + dest->AddHeader(header_name + ": " + header_value); +} + +} // namespace + // static void ProxyClientSocket::BuildTunnelRequest( const HttpRequestInfo& request_info, @@ -72,22 +86,39 @@ 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()); + + scoped_refptr<HttpResponseHeaders> old_headers = response->headers; + + const char kHeaders[] = "HTTP/1.1 407 Proxy Authentication Required\n\n"; + scoped_refptr<HttpResponseHeaders> new_headers = new HttpResponseHeaders( + HttpUtil::AssembleRawHeaders(kHeaders, arraysize(kHeaders))); + + new_headers->ReplaceStatusLine(old_headers->GetStatusLine()); + CopyHeaderValues(old_headers, new_headers, "Connection"); + CopyHeaderValues(old_headers, new_headers, "Proxy-Authenticate"); + + response->headers = new_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.0 302 Found\n" + "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()); diff --git a/net/http/proxy_client_socket.h b/net/http/proxy_client_socket.h index aa59038..f3347c8 100644 --- a/net/http/proxy_client_socket.h +++ b/net/http/proxy_client_socket.h @@ -74,13 +74,19 @@ class NET_EXPORT_PRIVATE ProxyClientSocket : public StreamSocket { const GURL& url, bool is_https_proxy); + // When a proxy authentication response is received during tunnel + // construction, this method should be called to strip everything + // but the auth header from the redirect response. If it returns + // false, the response should be discarded and tunnel construction should + // fail. + static bool SanitizeProxyAuth(HttpResponseInfo* response); + // When a redirect (e.g. 302 response) is received during tunnel // construction, this method should be called to strip everything // but the Location header from the redirect response. If it returns // false, the response should be discarded and tunnel construction should - // fail. |url| is for logging purposes. - static bool SanitizeProxyRedirect(HttpResponseInfo* response, - const GURL& url); + // fail. + static bool SanitizeProxyRedirect(HttpResponseInfo* response); private: DISALLOW_COPY_AND_ASSIGN(ProxyClientSocket); diff --git a/net/spdy/spdy_proxy_client_socket.cc b/net/spdy/spdy_proxy_client_socket.cc index afc1953..661cfd9 100644 --- a/net/spdy/spdy_proxy_client_socket.cc +++ b/net/spdy/spdy_proxy_client_socket.cc @@ -414,20 +414,24 @@ int SpdyProxyClientSocket::DoReadReplyComplete(int result) { case 302: // Found / Moved Temporarily // Try to return a sanitized response so we can follow auth redirects. // If we can't, fail the tunnel connection. - if (SanitizeProxyRedirect(&response_, request_.url)) { - redirect_has_load_timing_info_ = - spdy_stream_->GetLoadTimingInfo(&redirect_load_timing_info_); - // Note that this triggers a RST_STREAM_CANCEL. - spdy_stream_->DetachDelegate(); - next_state_ = STATE_DISCONNECTED; - return ERR_HTTPS_PROXY_TUNNEL_RESPONSE; - } else { + if (!SanitizeProxyRedirect(&response_)) { LogBlockedTunnelResponse(); return ERR_TUNNEL_CONNECTION_FAILED; } + redirect_has_load_timing_info_ = + spdy_stream_->GetLoadTimingInfo(&redirect_load_timing_info_); + // Note that this triggers a RST_STREAM_CANCEL. + spdy_stream_->DetachDelegate(); + next_state_ = STATE_DISCONNECTED; + return ERR_HTTPS_PROXY_TUNNEL_RESPONSE; + case 407: // Proxy Authentication Required next_state_ = STATE_OPEN; + if (!SanitizeProxyAuth(&response_)) { + LogBlockedTunnelResponse(); + return ERR_TUNNEL_CONNECTION_FAILED; + } return HandleProxyAuthChallenge(auth_.get(), &response_, net_log_); default: |