From b5477c5391bcbf2d8c1c01059ac3532ca9ccc542 Mon Sep 17 00:00:00 2001 From: Paragon Initiative Enterprises Date: Fri, 9 Jun 2017 17:23:43 -0400 Subject: [PATCH] More comments and boyscouting. --- src/Core/Util.php | 107 ++++++++++++++++++++++++++++++++++++++++++---- src/Crypto.php | 35 +++++++++++---- 2 files changed, 125 insertions(+), 17 deletions(-) diff --git a/src/Core/Util.php b/src/Core/Util.php index 7d6f3a94..cb3e2716 100644 --- a/src/Core/Util.php +++ b/src/Core/Util.php @@ -15,15 +15,21 @@ abstract class ParagonIE_Sodium_Core_Util * * @internal You should not use this directly from another application * - * @param string $bin_string (raw binary) + * @param string $binaryString (raw binary) * @return string + * @throws TypeError */ - public static function bin2hex($bin_string) + public static function bin2hex($binaryString) { + /* Type checks: */ + if (!is_string($binaryString)) { + throw new TypeError('Argument 1 must be a string, ' . gettype($binaryString) . ' given.'); + } + $hex = ''; - $len = self::strlen($bin_string); + $len = self::strlen($binaryString); for ($i = 0; $i < $len; ++$i) { - $chunk = unpack('C', self::substr($bin_string, $i, 2)); + $chunk = unpack('C', self::substr($binaryString, $i, 2)); $c = $chunk[1] & 0xf; $b = $chunk[1] >> 4; $hex .= pack( @@ -89,6 +95,10 @@ public static function bin2hexUpper($bin_string) */ public static function chrToInt($chr) { + /* Type checks: */ + if (!is_string($chr)) { + throw new TypeError('Argument 1 must be a string, ' . gettype($chr) . ' given.'); + } if (self::strlen($chr) !== 1) { throw new Error('chrToInt() expects a string that is exactly 1 character long'); } @@ -133,9 +143,18 @@ public static function compare($left, $right, $len = null) * @param string $left * @param string $right * @return bool + * @throws TypeError */ public static function hashEquals($left, $right) { + /* Type checks: */ + if (!is_string($left)) { + throw new TypeError('Argument 1 must be a string, ' . gettype($left) . ' given.'); + } + if (!is_string($right)) { + throw new TypeError('Argument 2 must be a string, ' . gettype($right) . ' given.'); + } + if (is_callable('hash_equals')) { return hash_equals($left, $right); } @@ -147,7 +166,9 @@ public static function hashEquals($left, $right) for ($i = 0; $i < $len; ++$i) { $d |= self::chrToInt($left[$i]) ^ self::chrToInt($right[$i]); } - if ($d !== 0) { + + /* branch-free variant of === 0 */ + if (-(1 & (($d - 1) >> 8))) { return false; } return $left === $right; @@ -163,9 +184,15 @@ public static function hashEquals($left, $right) * @param bool $strictPadding * @return string (raw binary) * @throws RangeException + * @throws TypeError */ public static function hex2bin($hexString, $strictPadding = false) { + /* Type checks: */ + if (!is_string($hexString)) { + throw new TypeError('Argument 1 must be a string, ' . gettype($hexString) . ' given.'); + } + $hex_pos = 0; $bin = ''; $c_acc = 0; @@ -235,9 +262,6 @@ public static function intArrayToString(array $ints) */ public static function intToChr($int) { - if (!is_int($int)) { - throw new TypeError('intToChr() expects an integer'); - } return pack('C', $int); } @@ -249,9 +273,16 @@ public static function intToChr($int) * @param string $string * @return int * @throws RangeException + * @throws TypeError */ public static function load_3($string) { + /* Type checks: */ + if (!is_string($string)) { + throw new TypeError('Argument 1 must be a string, ' . gettype($string) . ' given.'); + } + + /* Input validation: */ if (self::strlen($string) < 3) { throw new RangeException( 'String must be 3 bytes or more; ' . self::strlen($string) . ' given.' @@ -271,9 +302,16 @@ public static function load_3($string) * @param string $string * @return int * @throws RangeException + * @throws TypeError */ public static function load_4($string) { + /* Type checks: */ + if (!is_string($string)) { + throw new TypeError('Argument 1 must be a string, ' . gettype($string) . ' given.'); + } + + /* Input validation: */ if (self::strlen($string) < 4) { throw new RangeException( 'String must be 4 bytes or more; ' . self::strlen($string) . ' given.' @@ -294,9 +332,16 @@ public static function load_4($string) * @param string $string * @return int * @throws RangeException + * @throws TypeError */ public static function load64_le($string) { + /* Type checks: */ + if (!is_string($string)) { + throw new TypeError('Argument 1 must be a string, ' . gettype($string) . ' given.'); + } + + /* Input validation: */ if (self::strlen($string) < 4) { throw new RangeException( 'String must be 4 bytes or more; ' . self::strlen($string) . ' given.' @@ -425,9 +470,19 @@ public static function numericTo64BitInteger($num) * * @param int $int * @return string + * @throws TypeError */ public static function store_3($int) { + /* Type checks: */ + if (!is_int($int)) { + if (is_numeric($int)) { + $int = (int) $int; + } else { + throw new TypeError('Argument 1 must be an integer, ' . gettype($int) . ' given.'); + } + } + return self::intToChr(($int >> 16) & 0xff) . self::intToChr(($int >> 8) & 0xff) . self::intToChr($int & 0xff); @@ -440,9 +495,19 @@ public static function store_3($int) * * @param int $int * @return string + * @throws TypeError */ public static function store32_le($int) { + /* Type checks: */ + if (!is_int($int)) { + if (is_numeric($int)) { + $int = (int) $int; + } else { + throw new TypeError('Argument 1 must be an integer, ' . gettype($int) . ' given.'); + } + } + return self::intToChr($int & 0xff) . self::intToChr(($int >> 8) & 0xff) . self::intToChr(($int >> 16) & 0xff) . @@ -456,9 +521,19 @@ public static function store32_le($int) * * @param int $int * @return string + * @throws TypeError */ public static function store_4($int) { + /* Type checks: */ + if (!is_int($int)) { + if (is_numeric($int)) { + $int = (int) $int; + } else { + throw new TypeError('Argument 1 must be an integer, ' . gettype($int) . ' given.'); + } + } + return self::intToChr(($int >> 24) & 0xff) . self::intToChr(($int >> 16) & 0xff) . self::intToChr(($int >> 8) & 0xff) . @@ -472,9 +547,19 @@ public static function store_4($int) * * @param int $int * @return string + * @throws TypeError */ public static function store64_le($int) { + /* Type checks: */ + if (!is_int($int)) { + if (is_numeric($int)) { + $int = (int) $int; + } else { + throw new TypeError('Argument 1 must be an integer, ' . gettype($int) . ' given.'); + } + } + if (PHP_INT_SIZE === 8) { return self::intToChr($int & 0xff) . self::intToChr(($int >> 8) & 0xff) . @@ -513,6 +598,7 @@ public static function store64_le($int) */ public static function strlen($str) { + /* Type checks: */ if (!is_string($str)) { throw new TypeError('String expected'); } @@ -562,6 +648,7 @@ public static function stringToIntArray($string) */ public static function substr($str, $start = 0, $length = null) { + /* Type checks: */ if (!is_string($str)) { throw new TypeError('String expected'); } @@ -598,6 +685,7 @@ public static function substr($str, $start = 0, $length = null) */ public static function verify_16($a, $b) { + /* Type checks: */ if (!is_string($a)) { throw new TypeError('String expected'); } @@ -622,6 +710,7 @@ public static function verify_16($a, $b) */ public static function verify_32($a, $b) { + /* Type checks: */ if (!is_string($a)) { throw new TypeError('String expected'); } @@ -646,12 +735,14 @@ public static function verify_32($a, $b) */ public static function xorStrings($a, $b) { + /* Type checks: */ if (!is_string($a)) { throw new TypeError('Argument 1 must be a string'); } if (!is_string($b)) { throw new TypeError('Argument 2 must be a string'); } + return $a ^ $b; } diff --git a/src/Crypto.php b/src/Crypto.php index 16735b8c..ea582abc 100644 --- a/src/Crypto.php +++ b/src/Crypto.php @@ -99,6 +99,7 @@ public static function aead_chacha20poly1305_decrypt( $key ); + /* Recalculate the Poly1305 authentication tag (MAC): */ $state = new ParagonIE_Sodium_Core_Poly1305_State($block0); try { ParagonIE_Sodium_Compat::memzero($block0); @@ -111,6 +112,7 @@ public static function aead_chacha20poly1305_decrypt( $state->update(ParagonIE_Sodium_Core_Util::store64_le($clen)); $computed_mac = $state->finish(); + /* Compare the given MAC with the recalculated MAC: */ if (!ParagonIE_Sodium_Core_Util::verify_16($computed_mac, $mac)) { throw new Error('Invalid MAC'); } @@ -223,6 +225,7 @@ public static function aead_chacha20poly1305_ietf_decrypt( $len - self::aead_chacha20poly1305_IETF_ABYTES ); + /* Recalculate the Poly1305 authentication tag (MAC): */ $state = new ParagonIE_Sodium_Core_Poly1305_State($block0); try { ParagonIE_Sodium_Compat::memzero($block0); @@ -237,6 +240,7 @@ public static function aead_chacha20poly1305_ietf_decrypt( $state->update(ParagonIE_Sodium_Core_Util::store64_le($clen)); $computed_mac = $state->finish(); + /* Compare the given MAC with the recalculated MAC: */ if (!ParagonIE_Sodium_Core_Util::verify_16($computed_mac, $mac)) { throw new Error('Invalid MAC'); } @@ -775,13 +779,7 @@ public static function keyExchange($my_sk, $their_pk, $client_pk, $server_pk) public static function scalarmult($sKey, $pKey) { $q = ParagonIE_Sodium_Core_X25519::crypto_scalarmult_curve25519_ref10($sKey, $pKey); - $d = 0; - for ($i = 0; $i < self::box_curve25519xsalsa20poly1305_SECRETKEYBYTES; ++$i) { - $d |= ParagonIE_Sodium_Core_Util::chrToInt($q[$i]); - } - if (-(1 & (($d - 1) >> 8))) { - throw new Error('Zero public key is not allowed'); - } + self::scalarmult_throw_if_zero($q); return $q; } @@ -797,14 +795,28 @@ public static function scalarmult($sKey, $pKey) public static function scalarmult_base($secret) { $q = ParagonIE_Sodium_Core_X25519::crypto_scalarmult_curve25519_ref10_base($secret); + self::scalarmult_throw_if_zero($q); + return $q; + } + + /** + * This throws an Error if a zero public key was passed to the function. + * + * @param string $q + * @return void + * @throws Error + */ + protected static function scalarmult_throw_if_zero($q) + { $d = 0; for ($i = 0; $i < self::box_curve25519xsalsa20poly1305_SECRETKEYBYTES; ++$i) { $d |= ParagonIE_Sodium_Core_Util::chrToInt($q[$i]); } + + /* branch-free variant of === 0 */ if (-(1 & (($d - 1) >> 8))) { throw new Error('Zero public key is not allowed'); } - return $q; } /** @@ -918,7 +930,11 @@ public static function secretbox_open($ciphertext, $nonce, $key) ParagonIE_Sodium_Core_Util::substr($nonce, 16, 8), $subkey ); - $verified = ParagonIE_Sodium_Core_Poly1305::onetimeauth_verify($mac, $c, ParagonIE_Sodium_Core_Util::substr($block0, 0, 32)); + $verified = ParagonIE_Sodium_Core_Poly1305::onetimeauth_verify( + $mac, + $c, + ParagonIE_Sodium_Core_Util::substr($block0, 0, 32) + ); if (!$verified) { try { ParagonIE_Sodium_Compat::memzero($subkey); @@ -1068,6 +1084,7 @@ public static function secretbox_xchacha20poly1305_open($ciphertext, $nonce, $ke $c, ParagonIE_Sodium_Core_Util::substr($block0, 0, 32) ); + if (!$verified) { try { ParagonIE_Sodium_Compat::memzero($subkey);