From 90ae93cc155b7cdd8f956ff79ba76de6c4c4ba43 Mon Sep 17 00:00:00 2001 From: Lily Chen Date: Thu, 14 Feb 2019 01:15:39 +0000 Subject: [PATCH] Network Error Logging: Disable when using proxy Disallow setting NEL headers and queueing NEL reports when the HTTP response is fetched via a proxy. Disallow NEL reporting on 407 responses. Bug: 930777 Change-Id: I803c3e087630cd5e6c244771550c97091e5d6e74 Reviewed-on: https://chromium-review.googlesource.com/c/1466195 Commit-Queue: Lily Chen Reviewed-by: Matt Menke Cr-Commit-Position: refs/heads/master@{#632021} --- net/http/http_network_transaction.cc | 16 ++ net/http/http_network_transaction_unittest.cc | 160 ++++++++++++++++++ 2 files changed, 176 insertions(+) diff --git a/net/http/http_network_transaction.cc b/net/http/http_network_transaction.cc index da7dc7c34df479..b87afc1d1910f1 100644 --- a/net/http/http_network_transaction.cc +++ b/net/http/http_network_transaction.cc @@ -1420,6 +1420,11 @@ void HttpNetworkTransaction::ProcessNetworkErrorLoggingHeader() { return; } + // Don't accept NEL headers received via a proxy, because the IP address of + // the destination server is not known. + if (response_.was_fetched_via_proxy) + return; + // Only accept NEL headers on HTTPS connections that have no certificate // errors. if (!response_.ssl_info.is_valid()) { @@ -1462,6 +1467,17 @@ void HttpNetworkTransaction::GenerateNetworkErrorLoggingReport(int rv) { return; } + // Don't report on proxy auth challenges. + if (response_.headers && response_.headers->response_code() == + HTTP_PROXY_AUTHENTICATION_REQUIRED) { + return; + } + + // Don't generate NEL reports if we are behind a proxy, to avoid leaking + // internal network details. + if (response_.was_fetched_via_proxy) + return; + // Ignore errors from non-HTTPS origins. if (!url_.SchemeIsCryptographic()) { NetworkErrorLoggingService::RecordRequestDiscardedForInsecureOrigin(); diff --git a/net/http/http_network_transaction_unittest.cc b/net/http/http_network_transaction_unittest.cc index 1d1bd75dfa71b6..b74f01b27e43e3 100644 --- a/net/http/http_network_transaction_unittest.cc +++ b/net/http/http_network_transaction_unittest.cc @@ -18415,6 +18415,70 @@ TEST_F(HttpNetworkTransactionNetworkErrorLoggingTest, NetworkErrorLoggingService::HeaderOutcome::DISCARDED_INVALID_SSL_INFO, 1); } +// Don't set NEL policies received on a proxied connection. +TEST_F(HttpNetworkTransactionNetworkErrorLoggingTest, + DontProcessNelHeaderProxy) { + session_deps_.proxy_resolution_service = + ProxyResolutionService::CreateFixedFromPacResult( + "PROXY myproxy:70", TRAFFIC_ANNOTATION_FOR_TESTS); + BoundTestNetLog log; + session_deps_.net_log = log.bound().net_log(); + std::unique_ptr session(CreateSession(&session_deps_)); + + HttpRequestInfo request; + request.method = "GET"; + request.url = GURL("https://www.example.org/"); + request.traffic_annotation = + net::MutableNetworkTrafficAnnotationTag(TRAFFIC_ANNOTATION_FOR_TESTS); + + // Since we have proxy, should try to establish tunnel. + MockWrite data_writes1[] = { + MockWrite("CONNECT www.example.org:443 HTTP/1.1\r\n" + "Host: www.example.org:443\r\n" + "Proxy-Connection: keep-alive\r\n\r\n"), + + MockWrite("GET / HTTP/1.1\r\n" + "Host: www.example.org\r\n" + "Connection: keep-alive\r\n\r\n"), + }; + + MockRead data_reads1[] = { + MockRead("HTTP/1.1 200 Connection Established\r\n\r\n"), + + MockRead("HTTP/1.1 200 OK\r\n"), + MockRead("NEL: {\"report_to\": \"nel\", \"max_age\": 86400}\r\n"), + MockRead("Content-Type: text/html; charset=iso-8859-1\r\n"), + MockRead("Content-Length: 100\r\n\r\n"), + MockRead(SYNCHRONOUS, OK), + }; + + StaticSocketDataProvider data1(data_reads1, data_writes1); + session_deps_.socket_factory->AddSocketDataProvider(&data1); + SSLSocketDataProvider ssl(ASYNC, OK); + ssl.ssl_info.cert = + ImportCertFromFile(GetTestCertsDirectory(), "wildcard.pem"); + ASSERT_TRUE(ssl.ssl_info.cert); + ssl.ssl_info.cert_status = 0; + session_deps_.socket_factory->AddSSLSocketDataProvider(&ssl); + + TestCompletionCallback callback1; + HttpNetworkTransaction trans(DEFAULT_PRIORITY, session.get()); + + int rv = trans.Start(&request, callback1.callback(), log.bound()); + EXPECT_THAT(rv, IsError(ERR_IO_PENDING)); + + rv = callback1.WaitForResult(); + EXPECT_THAT(rv, IsOk()); + + const HttpResponseInfo* response = trans.GetResponseInfo(); + ASSERT_TRUE(response); + EXPECT_EQ(200, response->headers->response_code()); + EXPECT_TRUE(response->was_fetched_via_proxy); + + // No NEL header was set. + EXPECT_EQ(0u, network_error_logging_service()->headers().size()); +} + TEST_F(HttpNetworkTransactionNetworkErrorLoggingTest, ProcessNelHeaderHttps) { RequestPolicy(); ASSERT_EQ(1u, network_error_logging_service()->headers().size()); @@ -19210,6 +19274,102 @@ TEST_F(HttpNetworkTransactionNetworkErrorLoggingTest, EXPECT_EQ(1u, network_error_logging_service()->errors().size()); } +// Don't report on proxy auth challenges, don't report if connecting through a +// proxy. +TEST_F(HttpNetworkTransactionNetworkErrorLoggingTest, DontCreateReportProxy) { + HttpRequestInfo request; + request.method = "GET"; + request.url = GURL("https://www.example.org/"); + request.traffic_annotation = + net::MutableNetworkTrafficAnnotationTag(TRAFFIC_ANNOTATION_FOR_TESTS); + + // Configure against proxy server "myproxy:70". + session_deps_.proxy_resolution_service = + ProxyResolutionService::CreateFixedFromPacResult( + "PROXY myproxy:70", TRAFFIC_ANNOTATION_FOR_TESTS); + std::unique_ptr session(CreateSession(&session_deps_)); + + // Since we have proxy, should try to establish tunnel. + MockWrite data_writes1[] = { + MockWrite("CONNECT www.example.org:443 HTTP/1.1\r\n" + "Host: www.example.org:443\r\n" + "Proxy-Connection: keep-alive\r\n\r\n"), + }; + + // The proxy responds to the connect with a 407, using a non-persistent + // connection. + MockRead data_reads1[] = { + // No credentials. + MockRead("HTTP/1.1 407 Proxy Authentication Required\r\n"), + MockRead("Proxy-Authenticate: Basic realm=\"MyRealm1\"\r\n"), + MockRead("Proxy-Connection: close\r\n\r\n"), + }; + + MockWrite data_writes2[] = { + // After calling trans->RestartWithAuth(), this is the request we should + // be issuing -- the final header line contains the credentials. + MockWrite("CONNECT www.example.org:443 HTTP/1.1\r\n" + "Host: www.example.org:443\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.example.org\r\n" + "Connection: keep-alive\r\n\r\n"), + }; + + MockRead data_reads2[] = { + MockRead("HTTP/1.1 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, data_writes1); + session_deps_.socket_factory->AddSocketDataProvider(&data1); + StaticSocketDataProvider data2(data_reads2, data_writes2); + session_deps_.socket_factory->AddSocketDataProvider(&data2); + SSLSocketDataProvider ssl(ASYNC, OK); + session_deps_.socket_factory->AddSSLSocketDataProvider(&ssl); + + TestCompletionCallback callback1; + + auto trans = + std::make_unique(DEFAULT_PRIORITY, session.get()); + + int rv = trans->Start(&request, callback1.callback(), NetLogWithSource()); + EXPECT_THAT(callback1.GetResult(rv), IsOk()); + + const HttpResponseInfo* response = trans->GetResponseInfo(); + EXPECT_EQ(407, response->headers->response_code()); + + std::string response_data; + rv = ReadTransaction(trans.get(), &response_data); + EXPECT_THAT(rv, IsError(ERR_TUNNEL_CONNECTION_FAILED)); + + // No NEL report is generated for the 407. + EXPECT_EQ(0u, network_error_logging_service()->errors().size()); + + TestCompletionCallback callback2; + + rv = + trans->RestartWithAuth(AuthCredentials(kFoo, kBar), callback2.callback()); + EXPECT_THAT(callback2.GetResult(rv), IsOk()); + + response = trans->GetResponseInfo(); + EXPECT_EQ(200, response->headers->response_code()); + + ASSERT_THAT(ReadTransaction(trans.get(), &response_data), IsOk()); + EXPECT_EQ("hello", response_data); + + trans.reset(); + + // No NEL report is generated because we are behind a proxy. + EXPECT_EQ(0u, network_error_logging_service()->errors().size()); +} + TEST_F(HttpNetworkTransactionNetworkErrorLoggingTest, ReportContainsUploadDepth) { reporting_upload_depth_ = 7;