Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve Test Coverage #398

Merged
merged 14 commits into from
Apr 23, 2018
Merged

Improve Test Coverage #398

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
ed55664
Add and use Core::ensureTrue(, )
defuse Apr 23, 2018
7d8ace9
Forgot to document a parameter to ensureTrue
defuse Apr 23, 2018
d9df7cc
Test TypeErrors in Crypto and too-short legacy ciphertexts
defuse Apr 23, 2018
232908c
Be consistent between PHP 5.x and 7.x about what happens when you cal…
defuse Apr 23, 2018
3803b81
Fix error message in incrementCounter
defuse Apr 23, 2018
a5625d3
Add more ourSubstr tests and make CTR increment throw when incrementi…
defuse Apr 23, 2018
fb1c086
Catch bad types earlier in KeyOrPassword
defuse Apr 23, 2018
47e576b
Test loading a KeyProtectedByPassword with a bad header
defuse Apr 23, 2018
67d1e97
Fix whitespace
defuse Apr 23, 2018
b9a59fc
See if re-adding these checks satisfies psalm
defuse Apr 23, 2018
9335fb7
Fix my stupid mistake
defuse Apr 23, 2018
6f87f38
Psalm wasn't smart enough to realize this is safe
defuse Apr 23, 2018
aaebed6
Make Psalm pass
defuse Apr 23, 2018
d2b04d2
Revert accidental change to composer.json
defuse Apr 23, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
121 changes: 55 additions & 66 deletions src/Core.php
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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