From 661f077bf7000955cf6d91d2a2c2bb87ecb541a7 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 10 Oct 2011 19:22:30 -0700
Subject: [PATCH] Replace callsites to sha1() that use it to asciify entropy
 with Filesystem::readRandomCharacters()

Summary: See T547. To improve auditability of use of crypto-sensitive hash
functions, use Filesystem::readRandomCharacters() in place of
sha1(Filesystem::readRandomBytes()) when we're just generating random ASCII
strings.

Test Plan:
  - Generated a new PHID.
  - Logged out and logged back in (to test sessions).
  - Regenerated Conduit certificate.
  - Created a new task, verified mail key generated sensibly.
  - Created a new revision, verified mail key generated sensibly.
  - Ran "arc list", got blocked, installed new certificate, ran "arc list"
again.

Reviewers: jungejason, nh, tuomaspelkonen, aran, benmathews

Reviewed By: jungejason

CC: aran, epriestley, jungejason

Differential Revision: 1000
---
 .../token/PhabricatorConduitTokenController.php           | 2 +-
 .../storage/revision/DifferentialRevision.php             | 2 +-
 .../files/engine/s3/PhabricatorS3FileStorageEngine.php    | 2 +-
 src/applications/maniphest/storage/task/ManiphestTask.php | 2 +-
 src/applications/people/storage/user/PhabricatorUser.php  | 8 ++------
 src/applications/phid/storage/phid/PhabricatorPHID.php    | 5 +----
 6 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/src/applications/conduit/controller/token/PhabricatorConduitTokenController.php b/src/applications/conduit/controller/token/PhabricatorConduitTokenController.php
index 7dfe11e0d2..482a4b306f 100644
--- a/src/applications/conduit/controller/token/PhabricatorConduitTokenController.php
+++ b/src/applications/conduit/controller/token/PhabricatorConduitTokenController.php
@@ -40,7 +40,7 @@ class PhabricatorConduitTokenController extends PhabricatorConduitController {
 
     $token = id(new PhabricatorConduitCertificateToken())
       ->setUserPHID($user->getPHID())
-      ->setToken(sha1(Filesystem::readRandomBytes(128)))
+      ->setToken(Filesystem::readRandomCharacters(40))
       ->save();
 
     $panel = new AphrontPanelView();
diff --git a/src/applications/differential/storage/revision/DifferentialRevision.php b/src/applications/differential/storage/revision/DifferentialRevision.php
index 9ad6e4fb39..e6c12c4ef8 100644
--- a/src/applications/differential/storage/revision/DifferentialRevision.php
+++ b/src/applications/differential/storage/revision/DifferentialRevision.php
@@ -117,7 +117,7 @@ class DifferentialRevision extends DifferentialDAO {
 
   public function save() {
     if (!$this->getMailKey()) {
-      $this->mailKey = sha1(Filesystem::readRandomBytes(20));
+      $this->mailKey = Filesystem::readRandomCharacters(40);
     }
     return parent::save();
   }
diff --git a/src/applications/files/engine/s3/PhabricatorS3FileStorageEngine.php b/src/applications/files/engine/s3/PhabricatorS3FileStorageEngine.php
index 41f5fb5abf..d78b50fd19 100644
--- a/src/applications/files/engine/s3/PhabricatorS3FileStorageEngine.php
+++ b/src/applications/files/engine/s3/PhabricatorS3FileStorageEngine.php
@@ -47,7 +47,7 @@ final class PhabricatorS3FileStorageEngine
   public function writeFile($data, array $params) {
     $s3 = $this->newS3API();
 
-    $name = 'phabricator/'.sha1(Filesystem::readRandomBytes(20));
+    $name = 'phabricator/'.Filesystem::readRandomCharacters(20);
 
     AphrontWriteGuard::willWrite();
     $s3->putObject(
diff --git a/src/applications/maniphest/storage/task/ManiphestTask.php b/src/applications/maniphest/storage/task/ManiphestTask.php
index 0064116177..67a110583e 100644
--- a/src/applications/maniphest/storage/task/ManiphestTask.php
+++ b/src/applications/maniphest/storage/task/ManiphestTask.php
@@ -123,7 +123,7 @@ class ManiphestTask extends ManiphestDAO {
 
   public function save() {
     if (!$this->mailKey) {
-      $this->mailKey = sha1(Filesystem::readRandomBytes(20));
+      $this->mailKey = Filesystem::readRandomCharacters(20);
     }
 
     $result = parent::save();
diff --git a/src/applications/people/storage/user/PhabricatorUser.php b/src/applications/people/storage/user/PhabricatorUser.php
index f63690034c..008b2d834c 100644
--- a/src/applications/people/storage/user/PhabricatorUser.php
+++ b/src/applications/people/storage/user/PhabricatorUser.php
@@ -97,10 +97,7 @@ class PhabricatorUser extends PhabricatorUserDAO {
   }
 
   private function generateConduitCertificate() {
-    $entropy = Filesystem::readRandomBytes(256);
-    $entropy = base64_encode($entropy);
-    $entropy = substr($entropy, 0, 255);
-    return $entropy;
+    return Filesystem::readRandomCharacters(255);
   }
 
   public function comparePassword($password) {
@@ -259,8 +256,7 @@ class PhabricatorUser extends PhabricatorUserDAO {
 
     // Consume entropy to generate a new session key, forestalling the eventual
     // heat death of the universe.
-    $entropy = Filesystem::readRandomBytes(20);
-    $session_key = sha1($entropy);
+    $session_key = Filesystem::readRandomCharacters(40);
 
     // UNGUARDED WRITES: Logging-in users don't have CSRF stuff yet.
     $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
diff --git a/src/applications/phid/storage/phid/PhabricatorPHID.php b/src/applications/phid/storage/phid/PhabricatorPHID.php
index 08053a0d1c..f84e794bcf 100644
--- a/src/applications/phid/storage/phid/PhabricatorPHID.php
+++ b/src/applications/phid/storage/phid/PhabricatorPHID.php
@@ -31,10 +31,7 @@ class PhabricatorPHID extends PhabricatorPHIDDAO {
       throw new Exception("Can not generate PHID with no type.");
     }
 
-    $entropy = Filesystem::readRandomBytes(20);
-
-    $uniq = sha1($entropy);
-    $uniq = substr($uniq, 0, 20);
+    $uniq = Filesystem::readRandomCharacters(20);
     $phid = 'PHID-'.$type.'-'.$uniq;
 
     $phid_rec = new PhabricatorPHID();