mirror of
https://we.phorge.it/source/phorge.git
synced 2024-12-18 19:40:55 +01:00
Use better secrets in generating account tokens
Summary: When we generate account tokens for CSRF keys and email verification, one of the inputs we use is the user's password hash. Users won't always have a password hash, so this is a weak input to key generation. This also couples CSRF weirdly with auth concerns. Instead, give users a dedicated secret for use in token generation which is used only for this purpose. Test Plan: - Ran upgrade scripts. - Verified all users got new secrets. - Created a new user. - Verified they got a secret. - Submitted CSRF'd forms, they worked. - Adjusted the CSRF token and submitted CSRF'd forms, verified they don't work. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Differential Revision: https://secure.phabricator.com/D8748
This commit is contained in:
parent
3018a5eb09
commit
ab7d89edc8
3 changed files with 33 additions and 1 deletions
2
resources/sql/autopatches/20140410.accountsecret.1.sql
Normal file
2
resources/sql/autopatches/20140410.accountsecret.1.sql
Normal file
|
@ -0,0 +1,2 @@
|
||||||
|
ALTER TABLE {$NAMESPACE}_user.user
|
||||||
|
ADD accountSecret CHAR(64) NOT NULL COLLATE latin1_bin;
|
23
resources/sql/autopatches/20140410.accountsecret.2.php
Normal file
23
resources/sql/autopatches/20140410.accountsecret.2.php
Normal file
|
@ -0,0 +1,23 @@
|
||||||
|
<?php
|
||||||
|
|
||||||
|
echo "Updating users...\n";
|
||||||
|
|
||||||
|
|
||||||
|
foreach (new LiskMigrationIterator(new PhabricatorUser()) as $user) {
|
||||||
|
|
||||||
|
$id = $user->getID();
|
||||||
|
echo "Updating {$id}...\n";
|
||||||
|
|
||||||
|
if (strlen($user->getAccountSecret())) {
|
||||||
|
continue;
|
||||||
|
}
|
||||||
|
|
||||||
|
queryfx(
|
||||||
|
$user->establishConnection('w'),
|
||||||
|
'UPDATE %T SET accountSecret = %s WHERE id = %d',
|
||||||
|
$user->getTableName(),
|
||||||
|
Filesystem::readRandomCharacters(64),
|
||||||
|
$id);
|
||||||
|
}
|
||||||
|
|
||||||
|
echo "Done.\n";
|
|
@ -32,6 +32,8 @@ final class PhabricatorUser
|
||||||
protected $isEmailVerified = 0;
|
protected $isEmailVerified = 0;
|
||||||
protected $isApproved = 0;
|
protected $isApproved = 0;
|
||||||
|
|
||||||
|
protected $accountSecret;
|
||||||
|
|
||||||
private $profileImage = self::ATTACHABLE;
|
private $profileImage = self::ATTACHABLE;
|
||||||
private $profile = null;
|
private $profile = null;
|
||||||
private $status = self::ATTACHABLE;
|
private $status = self::ATTACHABLE;
|
||||||
|
@ -157,6 +159,11 @@ final class PhabricatorUser
|
||||||
if (!$this->getConduitCertificate()) {
|
if (!$this->getConduitCertificate()) {
|
||||||
$this->setConduitCertificate($this->generateConduitCertificate());
|
$this->setConduitCertificate($this->generateConduitCertificate());
|
||||||
}
|
}
|
||||||
|
|
||||||
|
if (!strlen($this->getAccountSecret())) {
|
||||||
|
$this->setAccountSecret(Filesystem::readRandomCharacters(64));
|
||||||
|
}
|
||||||
|
|
||||||
$result = parent::save();
|
$result = parent::save();
|
||||||
|
|
||||||
if ($this->profile) {
|
if ($this->profile) {
|
||||||
|
@ -305,7 +312,7 @@ final class PhabricatorUser
|
||||||
|
|
||||||
private function generateToken($epoch, $frequency, $key, $len) {
|
private function generateToken($epoch, $frequency, $key, $len) {
|
||||||
if ($this->getPHID()) {
|
if ($this->getPHID()) {
|
||||||
$vec = $this->getPHID().$this->getPasswordHash();
|
$vec = $this->getPHID().$this->getAccountSecret();
|
||||||
} else {
|
} else {
|
||||||
$vec = $this->getAlternateCSRFString();
|
$vec = $this->getAlternateCSRFString();
|
||||||
}
|
}
|
||||||
|
|
Loading…
Reference in a new issue