Skip to content

Commit

Permalink
Merge pull request #398 from defuse/make-throw-ifs-an-assert
Browse files Browse the repository at this point in the history 
Improve Test Coverage
  • Loading branch information
@defuse
defuse authored Apr 23, 2018
2 parents 39fba0c + d2b04d2 commit 0d4d27c
Show file tree
Hide file tree
Showing 12 changed files with 308 additions and 196 deletions.
121 changes: 55 additions & 66 deletions src/Core.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,29 +50,26 @@ 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.'
);

// 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 nonpositive amount'
);

Core::ensureTrue(
$inc <= PHP_INT_MAX - 255,
'Integer overflow may occur'
);

/*
* We start at the rightmost byte (big-endian)
Expand All @@ -82,11 +79,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;
Expand Down Expand Up @@ -146,12 +139,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)) {
Expand All @@ -166,9 +157,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 = '';
Expand All @@ -188,9 +177,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;
}

Expand Down Expand Up @@ -224,9 +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.
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);
Expand All @@ -243,9 +228,7 @@ public static function hashEquals($expected, $given)
*/
public static function ensureConstantExists($name)
{
if (! \defined($name)) {
throw new Ex\EnvironmentIsBrokenException();
}
Core::ensureTrue(\defined($name));
}

/**
Expand All @@ -258,8 +241,22 @@ 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
* @param string $message
* @return void
*
* @throws Ex\EnvironmentIsBrokenException
*/
public static function ensureTrue($condition, $message = '')
{
if (!$condition) {
throw new Ex\EnvironmentIsBrokenException($message);
}
}

Expand All @@ -286,9 +283,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);
Expand Down Expand Up @@ -403,28 +398,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!
Expand Down
58 changes: 22 additions & 36 deletions src/Crypto.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.'
Expand Down Expand Up @@ -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.'
Expand Down Expand Up @@ -181,16 +191,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(
Expand Down Expand Up @@ -221,17 +227,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);
Expand Down Expand Up @@ -320,9 +322,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 */
Expand All @@ -331,9 +331,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 */
Expand All @@ -342,9 +340,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 */
Expand All @@ -355,9 +351,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.
Expand Down Expand Up @@ -397,11 +391,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;
}
Expand Down Expand Up @@ -431,11 +421,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;
}
Expand Down
18 changes: 8 additions & 10 deletions src/Encoding.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 .
Expand Down Expand Up @@ -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. */
Expand Down
Loading

0 comments on commit 0d4d27c

Please sign in to comment.