From 2b3d3cf7e4c0ba9832e8bfb78fc62aef9c128e40 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 2 Apr 2015 13:42:22 -0700
Subject: [PATCH] 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
---
 src/__phutil_library_map__.php                |  1 +
 ...atorRepositoryManagementUpdateWorkflow.php |  5 +-
 .../util/PhabricatorGlobalLock.php            | 15 +++++-
 src/infrastructure/util/PhabricatorHash.php   | 50 ++++++++++++++++++-
 4 files changed, 65 insertions(+), 6 deletions(-)

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index 4d31355db7..4e3218ef9b 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -5235,6 +5235,7 @@ phutil_register_library_map(array(
     'PhabricatorHandleQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
     'PhabricatorHarbormasterApplication' => 'PhabricatorApplication',
     'PhabricatorHarbormasterConfigOptions' => 'PhabricatorApplicationConfigOptions',
+    'PhabricatorHash' => 'Phobject',
     'PhabricatorHashTestCase' => 'PhabricatorTestCase',
     'PhabricatorHelpApplication' => 'PhabricatorApplication',
     'PhabricatorHelpController' => 'PhabricatorController',
diff --git a/src/applications/repository/management/PhabricatorRepositoryManagementUpdateWorkflow.php b/src/applications/repository/management/PhabricatorRepositoryManagementUpdateWorkflow.php
index 4dfee595e1..a664362d91 100644
--- a/src/applications/repository/management/PhabricatorRepositoryManagementUpdateWorkflow.php
+++ b/src/applications/repository/management/PhabricatorRepositoryManagementUpdateWorkflow.php
@@ -51,10 +51,9 @@ final class PhabricatorRepositoryManagementUpdateWorkflow
     }
 
     $repository = head($repos);
-    $callsign = $repository->getCallsign();
 
     try {
-      $lock_name = get_class($this).':'.$callsign;
+      $lock_name = 'repository.update:'.$repository->getID();
       $lock = PhabricatorGlobalLock::newLock($lock_name);
 
       $lock->lock();
@@ -135,7 +134,7 @@ final class PhabricatorRepositoryManagementUpdateWorkflow
       $proxy = new PhutilProxyException(
         pht(
           'Error while pushing "%s" repository to mirrors.',
-          $repository->getCallsign()),
+          $repository->getMonogram()),
         $ex);
       phlog($proxy);
     }
diff --git a/src/infrastructure/util/PhabricatorGlobalLock.php b/src/infrastructure/util/PhabricatorGlobalLock.php
index 349c7f6531..eca6ee4228 100644
--- a/src/infrastructure/util/PhabricatorGlobalLock.php
+++ b/src/infrastructure/util/PhabricatorGlobalLock.php
@@ -39,7 +39,20 @@ final class PhabricatorGlobalLock extends PhutilLock {
 
   public static function newLock($name) {
     $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);
     if (!$lock) {
diff --git a/src/infrastructure/util/PhabricatorHash.php b/src/infrastructure/util/PhabricatorHash.php
index 64f02b867f..dd158d1168 100644
--- a/src/infrastructure/util/PhabricatorHash.php
+++ b/src/infrastructure/util/PhabricatorHash.php
@@ -1,7 +1,8 @@
 <?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.
@@ -68,11 +69,56 @@ final class PhabricatorHash {
     }
 
     $result = '';
-    for ($ii = 0; $ii < 12; $ii++) {
+    for ($ii = 0; $ii < self::INDEX_DIGEST_LENGTH; $ii++) {
       $result .= $map[(ord($hash[$ii]) & 0x3F)];
     }
 
     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;
+  }
+
+
 }