Cause:

To prevent a malicious client from guessing the PIN by spamming the host with bogus logins, the chromoting host can throttle incoming requests after too many unsuccessful login attempts.  In the current implementation, every time when there is an incoming request, we start incrementing the bad login counter, regardless of whether the host has actually starts authenticating.

Fix:
This change adds an extra flag on the authenticator to indicate whether authentication has started.
The JingleSession checks the flag and progagates the message back all the way up to the host through the callback  Session::OnSessionAuthenticationBegin

BUG=350208

Review URL: https://codereview.chromium.org/205583011

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

7 files changed, 102 insertions, 21 deletions

diff --git a/remoting/host/chromoting_host.cc b/remoting/host/chromoting_host.cc
index e4bdae7..469c5b2 100644
--- a/remoting/host/chromoting_host.cc
+++ b/remoting/host/chromoting_host.cc
@@ -171,6 +171,20 @@ void ChromotingHost::SetMaximumSessionDuration(
 
 ////////////////////////////////////////////////////////////////////////////
 // protocol::ClientSession::EventHandler implementation.
+void ChromotingHost::OnSessionAuthenticating(ClientSession* client) {
+  // We treat each incoming connection as a failure to authenticate,
+  // and clear the backoff when a connection successfully
+  // authenticates. This allows the backoff to protect from parallel
+  // connection attempts as well as sequential ones.
+  if (login_backoff_.ShouldRejectRequest()) {
+    LOG(WARNING) << "Disconnecting client " << client->client_jid() << " due to"
+                    " an overload of failed login attempts.";
+    client->DisconnectSession();
+    return;
+  }
+  login_backoff_.InformOfRequest(false);
+}
+
 bool ChromotingHost::OnSessionAuthenticated(ClientSession* client) {
   DCHECK(CalledOnValidThread());
 
@@ -265,16 +279,12 @@ void ChromotingHost::OnIncomingSession(
   }
 
   if (login_backoff_.ShouldRejectRequest()) {
+    LOG(WARNING) << "Rejecting connection due to"
+                    " an overload of failed login attempts.";
     *response = protocol::SessionManager::OVERLOAD;
     return;
   }
 
-  // We treat each incoming connection as a failure to authenticate,
-  // and clear the backoff when a connection successfully
-  // authenticates. This allows the backoff to protect from parallel
-  // connection attempts as well as sequential ones.
-  login_backoff_.InformOfRequest(false);
-
   protocol::SessionConfig config;
   if (!protocol_config_->Select(session->candidate_config(), &config)) {
     LOG(WARNING) << "Rejecting connection from " << session->jid()
diff --git a/remoting/host/chromoting_host.h b/remoting/host/chromoting_host.h
index 3e1fa74..a6642d4 100644
--- a/remoting/host/chromoting_host.h
+++ b/remoting/host/chromoting_host.h
@@ -115,6 +115,7 @@ class ChromotingHost : public base::NonThreadSafe,
 
   ////////////////////////////////////////////////////////////////////////////
   // ClientSession::EventHandler implementation.
+  virtual void OnSessionAuthenticating(ClientSession* client) OVERRIDE;
   virtual bool OnSessionAuthenticated(ClientSession* client) OVERRIDE;
   virtual void OnSessionChannelsConnected(ClientSession* client) OVERRIDE;
   virtual void OnSessionAuthenticationFailed(ClientSession* client) OVERRIDE;
diff --git a/remoting/host/chromoting_host_unittest.cc b/remoting/host/chromoting_host_unittest.cc
index 1410371..5ebe051 100644
--- a/remoting/host/chromoting_host_unittest.cc
+++ b/remoting/host/chromoting_host_unittest.cc
@@ -28,12 +28,13 @@ using ::remoting::protocol::MockConnectionToClientEventHandler;
 using ::remoting::protocol::MockHostStub;
 using ::remoting::protocol::MockSession;
 using ::remoting::protocol::MockVideoStub;
+using ::remoting::protocol::Session;
 using ::remoting::protocol::SessionConfig;
 
 using testing::_;
 using testing::AnyNumber;
-using testing::AtMost;
 using testing::AtLeast;
+using testing::AtMost;
 using testing::CreateFunctor;
 using testing::DeleteArg;
 using testing::DoAll;
@@ -44,6 +45,7 @@ using testing::InvokeArgument;
 using testing::InvokeWithoutArgs;
 using testing::Return;
 using testing::ReturnRef;
+using testing::SaveArg;
 using testing::Sequence;
 
 namespace remoting {
@@ -124,9 +126,10 @@ class ChromotingHostTest : public testing::Test {
         .Times(AnyNumber());
     EXPECT_CALL(*session_unowned1_, SetEventHandler(_))
         .Times(AnyNumber())
-        .WillRepeatedly(Invoke(this, &ChromotingHostTest::SetEventHandler));
+        .WillRepeatedly(SaveArg<0>(&session_unowned1_event_handler_));
     EXPECT_CALL(*session_unowned2_, SetEventHandler(_))
-        .Times(AnyNumber());
+        .Times(AnyNumber())
+        .WillRepeatedly(SaveArg<0>(&session_unowned2_event_handler_));
     EXPECT_CALL(*session1_, config())
         .WillRepeatedly(ReturnRef(session_config1_));
     EXPECT_CALL(*session2_, config())
@@ -287,13 +290,15 @@ class ChromotingHostTest : public testing::Test {
         get_connection(connection_index), protocol::OK);
   }
 
-  void SetEventHandler(protocol::Session::EventHandler* event_handler) {
-    session_event_handler_ = event_handler;
+  void NotifyConnectionClosed1() {
+    if (session_unowned1_event_handler_) {
+      session_unowned1_event_handler_->OnSessionStateChange(Session::CLOSED);
+    }
   }
 
-  void NotifyConnectionClosed() {
-    if (session_event_handler_) {
-      session_event_handler_->OnSessionStateChange(protocol::Session::CLOSED);
+  void NotifyConnectionClosed2() {
+    if (session_unowned2_event_handler_) {
+      session_unowned2_event_handler_->OnSessionStateChange(Session::CLOSED);
     }
   }
 
@@ -424,7 +429,8 @@ class ChromotingHostTest : public testing::Test {
   scoped_ptr<MockSession> session_unowned2_;  // Not owned by a connection.
   SessionConfig session_unowned_config2_;
   std::string session_unowned_jid2_;
-  protocol::Session::EventHandler* session_event_handler_;
+  protocol::Session::EventHandler* session_unowned1_event_handler_;
+  protocol::Session::EventHandler* session_unowned2_event_handler_;
   scoped_ptr<protocol::CandidateSessionConfig> empty_candidate_config_;
   scoped_ptr<protocol::CandidateSessionConfig> default_candidate_config_;
 
@@ -432,10 +438,16 @@ class ChromotingHostTest : public testing::Test {
     return (connection_index == 0) ? connection1_ : connection2_;
   }
 
+  // Returns the cached client pointers client1_ or client2_.
   ClientSession*& get_client(int connection_index) {
     return (connection_index == 0) ? client1_ : client2_;
   }
 
+  // Returns the list of clients of the host_.
+  std::list<ClientSession*>& get_clients_from_host() {
+    return host_->clients_;
+  }
+
   const std::string& get_session_jid(int connection_index) {
     return (connection_index == 0) ? session_jid1_ : session_jid2_;
   }
@@ -578,7 +590,7 @@ TEST_F(ChromotingHostTest, IncomingSessionAccepted) {
       default_candidate_config_.get()));
   EXPECT_CALL(*session_unowned1_, set_config(_));
   EXPECT_CALL(*session_unowned1_, Close()).WillOnce(InvokeWithoutArgs(
-      this, &ChromotingHostTest::NotifyConnectionClosed));
+    this, &ChromotingHostTest::NotifyConnectionClosed1));
   EXPECT_CALL(host_status_observer_, OnAccessDenied(_));
   EXPECT_CALL(host_status_observer_, OnShutdown());
 
@@ -593,13 +605,13 @@ TEST_F(ChromotingHostTest, IncomingSessionAccepted) {
   message_loop_.Run();
 }
 
-TEST_F(ChromotingHostTest, IncomingSessionOverload) {
+TEST_F(ChromotingHostTest, LoginBackOffUponConnection) {
   ExpectHostAndSessionManagerStart();
-  EXPECT_CALL(*session_unowned1_, candidate_config()).WillOnce(Return(
-      default_candidate_config_.get()));
+  EXPECT_CALL(*session_unowned1_, candidate_config()).WillOnce(
+    Return(default_candidate_config_.get()));
   EXPECT_CALL(*session_unowned1_, set_config(_));
-  EXPECT_CALL(*session_unowned1_, Close()).WillOnce(InvokeWithoutArgs(
-      this, &ChromotingHostTest::NotifyConnectionClosed));
+  EXPECT_CALL(*session_unowned1_, Close()).WillOnce(
+    InvokeWithoutArgs(this, &ChromotingHostTest::NotifyConnectionClosed1));
   EXPECT_CALL(host_status_observer_, OnAccessDenied(_));
   EXPECT_CALL(host_status_observer_, OnShutdown());
 
@@ -607,9 +619,11 @@ TEST_F(ChromotingHostTest, IncomingSessionOverload) {
 
   protocol::SessionManager::IncomingSessionResponse response =
       protocol::SessionManager::DECLINE;
+
   host_->OnIncomingSession(session_unowned1_.release(), &response);
   EXPECT_EQ(protocol::SessionManager::ACCEPT, response);
 
+  host_->OnSessionAuthenticating(get_clients_from_host().front());
   host_->OnIncomingSession(session_unowned2_.get(), &response);
   EXPECT_EQ(protocol::SessionManager::OVERLOAD, response);
 
@@ -617,6 +631,46 @@ TEST_F(ChromotingHostTest, IncomingSessionOverload) {
   message_loop_.Run();
 }
 
+TEST_F(ChromotingHostTest, LoginBackOffUponAuthenticating) {
+  Expectation start = ExpectHostAndSessionManagerStart();
+  EXPECT_CALL(*session_unowned1_, candidate_config()).WillOnce(
+    Return(default_candidate_config_.get()));
+  EXPECT_CALL(*session_unowned1_, set_config(_));
+  EXPECT_CALL(*session_unowned1_, Close()).WillOnce(
+    InvokeWithoutArgs(this, &ChromotingHostTest::NotifyConnectionClosed1));
+
+  EXPECT_CALL(*session_unowned2_, candidate_config()).WillOnce(
+    Return(default_candidate_config_.get()));
+  EXPECT_CALL(*session_unowned2_, set_config(_));
+  EXPECT_CALL(*session_unowned2_, Close()).WillOnce(
+    InvokeWithoutArgs(this, &ChromotingHostTest::NotifyConnectionClosed2));
+
+  EXPECT_CALL(host_status_observer_, OnShutdown());
+
+  host_->Start(xmpp_login_);
+
+  protocol::SessionManager::IncomingSessionResponse response =
+      protocol::SessionManager::DECLINE;
+
+  host_->OnIncomingSession(session_unowned1_.release(), &response);
+  EXPECT_EQ(protocol::SessionManager::ACCEPT, response);
+
+  host_->OnIncomingSession(session_unowned2_.release(), &response);
+  EXPECT_EQ(protocol::SessionManager::ACCEPT, response);
+
+  // This will set the backoff.
+  host_->OnSessionAuthenticating(get_clients_from_host().front());
+
+  // This should disconnect client2.
+  host_->OnSessionAuthenticating(get_clients_from_host().back());
+
+  // Verify that the host only has 1 client at this point.
+  EXPECT_EQ(get_clients_from_host().size(), 1U);
+
+  ShutdownHost();
+  message_loop_.Run();
+}
+
 TEST_F(ChromotingHostTest, OnSessionRouteChange) {
   std::string channel_name("ChannelName");
   protocol::TransportRoute route;
diff --git a/remoting/host/client_session.cc b/remoting/host/client_session.cc
index 7a3c76c..2661e0e 100644
--- a/remoting/host/client_session.cc
+++ b/remoting/host/client_session.cc
@@ -210,6 +210,11 @@ void ClientSession::DeliverClientMessage(
             << message.type() << ": " << message.data();
 }
 
+void ClientSession::OnConnectionAuthenticating(
+    protocol::ConnectionToClient* connection) {
+  event_handler_->OnSessionAuthenticating(this);
+}
+
 void ClientSession::OnConnectionAuthenticated(
     protocol::ConnectionToClient* connection) {
   DCHECK(CalledOnValidThread());
diff --git a/remoting/host/client_session.h b/remoting/host/client_session.h
index ef75b25..f892329 100644
--- a/remoting/host/client_session.h
+++ b/remoting/host/client_session.h
@@ -54,6 +54,9 @@ class ClientSession
   // Callback interface for passing events to the ChromotingHost.
   class EventHandler {
    public:
+    // Called after authentication has started.
+    virtual void OnSessionAuthenticating(ClientSession* client) = 0;
+
     // Called after authentication has finished successfully. Returns true if
     // the connection is allowed, or false otherwise.
     virtual bool OnSessionAuthenticated(ClientSession* client) = 0;
@@ -115,6 +118,8 @@ class ClientSession
       const protocol::ExtensionMessage& message) OVERRIDE;
 
   // protocol::ConnectionToClient::EventHandler interface.
+  virtual void OnConnectionAuthenticating(
+      protocol::ConnectionToClient* connection) OVERRIDE;
   virtual void OnConnectionAuthenticated(
       protocol::ConnectionToClient* connection) OVERRIDE;
   virtual void OnConnectionChannelsConnected(
diff --git a/remoting/host/host_mock_objects.h b/remoting/host/host_mock_objects.h
index d365695..d916f04 100644
--- a/remoting/host/host_mock_objects.h
+++ b/remoting/host/host_mock_objects.h
@@ -68,6 +68,7 @@ class MockClientSessionEventHandler : public ClientSession::EventHandler {
   MockClientSessionEventHandler();
   virtual ~MockClientSessionEventHandler();
 
+  MOCK_METHOD1(OnSessionAuthenticating, void(ClientSession* client));
   MOCK_METHOD1(OnSessionAuthenticated, bool(ClientSession* client));
   MOCK_METHOD1(OnSessionChannelsConnected, void(ClientSession* client));
   MOCK_METHOD1(OnSessionAuthenticationFailed, void(ClientSession* client));
diff --git a/remoting/host/pam_authorization_factory_posix.cc b/remoting/host/pam_authorization_factory_posix.cc
index c89c71f..eef0c48 100644
--- a/remoting/host/pam_authorization_factory_posix.cc
+++ b/remoting/host/pam_authorization_factory_posix.cc
@@ -24,6 +24,7 @@ class PamAuthorizer : public protocol::Authenticator {
 
   // protocol::Authenticator interface.
   virtual State state() const OVERRIDE;
+  virtual bool started() const OVERRIDE;
   virtual RejectionReason rejection_reason() const OVERRIDE;
   virtual void ProcessMessage(const buzz::XmlElement* message,
                               const base::Closure& resume_callback) OVERRIDE;
@@ -62,6 +63,10 @@ protocol::Authenticator::State PamAuthorizer::state() const {
   }
 }
 
+bool PamAuthorizer::started() const {
+  return underlying_->started();
+}
+
 protocol::Authenticator::RejectionReason
 PamAuthorizer::rejection_reason() const {
   if (local_login_status_ == DISALLOWED) {