Don't use IDENT_SRC_URL for HttpAuth challenges. IE hasn't supported it for years, and at worst it represents a session fixation attack.

BUG=94578
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=120836

Review URL: http://codereview.chromium.org/9307093

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@121506 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 11 insertions, 193 deletions

diff --git a/net/http/http_auth_controller.cc b/net/http/http_auth_controller.cc
index 8a23969..34c2469 100644
--- a/net/http/http_auth_controller.cc
+++ b/net/http/http_auth_controller.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -450,21 +450,17 @@ bool HttpAuthController::SelectNextAuthIdentityToTry() {
   DCHECK(handler_.get());
   DCHECK(identity_.invalid);
 
-  // Try to use the username:password encoded into the URL first.
+  // Do not try to use the username:password encoded into the URL.  At worst,
+  // this represents a session fixation attack against basic auth, and as it
+  // turns out, IE hasn't supported this for years. If a caller really wants
+  // to use embedded identities, the can add an URLRequest::Delegate that
+  // inspects the URL and supplies the username/password at OnAuthRequired()
+  // time. Past data shows this is used extremely infrequently in web pages,
+  // but continue to collect this data.
   if (target_ == HttpAuth::AUTH_SERVER && auth_url_.has_username() &&
       !embedded_identity_used_) {
-    identity_.source = HttpAuth::IDENT_SRC_URL;
-    identity_.invalid = false;
-    // Extract the username:password from the URL.
-    string16 username;
-    string16 password;
-    GetIdentityFromURL(auth_url_, &username, &password);
-    identity_.credentials.Set(username, password);
     embedded_identity_used_ = true;
-    // TODO(eroman): If the password is blank, should we also try combining
-    // with a password from the cache?
     UMA_HISTOGRAM_BOOLEAN("net.HttpIdentSrcURL", true);
-    return true;
   }
 
   // Check the auth cache for a realm entry.
diff --git a/net/http/http_network_transaction_unittest.cc b/net/http/http_network_transaction_unittest.cc
index 8457a1d..c482b31 100644
--- a/net/http/http_network_transaction_unittest.cc
+++ b/net/http/http_network_transaction_unittest.cc
@@ -3718,12 +3718,12 @@ TEST_F(HttpNetworkTransactionTest, ResendRequestOnWriteBodyError) {
 
 // Test the request-challenge-retry sequence for basic auth when there is
 // an identity in the URL. The request should be sent as normal, but when
-// it fails the identity from the URL is used to answer the challenge.
-TEST_F(HttpNetworkTransactionTest, AuthIdentityInURL) {
+// it fails the identity from the URL is no longer used.
+TEST_F(HttpNetworkTransactionTest, IgnoreAuthIdentityInURL) {
   HttpRequestInfo request;
   request.method = "GET";
-  // Note: the URL has a username:password in it.
   request.url = GURL("http://foo:b@r@www.google.com/");
+  request.load_flags = LOAD_NORMAL;
 
   SessionDependencies session_deps;
   scoped_ptr<HttpTransaction> trans(
@@ -3733,8 +3733,6 @@ TEST_F(HttpNetworkTransactionTest, AuthIdentityInURL) {
   // will need to be unescaped by HttpNetworkTransaction.
   EXPECT_EQ("b%40r", request.url.password());
 
-  request.load_flags = LOAD_NORMAL;
-
   MockWrite data_writes1[] = {
     MockWrite("GET / HTTP/1.1\r\n"
               "Host: www.google.com\r\n"
@@ -3748,162 +3746,17 @@ TEST_F(HttpNetworkTransactionTest, AuthIdentityInURL) {
     MockRead(false, ERR_FAILED),
   };
 
-  // After the challenge above, the transaction will be restarted using the
-  // identity from the url (foo, b@r) to answer the challenge.
-  MockWrite data_writes2[] = {
-    MockWrite("GET / HTTP/1.1\r\n"
-              "Host: www.google.com\r\n"
-              "Connection: keep-alive\r\n"
-              "Authorization: Basic Zm9vOmJAcg==\r\n\r\n"),
-  };
-
-  MockRead data_reads2[] = {
-    MockRead("HTTP/1.0 200 OK\r\n"),
-    MockRead("Content-Length: 100\r\n\r\n"),
-    MockRead(false, OK),
-  };
-
   StaticSocketDataProvider data1(data_reads1, arraysize(data_reads1),
                                  data_writes1, arraysize(data_writes1));
-  StaticSocketDataProvider data2(data_reads2, arraysize(data_reads2),
-                                 data_writes2, arraysize(data_writes2));
   session_deps.socket_factory.AddSocketDataProvider(&data1);
-  session_deps.socket_factory.AddSocketDataProvider(&data2);
 
   TestCompletionCallback callback1;
-
   int rv = trans->Start(&request, callback1.callback(), BoundNetLog());
   EXPECT_EQ(ERR_IO_PENDING, rv);
-
   rv = callback1.WaitForResult();
   EXPECT_EQ(OK, rv);
-
-  EXPECT_TRUE(trans->IsReadyToRestartForAuth());
-  TestCompletionCallback callback2;
-  rv = trans->RestartWithAuth(AuthCredentials(), callback2.callback());
-  EXPECT_EQ(ERR_IO_PENDING, rv);
-  rv = callback2.WaitForResult();
-  EXPECT_EQ(OK, rv);
   EXPECT_FALSE(trans->IsReadyToRestartForAuth());
 
-  const HttpResponseInfo* response = trans->GetResponseInfo();
-  ASSERT_TRUE(response != NULL);
-
-  // There is no challenge info, since the identity in URL worked.
-  EXPECT_TRUE(response->auth_challenge.get() == NULL);
-
-  EXPECT_EQ(100, response->headers->GetContentLength());
-
-  // Empty the current queue.
-  MessageLoop::current()->RunAllPending();
-}
-
-// Test the request-challenge-retry sequence for basic auth when there is
-// an incorrect identity in the URL. The identity from the URL should be used
-// only once.
-TEST_F(HttpNetworkTransactionTest, WrongAuthIdentityInURL) {
-  HttpRequestInfo request;
-  request.method = "GET";
-  // Note: the URL has a username:password in it.  The password "baz" is
-  // wrong (should be "bar").
-  request.url = GURL("http://foo:baz@www.google.com/");
-
-  request.load_flags = LOAD_NORMAL;
-
-  SessionDependencies session_deps;
-  scoped_ptr<HttpTransaction> trans(
-      new HttpNetworkTransaction(CreateSession(&session_deps)));
-
-  MockWrite data_writes1[] = {
-    MockWrite("GET / HTTP/1.1\r\n"
-              "Host: www.google.com\r\n"
-              "Connection: keep-alive\r\n\r\n"),
-  };
-
-  MockRead data_reads1[] = {
-    MockRead("HTTP/1.0 401 Unauthorized\r\n"),
-    MockRead("WWW-Authenticate: Basic realm=\"MyRealm1\"\r\n"),
-    MockRead("Content-Length: 10\r\n\r\n"),
-    MockRead(false, ERR_FAILED),
-  };
-
-  // After the challenge above, the transaction will be restarted using the
-  // identity from the url (foo, baz) to answer the challenge.
-  MockWrite data_writes2[] = {
-    MockWrite("GET / HTTP/1.1\r\n"
-              "Host: www.google.com\r\n"
-              "Connection: keep-alive\r\n"
-              "Authorization: Basic Zm9vOmJheg==\r\n\r\n"),
-  };
-
-  MockRead data_reads2[] = {
-    MockRead("HTTP/1.0 401 Unauthorized\r\n"),
-    MockRead("WWW-Authenticate: Basic realm=\"MyRealm1\"\r\n"),
-    MockRead("Content-Length: 10\r\n\r\n"),
-    MockRead(false, ERR_FAILED),
-  };
-
-  // After the challenge above, the transaction will be restarted using the
-  // identity supplied by the user (foo, bar) to answer the challenge.
-  MockWrite data_writes3[] = {
-    MockWrite("GET / HTTP/1.1\r\n"
-              "Host: www.google.com\r\n"
-              "Connection: keep-alive\r\n"
-              "Authorization: Basic Zm9vOmJhcg==\r\n\r\n"),
-  };
-
-  MockRead data_reads3[] = {
-    MockRead("HTTP/1.0 200 OK\r\n"),
-    MockRead("Content-Length: 100\r\n\r\n"),
-    MockRead(false, OK),
-  };
-
-  StaticSocketDataProvider data1(data_reads1, arraysize(data_reads1),
-                                 data_writes1, arraysize(data_writes1));
-  StaticSocketDataProvider data2(data_reads2, arraysize(data_reads2),
-                                 data_writes2, arraysize(data_writes2));
-  StaticSocketDataProvider data3(data_reads3, arraysize(data_reads3),
-                                 data_writes3, arraysize(data_writes3));
-  session_deps.socket_factory.AddSocketDataProvider(&data1);
-  session_deps.socket_factory.AddSocketDataProvider(&data2);
-  session_deps.socket_factory.AddSocketDataProvider(&data3);
-
-  TestCompletionCallback callback1;
-
-  int rv = trans->Start(&request, callback1.callback(), BoundNetLog());
-  EXPECT_EQ(ERR_IO_PENDING, rv);
-
-  rv = callback1.WaitForResult();
-  EXPECT_EQ(OK, rv);
-
-  EXPECT_TRUE(trans->IsReadyToRestartForAuth());
-  TestCompletionCallback callback2;
-  rv = trans->RestartWithAuth(AuthCredentials(), callback2.callback());
-  EXPECT_EQ(ERR_IO_PENDING, rv);
-  rv = callback2.WaitForResult();
-  EXPECT_EQ(OK, rv);
-  EXPECT_FALSE(trans->IsReadyToRestartForAuth());
-
-  const HttpResponseInfo* response = trans->GetResponseInfo();
-  ASSERT_TRUE(response != NULL);
-  EXPECT_TRUE(CheckBasicServerAuth(response->auth_challenge.get()));
-
-  TestCompletionCallback callback3;
-  rv = trans->RestartWithAuth(
-      AuthCredentials(kFoo, kBar), callback3.callback());
-  EXPECT_EQ(ERR_IO_PENDING, rv);
-  rv = callback3.WaitForResult();
-  EXPECT_EQ(OK, rv);
-  EXPECT_FALSE(trans->IsReadyToRestartForAuth());
-
-  response = trans->GetResponseInfo();
-  ASSERT_TRUE(response != NULL);
-
-  // There is no challenge info, since the identity worked.
-  EXPECT_TRUE(response->auth_challenge.get() == NULL);
-
-  EXPECT_EQ(100, response->headers->GetContentLength());
-
   // Empty the current queue.
   MessageLoop::current()->RunAllPending();
 }
diff --git a/net/url_request/url_request_unittest.cc b/net/url_request/url_request_unittest.cc
index bafbd6f..287e21b 100644
--- a/net/url_request/url_request_unittest.cc
+++ b/net/url_request/url_request_unittest.cc
@@ -2465,37 +2465,6 @@ TEST_F(URLRequestTestHTTP, BasicAuthWithCookies) {
     EXPECT_TRUE(d.data_received().find("Cookie: got_challenged=true")
         != std::string::npos);
   }
-
-  // Same test as above, except this time the restart is initiated earlier
-  // (without user intervention since identity is embedded in the URL).
-  {
-    TestNetworkDelegate network_delegate;  // must outlive URLRequest
-    scoped_refptr<TestURLRequestContext> context(
-        new TestURLRequestContext(true));
-    context->set_network_delegate(&network_delegate);
-    context->Init();
-
-    TestDelegate d;
-
-    GURL::Replacements replacements;
-    std::string username("user2");
-    std::string password("secret");
-    replacements.SetUsernameStr(username);
-    replacements.SetPasswordStr(password);
-    GURL url_with_identity = url_requiring_auth.ReplaceComponents(replacements);
-
-    URLRequest r(url_with_identity, &d);
-    r.set_context(context);
-    r.Start();
-
-    MessageLoop::current()->Run();
-
-    EXPECT_TRUE(d.data_received().find("user2/secret") != std::string::npos);
-
-    // Make sure we sent the cookie in the restarted transaction.
-    EXPECT_TRUE(d.data_received().find("Cookie: got_challenged=true")
-        != std::string::npos);
-  }
 }
 
 TEST_F(URLRequestTest, DelayedCookieCallback) {