From ed556643199e912cba2bdc58ca008ed77ab82121 Mon Sep 17 00:00:00 2001 From: Taylor Hornby Date: Mon, 23 Apr 2018 04:19:23 -0400 Subject: [PATCH 01/14] Add and use Core::ensureTrue(, ) --- src/Core.php | 119 +++++++++++++++++++----------------------- src/Crypto.php | 44 ++++------------ src/Encoding.php | 18 +++---- src/File.php | 48 +++++------------ src/Key.php | 9 ++-- src/KeyOrPassword.php | 7 +-- src/RuntimeTests.php | 39 ++++---------- 7 files changed, 102 insertions(+), 182 deletions(-) diff --git a/src/Core.php b/src/Core.php index 73c3029..856d8d5 100644 --- a/src/Core.php +++ b/src/Core.php @@ -50,29 +50,25 @@ final class Core */ public static function incrementCounter($ctr, $inc) { - if (Core::ourStrlen($ctr) !== Core::BLOCK_BYTE_SIZE) { - throw new Ex\EnvironmentIsBrokenException( - 'Trying to increment a nonce of the wrong size.' - ); - } - - if (! \is_int($inc)) { - throw new Ex\EnvironmentIsBrokenException( - 'Trying to increment nonce by a non-integer.' - ); - } - - if ($inc < 0) { - throw new Ex\EnvironmentIsBrokenException( - 'Trying to increment nonce by a negative amount.' - ); - } - - if ($inc > PHP_INT_MAX - 255) { - throw new Ex\EnvironmentIsBrokenException( - 'Integer overflow may occur.' - ); - } + Core::ensureTrue( + Core::ourStrlen($ctr) === Core::BLOCK_BYTE_SIZE, + 'Trying to increment a nonce of the wrong size.' + ); + + Core::ensureTrue( + \is_int($inc), + 'Trying to increment nonce by a non-integer.' + ); + + Core::ensureTrue( + $inc >= 0, + 'Trying to increment a nonce by a nonpositive amount' + ); + + Core::ensureTrue( + $inc <= PHP_INT_MAX - 255, + 'Integer overflow may occur' + ); /* * We start at the rightmost byte (big-endian) @@ -82,11 +78,7 @@ public static function incrementCounter($ctr, $inc) $sum = \ord($ctr[$i]) + $inc; /* Detect integer overflow and fail. */ - if (! \is_int($sum)) { - throw new Ex\EnvironmentIsBrokenException( - 'Integer overflow in CTR mode nonce increment.' - ); - } + Core::ensureTrue(\is_int($sum), 'Integer overflow in CTR mode nonce increment'); $ctr[$i] = \pack('C', $sum & 0xFF); $inc = $sum >> 8; @@ -146,12 +138,10 @@ public static function HKDF($hash, $ikm, $length, $info = '', $salt = null) $digest_length = Core::ourStrlen(\hash_hmac($hash, '', '', true)); // Sanity-check the desired output length. - if (empty($length) || ! \is_int($length) || - $length < 0 || $length > 255 * $digest_length) { - throw new Ex\EnvironmentIsBrokenException( - 'Bad output length requested of HKDF.' - ); - } + Core::ensureTrue( + !empty($length) && \is_int($length) && $length >= 0 && $length <= 255 * $digest_length, + 'Bad output length requested of HDKF.' + ); // "if [salt] not provided, is set to a string of HashLen zeroes." if (\is_null($salt)) { @@ -166,9 +156,7 @@ public static function HKDF($hash, $ikm, $length, $info = '', $salt = null) // HKDF-Expand: // This check is useless, but it serves as a reminder to the spec. - if (Core::ourStrlen($prk) < $digest_length) { - throw new Ex\EnvironmentIsBrokenException(); - } + Core::ensureTrue(Core::ourStrlen($prk) >= $digest_length); // T(0) = '' $t = ''; @@ -188,9 +176,7 @@ public static function HKDF($hash, $ikm, $length, $info = '', $salt = null) // ORM = first L octets of T /** @var string $orm */ $orm = Core::ourSubstr($t, 0, $length); - if (!\is_string($orm)) { - throw new Ex\EnvironmentIsBrokenException(); - } + Core::ensureTrue(\is_string($orm)); return $orm; } @@ -224,9 +210,7 @@ public static function hashEquals($expected, $given) // We're not attempting to make variable-length string comparison // secure, as that's very difficult. Make sure the strings are the same // length. - if (Core::ourStrlen($expected) !== Core::ourStrlen($given)) { - throw new Ex\EnvironmentIsBrokenException(); - } + Core::ensureTrue( Core::ourStrlen($expected) === Core::ourStrlen($given)); $blind = Core::secureRandom(32); $message_compare = \hash_hmac(Core::HASH_FUNCTION_NAME, $given, $blind); @@ -243,9 +227,7 @@ public static function hashEquals($expected, $given) */ public static function ensureConstantExists($name) { - if (! \defined($name)) { - throw new Ex\EnvironmentIsBrokenException(); - } + Core::ensureTrue(\defined($name)); } /** @@ -258,8 +240,21 @@ public static function ensureConstantExists($name) */ public static function ensureFunctionExists($name) { - if (! \function_exists($name)) { - throw new Ex\EnvironmentIsBrokenException(); + Core::ensureTrue(\function_exists($name)); + } + + /** + * Throws an exception if the condition is false. + * + * @param bool $condition + * @return void + * + * @throws Ex\EnvironmentIsBrokenException + */ + public static function ensureTrue($condition, $message = '') + { + if (!$condition) { + throw new Ex\EnvironmentIsBrokenException($message); } } @@ -286,9 +281,7 @@ public static function ourStrlen($str) } if ($exists) { $length = \mb_strlen($str, '8bit'); - if ($length === false) { - throw new Ex\EnvironmentIsBrokenException(); - } + Core::ensureTrue($length !== false); return $length; } else { return \strlen($str); @@ -403,28 +396,22 @@ public static function pbkdf2($algorithm, $password, $salt, $count, $key_length, $key_length += 0; $algorithm = \strtolower($algorithm); - if (! \in_array($algorithm, \hash_algos(), true)) { - throw new Ex\EnvironmentIsBrokenException( - 'Invalid or unsupported hash algorithm.' - ); - } + Core::ensureTrue( + \in_array($algorithm, \hash_algos(), true), + 'Invalid or unsupported hash algorithm.' + ); // Whitelist, or we could end up with people using CRC32. $ok_algorithms = [ 'sha1', 'sha224', 'sha256', 'sha384', 'sha512', 'ripemd160', 'ripemd256', 'ripemd320', 'whirlpool', ]; - if (! \in_array($algorithm, $ok_algorithms, true)) { - throw new Ex\EnvironmentIsBrokenException( - 'Algorithm is not a secure cryptographic hash function.' - ); - } + Core::ensureTrue( + \in_array($algorithm, $ok_algorithms, true), + 'Algorithm is not a secure cryptographic hash function.' + ); - if ($count <= 0 || $key_length <= 0) { - throw new Ex\EnvironmentIsBrokenException( - 'Invalid PBKDF2 parameters.' - ); - } + Core::ensureTrue($count > 0 && $key_length > 0, 'Invalid PBKDF2 parameters.'); if (\function_exists('hash_pbkdf2')) { // The output length is in NIBBLES (4-bits) if $raw_output is false! diff --git a/src/Crypto.php b/src/Crypto.php index bac67d2..016c6a1 100644 --- a/src/Crypto.php +++ b/src/Crypto.php @@ -181,16 +181,12 @@ public static function legacyDecrypt($ciphertext, $key) * @var string */ $hmac = Core::ourSubstr($ciphertext, 0, Core::LEGACY_MAC_BYTE_SIZE); - if (!\is_string($hmac)) { - throw new Ex\EnvironmentIsBrokenException(); - } + Core::ensureTrue(\is_string($hmac)); /** * @var string */ $messageCiphertext = Core::ourSubstr($ciphertext, Core::LEGACY_MAC_BYTE_SIZE); - if (!\is_string($messageCiphertext)) { - throw new Ex\EnvironmentIsBrokenException(); - } + Core::ensureTrue(\is_string($messageCiphertext)); // Regenerate the same authentication sub-key. $akey = Core::HKDF( @@ -221,17 +217,13 @@ public static function legacyDecrypt($ciphertext, $key) * @var string */ $iv = Core::ourSubstr($messageCiphertext, 0, Core::LEGACY_BLOCK_BYTE_SIZE); - if (!\is_string($iv)) { - throw new Ex\EnvironmentIsBrokenException(); - } + Core::ensureTrue(\is_string($iv)); /** * @var string */ $actualCiphertext = Core::ourSubstr($messageCiphertext, Core::LEGACY_BLOCK_BYTE_SIZE); - if (!\is_string($actualCiphertext)) { - throw new Ex\EnvironmentIsBrokenException(); - } + Core::ensureTrue(\is_string($actualCiphertext)); // Do the decryption. $plaintext = self::plainDecrypt($actualCiphertext, $ekey, $iv, Core::LEGACY_CIPHER_METHOD); @@ -320,9 +312,7 @@ private static function decryptInternal($ciphertext, KeyOrPassword $secret, $raw Core::HEADER_VERSION_SIZE, Core::SALT_BYTE_SIZE ); - if (!\is_string($salt)) { - throw new Ex\EnvironmentIsBrokenException(); - } + Core::ensureTrue(\is_string($salt)); // Get the IV. /** @var string $iv */ @@ -331,9 +321,7 @@ private static function decryptInternal($ciphertext, KeyOrPassword $secret, $raw Core::HEADER_VERSION_SIZE + Core::SALT_BYTE_SIZE, Core::BLOCK_BYTE_SIZE ); - if (!\is_string($iv)) { - throw new Ex\EnvironmentIsBrokenException(); - } + Core::ensureTrue(\is_string($iv)); // Get the HMAC. /** @var string $hmac */ @@ -342,9 +330,7 @@ private static function decryptInternal($ciphertext, KeyOrPassword $secret, $raw Core::ourStrlen($ciphertext) - Core::MAC_BYTE_SIZE, Core::MAC_BYTE_SIZE ); - if (!\is_string($hmac)) { - throw new Ex\EnvironmentIsBrokenException(); - } + Core::ensureTrue(\is_string($hmac)); // Get the actual encrypted ciphertext. /** @var string $encrypted */ @@ -355,9 +341,7 @@ private static function decryptInternal($ciphertext, KeyOrPassword $secret, $raw Core::ourStrlen($ciphertext) - Core::MAC_BYTE_SIZE - Core::SALT_BYTE_SIZE - Core::BLOCK_BYTE_SIZE - Core::HEADER_VERSION_SIZE ); - if (!\is_string($encrypted)) { - throw new Ex\EnvironmentIsBrokenException(); - } + Core::ensureTrue(\is_string($encrypted)); // Derive the separate encryption and authentication keys from the key // or password, whichever it is. @@ -397,11 +381,7 @@ protected static function plainEncrypt($plaintext, $key, $iv) $iv ); - if (!\is_string($ciphertext)) { - throw new Ex\EnvironmentIsBrokenException( - 'openssl_encrypt() failed.' - ); - } + Core::ensureTrue(\is_string($ciphertext), 'openssl_encrypt() failed'); return $ciphertext; } @@ -431,11 +411,7 @@ protected static function plainDecrypt($ciphertext, $key, $iv, $cipherMethod) OPENSSL_RAW_DATA, $iv ); - if (!\is_string($plaintext)) { - throw new Ex\EnvironmentIsBrokenException( - 'openssl_decrypt() failed.' - ); - } + Core::ensureTrue(\is_string($plaintext), 'openssl_decrypt() failed.'); return $plaintext; } diff --git a/src/Encoding.php b/src/Encoding.php index 001fb6e..8f933cf 100644 --- a/src/Encoding.php +++ b/src/Encoding.php @@ -178,11 +178,10 @@ public static function saveBytesToChecksummedAsciiSafeString($header, $bytes) { // Headers must be a constant length to prevent one type's header from // being a prefix of another type's header, leading to ambiguity. - if (Core::ourStrlen($header) !== self::SERIALIZE_HEADER_BYTES) { - throw new Ex\EnvironmentIsBrokenException( - 'Header must be ' . self::SERIALIZE_HEADER_BYTES . ' bytes.' - ); - } + Core::ensureTrue( + Core::ourStrlen($header) === self::SERIALIZE_HEADER_BYTES, + 'Header must be ' . self::SERIALIZE_HEADER_BYTES . ' bytes.' + ); return Encoding::binToHex( $header . @@ -211,11 +210,10 @@ public static function loadBytesFromChecksummedAsciiSafeString($expected_header, { // Headers must be a constant length to prevent one type's header from // being a prefix of another type's header, leading to ambiguity. - if (Core::ourStrlen($expected_header) !== self::SERIALIZE_HEADER_BYTES) { - throw new Ex\EnvironmentIsBrokenException( - 'Header must be 4 bytes.' - ); - } + Core::ensureTrue( + Core::ourStrlen($expected_header) === self::SERIALIZE_HEADER_BYTES, + 'Header must be 4 bytes.' + ); /* If you get an exception here when attempting to load from a file, first pass your key to Encoding::trimTrailingWhitespace() to remove newline characters, etc. */ diff --git a/src/File.php b/src/File.php index 9f068a5..0d4ed74 100644 --- a/src/File.php +++ b/src/File.php @@ -348,11 +348,10 @@ private static function encryptResourceInternal($inputHandle, $outputHandle, Key /* Initialize a streaming HMAC state. */ /** @var resource $hmac */ $hmac = \hash_init(Core::HASH_FUNCTION_NAME, HASH_HMAC, $akey); - if (!\is_resource($hmac) && !\is_object($hmac)) { - throw new Ex\EnvironmentIsBrokenException( - 'Cannot initialize a hash context' - ); - } + Core::ensureTrue( + \is_resource($hmac) || \is_object($hmac), + 'Cannot initialize a hash context' + ); /* Write the header, salt, and IV. */ self::writeBytes( @@ -407,11 +406,7 @@ private static function encryptResourceInternal($inputHandle, $outputHandle, Key $thisIv ); - if (!\is_string($encrypted)) { - throw new Ex\EnvironmentIsBrokenException( - 'OpenSSL encryption error' - ); - } + Core::ensureTrue(\is_string($encrypted), 'OpenSSL encryption error'); /* Write this buffer's ciphertext. */ self::writeBytes($outputHandle, $encrypted, Core::ourStrlen($encrypted)); @@ -518,11 +513,7 @@ public static function decryptResourceInternal($inputHandle, $outputHandle, KeyO /* Initialize a streaming HMAC state. */ /** @var resource $hmac */ $hmac = \hash_init(Core::HASH_FUNCTION_NAME, HASH_HMAC, $akey); - if (!\is_resource($hmac) && !\is_object($hmac)) { - throw new Ex\EnvironmentIsBrokenException( - 'Cannot initialize a hash context' - ); - } + Core::ensureTrue(\is_resource($hmac) || \is_object($hmac), 'Cannot initialize a hash context'); /* Reset file pointer to the beginning of the file after the header */ if (\fseek($inputHandle, Core::HEADER_VERSION_SIZE, SEEK_SET) === false) { @@ -576,11 +567,7 @@ public static function decryptResourceInternal($inputHandle, $outputHandle, KeyO /* Remember this buffer-sized chunk's HMAC. */ /** @var resource $chunk_mac */ $chunk_mac = \hash_copy($hmac); - if (!\is_resource($chunk_mac) && !\is_object($chunk_mac)) { - throw new Ex\EnvironmentIsBrokenException( - 'Cannot duplicate a hash context' - ); - } + Core::ensureTrue(\is_resource($chunk_mac) || \is_object($chunk_mac), 'Cannot duplicate a hash context'); $macs []= \hash_final($chunk_mac); } @@ -634,11 +621,7 @@ public static function decryptResourceInternal($inputHandle, $outputHandle, KeyO \hash_update($hmac2, $read); /** @var resource $calc_mac */ $calc_mac = \hash_copy($hmac2); - if (!\is_resource($calc_mac) && !\is_object($calc_mac)) { - throw new Ex\EnvironmentIsBrokenException( - 'Cannot duplicate a hash context' - ); - } + Core::ensureTrue(\is_resource($calc_mac) || \is_object($calc_mac), 'Cannot duplicate a hash context'); $calc = \hash_final($calc_mac); if (empty($macs)) { @@ -660,11 +643,7 @@ public static function decryptResourceInternal($inputHandle, $outputHandle, KeyO OPENSSL_RAW_DATA, $thisIv ); - if (!\is_string($decrypted)) { - throw new Ex\EnvironmentIsBrokenException( - 'OpenSSL decryption error' - ); - } + Core::ensureTrue(\is_string($decrypted), 'OpenSSL decryption error'); /* Write the plaintext to the output file. */ self::writeBytes( @@ -696,13 +675,12 @@ public static function decryptResourceInternal($inputHandle, $outputHandle, KeyO */ public static function readBytes($stream, $num_bytes) { - if ($num_bytes < 0) { - throw new Ex\EnvironmentIsBrokenException( - 'Tried to read less than 0 bytes' - ); - } elseif ($num_bytes === 0) { + Core::ensureTrue($num_bytes >= 0, 'Tried to read less than 0 bytes'); + + if ($num_bytes === 0) { return ''; } + $buf = ''; $remaining = $num_bytes; while ($remaining > 0 && ! \feof($stream)) { diff --git a/src/Key.php b/src/Key.php index fe4bf7d..27b919f 100644 --- a/src/Key.php +++ b/src/Key.php @@ -84,11 +84,10 @@ public function getRawBytes() */ private function __construct($bytes) { - if (Core::ourStrlen($bytes) !== self::KEY_BYTE_SIZE) { - throw new Ex\EnvironmentIsBrokenException( - 'Bad key length.' - ); - } + Core::ensureTrue( + Core::ourStrlen($bytes) === self::KEY_BYTE_SIZE, + 'Bad key length.' + ); $this->key_bytes = $bytes; } diff --git a/src/KeyOrPassword.php b/src/KeyOrPassword.php index 4a810d3..9de24ca 100644 --- a/src/KeyOrPassword.php +++ b/src/KeyOrPassword.php @@ -57,9 +57,10 @@ public static function createFromPassword($password) */ public function deriveKeys($salt) { - if (Core::ourStrlen($salt) !== Core::SALT_BYTE_SIZE) { - throw new Ex\EnvironmentIsBrokenException('Bad salt.'); - } + Core::ensureTrue( + Core::ourStrlen($salt) === Core::SALT_BYTE_SIZE, + 'Bad salt.' + ); if ($this->secret_type === self::SECRET_TYPE_KEY) { if (!($this->secret instanceof Key)) { diff --git a/src/RuntimeTests.php b/src/RuntimeTests.php index 9f00a97..65ce55d 100644 --- a/src/RuntimeTests.php +++ b/src/RuntimeTests.php @@ -59,13 +59,9 @@ public static function runtimeTest() RuntimeTests::HKDFTestVector(); RuntimeTests::testEncryptDecrypt(); - if (Core::ourStrlen(Key::createNewRandomKey()->getRawBytes()) != Core::KEY_BYTE_SIZE) { - throw new Ex\EnvironmentIsBrokenException(); - } + Core::ensureTrue(Core::ourStrlen(Key::createNewRandomKey()->getRawBytes()) === Core::KEY_BYTE_SIZE); - if (Core::ENCRYPTION_INFO_STRING == Core::AUTHENTICATION_INFO_STRING) { - throw new Ex\EnvironmentIsBrokenException(); - } + Core::ensureTrue(Core::ENCRYPTION_INFO_STRING !== Core::AUTHENTICATION_INFO_STRING); } catch (Ex\EnvironmentIsBrokenException $ex) { // Do this, otherwise it will stay in the "tests are running" state. $test_state = 3; @@ -97,9 +93,7 @@ private static function testEncryptDecrypt() // the user into thinking it's just an invalid ciphertext! throw new Ex\EnvironmentIsBrokenException(); } - if ($decrypted !== $data) { - throw new Ex\EnvironmentIsBrokenException(); - } + Core::ensureTrue($decrypted === $data); // Modifying the ciphertext: Appending a string. try { @@ -167,9 +161,7 @@ private static function HKDFTestVector() '34007208d5b887185865' ); $computed_okm = Core::HKDF('sha256', $ikm, $length, $info, $salt); - if ($computed_okm !== $okm) { - throw new Ex\EnvironmentIsBrokenException(); - } + Core::ensureTrue($computed_okm === $okm); // Test Case 7 $ikm = \str_repeat("\x0c", 22); @@ -180,9 +172,7 @@ private static function HKDFTestVector() '673a081d70cce7acfc48' ); $computed_okm = Core::HKDF('sha1', $ikm, $length, '', null); - if ($computed_okm !== $okm) { - throw new Ex\EnvironmentIsBrokenException(); - } + Core::ensureTrue($computed_okm === $okm); } /** @@ -197,9 +187,9 @@ private static function HMACTestVector() $key = \str_repeat("\x0b", 20); $data = 'Hi There'; $correct = 'b0344c61d8db38535ca8afceaf0bf12b881dc200c9833da726e9376c2e32cff7'; - if (\hash_hmac(Core::HASH_FUNCTION_NAME, $data, $key) !== $correct) { - throw new Ex\EnvironmentIsBrokenException(); - } + Core::ensureTrue( + \hash_hmac(Core::HASH_FUNCTION_NAME, $data, $key) === $correct + ); } /** @@ -230,18 +220,9 @@ private static function AESTestVector() ); $computed_ciphertext = Crypto::plainEncrypt($plaintext, $key, $iv); - if ($computed_ciphertext !== $ciphertext) { - echo \str_repeat("\n", 30); - echo \bin2hex($computed_ciphertext); - echo "\n---\n"; - echo \bin2hex($ciphertext); - echo \str_repeat("\n", 30); - throw new Ex\EnvironmentIsBrokenException(); - } + Core::ensureTrue($computed_ciphertext === $ciphertext); $computed_plaintext = Crypto::plainDecrypt($ciphertext, $key, $iv, Core::CIPHER_METHOD); - if ($computed_plaintext !== $plaintext) { - throw new Ex\EnvironmentIsBrokenException(); - } + Core::ensureTrue($computed_plaintext === $plaintext); } } From 7d8ace9a0487ef08abfcd0b7dbba90a343209679 Mon Sep 17 00:00:00 2001 From: Taylor Hornby Date: Mon, 23 Apr 2018 04:39:42 -0400 Subject: [PATCH 02/14] Forgot to document a parameter to ensureTrue --- src/Core.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Core.php b/src/Core.php index 856d8d5..88036e9 100644 --- a/src/Core.php +++ b/src/Core.php @@ -247,6 +247,7 @@ public static function ensureFunctionExists($name) * Throws an exception if the condition is false. * * @param bool $condition + * @param string $message * @return void * * @throws Ex\EnvironmentIsBrokenException From d9df7cc05fba9d5ac6902d20a2e87205393a9e44 Mon Sep 17 00:00:00 2001 From: Taylor Hornby Date: Mon, 23 Apr 2018 06:15:25 -0400 Subject: [PATCH 03/14] Test TypeErrors in Crypto and too-short legacy ciphertexts --- test/unit/CryptoTest.php | 117 ++++++++++++++++++++++++++++++++ test/unit/LegacyDecryptTest.php | 12 ++++ 2 files changed, 129 insertions(+) diff --git a/test/unit/CryptoTest.php b/test/unit/CryptoTest.php index 1ed8d7a..e94153d 100644 --- a/test/unit/CryptoTest.php +++ b/test/unit/CryptoTest.php @@ -110,4 +110,121 @@ public function testDecryptHexAsRaw() $ciphertext = Crypto::encryptWithPassword('testdata', 'password', false); Crypto::decryptWithPassword($ciphertext, 'password', true); } + + /** + * @expectedException \TypeError + */ + public function testEncryptTypeErrorA() + { + $key = Key::createNewRandomKey(); + Crypto::encrypt(3, $key, false); + } + + /** + * @expectedException \TypeError + */ + public function testEncryptTypeErrorB() + { + Crypto::encrypt("plaintext", 3, false); + } + + /** + * @expectedException \TypeError + */ + public function testEncryptTypeErrorC() + { + $key = Key::createNewRandomKey(); + Crypto::encrypt("plaintext", $key, 3); + } + + /** + * @expectedException \TypeError + */ + public function testEncryptWithPasswordTypeErrorA() + { + Crypto::encryptWithPassword(3, "password", false); + } + + /** + * @expectedException \TypeError + */ + public function testEncryptWithPasswordTypeErrorB() + { + Crypto::encryptWithPassword("plaintext", 3, false); + } + + /** + * @expectedException \TypeError + */ + public function testEncryptWithPasswordTypeErrorC() + { + Crypto::encryptWithPassword("plaintext", "password", 3); + } + + /** + * @expectedException \TypeError + */ + public function testDecryptTypeErrorA() + { + $key = Key::createNewRandomKey(); + Crypto::decrypt(3, $key, false); + } + + /** + * @expectedException \TypeError + */ + public function testDecryptTypeErrorB() + { + Crypto::decrypt("ciphertext", 3, false); + } + + /** + * @expectedException \TypeError + */ + public function testDecryptTypeErrorC() + { + $key = Key::createNewRandomKey(); + Crypto::decrypt("ciphertext", $key, 3); + } + + /** + * @expectedException \TypeError + */ + public function testDecryptWithPasswordTypeErrorA() + { + Crypto::decryptWithPassword(3, "password", false); + } + + /** + * @expectedException \TypeError + */ + public function testDecryptWithPasswordTypeErrorB() + { + Crypto::decryptWithPassword("ciphertext", 3, false); + } + + /** + * @expectedException \TypeError + */ + public function testDecryptWithPasswordTypeErrorC() + { + Crypto::decryptWithPassword("ciphertext", "password", 3); + } + + /** + * @expectedException \TypeError + */ + public function testLegacyDecryptTypeErrorA() + { + Crypto::legacyDecrypt(3, "key"); + } + + /** + * @expectedException \TypeError + */ + public function testLegacyDecryptTypeErrorB() + { + Crypto::legacyDecrypt("ciphertext", 3); + } + } diff --git a/test/unit/LegacyDecryptTest.php b/test/unit/LegacyDecryptTest.php index e7f93b7..59832b8 100644 --- a/test/unit/LegacyDecryptTest.php +++ b/test/unit/LegacyDecryptTest.php @@ -1,5 +1,6 @@ Date: Mon, 23 Apr 2018 06:48:33 -0400 Subject: [PATCH 04/14] Be consistent between PHP 5.x and 7.x about what happens when you call encrypt() or decrypt() with a key of the wrong type --- src/Crypto.php | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/Crypto.php b/src/Crypto.php index 016c6a1..86eb204 100644 --- a/src/Crypto.php +++ b/src/Crypto.php @@ -18,13 +18,18 @@ class Crypto * * @return string */ - public static function encrypt($plaintext, Key $key, $raw_binary = false) + public static function encrypt($plaintext, $key, $raw_binary = false) { if (!\is_string($plaintext)) { throw new \TypeError( 'String expected for argument 1. ' . \ucfirst(\gettype($plaintext)) . ' given instead.' ); } + if (!($key instanceof Key)) { + throw new \TypeError( + 'Key expected for argument 2. ' . \ucfirst(\gettype($key)) . ' given instead.' + ); + } if (!\is_bool($raw_binary)) { throw new \TypeError( 'Boolean expected for argument 3. ' . \ucfirst(\gettype($raw_binary)) . ' given instead.' @@ -87,13 +92,18 @@ public static function encryptWithPassword($plaintext, $password, $raw_binary = * * @return string */ - public static function decrypt($ciphertext, Key $key, $raw_binary = false) + public static function decrypt($ciphertext, $key, $raw_binary = false) { if (!\is_string($ciphertext)) { throw new \TypeError( 'String expected for argument 1. ' . \ucfirst(\gettype($ciphertext)) . ' given instead.' ); } + if (!($key instanceof Key)) { + throw new \TypeError( + 'Key expected for argument 2. ' . \ucfirst(\gettype($key)) . ' given instead.' + ); + } if (!\is_bool($raw_binary)) { throw new \TypeError( 'Boolean expected for argument 3. ' . \ucfirst(\gettype($raw_binary)) . ' given instead.' From 3803b816fb7067992f35d53e8afc1ff6b9503d09 Mon Sep 17 00:00:00 2001 From: Taylor Hornby Date: Mon, 23 Apr 2018 13:26:46 -0400 Subject: [PATCH 05/14] Fix error message in incrementCounter --- src/Core.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Core.php b/src/Core.php index 88036e9..652581f 100644 --- a/src/Core.php +++ b/src/Core.php @@ -62,7 +62,7 @@ public static function incrementCounter($ctr, $inc) Core::ensureTrue( $inc >= 0, - 'Trying to increment a nonce by a nonpositive amount' + 'Trying to increment a nonce by a negative amount' ); Core::ensureTrue( From a5625d3c87944563050d4e3b62ac4ed11520524f Mon Sep 17 00:00:00 2001 From: Taylor Hornby Date: Mon, 23 Apr 2018 13:51:44 -0400 Subject: [PATCH 06/14] Add more ourSubstr tests and make CTR increment throw when incrementing by 0 --- src/Core.php | 5 +++-- test/unit/CoreTest.php | 18 ++++++++++++++++++ test/unit/CtrModeTest.php | 17 +++++++++++------ 3 files changed, 32 insertions(+), 8 deletions(-) diff --git a/src/Core.php b/src/Core.php index 652581f..83f7018 100644 --- a/src/Core.php +++ b/src/Core.php @@ -60,9 +60,10 @@ public static function incrementCounter($ctr, $inc) 'Trying to increment nonce by a non-integer.' ); + // The caller is probably re-using CTR-mode keystream if they increment by 0. Core::ensureTrue( - $inc >= 0, - 'Trying to increment a nonce by a negative amount' + $inc > 0, + 'Trying to increment a nonce by a nonpositive amount' ); Core::ensureTrue( diff --git a/test/unit/CoreTest.php b/test/unit/CoreTest.php index 41d62c0..5873f9b 100644 --- a/test/unit/CoreTest.php +++ b/test/unit/CoreTest.php @@ -106,4 +106,22 @@ public function testOurSubstrOutOfBorders() Core::ourSubstr('abc', 5, 2) ); } + + /** + * @expectedException \InvalidArgumentException + */ + public function testOurSubstrNegativeLength() + { + Core::ourSubstr('abc', 0, -1); + } + + public function testOurSubstrNegativeStart() + { + $this->assertSame('c', Core::ourSubstr('abc', -1, 1)); + } + + public function testOurSubstrLengthIsMax() + { + $this->assertSame('bc', Core::ourSubstr('abc', 1, 500)); + } } diff --git a/test/unit/CtrModeTest.php b/test/unit/CtrModeTest.php index cd548c0..f4ab297 100644 --- a/test/unit/CtrModeTest.php +++ b/test/unit/CtrModeTest.php @@ -7,12 +7,6 @@ class CtrModeTest extends PHPUnit_Framework_TestCase public function counterTestVectorProvider() { return [ - /* Incrementing by zero makes no change. */ - [ - '01234567890123456789012345778901', - '01234567890123456789012345778901', - 0, - ], /* First byte, no overflow. */ [ '00000000000000000000000000000000', @@ -143,6 +137,17 @@ public function testIncrementByNegativeValue() ); } + /** + * @expectedException \Defuse\Crypto\Exception\EnvironmentIsBrokenException + */ + public function testIncrementByZero() + { + \Defuse\Crypto\Core::incrementCounter( + str_repeat("\x00", 16), + 0 + ); + } + public function allNonZeroByteValuesProvider() { $all_bytes = []; From fb1c086d77ecccff44efe20d7fa3c6c14551d101 Mon Sep 17 00:00:00 2001 From: Taylor Hornby Date: Mon, 23 Apr 2018 14:08:51 -0400 Subject: [PATCH 07/14] Catch bad types earlier in KeyOrPassword --- src/KeyOrPassword.php | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/KeyOrPassword.php b/src/KeyOrPassword.php index 9de24ca..a2309b4 100644 --- a/src/KeyOrPassword.php +++ b/src/KeyOrPassword.php @@ -63,9 +63,6 @@ public function deriveKeys($salt) ); if ($this->secret_type === self::SECRET_TYPE_KEY) { - if (!($this->secret instanceof Key)) { - throw new Ex\CryptoException('Expected a Key object'); - } $akey = Core::HKDF( Core::HASH_FUNCTION_NAME, $this->secret->getRawBytes(), @@ -82,9 +79,6 @@ public function deriveKeys($salt) ); return new DerivedKeys($akey, $ekey); } elseif ($this->secret_type === self::SECRET_TYPE_PASSWORD) { - if (!\is_string($this->secret)) { - throw new Ex\CryptoException('Expected a string'); - } /* Our PBKDF2 polyfill is vulnerable to a DoS attack documented in * GitHub issue #230. The fix is to pre-hash the password to ensure * it is short. We do the prehashing here instead of in pbkdf2() so @@ -128,6 +122,14 @@ public function deriveKeys($salt) */ private function __construct($secret_type, $secret) { + // The constructor is private, so these should never throw. + if ($secret_type === self::SECRET_TYPE_KEY) { + Core::ensureTrue($secret instanceof Key); + } elseif ($secret_type === self::SECRET_TYPE_PASSWORD) { + Core::ensureTrue(\is_string($secret)); + } else { + throw new Ex\EnvironmentIsBrokenException('Bad secret type.'); + } $this->secret_type = $secret_type; $this->secret = $secret; } From 47e576b0d784e9f1008f90246c442f08cbbf0a1f Mon Sep 17 00:00:00 2001 From: Taylor Hornby Date: Mon, 23 Apr 2018 14:09:19 -0400 Subject: [PATCH 08/14] Test loading a KeyProtectedByPassword with a bad header --- test/unit/PasswordTest.php | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/test/unit/PasswordTest.php b/test/unit/PasswordTest.php index d76b5c9..a1c2370 100644 --- a/test/unit/PasswordTest.php +++ b/test/unit/PasswordTest.php @@ -56,4 +56,17 @@ function testPasswordActuallyChanges() $pkey1->unlockKey('password'); } + + /** + * @expectedException \Defuse\Crypto\Exception\BadFormatException + */ + function testMalformedLoad() + { + $pkey1 = KeyProtectedByPassword::createRandomPasswordProtectedKey('password'); + $pkey1_enc_ascii = $pkey1->saveToAsciiSafeString(); + + $pkey1_enc_ascii[0] = "\xFF"; + + KeyProtectedByPassword::loadFromAsciiSafeString($pkey1_enc_ascii); + } } From 67d1e97a02f0beda1b74d78aec91ef19d7933991 Mon Sep 17 00:00:00 2001 From: Taylor Hornby Date: Mon, 23 Apr 2018 14:18:15 -0400 Subject: [PATCH 09/14] Fix whitespace --- src/Core.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Core.php b/src/Core.php index 83f7018..3506cda 100644 --- a/src/Core.php +++ b/src/Core.php @@ -211,7 +211,7 @@ public static function hashEquals($expected, $given) // We're not attempting to make variable-length string comparison // secure, as that's very difficult. Make sure the strings are the same // length. - Core::ensureTrue( Core::ourStrlen($expected) === Core::ourStrlen($given)); + Core::ensureTrue(Core::ourStrlen($expected) === Core::ourStrlen($given)); $blind = Core::secureRandom(32); $message_compare = \hash_hmac(Core::HASH_FUNCTION_NAME, $given, $blind); From b9a59fc6d54fc404551b2ebac8a8e98086607b27 Mon Sep 17 00:00:00 2001 From: Taylor Hornby Date: Mon, 23 Apr 2018 14:28:06 -0400 Subject: [PATCH 10/14] See if re-adding these checks satisfies psalm --- src/KeyOrPassword.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/KeyOrPassword.php b/src/KeyOrPassword.php index a2309b4..ae08d79 100644 --- a/src/KeyOrPassword.php +++ b/src/KeyOrPassword.php @@ -63,6 +63,7 @@ public function deriveKeys($salt) ); if ($this->secret_type === self::SECRET_TYPE_KEY) { + Core::ensureTrue($secret instanceof Key); $akey = Core::HKDF( Core::HASH_FUNCTION_NAME, $this->secret->getRawBytes(), @@ -79,6 +80,7 @@ public function deriveKeys($salt) ); return new DerivedKeys($akey, $ekey); } elseif ($this->secret_type === self::SECRET_TYPE_PASSWORD) { + Core::ensureTrue(\is_string($secret)); /* Our PBKDF2 polyfill is vulnerable to a DoS attack documented in * GitHub issue #230. The fix is to pre-hash the password to ensure * it is short. We do the prehashing here instead of in pbkdf2() so From 9335fb74aa3edd2a8cb2292798d71485bc7f2331 Mon Sep 17 00:00:00 2001 From: Taylor Hornby Date: Mon, 23 Apr 2018 14:32:22 -0400 Subject: [PATCH 11/14] Fix my stupid mistake --- src/KeyOrPassword.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/KeyOrPassword.php b/src/KeyOrPassword.php index ae08d79..c943879 100644 --- a/src/KeyOrPassword.php +++ b/src/KeyOrPassword.php @@ -63,7 +63,7 @@ public function deriveKeys($salt) ); if ($this->secret_type === self::SECRET_TYPE_KEY) { - Core::ensureTrue($secret instanceof Key); + Core::ensureTrue($this->secret instanceof Key); $akey = Core::HKDF( Core::HASH_FUNCTION_NAME, $this->secret->getRawBytes(), @@ -80,7 +80,7 @@ public function deriveKeys($salt) ); return new DerivedKeys($akey, $ekey); } elseif ($this->secret_type === self::SECRET_TYPE_PASSWORD) { - Core::ensureTrue(\is_string($secret)); + Core::ensureTrue(\is_string($this->secret)); /* Our PBKDF2 polyfill is vulnerable to a DoS attack documented in * GitHub issue #230. The fix is to pre-hash the password to ensure * it is short. We do the prehashing here instead of in pbkdf2() so From 6f87f381f69b46611a63c4a2d32c8c438bf5033f Mon Sep 17 00:00:00 2001 From: Taylor Hornby Date: Mon, 23 Apr 2018 14:46:39 -0400 Subject: [PATCH 12/14] Psalm wasn't smart enough to realize this is safe --- src/KeyOrPassword.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/KeyOrPassword.php b/src/KeyOrPassword.php index c943879..ca02c55 100644 --- a/src/KeyOrPassword.php +++ b/src/KeyOrPassword.php @@ -64,6 +64,9 @@ public function deriveKeys($salt) if ($this->secret_type === self::SECRET_TYPE_KEY) { Core::ensureTrue($this->secret instanceof Key); + /** + * @psalm-suppress PossiblyInvalidMethodCall + */ $akey = Core::HKDF( Core::HASH_FUNCTION_NAME, $this->secret->getRawBytes(), From aaebed6697ce91019e6c5f9f21bf015b1ccd7f6a Mon Sep 17 00:00:00 2001 From: Taylor Hornby Date: Mon, 23 Apr 2018 14:50:47 -0400 Subject: [PATCH 13/14] Make Psalm pass --- composer.json | 3 ++- src/KeyOrPassword.php | 12 ++++++++++-- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/composer.json b/composer.json index e90db62..9c825a8 100644 --- a/composer.json +++ b/composer.json @@ -27,7 +27,8 @@ }, "require-dev": { "phpunit/phpunit": "^4|^5", - "nikic/php-parser": "^2.0|^3.0|^4.0" + "nikic/php-parser": "^2.0|^3.0|^4.0", + "vimeo/psalm": "dev-master" }, "bin": [ "bin/generate-defuse-key" diff --git a/src/KeyOrPassword.php b/src/KeyOrPassword.php index ca02c55..890b2c2 100644 --- a/src/KeyOrPassword.php +++ b/src/KeyOrPassword.php @@ -65,8 +65,8 @@ public function deriveKeys($salt) if ($this->secret_type === self::SECRET_TYPE_KEY) { Core::ensureTrue($this->secret instanceof Key); /** - * @psalm-suppress PossiblyInvalidMethodCall - */ + * @psalm-suppress PossiblyInvalidMethodCall + */ $akey = Core::HKDF( Core::HASH_FUNCTION_NAME, $this->secret->getRawBytes(), @@ -74,6 +74,9 @@ public function deriveKeys($salt) Core::AUTHENTICATION_INFO_STRING, $salt ); + /** + * @psalm-suppress PossiblyInvalidMethodCall + */ $ekey = Core::HKDF( Core::HASH_FUNCTION_NAME, $this->secret->getRawBytes(), @@ -89,7 +92,12 @@ public function deriveKeys($salt) * it is short. We do the prehashing here instead of in pbkdf2() so * that pbkdf2() still computes the function as defined by the * standard. */ + + /** + * @psalm-suppress PossiblyInvalidArgument + */ $prehash = \hash(Core::HASH_FUNCTION_NAME, $this->secret, true); + $prekey = Core::pbkdf2( Core::HASH_FUNCTION_NAME, $prehash, From d2b04d2bac29eb30d91ea433084a58f3600b86d9 Mon Sep 17 00:00:00 2001 From: Taylor Hornby Date: Mon, 23 Apr 2018 15:00:59 -0400 Subject: [PATCH 14/14] Revert accidental change to composer.json --- composer.json | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/composer.json b/composer.json index 9c825a8..e90db62 100644 --- a/composer.json +++ b/composer.json @@ -27,8 +27,7 @@ }, "require-dev": { "phpunit/phpunit": "^4|^5", - "nikic/php-parser": "^2.0|^3.0|^4.0", - "vimeo/psalm": "dev-master" + "nikic/php-parser": "^2.0|^3.0|^4.0" }, "bin": [ "bin/generate-defuse-key"