From 3b73f9ae17596872d2eb7a74c23461e8740243c8 Mon Sep 17 00:00:00 2001 From: Paragon Initiative Enterprises Date: Tue, 27 Mar 2018 10:25:56 -0400 Subject: [PATCH 1/5] This check is actually not needed with constant_time_encoding v2+ --- src/Util.php | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/Util.php b/src/Util.php index 24329bf..57fc5e1 100644 --- a/src/Util.php +++ b/src/Util.php @@ -160,14 +160,6 @@ public static function hkdfBlake2b( // ORM = first L octets of T /** @var string $orm */ $orm = Binary::safeSubstr($t, 0, $length); - - // @codeCoverageIgnoreStart - if (!\is_string($orm)) { - throw new CannotPerformOperation( - 'An unknown error has occurred' - ); - } - // @codeCoverageIgnoreEnd return $orm; } From 1337674c8f11f3a9e02b2f847308a222d986b026 Mon Sep 17 00:00:00 2001 From: Paragon Initiative Enterprises Date: Tue, 27 Mar 2018 10:26:58 -0400 Subject: [PATCH 2/5] This condition will never happen, a fatal error will instead. --- src/Symmetric/Crypto.php | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/Symmetric/Crypto.php b/src/Symmetric/Crypto.php index 992ceff..22a32e1 100644 --- a/src/Symmetric/Crypto.php +++ b/src/Symmetric/Crypto.php @@ -210,13 +210,6 @@ public static function decryptWithAd( (string) $nonce, (string) $encKey ); - if (!\is_string($plaintext)) { - // @codeCoverageIgnoreStart - throw new InvalidMessage( - 'Invalid message' - ); - // @codeCoverageIgnoreEnd - } \sodium_memzero($encrypted); \sodium_memzero($nonce); \sodium_memzero($encKey); From 3a78a324923e42d80fe44cf47eddb0acf5510a3c Mon Sep 17 00:00:00 2001 From: Paragon Initiative Enterprises Date: Tue, 27 Mar 2018 10:27:41 -0400 Subject: [PATCH 3/5] This paranoid check adds no value. --- src/Symmetric/Crypto.php | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/Symmetric/Crypto.php b/src/Symmetric/Crypto.php index 22a32e1..8709b59 100644 --- a/src/Symmetric/Crypto.php +++ b/src/Symmetric/Crypto.php @@ -160,14 +160,6 @@ public static function decryptWithAd( /** @var string $auth */ $auth = $pieces[5]; - // @codeCoverageIgnoreStart - if (!($config instanceof Config)) { - throw new CannotPerformOperation( - 'Config is not an instance of Config. This should not happen.' - ); - } - // @codeCoverageIgnoreEnd - /* Split our key into two keys: One for encryption, the other for authentication. By using separate keys, we can reasonably dismiss likely cross-protocol attacks. From 8f4f70fbf5473f86b8e1d3f79af033e372fe0258 Mon Sep 17 00:00:00 2001 From: Paragon Initiative Enterprises Date: Tue, 27 Mar 2018 10:41:51 -0400 Subject: [PATCH 4/5] Docblock clean up --- psalm.xml | 1 - src/Asymmetric/Crypto.php | 10 ---------- src/Cookie.php | 6 +----- src/KeyFactory.php | 7 ++----- src/Password.php | 6 ------ src/Stream/MutableFile.php | 3 ++- src/Stream/ReadOnlyFile.php | 4 +++- test/unit/KeyTest.php | 6 ++++++ test/unit/StreamTest.php | 7 +++++++ 9 files changed, 21 insertions(+), 29 deletions(-) diff --git a/psalm.xml b/psalm.xml index b3bcc71..2bbc9ab 100644 --- a/psalm.xml +++ b/psalm.xml @@ -7,7 +7,6 @@ - diff --git a/src/Asymmetric/Crypto.php b/src/Asymmetric/Crypto.php index f3b034b..5553b82 100644 --- a/src/Asymmetric/Crypto.php +++ b/src/Asymmetric/Crypto.php @@ -109,11 +109,6 @@ public static function encryptWithAd( $ourPrivateKey, $theirPublicKey ); - // @codeCoverageIgnoreStart - if (!($ss instanceof HiddenString)) { - throw new \TypeError(); - } - // @codeCoverageIgnoreEnd $sharedSecretKey = new EncryptionKey($ss); $ciphertext = SymmetricCrypto::encryptWithAd( $plaintext, @@ -188,11 +183,6 @@ public static function decryptWithAd( $ourPrivateKey, $theirPublicKey ); - // @codeCoverageIgnoreStart - if (!($ss instanceof HiddenString)) { - throw new \TypeError(); - } - // @codeCoverageIgnoreEnd $sharedSecretKey = new EncryptionKey($ss); $plaintext = SymmetricCrypto::decryptWithAd( $ciphertext, diff --git a/src/Cookie.php b/src/Cookie.php index cc9b03e..f57b17d 100644 --- a/src/Cookie.php +++ b/src/Cookie.php @@ -82,7 +82,7 @@ public function fetch(string $name) return null; } try { - /** @var string $stored */ + /** @var string|array|int|float|bool $stored */ $stored = $_COOKIE[$name]; if (!\is_string($stored)) { throw new InvalidType('Cookie value is not a string'); @@ -120,10 +120,6 @@ protected static function getConfig(string $stored): SymmetricConfig if (\hash_equals(Binary::safeSubstr($stored, 0, 5), Halite::VERSION_PREFIX)) { /** @var string $decoded */ $decoded = Base64UrlSafe::decode($stored); - if (!\is_string($decoded)) { - \sodium_memzero($stored); - throw new InvalidMessage('Incorrect encoding'); - } return SymmetricConfig::getConfig( $decoded, 'encrypt' diff --git a/src/KeyFactory.php b/src/KeyFactory.php index a89cdce..9a11334 100644 --- a/src/KeyFactory.php +++ b/src/KeyFactory.php @@ -732,7 +732,7 @@ public static function loadSignatureKeyPair(string $filePath): SignatureKeyPair /** * Export a cryptography key to a string (with a checksum) * - * @param Key|KeyPair $key + * @param object $key * @return HiddenString * * @throws CannotPerformOperation @@ -745,8 +745,7 @@ public static function export($key): HiddenString return self::export( $key->getSecretKey() ); - } - if ($key instanceof Key) { + } elseif ($key instanceof Key) { return new HiddenString( Hex::encode( Halite::HALITE_VERSION_KEYS . $key->getRawKeyMaterial() . @@ -758,9 +757,7 @@ public static function export($key): HiddenString ) ); } - // @codeCoverageIgnoreStart throw new \TypeError('Expected a Key.'); - // @codeCoverageIgnoreEnd } /** diff --git a/src/Password.php b/src/Password.php index 40f608f..5e7cafd 100644 --- a/src/Password.php +++ b/src/Password.php @@ -164,12 +164,6 @@ protected static function getConfig(string $stored): SymmetricConfig ) { /** @var string $decoded */ $decoded = Base64UrlSafe::decode($stored); - if (!\is_string($decoded)) { - // @codeCoverageIgnoreStart - \sodium_memzero($stored); - throw new InvalidMessage('Invalid encoding'); - // @codeCoverageIgnoreEnd - } return SymmetricConfig::getConfig( $decoded, 'encrypt' diff --git a/src/Stream/MutableFile.php b/src/Stream/MutableFile.php index 1068bf1..594e1ea 100644 --- a/src/Stream/MutableFile.php +++ b/src/Stream/MutableFile.php @@ -56,6 +56,7 @@ class MutableFile implements StreamInterface * @param string|resource $file * @throws InvalidType * @throws FileAccessDenied + * @psalm-suppress RedundantConditionGivenDocblockType */ public function __construct($file) { @@ -182,7 +183,7 @@ public function readBytes(int $num, bool $skipTests = false): string } /** @var int $bufSize */ $bufSize = \min($remaining, self::CHUNK); - /** @var string $read */ + /** @var string|bool $read */ $read = \fread($this->fp, $bufSize); if (!\is_string($read)) { // @codeCoverageIgnoreStart diff --git a/src/Stream/ReadOnlyFile.php b/src/Stream/ReadOnlyFile.php index 0a7ca13..7dd0e14 100644 --- a/src/Stream/ReadOnlyFile.php +++ b/src/Stream/ReadOnlyFile.php @@ -72,6 +72,7 @@ class ReadOnlyFile implements StreamInterface * @throws FileError * @throws InvalidType * @throws \TypeError + * @psalm-suppress RedundantConditionGivenDocblockType */ public function __construct($file, Key $key = null) { @@ -81,6 +82,7 @@ public function __construct($file, Key $key = null) 'Could not open file for reading' ); } + /** @var resource|bool $fp */ $fp = \fopen($file, 'rb'); // @codeCoverageIgnoreStart if (!\is_resource($fp)) { @@ -241,7 +243,7 @@ public function readBytes(int $num, bool $skipTests = false): string break; } // @codeCoverageIgnoreEnd - /** @var string $read */ + /** @var string|bool $read */ $read = \fread($this->fp, $remaining); if (!\is_string($read)) { // @codeCoverageIgnoreStart diff --git a/test/unit/KeyTest.php b/test/unit/KeyTest.php index 946f5cc..d589fa4 100644 --- a/test/unit/KeyTest.php +++ b/test/unit/KeyTest.php @@ -246,6 +246,12 @@ public function testImport() bin2hex($encKeypair->getPublicKey()->getRawKeyMaterial()), bin2hex($import->getRawKeyMaterial()) ); + + try { + KeyFactory::export(new stdClass()); + $this->fail('Expected a TypeError to be raised'); + } catch (TypeError $ex) { + } } /** diff --git a/test/unit/StreamTest.php b/test/unit/StreamTest.php index 17d7b8f..dfe54c6 100644 --- a/test/unit/StreamTest.php +++ b/test/unit/StreamTest.php @@ -72,6 +72,13 @@ public function testUnreadableFile() $this->assertSame('Could not open file for writing', $ex->getMessage()); } unlink($filename); + + try { + new ReadOnlyFile('/etc/shadow'); + $this->fail('File should not be readable'); + } catch (CryptoException\FileAccessDenied $ex) { + $this->assertSame('Could not open file for reading', $ex->getMessage()); + } } /** From c7c16a00bd7b42c6e188cfaf6769b99107472b06 Mon Sep 17 00:00:00 2001 From: Paragon Initiative Enterprises Date: Tue, 27 Mar 2018 10:47:30 -0400 Subject: [PATCH 5/5] Unneeded type cast. --- src/Cookie.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Cookie.php b/src/Cookie.php index f57b17d..cb73376 100644 --- a/src/Cookie.php +++ b/src/Cookie.php @@ -87,7 +87,7 @@ public function fetch(string $name) if (!\is_string($stored)) { throw new InvalidType('Cookie value is not a string'); } - $config = self::getConfig((string) $stored); + $config = self::getConfig($stored); $decrypted = Crypto::decrypt( $stored, $this->key,