diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2190,6 +2190,7 @@ 'PhabricatorAuthApplication' => 'applications/auth/application/PhabricatorAuthApplication.php', 'PhabricatorAuthAuthFactorPHIDType' => 'applications/auth/phid/PhabricatorAuthAuthFactorPHIDType.php', 'PhabricatorAuthAuthProviderPHIDType' => 'applications/auth/phid/PhabricatorAuthAuthProviderPHIDType.php', + 'PhabricatorAuthCSRFEngine' => 'applications/auth/engine/PhabricatorAuthCSRFEngine.php', 'PhabricatorAuthChallenge' => 'applications/auth/storage/PhabricatorAuthChallenge.php', 'PhabricatorAuthChallengeGarbageCollector' => 'applications/auth/garbagecollector/PhabricatorAuthChallengeGarbageCollector.php', 'PhabricatorAuthChallengePHIDType' => 'applications/auth/phid/PhabricatorAuthChallengePHIDType.php', @@ -7828,6 +7829,7 @@ 'PhabricatorAuthApplication' => 'PhabricatorApplication', 'PhabricatorAuthAuthFactorPHIDType' => 'PhabricatorPHIDType', 'PhabricatorAuthAuthProviderPHIDType' => 'PhabricatorPHIDType', + 'PhabricatorAuthCSRFEngine' => 'Phobject', 'PhabricatorAuthChallenge' => array( 'PhabricatorAuthDAO', 'PhabricatorPolicyInterface', diff --git a/src/applications/auth/engine/PhabricatorAuthCSRFEngine.php b/src/applications/auth/engine/PhabricatorAuthCSRFEngine.php new file mode 100644 --- /dev/null +++ b/src/applications/auth/engine/PhabricatorAuthCSRFEngine.php @@ -0,0 +1,119 @@ +salt = $salt; + return $this; + } + + public function getSalt() { + return $this->salt; + } + + public function setSecret(PhutilOpaqueEnvelope $secret) { + $this->secret = $secret; + return $this; + } + + public function getSecret() { + return $this->secret; + } + + public function newSalt() { + $salt_length = $this->getSaltLength(); + return Filesystem::readRandomCharacters($salt_length); + } + + public function newToken() { + $salt = $this->getSalt(); + + if (!$salt) { + throw new PhutilInvalidStateException('setSalt'); + } + + $token = $this->newRawToken($salt); + $prefix = $this->getBREACHPrefix(); + + return sprintf('%s%s%s', $prefix, $salt, $token); + } + + public function isValidToken($token) { + $salt_length = $this->getSaltLength(); + + // We expect a BREACH-mitigating token. See T3684. + $breach_prefix = $this->getBREACHPrefix(); + $breach_prelen = strlen($breach_prefix); + if (strncmp($token, $breach_prefix, $breach_prelen) !== 0) { + return false; + } + + $salt = substr($token, $breach_prelen, $salt_length); + $token = substr($token, $breach_prelen + $salt_length); + + foreach ($this->getWindowOffsets() as $offset) { + $expect_token = $this->newRawToken($salt, $offset); + if (phutil_hashes_are_identical($expect_token, $token)) { + return true; + } + } + + return false; + } + + private function newRawToken($salt, $offset = 0) { + $now = PhabricatorTime::getNow(); + $cycle_frequency = $this->getCycleFrequency(); + + $time_block = (int)floor($now / $cycle_frequency); + $time_block = $time_block + $offset; + + $secret = $this->getSecret(); + if (!$secret) { + throw new PhutilInvalidStateException('setSecret'); + } + $secret = $secret->openEnvelope(); + + $hash = PhabricatorHash::digestWithNamedKey( + $secret.$time_block.$salt, + 'csrf'); + + return substr($hash, 0, $this->getTokenLength()); + } + + private function getBREACHPrefix() { + return 'B@'; + } + + private function getSaltLength() { + return 8; + } + + private function getTokenLength() { + return 16; + } + + private function getCycleFrequency() { + return phutil_units('1 hour in seconds'); + } + + private function getWindowOffsets() { + // We accept some tokens from the recent past and near future. Users may + // have older tokens if they close their laptop and open it up again + // later. Users may have newer tokens if there are multiple web hosts with + // a bit of clock skew. + + // Javascript on the client tries to keep CSRF tokens up to date, but + // it may fail, and it doesn't run if the user closes their laptop. + + // The window during which our tokens remain valid is generally more + // conservative than other platforms. For example, Rails uses "session + // duration" and Django uses "forever". + + return range(-6, 1); + } + +} diff --git a/src/applications/base/controller/PhabricatorController.php b/src/applications/base/controller/PhabricatorController.php --- a/src/applications/base/controller/PhabricatorController.php +++ b/src/applications/base/controller/PhabricatorController.php @@ -98,7 +98,8 @@ if (!$user->isLoggedIn()) { - $user->attachAlternateCSRFString(PhabricatorHash::weakDigest($phsid)); + $csrf = PhabricatorHash::digestWithNamedKey($phsid, 'csrf.alternate'); + $user->attachAlternateCSRFString($csrf); } $request->setUser($user); diff --git a/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php b/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php --- a/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php +++ b/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php @@ -388,6 +388,9 @@ 'metamta.mail-key' => pht( 'Mail object address hash keys are now generated automatically.'), + + 'phabricator.csrf-key' => pht( + 'CSRF HMAC keys are now managed automatically.'), ); return $ancient_config; diff --git a/src/applications/config/option/PhabricatorSecurityConfigOptions.php b/src/applications/config/option/PhabricatorSecurityConfigOptions.php --- a/src/applications/config/option/PhabricatorSecurityConfigOptions.php +++ b/src/applications/config/option/PhabricatorSecurityConfigOptions.php @@ -154,21 +154,6 @@ pht('Multi-Factor Required'), pht('Multi-Factor Optional'), )), - $this->newOption( - 'phabricator.csrf-key', - 'string', - '0b7ec0592e0a2829d8b71df2fa269b2c6172eca3') - ->setHidden(true) - ->setSummary( - pht('Hashed with other inputs to generate CSRF tokens.')) - ->setDescription( - pht( - 'This is hashed with other inputs to generate CSRF tokens. If '. - 'you want, you can change it to some other string which is '. - 'unique to your install. This will make your install more secure '. - 'in a vague, mostly theoretical way. But it will take you like 3 '. - 'seconds of mashing on your keyboard to set it up so you might '. - 'as well.')), $this->newOption( 'uri.allowed-protocols', 'set', diff --git a/src/applications/people/storage/PhabricatorUser.php b/src/applications/people/storage/PhabricatorUser.php --- a/src/applications/people/storage/PhabricatorUser.php +++ b/src/applications/people/storage/PhabricatorUser.php @@ -318,112 +318,9 @@ return Filesystem::readRandomCharacters(255); } - const CSRF_CYCLE_FREQUENCY = 3600; - const CSRF_SALT_LENGTH = 8; - const CSRF_TOKEN_LENGTH = 16; - const CSRF_BREACH_PREFIX = 'B@'; - const EMAIL_CYCLE_FREQUENCY = 86400; const EMAIL_TOKEN_LENGTH = 24; - private function getRawCSRFToken($offset = 0) { - return $this->generateToken( - time() + (self::CSRF_CYCLE_FREQUENCY * $offset), - self::CSRF_CYCLE_FREQUENCY, - PhabricatorEnv::getEnvConfig('phabricator.csrf-key'), - self::CSRF_TOKEN_LENGTH); - } - - public function getCSRFToken() { - if ($this->isOmnipotent()) { - // We may end up here when called from the daemons. The omnipotent user - // has no meaningful CSRF token, so just return `null`. - return null; - } - - if ($this->csrfSalt === null) { - $this->csrfSalt = Filesystem::readRandomCharacters( - self::CSRF_SALT_LENGTH); - } - - $salt = $this->csrfSalt; - - // Generate a token hash to mitigate BREACH attacks against SSL. See - // discussion in T3684. - $token = $this->getRawCSRFToken(); - $hash = PhabricatorHash::weakDigest($token, $salt); - return self::CSRF_BREACH_PREFIX.$salt.substr( - $hash, 0, self::CSRF_TOKEN_LENGTH); - } - - public function validateCSRFToken($token) { - // We expect a BREACH-mitigating token. See T3684. - $breach_prefix = self::CSRF_BREACH_PREFIX; - $breach_prelen = strlen($breach_prefix); - if (strncmp($token, $breach_prefix, $breach_prelen) !== 0) { - return false; - } - - $salt = substr($token, $breach_prelen, self::CSRF_SALT_LENGTH); - $token = substr($token, $breach_prelen + self::CSRF_SALT_LENGTH); - - // When the user posts a form, we check that it contains a valid CSRF token. - // Tokens cycle each hour (every CSRF_CYCLE_FREQUENCY seconds) and we accept - // either the current token, the next token (users can submit a "future" - // token if you have two web frontends that have some clock skew) or any of - // the last 6 tokens. This means that pages are valid for up to 7 hours. - // There is also some Javascript which periodically refreshes the CSRF - // tokens on each page, so theoretically pages should be valid indefinitely. - // However, this code may fail to run (if the user loses their internet - // connection, or there's a JS problem, or they don't have JS enabled). - // Choosing the size of the window in which we accept old CSRF tokens is - // an issue of balancing concerns between security and usability. We could - // choose a very narrow (e.g., 1-hour) window to reduce vulnerability to - // attacks using captured CSRF tokens, but it's also more likely that real - // users will be affected by this, e.g. if they close their laptop for an - // hour, open it back up, and try to submit a form before the CSRF refresh - // can kick in. Since the user experience of submitting a form with expired - // CSRF is often quite bad (you basically lose data, or it's a big pain to - // recover at least) and I believe we gain little additional protection - // by keeping the window very short (the overwhelming value here is in - // preventing blind attacks, and most attacks which can capture CSRF tokens - // can also just capture authentication information [sniffing networks] - // or act as the user [xss]) the 7 hour default seems like a reasonable - // balance. Other major platforms have much longer CSRF token lifetimes, - // like Rails (session duration) and Django (forever), which suggests this - // is a reasonable analysis. - $csrf_window = 6; - - for ($ii = -$csrf_window; $ii <= 1; $ii++) { - $valid = $this->getRawCSRFToken($ii); - - $digest = PhabricatorHash::weakDigest($valid, $salt); - $digest = substr($digest, 0, self::CSRF_TOKEN_LENGTH); - if (phutil_hashes_are_identical($digest, $token)) { - return true; - } - } - - return false; - } - - private function generateToken($epoch, $frequency, $key, $len) { - if ($this->getPHID()) { - $vec = $this->getPHID().$this->getAccountSecret(); - } else { - $vec = $this->getAlternateCSRFString(); - } - - if ($this->hasSession()) { - $vec = $vec.$this->getSession()->getSessionKey(); - } - - $time_block = floor($epoch / $frequency); - $vec = $vec.$key.$time_block; - - return substr(PhabricatorHash::weakDigest($vec), 0, $len); - } - public function getUserProfile() { return $this->assertAttached($this->profile); } @@ -623,15 +520,6 @@ return (string)$uri; } - public function getAlternateCSRFString() { - return $this->assertAttached($this->alternateCSRFString); - } - - public function attachAlternateCSRFString($string) { - $this->alternateCSRFString = $string; - return $this; - } - /** * Populate the nametoken table, which used to fetch typeahead results. When * a user types "linc", we want to match "Abraham Lincoln" from on-demand @@ -1216,6 +1104,58 @@ return $this->assertAttached($this->badgePHIDs); } +/* -( CSRF )--------------------------------------------------------------- */ + + + public function getCSRFToken() { + if ($this->isOmnipotent()) { + // We may end up here when called from the daemons. The omnipotent user + // has no meaningful CSRF token, so just return `null`. + return null; + } + + return $this->newCSRFEngine() + ->newToken(); + } + + public function validateCSRFToken($token) { + return $this->newCSRFengine() + ->isValidToken($token); + } + + public function getAlternateCSRFString() { + return $this->assertAttached($this->alternateCSRFString); + } + + public function attachAlternateCSRFString($string) { + $this->alternateCSRFString = $string; + return $this; + } + + private function newCSRFEngine() { + if ($this->getPHID()) { + $vec = $this->getPHID().$this->getAccountSecret(); + } else { + $vec = $this->getAlternateCSRFString(); + } + + if ($this->hasSession()) { + $vec = $vec.$this->getSession()->getSessionKey(); + } + + $engine = new PhabricatorAuthCSRFEngine(); + + if ($this->csrfSalt === null) { + $this->csrfSalt = $engine->newSalt(); + } + + $engine + ->setSalt($this->csrfSalt) + ->setSecret(new PhutilOpaqueEnvelope($vec)); + + return $engine; + } + /* -( PhabricatorPolicyInterface )----------------------------------------- */