mirror of
https://we.phorge.it/source/phorge.git
synced 2025-01-31 08:58:20 +01:00
Enforce that global locks have keys shorter than 64 characters
Summary: Fixes T7484. There's a bunch of spooky mystery here but the current behavior can probably cause problems in at least some situations. Also moves a couple callsigns to monograms (see T4245). Test Plan: - Faked a short lock length to hit the exception. - Updated normally. - Grepped for other use sites, none seemed suspicious or likely to overflow the lock length. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T7484 Differential Revision: https://secure.phabricator.com/D12263
This commit is contained in:
parent
c39b64d963
commit
2b3d3cf7e4
4 changed files with 65 additions and 6 deletions
|
@ -5235,6 +5235,7 @@ phutil_register_library_map(array(
|
||||||
'PhabricatorHandleQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
|
'PhabricatorHandleQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
|
||||||
'PhabricatorHarbormasterApplication' => 'PhabricatorApplication',
|
'PhabricatorHarbormasterApplication' => 'PhabricatorApplication',
|
||||||
'PhabricatorHarbormasterConfigOptions' => 'PhabricatorApplicationConfigOptions',
|
'PhabricatorHarbormasterConfigOptions' => 'PhabricatorApplicationConfigOptions',
|
||||||
|
'PhabricatorHash' => 'Phobject',
|
||||||
'PhabricatorHashTestCase' => 'PhabricatorTestCase',
|
'PhabricatorHashTestCase' => 'PhabricatorTestCase',
|
||||||
'PhabricatorHelpApplication' => 'PhabricatorApplication',
|
'PhabricatorHelpApplication' => 'PhabricatorApplication',
|
||||||
'PhabricatorHelpController' => 'PhabricatorController',
|
'PhabricatorHelpController' => 'PhabricatorController',
|
||||||
|
|
|
@ -51,10 +51,9 @@ final class PhabricatorRepositoryManagementUpdateWorkflow
|
||||||
}
|
}
|
||||||
|
|
||||||
$repository = head($repos);
|
$repository = head($repos);
|
||||||
$callsign = $repository->getCallsign();
|
|
||||||
|
|
||||||
try {
|
try {
|
||||||
$lock_name = get_class($this).':'.$callsign;
|
$lock_name = 'repository.update:'.$repository->getID();
|
||||||
$lock = PhabricatorGlobalLock::newLock($lock_name);
|
$lock = PhabricatorGlobalLock::newLock($lock_name);
|
||||||
|
|
||||||
$lock->lock();
|
$lock->lock();
|
||||||
|
@ -135,7 +134,7 @@ final class PhabricatorRepositoryManagementUpdateWorkflow
|
||||||
$proxy = new PhutilProxyException(
|
$proxy = new PhutilProxyException(
|
||||||
pht(
|
pht(
|
||||||
'Error while pushing "%s" repository to mirrors.',
|
'Error while pushing "%s" repository to mirrors.',
|
||||||
$repository->getCallsign()),
|
$repository->getMonogram()),
|
||||||
$ex);
|
$ex);
|
||||||
phlog($proxy);
|
phlog($proxy);
|
||||||
}
|
}
|
||||||
|
|
|
@ -39,7 +39,20 @@ final class PhabricatorGlobalLock extends PhutilLock {
|
||||||
|
|
||||||
public static function newLock($name) {
|
public static function newLock($name) {
|
||||||
$namespace = PhabricatorLiskDAO::getStorageNamespace();
|
$namespace = PhabricatorLiskDAO::getStorageNamespace();
|
||||||
$full_name = 'global:'.$namespace.':'.$name;
|
$namespace = PhabricatorHash::digestToLength($namespace, 20);
|
||||||
|
|
||||||
|
$full_name = $namespace.'-g:'.$name;
|
||||||
|
|
||||||
|
$length_limit = 64;
|
||||||
|
if (strlen($full_name) > $length_limit) {
|
||||||
|
throw new Exception(
|
||||||
|
pht(
|
||||||
|
'Lock name "%s" is too long (full lock name is "%s"). The '.
|
||||||
|
'full lock name must not be longer than %s bytes.',
|
||||||
|
$name,
|
||||||
|
$full_name,
|
||||||
|
new PhutilNumber($length_limit)));
|
||||||
|
}
|
||||||
|
|
||||||
$lock = self::getLock($full_name);
|
$lock = self::getLock($full_name);
|
||||||
if (!$lock) {
|
if (!$lock) {
|
||||||
|
|
|
@ -1,7 +1,8 @@
|
||||||
<?php
|
<?php
|
||||||
|
|
||||||
final class PhabricatorHash {
|
final class PhabricatorHash extends Phobject {
|
||||||
|
|
||||||
|
const INDEX_DIGEST_LENGTH = 12;
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Digest a string for general use, including use which relates to security.
|
* Digest a string for general use, including use which relates to security.
|
||||||
|
@ -68,11 +69,56 @@ final class PhabricatorHash {
|
||||||
}
|
}
|
||||||
|
|
||||||
$result = '';
|
$result = '';
|
||||||
for ($ii = 0; $ii < 12; $ii++) {
|
for ($ii = 0; $ii < self::INDEX_DIGEST_LENGTH; $ii++) {
|
||||||
$result .= $map[(ord($hash[$ii]) & 0x3F)];
|
$result .= $map[(ord($hash[$ii]) & 0x3F)];
|
||||||
}
|
}
|
||||||
|
|
||||||
return $result;
|
return $result;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Shorten a string to a maximum byte length in a collision-resistant way
|
||||||
|
* while retaining some degree of human-readability.
|
||||||
|
*
|
||||||
|
* This function converts an input string into a prefix plus a hash. For
|
||||||
|
* example, a very long string beginning with "crabapplepie..." might be
|
||||||
|
* digested to something like "crabapp-N1wM1Nz3U84k".
|
||||||
|
*
|
||||||
|
* This allows the maximum length of identifiers to be fixed while
|
||||||
|
* maintaining a high degree of collision resistance and a moderate degree
|
||||||
|
* of human readability.
|
||||||
|
*
|
||||||
|
* @param string The string to shorten.
|
||||||
|
* @param int Maximum length of the result.
|
||||||
|
* @return string String shortened in a collision-resistant way.
|
||||||
|
*/
|
||||||
|
public static function digestToLength($string, $length) {
|
||||||
|
// We need at least two more characters than the hash length to fit in a
|
||||||
|
// a 1-character prefix and a separator.
|
||||||
|
$min_length = self::INDEX_DIGEST_LENGTH + 2;
|
||||||
|
if ($length < $min_length) {
|
||||||
|
throw new Exception(
|
||||||
|
pht(
|
||||||
|
'Length parameter in digestToLength() must be at least %s, '.
|
||||||
|
'but %s was provided.',
|
||||||
|
new PhutilNumber($min_length),
|
||||||
|
new PhutilNumber($length)));
|
||||||
|
}
|
||||||
|
|
||||||
|
// We could conceivably return the string unmodified if it's shorter than
|
||||||
|
// the specified length. Instead, always hash it. This makes the output of
|
||||||
|
// the method more recognizable and consistent (no surprising new behavior
|
||||||
|
// once you hit a string longer than `$length`) and prevents an attacker
|
||||||
|
// who can control the inputs from intentionally using the hashed form
|
||||||
|
// of a string to cause a collision.
|
||||||
|
|
||||||
|
$hash = PhabricatorHash::digestForIndex($string);
|
||||||
|
|
||||||
|
$prefix = substr($string, 0, ($length - ($min_length - 1)));
|
||||||
|
|
||||||
|
return $prefix.'-'.$hash;
|
||||||
|
}
|
||||||
|
|
||||||
|
|
||||||
}
|
}
|
||||||
|
|
Loading…
Add table
Reference in a new issue