Page MenuHomePhabricator
Files F13194293

D19946.diff
No OneTemporary
Actions

D19946.diff
View Options

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 @@
+<?php
+
+final class PhabricatorAuthCSRFEngine extends Phobject {
+
+ private $salt;
+ private $secret;
+
+ public function setSalt($salt) {
+ $this->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 )----------------------------------------- */

File Metadata

Mime Type
text/plain
Expires
Mon, May 13, 9:28 PM (2 w, 6 d ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
6275633
Default Alt Text
D19946.diff (13 KB)

Event Timeline

Phabricator · Built by Phacility