Support CSRF for logged-out users

Summary: Fixes T4339. If you're anonymous, we use a digest of your session key to generate a CSRF token. Otherwise, everything works normally.

Test Plan: Logged out, logged in, tweaked CSRF in forms -- I'll add some inlines.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4339

Differential Revision: https://secure.phabricator.com/D8046

src/aphront/AphrontRequest.php

@@ -212,22 +212,25 @@ final class AphrontRequest {
       // Add some diagnostic details so we can figure out if some CSRF issues
       // are JS problems or people accessing Ajax URIs directly with their
       // browsers.
-      if ($token) {
+      $more_info = array();
-        $token_info = "with an invalid CSRF token";
-      } else {
-        $token_info = "without a CSRF token";
-      }
 
       if ($this->isAjax()) {
-        $more_info = "(This was an Ajax request, {$token_info}.)";
+        $more_info[] = pht('This was an Ajax request.');
       } else {
-        $more_info = "(This was a web request, {$token_info}.)";
+        $more_info[] = pht('This was a Web request.');
+      }
+
+      if ($token) {
+        $more_info[] = pht('This request had an invalid CSRF token.');
+      } else {
+        $more_info[] = pht('This request had no CSRF token.');
       }
 
       // Give a more detailed explanation of how to avoid the exception
       // in developer mode.
       if (PhabricatorEnv::getEnvConfig('phabricator.developer-mode')) {
-        $more_info = $more_info .
+        // TODO: Clean this up, see T1921.
+        $more_info[] =
           "To avoid this error, use phabricator_form() to construct forms. " .
           "If you are already using phabricator_form(), make sure the form " .
           "'action' uses a relative URI (i.e., begins with a '/'). Forms " .
@@ -249,14 +252,11 @@ final class AphrontRequest {
       // but give the user some indication of what happened since the workflow
       // is incredibly confusing otherwise.
       throw new AphrontCSRFException(
-        "The form you just submitted did not include a valid CSRF token. ".
+        pht(
-        "This token is a technical security measure which prevents a ".
+          "You are trying to save some data to Phabricator, but the request ".
-        "certain type of login hijacking attack. However, the token can ".
+          "your browser made included an incorrect token. Reload the page ".
-        "become invalid if you leave a page open for more than six hours ".
+          "and try again. You may need to clear your cookies.\n\n%s",
-        "without a connection to the internet. To fix this problem: reload ".
+          implode("\n", $more_info)));
-        "the page, and then resubmit it. All data inserted to the form will ".
-        "be lost in some browsers so copy them somewhere before reloading.\n\n".
-        $more_info);
     }
 
     return true;

src/applications/auth/provider/PhabricatorAuthProviderPassword.php

@@ -185,14 +185,15 @@ final class PhabricatorAuthProviderPassword
     }
 
     if (!$account) {
-      $log = PhabricatorUserLog::initializeNewLog(
+      if ($request->isFormPost()) {
-        null,
+        $log = PhabricatorUserLog::initializeNewLog(
-        $log_user ? $log_user->getPHID() : null,
+          null,
-        PhabricatorUserLog::ACTION_LOGIN_FAILURE);
+          $log_user ? $log_user->getPHID() : null,
-      $log->save();
+          PhabricatorUserLog::ACTION_LOGIN_FAILURE);
+        $log->save();
+      }
 
       $request->clearCookie(PhabricatorCookies::COOKIE_USERNAME);
-      $request->clearCookie(PhabricatorCookies::COOKIE_SESSION);
 
       $response = $controller->buildProviderPageResponse(
         $this,

src/applications/base/controller/PhabricatorController.php

@@ -34,14 +34,27 @@ abstract class PhabricatorController extends AphrontController {
       $user = $request->getUser();
     } else {
       $user = new PhabricatorUser();
+      $session_engine = new PhabricatorAuthSessionEngine();
 
       $phsid = $request->getCookie(PhabricatorCookies::COOKIE_SESSION);
       if (strlen($phsid)) {
-        $session_user = id(new PhabricatorAuthSessionEngine())
+        $session_user = $session_engine->loadUserForSession(
-          ->loadUserForSession(PhabricatorAuthSession::TYPE_WEB, $phsid);
+          PhabricatorAuthSession::TYPE_WEB,
+          $phsid);
         if ($session_user) {
           $user = $session_user;
         }
+      } else {
+        // If the client doesn't have a session token, generate an anonymous
+        // session. This is used to provide CSRF protection to logged-out users.
+        $phsid = $session_engine->establishSession(
+          PhabricatorAuthSession::TYPE_WEB,
+          null);
+        $request->setCookie(PhabricatorCookies::COOKIE_SESSION, $phsid);
+      }
+
+      if (!$user->isLoggedIn()) {
+        $user->attachAlternateCSRFString(PhabricatorHash::digest($phsid));
       }
 
       $request->setUser($user);

src/applications/people/storage/PhabricatorUser.php

@@ -39,6 +39,8 @@ final class PhabricatorUser
   private $omnipotent = false;
   private $customFields = self::ATTACHABLE;
 
+  private $alternateCSRFString = self::ATTACHABLE;
+
   protected function readField($field) {
     switch ($field) {
       case 'timezoneIdentifier':
@@ -217,12 +219,7 @@ final class PhabricatorUser
   }
 
   public function validateCSRFToken($token) {
-    if (!$this->getPHID()) {
-      return true;
-    }
-
     $salt = null;
-
     $version = 'plain';
 
     // This is a BREACH-mitigating token. See T3684.
@@ -287,8 +284,15 @@ final class PhabricatorUser
   }
 
   private function generateToken($epoch, $frequency, $key, $len) {
+    if ($this->getPHID()) {
+      $vec = $this->getPHID().$this->getPasswordHash();
+    } else {
+      $vec = $this->getAlternateCSRFString();
+    }
+
     $time_block = floor($epoch / $frequency);
-    $vec = $this->getPHID().$this->getPasswordHash().$key.$time_block;
+    $vec = $vec.$key.$time_block;
+
     return substr(PhabricatorHash::digest($vec), 0, $len);
   }
 
@@ -439,6 +443,15 @@ final class PhabricatorUser
     }
   }
 
+  public function getAlternateCSRFString() {
+    return $this->assertAttached($this->alternateCSRFString);
+  }
+
+  public function attachAlternateCSRFString($string) {
+    $this->alternateCSRFString = $string;
+    return $this;
+  }
+
   private static function tokenizeName($name) {
     if (function_exists('mb_strtolower')) {
       $name = mb_strtolower($name, 'UTF-8');

src/view/page/PhabricatorStandardPageView.php

@@ -163,17 +163,6 @@ final class PhabricatorStandardPageView extends PhabricatorBarePageView {
     Javelin::initBehavior('history-install');
     Javelin::initBehavior('phabricator-gesture');
 
-    // If the client doesn't have a session token, generate an anonymous
-    // session. This is used to provide CSRF protection to logged-out users.
-    $session_token = $request->getCookie(PhabricatorCookies::COOKIE_SESSION);
-    if (!strlen($session_token)) {
-      $anonymous_session = id(new PhabricatorAuthSessionEngine())
-        ->establishSession('web', null);
-      $request->setCookie(
-        PhabricatorCookies::COOKIE_SESSION,
-        $anonymous_session);
-    }
-
     $current_token = null;
     if ($user) {
       $current_token = $user->getCSRFToken();