From 34f63b55b51e8fc46ad86334783a665d5f487738 Mon Sep 17 00:00:00 2001 From: ttuttle Date: Wed, 4 Mar 2015 20:33:01 -0800 Subject: [PATCH] SanitizeProxyAuth: Whitelist all hop-by-hop headers We were discarding "Proxy-Authenticate: keep-alive" headers in HTTP/1.0 responses, which broke multi-step connection-level authentication methods for proxies that sent HTTP/1.0 responses. BUG=463937 Review URL: https://codereview.chromium.org/982733002 Cr-Commit-Position: refs/heads/master@{#319219} --- net/http/http_network_transaction_unittest.cc | 236 +++++++++++++++++- net/http/proxy_client_socket.cc | 14 +- 2 files changed, 243 insertions(+), 7 deletions(-) diff --git a/net/http/http_network_transaction_unittest.cc b/net/http/http_network_transaction_unittest.cc index 6e1ad43b13812..727ef700f95e5 100644 --- a/net/http/http_network_transaction_unittest.cc +++ b/net/http/http_network_transaction_unittest.cc @@ -2372,7 +2372,125 @@ TEST_P(HttpNetworkTransactionTest, BasicAuthKeepAliveImpatientServer) { // Test the request-challenge-retry sequence for basic auth, over a connection // that requires a restart when setting up an SSL tunnel. -TEST_P(HttpNetworkTransactionTest, BasicAuthProxyNoKeepAlive) { +TEST_P(HttpNetworkTransactionTest, BasicAuthProxyNoKeepAliveHttp10) { + HttpRequestInfo request; + request.method = "GET"; + request.url = GURL("https://www.google.com/"); + // when the no authentication data flag is set. + request.load_flags = net::LOAD_DO_NOT_SEND_AUTH_DATA; + + // Configure against proxy server "myproxy:70". + session_deps_.proxy_service.reset( + ProxyService::CreateFixedFromPacResult("PROXY myproxy:70")); + CapturingBoundNetLog log; + session_deps_.net_log = log.bound().net_log(); + scoped_refptr session(CreateSession(&session_deps_)); + + // Since we have proxy, should try to establish tunnel. + MockWrite data_writes1[] = { + 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"), + + // After calling trans->RestartWithAuth(), this is the request we should + // be issuing -- the final header line contains the credentials. + MockWrite( + "CONNECT www.google.com:443 HTTP/1.1\r\n" + "Host: www.google.com\r\n" + "Proxy-Connection: keep-alive\r\n" + "Proxy-Authorization: Basic Zm9vOmJhcg==\r\n\r\n"), + + MockWrite( + "GET / HTTP/1.1\r\n" + "Host: www.google.com\r\n" + "Connection: keep-alive\r\n\r\n"), + }; + + // The proxy responds to the connect with a 407, using a persistent + // connection. + MockRead data_reads1[] = { + // No credentials. + MockRead("HTTP/1.0 407 Proxy Authentication Required\r\n"), + MockRead("Proxy-Authenticate: Basic realm=\"MyRealm1\"\r\n\r\n"), + + MockRead("HTTP/1.0 200 Connection Established\r\n\r\n"), + + MockRead("HTTP/1.1 200 OK\r\n"), + MockRead("Content-Type: text/html; charset=iso-8859-1\r\n"), + MockRead("Content-Length: 5\r\n\r\n"), + MockRead(SYNCHRONOUS, "hello"), + }; + + StaticSocketDataProvider data1(data_reads1, arraysize(data_reads1), + data_writes1, arraysize(data_writes1)); + session_deps_.socket_factory->AddSocketDataProvider(&data1); + SSLSocketDataProvider ssl(ASYNC, OK); + session_deps_.socket_factory->AddSSLSocketDataProvider(&ssl); + + TestCompletionCallback callback1; + + scoped_ptr trans( + new HttpNetworkTransaction(DEFAULT_PRIORITY, session.get())); + + int rv = trans->Start(&request, callback1.callback(), log.bound()); + EXPECT_EQ(ERR_IO_PENDING, rv); + + rv = callback1.WaitForResult(); + EXPECT_EQ(OK, rv); + net::CapturingNetLog::CapturedEntryList entries; + log.GetEntries(&entries); + size_t pos = ExpectLogContainsSomewhere( + entries, 0, NetLog::TYPE_HTTP_TRANSACTION_SEND_TUNNEL_HEADERS, + NetLog::PHASE_NONE); + ExpectLogContainsSomewhere( + entries, pos, NetLog::TYPE_HTTP_TRANSACTION_READ_TUNNEL_RESPONSE_HEADERS, + NetLog::PHASE_NONE); + + const HttpResponseInfo* response = trans->GetResponseInfo(); + ASSERT_TRUE(response != NULL); + EXPECT_FALSE(response->headers->IsKeepAlive()); + ASSERT_FALSE(response->headers.get() == NULL); + EXPECT_EQ(407, response->headers->response_code()); + EXPECT_TRUE(HttpVersion(1, 0) == response->headers->GetHttpVersion()); + EXPECT_TRUE(CheckBasicProxyAuth(response->auth_challenge.get())); + + LoadTimingInfo load_timing_info; + // CONNECT requests and responses are handled at the connect job level, so + // the transaction does not yet have a connection. + EXPECT_FALSE(trans->GetLoadTimingInfo(&load_timing_info)); + + TestCompletionCallback callback2; + + rv = + trans->RestartWithAuth(AuthCredentials(kFoo, kBar), callback2.callback()); + EXPECT_EQ(ERR_IO_PENDING, rv); + + rv = callback2.WaitForResult(); + EXPECT_EQ(OK, rv); + + response = trans->GetResponseInfo(); + ASSERT_TRUE(response != NULL); + + EXPECT_TRUE(response->headers->IsKeepAlive()); + EXPECT_EQ(200, response->headers->response_code()); + EXPECT_EQ(5, response->headers->GetContentLength()); + EXPECT_TRUE(HttpVersion(1, 1) == response->headers->GetHttpVersion()); + + // The password prompt info should not be set. + EXPECT_TRUE(response->auth_challenge.get() == NULL); + + EXPECT_TRUE(trans->GetLoadTimingInfo(&load_timing_info)); + TestLoadTimingNotReusedWithPac(load_timing_info, + CONNECT_TIMING_HAS_SSL_TIMES); + + trans.reset(); + session->CloseAllConnections(); +} + +// Test the request-challenge-retry sequence for basic auth, over a connection +// that requires a restart when setting up an SSL tunnel. +TEST_P(HttpNetworkTransactionTest, BasicAuthProxyNoKeepAliveHttp11) { HttpRequestInfo request; request.method = "GET"; request.url = GURL("https://www.google.com/"); @@ -2448,6 +2566,7 @@ TEST_P(HttpNetworkTransactionTest, BasicAuthProxyNoKeepAlive) { const HttpResponseInfo* response = trans->GetResponseInfo(); ASSERT_TRUE(response != NULL); + EXPECT_FALSE(response->headers->IsKeepAlive()); ASSERT_FALSE(response->headers.get() == NULL); EXPECT_EQ(407, response->headers->response_code()); EXPECT_TRUE(HttpVersion(1, 1) == response->headers->GetHttpVersion()); @@ -2487,8 +2606,115 @@ TEST_P(HttpNetworkTransactionTest, BasicAuthProxyNoKeepAlive) { } // Test the request-challenge-retry sequence for basic auth, over a keep-alive -// proxy connection, when setting up an SSL tunnel. -TEST_P(HttpNetworkTransactionTest, BasicAuthProxyKeepAlive) { +// proxy connection with HTTP/1.0 responses, when setting up an SSL tunnel. +TEST_P(HttpNetworkTransactionTest, BasicAuthProxyKeepAliveHttp10) { + HttpRequestInfo request; + request.method = "GET"; + request.url = GURL("https://www.google.com/"); + // Ensure that proxy authentication is attempted even + // when the no authentication data flag is set. + request.load_flags = net::LOAD_DO_NOT_SEND_AUTH_DATA; + + // Configure against proxy server "myproxy:70". + session_deps_.proxy_service.reset(ProxyService::CreateFixed("myproxy:70")); + CapturingBoundNetLog log; + session_deps_.net_log = log.bound().net_log(); + scoped_refptr session(CreateSession(&session_deps_)); + + scoped_ptr trans( + new HttpNetworkTransaction(DEFAULT_PRIORITY, session.get())); + + // Since we have proxy, should try to establish tunnel. + MockWrite data_writes1[] = { + 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"), + + // After calling trans->RestartWithAuth(), this is the request we should + // be issuing -- the final header line contains the credentials. + MockWrite( + "CONNECT www.google.com:443 HTTP/1.1\r\n" + "Host: www.google.com\r\n" + "Proxy-Connection: keep-alive\r\n" + "Proxy-Authorization: Basic Zm9vOmJheg==\r\n\r\n"), + }; + + // The proxy responds to the connect with a 407, using a persistent + // connection. (Since it's HTTP/1.0, keep-alive has to be explicit.) + MockRead data_reads1[] = { + // No credentials. + MockRead("HTTP/1.0 407 Proxy Authentication Required\r\n"), + MockRead("Proxy-Authenticate: Basic realm=\"MyRealm1\"\r\n"), + MockRead("Proxy-Connection: keep-alive\r\n"), + MockRead("Content-Length: 10\r\n\r\n"), + MockRead("0123456789"), + + // Wrong credentials (wrong password). + MockRead("HTTP/1.0 407 Proxy Authentication Required\r\n"), + MockRead("Proxy-Authenticate: Basic realm=\"MyRealm1\"\r\n"), + MockRead("Proxy-Connection: keep-alive\r\n"), + MockRead("Content-Length: 10\r\n\r\n"), + // No response body because the test stops reading here. + MockRead(SYNCHRONOUS, ERR_UNEXPECTED), // Should not be reached. + }; + + StaticSocketDataProvider data1(data_reads1, arraysize(data_reads1), + data_writes1, arraysize(data_writes1)); + session_deps_.socket_factory->AddSocketDataProvider(&data1); + + TestCompletionCallback callback1; + + int rv = trans->Start(&request, callback1.callback(), log.bound()); + EXPECT_EQ(ERR_IO_PENDING, rv); + + rv = callback1.WaitForResult(); + EXPECT_EQ(OK, rv); + net::CapturingNetLog::CapturedEntryList entries; + log.GetEntries(&entries); + size_t pos = ExpectLogContainsSomewhere( + entries, 0, NetLog::TYPE_HTTP_TRANSACTION_SEND_TUNNEL_HEADERS, + NetLog::PHASE_NONE); + ExpectLogContainsSomewhere( + entries, pos, NetLog::TYPE_HTTP_TRANSACTION_READ_TUNNEL_RESPONSE_HEADERS, + NetLog::PHASE_NONE); + + 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, 0) == response->headers->GetHttpVersion()); + EXPECT_TRUE(CheckBasicProxyAuth(response->auth_challenge.get())); + + TestCompletionCallback callback2; + + // Wrong password (should be "bar"). + rv = + trans->RestartWithAuth(AuthCredentials(kFoo, kBaz), callback2.callback()); + EXPECT_EQ(ERR_IO_PENDING, rv); + + rv = callback2.WaitForResult(); + EXPECT_EQ(OK, rv); + + 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, 0) == response->headers->GetHttpVersion()); + EXPECT_TRUE(CheckBasicProxyAuth(response->auth_challenge.get())); + + // Flush the idle socket before the NetLog and HttpNetworkTransaction go + // out of scope. + session->CloseAllConnections(); +} + +// Test the request-challenge-retry sequence for basic auth, over a keep-alive +// proxy connection with HTTP/1.1 responses, when setting up an SSL tunnel. +TEST_P(HttpNetworkTransactionTest, BasicAuthProxyKeepAliveHttp11) { HttpRequestInfo request; request.method = "GET"; request.url = GURL("https://www.google.com/"); @@ -2562,7 +2788,7 @@ TEST_P(HttpNetworkTransactionTest, BasicAuthProxyKeepAlive) { ASSERT_TRUE(response->headers); EXPECT_TRUE(response->headers->IsKeepAlive()); EXPECT_EQ(407, response->headers->response_code()); - EXPECT_EQ(-1, response->headers->GetContentLength()); + EXPECT_EQ(10, response->headers->GetContentLength()); EXPECT_TRUE(HttpVersion(1, 1) == response->headers->GetHttpVersion()); EXPECT_TRUE(CheckBasicProxyAuth(response->auth_challenge.get())); @@ -2581,7 +2807,7 @@ TEST_P(HttpNetworkTransactionTest, BasicAuthProxyKeepAlive) { ASSERT_TRUE(response->headers); EXPECT_TRUE(response->headers->IsKeepAlive()); EXPECT_EQ(407, response->headers->response_code()); - EXPECT_EQ(-1, response->headers->GetContentLength()); + EXPECT_EQ(10, response->headers->GetContentLength()); EXPECT_TRUE(HttpVersion(1, 1) == response->headers->GetHttpVersion()); EXPECT_TRUE(CheckBasicProxyAuth(response->auth_challenge.get())); diff --git a/net/http/proxy_client_socket.cc b/net/http/proxy_client_socket.cc index 3c539c6895e29..3c0d12d5cb2ad 100644 --- a/net/http/proxy_client_socket.cc +++ b/net/http/proxy_client_socket.cc @@ -95,9 +95,19 @@ bool ProxyClientSocket::SanitizeProxyAuth(HttpResponseInfo* response) { scoped_refptr new_headers = new HttpResponseHeaders( HttpUtil::AssembleRawHeaders(kHeaders, arraysize(kHeaders))); + // Copy status line and all hop-by-hop headers to preserve keep-alive + // behavior. new_headers->ReplaceStatusLine(old_headers->GetStatusLine()); - CopyHeaderValues(old_headers, new_headers, "Connection"); - CopyHeaderValues(old_headers, new_headers, "Proxy-Authenticate"); + CopyHeaderValues(old_headers, new_headers, "connection"); + CopyHeaderValues(old_headers, new_headers, "proxy-connection"); + CopyHeaderValues(old_headers, new_headers, "keep-alive"); + CopyHeaderValues(old_headers, new_headers, "trailer"); + CopyHeaderValues(old_headers, new_headers, "transfer-encoding"); + CopyHeaderValues(old_headers, new_headers, "upgrade"); + + CopyHeaderValues(old_headers, new_headers, "content-length"); + + CopyHeaderValues(old_headers, new_headers, "proxy-authenticate"); response->headers = new_headers; return true;