removed encryptor string compare: php7.2+ has hash_equals

pavarnos · Aug 12, 2018 · 447c7a3 · 447c7a3
1 parent 0b26785
commit 447c7a3
Show file tree

Hide file tree

Showing 3 changed files with 6 additions and 88 deletions.
diff --git a/src/Threema/MsgApi/Encryptor/AbstractEncryptor.php b/src/Threema/MsgApi/Encryptor/AbstractEncryptor.php
@@ -29,20 +29,19 @@
 abstract class AbstractEncryptor
 {
  const TYPE_SODIUM = 'sodium';
- const TYPE_SALT = 'salt';
 
  const FILE_NONCE = "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x01";
  const FILE_THUMBNAIL_NONCE = "\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02";
 
- const MESSAGE_ID_LEN  = 8;
+ const MESSAGE_ID_LEN = 8;
 
- const BLOB_ID_LEN  = 16;
+ const BLOB_ID_LEN = 16;
 
  const IMAGE_FILE_SIZE_LEN = 4;
 
- const IMAGE_NONCE_LEN  = 24;
+ const IMAGE_NONCE_LEN = 24;
 
- const EMAIL_HMAC_KEY  = "\x30\xa5\x50\x0f\xed\x97\x01\xfa\x6d\xef\xdb\x61\x08\x41\x90\x0f\xeb\xb8\xe4\x30\x88\x1f\x7a\xd8\x16\x82\x62\x64\xec\x09\xba\xd7";
+ const EMAIL_HMAC_KEY = "\x30\xa5\x50\x0f\xed\x97\x01\xfa\x6d\xef\xdb\x61\x08\x41\x90\x0f\xeb\xb8\xe4\x30\x88\x1f\x7a\xd8\x16\x82\x62\x64\xec\x09\xba\xd7";
 
  const PHONENO_HMAC_KEY = "\x85\xad\xf8\x22\x69\x53\xf3\xd9\x6c\xfd\x5d\x09\xbf\x29\x55\x5e\xb9\x55\xfc\xd8\xaa\x5e\xc4\xf9\xfc\xd8\x69\xe2\x58\x37\x07\x23";
 
@@ -425,7 +424,7 @@ public function bin2hex($binaryString)
  }
 
  /**
- * Converts an hexsdecimal string to a binary string.
+ * Converts an hexadecimal string to a binary string.
  *
  * This is the same as PHP s hex2bin() implementation, but it is resistant to
  * timing attacks.
@@ -445,46 +444,6 @@ public function hex2bin($hexString, $ignore = null)
  return hex2bin($hexString);
  }
 
- /**
- * Compares two strings in a secure way.
- *
- * This is the same as PHP's strcmp() implementation, but it is resistant to
- * timing attacks.
- *
- * @link https://paragonie.com/book/pecl-libsodium/read/03-utilities-helpers.md#compare
- * @param string $str1 The first string
- * @param string $str2 The second string
- * @return bool
- */
- public function stringCompare($str1, $str2)
- {
- if (function_exists('hash_equals')) {
- return hash_equals($str1, $str2);
- } else {
- // check variable type manually
- if (!is_string($str1) || !is_string($str2)) {
- return false;
- }
-
- // fast comparison: check string length
- if (strlen($str1) != strlen($str2)) {
- return false;
- }
-
- # PHP implementation of hash_equals
- # partly taken from https://github.com/symfony/polyfill-php56/blob/master/Php56.php#L45-L51
- #
- # Note that this is really slow!!
- #
- $ret = 0;
- $length = strlen($str1);
- for ($i = 0; $i < $length; ++$i) {
- $ret |= ord($str1[$i]) ^ ord($str2[$i]);
- }
- return 0 === $ret;
- }
- }
-
  /**
  * @return string
  */

diff --git a/src/Threema/MsgApi/Helpers/E2EHelper.php b/src/Threema/MsgApi/Helpers/E2EHelper.php
@@ -308,7 +308,7 @@ public final function receiveMessage(
  public final function checkMac($threemaId, $gatewayId, $messageId, $date, $nonce, $box, $mac, $secret)
  {
  $calculatedMac = hash_hmac('sha256', $threemaId . $gatewayId . $messageId . $date . $nonce . $box, $secret);
- return $this->encryptor->stringCompare($calculatedMac, $mac) === true;
+ return hash_equals($calculatedMac, $mac);
  }
 
  private final function assertIsCapable(string $threemaId, string $wantedCapability)

diff --git a/tests/Threema/MsgApi/EncryptorTest.php b/tests/Threema/MsgApi/EncryptorTest.php
@@ -157,47 +157,6 @@ public function testHexBin()
  });
  }
 
- /**
- * test compare functions to make sure they are resistant to timing attacks
- */
- public function testCompare()
- {
- $this->doTest(function (AbstractEncryptor $encryptor, $prefix) {
- // make strings large enough
- $string1 = str_repeat(TestConstants::myPrivateKey, 100000);
- $string2 = str_repeat(TestConstants::otherPrivateKey, 100000);
- echo PHP_EOL;
-
- $humanDescr = [
- 'length' => 'different length',
- 'diff' => 'same length, different content',
- 'same' => 'same length, same content',
- ];
-
- // test different strings when comparing
- $comparisonResult = [];
- foreach ([
- 'length' => [$string1, $string1 . 'a'],
- 'diff' => [$string1, $string2],
- 'same' => [$string1, $string1],
- ] as $testName => $strings) {
- for ($i = 0; $i < 3; $i++) {
- // test run with delay
- $comparisonResult[$testName][$i] = $encryptor->stringCompare($strings[0], $strings[1]);
-
- // check result
- if ($testName == 'length' || $testName == 'diff') {
- $this->assertEquals(false, $comparisonResult[$testName][$i],
- $prefix . ': comparison of "' . $humanDescr[$testName] . ' #' . $i . '" is wrong: expected: false, got ' . $comparisonResult[$testName][$i]);
- } else {
- $this->assertEquals(true, $comparisonResult[$testName][$i],
- $prefix . ': comparison of "' . $humanDescr[$testName] . ' #' . $i . '" is wrong: expected: true, got ' . $comparisonResult[$testName][$i]);
- }
- }
- }
- });
- }
-
  private function doTest(\Closure $c)
  {
  foreach ([