cros: Terminate if merge session fails for online sign-in

Merge session should always succeed for a just-processed online sign-in. Terminate current user session when it happens. Otherwise, login code ends up into an invalid state and things depends on auth token such as chrome sync etc would be broken. BUG=677312 TEST=OAuth2Test.TerminateOnBadMergeSessionAfterOnlineAuth Review-Url: https://codereview.chromium.org/2903123002 Cr-Commit-Position: refs/heads/master@{#475550}
Denger-Network · May 30, 2017 · c3ac4d2 · c3ac4d2
1 parent 27875ca
commit c3ac4d2
Show file tree

Hide file tree

Showing 3 changed files with 46 additions and 0 deletions.
diff --git a/chrome/browser/chromeos/login/session/user_session_manager.cc b/chrome/browser/chromeos/login/session/user_session_manager.cc
@@ -867,6 +867,20 @@ void UserSessionManager::OnSessionRestoreStateChanged(
 
   login_manager->RemoveObserver(this);
 
+  // Terminate user session if merge session fails for an online sign-in.
+  // Otherwise, auth token dependent code would be in an invalid state.
+  // Important piece such as policy code might be broken because of this and
+  // subject to an exploit. See http://crbug.com/677312.
+  const bool is_online_signin =
+      user_context_.GetAuthFlow() == UserContext::AUTH_FLOW_GAIA_WITH_SAML ||
+      user_context_.GetAuthFlow() == UserContext::AUTH_FLOW_GAIA_WITHOUT_SAML;
+  if (is_online_signin && state == OAuth2LoginManager::SESSION_RESTORE_FAILED) {
+    LOG(ERROR)
+        << "Session restore failed for online sign-in, terminating session.";
+    chrome::AttemptUserExit();
+    return;
+  }
+
   if (exit_after_session_restore_ &&
       (state  == OAuth2LoginManager::SESSION_RESTORE_DONE ||
        state  == OAuth2LoginManager::SESSION_RESTORE_FAILED ||

diff --git a/chrome/browser/chromeos/login/signin/oauth2_browsertest.cc b/chrome/browser/chromeos/login/signin/oauth2_browsertest.cc
@@ -586,6 +586,37 @@ IN_PROC_BROWSER_TEST_F(OAuth2Test, OverlappingContinueSessionRestore) {
   EXPECT_TRUE(token_service->RefreshTokenIsAvailable(account_id));
 }
 
+// Tests that user session is terminated if merge session fails for an online
+// sign-in. This is necessary to prevent policy exploit.
+// See http://crbug.com/677312
+IN_PROC_BROWSER_TEST_F(OAuth2Test, TerminateOnBadMergeSessionAfterOnlineAuth) {
+  SimulateNetworkOnline();
+  WaitForGaiaPageLoad();
+
+  content::WindowedNotificationObserver termination_waiter(
+      chrome::NOTIFICATION_APP_TERMINATING,
+      content::NotificationService::AllSources());
+
+  // Configure FakeGaia so that online auth succeeds but merge session fails.
+  FakeGaia::MergeSessionParams params;
+  params.auth_sid_cookie = kTestAuthSIDCookie;
+  params.auth_lsid_cookie = kTestAuthLSIDCookie;
+  params.auth_code = kTestAuthCode;
+  params.refresh_token = kTestRefreshToken;
+  params.access_token = kTestAuthLoginAccessToken;
+  fake_gaia_->SetMergeSessionParams(params);
+
+  // Simulate an online sign-in.
+  GetLoginDisplay()->ShowSigninScreenForCreds(kTestEmail, kTestAccountPassword);
+
+  // User session should be terminated.
+  termination_waiter.Wait();
+
+  // Merge session should fail. Check after |termination_waiter| to ensure
+  // user profile is initialized and there is an OAuth2LoginManage.
+  WaitForMergeSessionCompletion(OAuth2LoginManager::SESSION_RESTORE_FAILED);
+}
+
 const char kGooglePageContent[] =
     "<html><title>Hello!</title><script>alert('hello');</script>"
     "<body>Hello Google!</body></html>";

diff --git a/google_apis/gaia/fake_gaia.cc b/google_apis/gaia/fake_gaia.cc
@@ -525,6 +525,7 @@ void FakeGaia::HandleOAuthLogin(const HttpRequest& request,
   http_response->set_code(net::HTTP_UNAUTHORIZED);
   if (merge_session_params_.gaia_uber_token.empty()) {
     http_response->set_code(net::HTTP_FORBIDDEN);
+    http_response->set_content("Error=BadAuthentication");
     return;
   }