From e8c3e3b0893c504cb73d262e41b229d71aae7f1d Mon Sep 17 00:00:00 2001 From: Scott Date: Mon, 2 Nov 2015 03:48:22 -0500 Subject: [PATCH] Eliminate race conditions; verify hashes to prevent TOCTOU --- src/Alerts/FileModified.php | 7 + src/Alerts/InvalidType.php | 7 + src/Contract/StreamInterface.php | 26 ++ src/File.php | 431 ++++++++++++++++++++----------- src/Stream/MutableFile.php | 106 ++++++++ src/Stream/ReadOnlyFile.php | 157 +++++++++++ src/Util.php | 2 - test/unit/FileLazyTest.php | 20 -- test/unit/StreamTest.php | 60 +++++ 9 files changed, 647 insertions(+), 169 deletions(-) create mode 100644 src/Alerts/FileModified.php create mode 100644 src/Alerts/InvalidType.php create mode 100644 src/Contract/StreamInterface.php create mode 100644 src/Stream/MutableFile.php create mode 100644 src/Stream/ReadOnlyFile.php create mode 100644 test/unit/StreamTest.php diff --git a/src/Alerts/FileModified.php b/src/Alerts/FileModified.php new file mode 100644 index 0000000..3c269bd --- /dev/null +++ b/src/Alerts/FileModified.php @@ -0,0 +1,7 @@ +get(), $config->HASH_LEN); } else { $state = \Sodium\crypto_generichash_init(null, $config->HASH_LEN); } - $stat = \fstat($fileHandle); - $size = $stat['size']; - - while (!\feof($fileHandle) && $size > 0) { - if ($size < $config->BUFFER) { - $read = Util::readBytes($fileHandle, $size); - } else { - $read = Util::readBytes($fileHandle, $config->BUFFER); - } + $size = $fileStream->getSize(); + while ($fileStream->remainingBytes() > 0) { + $read = $fileStream->readBytes( + ($fileStream->getPos() + $config->BUFFER) > $size + ? ($size - $fileStream->getPos()) + : $config->BUFFER + ); \Sodium\crypto_generichash_update($state, $read); - $size -= Util::safeStrlen($read); } if ($raw) { return \Sodium\crypto_generichash_final($state, $config->HASH_LEN); @@ -595,6 +654,25 @@ public static function encryptResource( 'Expected a key intended for symmetric-key cryptography' ); } + return self::encryptStream( + new ReadOnlyFile($input), + new MutableFile($output), + $key + ); + } + + /** + * Encrypt a (file handle) + * + * @param $input + * @param $output + * @param EncryptionKey $key + */ + public static function encryptStream( + ReadOnlyFile $input, + MutableFile $output, + EncryptionKey $key + ) { $config = self::getConfig(Halite::HALITE_VERSION_FILE, 'encrypt'); // Generate a nonce and HKDF salt @@ -608,9 +686,9 @@ public static function encryptResource( unset($authKey); // Write the header - Util::writeBytes($output, Halite::HALITE_VERSION_FILE, Halite::VERSION_TAG_LEN); - Util::writeBytes($output, $firstnonce, \Sodium\CRYPTO_STREAM_NONCEBYTES); - Util::writeBytes($output, $hkdfsalt, $config->HKDF_SALT_LEN); + $output->writeBytes(Halite::HALITE_VERSION_FILE, Halite::VERSION_TAG_LEN); + $output->writeBytes($firstnonce, \Sodium\CRYPTO_STREAM_NONCEBYTES); + $output->writeBytes($hkdfsalt, $config->HKDF_SALT_LEN); \hash_update($mac, Halite::HALITE_VERSION_FILE); \hash_update($mac, $firstnonce); @@ -659,16 +737,35 @@ public static function decryptResource( 'Expected a key intended for symmetric-key cryptography' ); } - + return self::decryptStream( + new ReadOnlyFile($input), + new MutableFile($output), + $key + ); + } + + /** + * Decrypt a (file handle) + * + * @param $input + * @param $output + * @param EncryptionKey $key + */ + public static function decryptStream( + ReadOnlyFile $input, + MutableFile $output, + EncryptionKey $key + ) { + $input->reset(0); // Parse the header, ensuring we get 4 bytes - $header = Util::readBytes($input, Halite::VERSION_TAG_LEN); + $header = $input->readBytes(Halite::VERSION_TAG_LEN); // Load the config $config = self::getConfig($header, 'encrypt'); // Let's grab the first nonce and salt - $firstnonce = Util::readBytes($input, $config->NONCE_BYTES); - $hkdfsalt = Util::readBytes($input, $config->HKDF_SALT_LEN); + $firstnonce = $input->readBytes($config->NONCE_BYTES); + $hkdfsalt = $input->readBytes($config->HKDF_SALT_LEN); // Split our keys, begin the HMAC instance list ($encKey, $authKey) = self::splitKeys($key, $hkdfsalt, $config); @@ -683,7 +780,7 @@ public static function decryptResource( $ret = self::streamDecrypt( $input, - $output, + $output, new EncryptionKey($encKey), $firstnonce, $mac, @@ -733,6 +830,25 @@ public static function sealResource( 'Expected a key intended for asymmetric-key cryptography' ); } + return self::sealStream( + new ReadOnlyFile($input), + new MutableFile($output), + $publickey + ); + } + + /** + * Seal a (file handle) + * + * @param ReadOnlyFile $input + * @param MutableFile $output + * @param EncryptionPublicKey $publickey + */ + public static function sealStream( + ReadOnlyFile $input, + MutableFile $output, + EncryptionPublicKey $publickey + ) { // Generate a new keypair for this encryption $eph_kp = KeyFactory::generateEncryptionKeyPair(); $eph_secret = $eph_kp->getSecretKey(); @@ -766,9 +882,9 @@ public static function sealResource( // We no longer need to retain this after we've set up the hash context unset($authKey); - Util::writeBytes($output, Halite::HALITE_VERSION_FILE, Halite::VERSION_TAG_LEN); - Util::writeBytes($output, $eph_public->get(), \Sodium\CRYPTO_BOX_PUBLICKEYBYTES); - Util::writeBytes($output, $hkdfsalt, $config->HKDF_SALT_LEN); + $output->writeBytes(Halite::HALITE_VERSION_FILE, Halite::VERSION_TAG_LEN); + $output->writeBytes($eph_public->get(), \Sodium\CRYPTO_BOX_PUBLICKEYBYTES); + $output->writeBytes($hkdfsalt, $config->HKDF_SALT_LEN); \hash_update($mac, Halite::HALITE_VERSION_FILE); \hash_update($mac, $eph_public->get()); @@ -819,17 +935,35 @@ public static function unsealResource( 'Expected a key intended for asymmetric-key cryptography' ); } - + return self::unsealStream( + new ReadOnlyFile($input), + new MutableFile($output), + $secretkey + ); + } + + /** + * Unseal a (file handle) + * + * @param ReadOnlyFile $input + * @param MutableFile $output + * @param EncryptionSecretKey $secretkey + */ + public static function unsealStream( + ReadOnlyFile $input, + MutableFile $output, + EncryptionSecretKey $secretkey + ) { $secret_key = $secretkey->get(); $public_key = \Sodium\crypto_box_publickey_from_secretkey($secret_key); // Parse the header, ensuring we get 4 bytes - $header = Util::readBytes($input, Halite::VERSION_TAG_LEN); + $header = $input->readBytes(Halite::VERSION_TAG_LEN); // Load the config $config = self::getConfig($header, 'seal'); // Let's grab the public key and salt - $eph_public = Util::readBytes($input, $config->PUBLICKEY_BYTES); - $hkdfsalt = Util::readBytes($input, $config->HKDF_SALT_LEN); + $eph_public = $input->readBytes($config->PUBLICKEY_BYTES); + $hkdfsalt = $input->readBytes($config->HKDF_SALT_LEN); $nonce = \Sodium\crypto_generichash( $eph_public . $public_key, @@ -888,7 +1022,26 @@ public static function signResource( SignatureSecretKey $secretkey, $raw_binary = false ) { - $csum = self::checksumResource($input, null, true); + return self::signStream( + new ReadOnlyFile($input), + $secretkey, + $raw_binary + ); + } + + /** + * Sign the contents of a file + * + * @param ReadOnlyFile $input + * @param SignatureSecretKey $secretkey + * @param bool $raw_binary Don't hex encode? + */ + public static function signStream( + ReadOnlyFile $input, + SignatureSecretKey $secretkey, + $raw_binary = false + ) { + $csum = self::checksumStream($input, null, true); return AsymmetricCrypto::sign($csum, $secretkey, $raw_binary); } @@ -908,7 +1061,32 @@ public static function verifyResource( $signature, $raw_binary = false ) { - $csum = self::checksumResource($input, null, true); + return self::verifyStream( + new ReadOnlyFile($input), + $publickey, + $signature, + $raw_binary + ); + } + + + /** + * Verify the contents of a file + * + * @param $input (file handle) + * @param \ParagonIE\Halite\Contract\CryptoKeyInterface $publickey + * @param string $signature + * @param bool $raw_binary Don't hex encode? + * + * @return bool + */ + public static function verifyStream( + ReadOnlyFile $input, + SignaturePublicKey $publickey, + $signature, + $raw_binary = false + ) { + $csum = self::checksumStream($input, null, true); return AsymmetricCrypto::verify( $csum, $publickey, @@ -1070,8 +1248,8 @@ protected static function splitKeys( /** * Stream encryption - Do not call directly * - * @param resource $input - * @param resource $output + * @param ReadOnlyFile $input + * @param MutableFile $output * @param Key $encKey * @param string $nonce * @param resource $mac (hash context) @@ -1079,45 +1257,42 @@ protected static function splitKeys( * @throws FileAlert\AccessDenied */ final private static function streamEncrypt( - $input, - $output, + ReadOnlyFile $input, + MutableFile $output, EncryptionKey $encKey, $nonce, $mac, Config $config ) { - $fstat = \fstat($input); - $fsize = $fstat['size']; - $break = false; // Begin the streaming decryption - while (!\feof($input) && !$break) { - $pos = \ftell($input); - if (($pos + $config->BUFFER) > $fsize) { - $break = true; - $read = Util::readBytes($input, ($fsize - $pos)); - } else { - $read = Util::readBytes($input, $config->BUFFER); - } + $size = $input->getSize(); + while ($input->remainingBytes() > 0) { + $read = $input->readBytes( + ($input->getPos() + $config->BUFFER) > $size + ? ($size - $input->getPos()) + : $config->BUFFER + ); + $encrypted = \Sodium\crypto_stream_xor( $read, $nonce, $encKey->get() ); - \hash_update($mac, $encrypted); - - Util::writeBytes($output, $encrypted); + $output->writeBytes($encrypted); \Sodium\increment($nonce); } \Sodium\memzero($nonce); - return Util::writeBytes($output, \hash_final($mac, true)); + return $output->writeBytes( + \hash_final($mac, true) + ); } /** * Stream decryption - Do not call directly * - * @param resource $input - * @param resource $output + * @param ReadOnlyFile $input + * @param MutableFile $output * @param Key $encKey * @param string $nonce * @param resource $mac (hash context) @@ -1125,45 +1300,27 @@ final private static function streamEncrypt( * @throws FileAlert\AccessDenied */ final private static function streamDecrypt( - $input, - $output, + ReadOnlyFile $input, + MutableFile $output, EncryptionKey $encKey, $nonce, $mac, Config $config, array &$chunk_macs ) { - // Reset the stream pointer to the beginning of the ciphertext - $start = \ftell($input); - if (\fseek($input, (-1 * $config->MAC_SIZE), SEEK_END) === false) { - throw new CryptoException\CannotPerformOperation( - 'Stream error' - ); - } - $cipher_end = \ftell($input) - 1; - if (\fseek($input, $start, SEEK_SET) === false) { - throw new CryptoException\CannotPerformOperation( - 'Stream error' - ); - } - $break = false; - while (!$break) { - $pos = \ftell($input); - if ($pos === false) { - throw new CryptoException\CannotPerformOperation( - 'Stream error' + $start = $input->getPos(); + $cipher_end = $input->getSize() - $config->MAC_SIZE; + // Begin the streaming decryption + $input->reset($start); + + while ($input->remainingBytes() > $config->MAC_SIZE) { + if (($input->getPos() + $config->BUFFER) > $cipher_end) { + $read = $input->readBytes( + $cipher_end - $input->getPos() ); - } - - // Read the data from the input buffer - if ($pos + $config->BUFFER >= $cipher_end) { - $break = true; - $read = Util::readBytes($input, $cipher_end - $pos + 1); } else { - $read = Util::readBytes($input, $config->BUFFER); + $read = $input->readBytes($config->BUFFER); } - - // Let's reculcualte the MAC of this chunk, then verify it \hash_update($mac, $read); $calcMAC = \hash_copy($mac); if ($calcMAC === false) { @@ -1171,16 +1328,19 @@ final private static function streamDecrypt( 'An unknown error has occurred' ); } - $calc = \hash_final($calcMAC, true); + if (empty($chunk_macs)) { throw new CryptoException\InvalidMessage( 'Invalid message authentication code' ); - } elseif (!\hash_equals(\array_shift($chunk_macs), $calc)) { - throw new CryptoException\InvalidMessage( - 'Invalid message authentication code' - ); + } else { + $chkmac = \array_shift($chunk_macs); + if (!\hash_equals($chkmac, $calc)) { + throw new CryptoException\InvalidMessage( + 'Invalid message authentication code' + ); + } } $decrypted = \Sodium\crypto_stream_xor( @@ -1188,9 +1348,10 @@ final private static function streamDecrypt( $nonce, $encKey->get() ); - Util::writeBytes($output, $decrypted); + $output->writeBytes($decrypted); \Sodium\increment($nonce); } + \Sodium\memzero($nonce); return true; } @@ -1205,50 +1366,30 @@ final private static function streamDecrypt( * @throws FileAlert\AccessDenied */ final private static function streamVerify( - $input, + ReadOnlyFile $input, $mac, Config $config ) { - $start = \ftell($input); - if (\fseek($input, (-1 * $config->MAC_SIZE), SEEK_END) === false) { - throw new CryptoException\CannotPerformOperation( - 'Stream error' - ); - } - $cipher_end = \ftell($input) - 1; + $start = $input->getPos(); - $stored_mac = Util::readBytes($input, $config->MAC_SIZE); + $cipher_end = $input->getSize() - $config->MAC_SIZE; + $input->reset($cipher_end); + $stored_mac = $input->readBytes($config->MAC_SIZE); + $input->reset($start); - if (\fseek($input, $start, SEEK_SET) === false) { - throw new CryptoException\CannotPerformOperation( - 'Stream error' - ); - } $chunk_macs = []; $break = false; - while (!$break) { - /** - * First, grab the current position - */ - $pos = \ftell($input); - if ($pos === false) { - throw new CryptoException\CannotPerformOperation( - 'Stream error' - ); - } - if ($pos >= $cipher_end) { - break; - } + while (!$break && $input->getPos() < $cipher_end) { /** * Would a full BUFFER read put it past the end of the * ciphertext? If so, only return a portion of the file. */ - if ($pos + $config->BUFFER >= $cipher_end) { + if ($input->getPos() + $config->BUFFER >= $cipher_end) { $break = true; - $read = Util::readBytes($input, $cipher_end - $pos + 1); + $read = $input->readBytes($cipher_end - $input->getPos()); } else { - $read = Util::readBytes($input, $config->BUFFER); + $read = $input->readBytes($config->BUFFER); } /** @@ -1281,11 +1422,7 @@ final private static function streamVerify( 'Invalid message authentication code' ); } - if (\fseek($input, $start, SEEK_SET) === false) { - throw new CryptoException\CannotPerformOperation( - 'Stream error' - ); - } + $input->reset($start); return $chunk_macs; } } diff --git a/src/Stream/MutableFile.php b/src/Stream/MutableFile.php new file mode 100644 index 0000000..367dd67 --- /dev/null +++ b/src/Stream/MutableFile.php @@ -0,0 +1,106 @@ +fp = \fopen($file, 'wb'); + $this->pos = 0; + $this->stat = \fstat($this->fp); + } elseif (is_resource($file)) { + $this->fp = $file; + $this->pos = \ftell($this->fp); + $this->stat = \fstat($this->fp); + } + } + + /** + * Read from a stream; prevent partial reads + * + * @param int $num + * @return string + * @throws FileAlert\AccessDenied + */ + public function readBytes($num) + { + if ($num <= 0) { + throw new \Exception('num < 0'); + } + if (($this->pos + $num) > $this->stat['size']) { + throw new \Exception('Out-of-bounds read'); + } + $buf = ''; + $remaining = $num; + do { + if ($remaining <= 0) { + break; + } + $read = \fread($this->fp, $remaining); + if ($read === false) { + throw new CryptoException\FileAccessDenied( + 'Could not read from the file' + ); + } + $buf .= $read; + $readSize = Util::safeStrlen($read); + $this->pos += $readSize; + $remaining -= $readSize; + \var_dump($remaining); + } while ($remaining > 0); + return $buf; + } + + /** + * Write to a stream; prevent partial writes + * + * @param resource $stream + * @param string $buf + * @param int $num (number of bytes) + * @throws FileAlert\AccessDenied + */ + public function writeBytes($buf, $num = null) + { + $bufSize = Util::safeStrlen($buf); + if ($num === null || $num > $bufSize) { + $num = $bufSize; + } + if ($num < 0) { + throw new \Exception('num < 0'); + } + $remaining = $num; + do { + if ($remaining <= 0) { + break; + } + $written = \fwrite($this->fp, $buf, $remaining); + if ($written === false) { + throw new CryptoException\FileAccessDenied( + 'Could not write to the file' + ); + } + $buf = Util::safeSubstr($buf, $written, null); + $this->pos += $written; + $this->stat = \fstat($this->fp); + $remaining -= $written; + } while ($remaining > 0); + return $num; + } + + public function reset($i = 0) + { + $this->pos = $i; + \fseek($this->fp, $i, SEEK_SET); + } +} diff --git a/src/Stream/ReadOnlyFile.php b/src/Stream/ReadOnlyFile.php new file mode 100644 index 0000000..f805b3a --- /dev/null +++ b/src/Stream/ReadOnlyFile.php @@ -0,0 +1,157 @@ +fp = \fopen($file, 'rb'); + $this->pos = 0; + $this->stat = \fstat($this->fp); + } elseif (is_resource($file)) { + $this->fp = $file; + $this->pos = \ftell($this->fp); + $this->stat = \fstat($this->fp); + } + $this->hash = $this->getHash(); + } + + /** + * Where are we in the buffeR? + * + * @return int + */ + public function getPos() + { + return $this->pos; + } + /** + * How big is this buffer? + * + * @return int + */ + public function getSize() + { + return $this->stat['size']; + } + + /** + * Read from a stream; prevent partial reads + * + * @param int $num + * @return string + * @throws FileAlert\AccessDenied + */ + public function readBytes($num) + { + if ($num <= 0) { + throw new \Exception('num < 0'); + } + if (($this->pos + $num) > $this->stat['size']) { + throw new \Exception('Out-of-bounds read'); + } + $buf = ''; + $remaining = $num; + $this->toctouTest(); + do { + if ($remaining <= 0) { + break; + } + $read = \fread($this->fp, $remaining); + if ($read === false) { + throw new CryptoException\FileAccessDenied( + 'Could not read from the file' + ); + } + $buf .= $read; + $readSize = Util::safeStrlen($read); + $this->pos += $readSize; + $remaining -= $readSize; + } while ($remaining > 0); + return $buf; + } + + /** + * Get number of bytes remaining + * + * @return int + */ + public function remainingBytes() + { + return (PHP_INT_MAX & ($this->stat['size'] - $this->pos)); + } + + /** + * Write to a stream; prevent partial writes + * + * @param string $buf + * @param int $num (number of bytes) + * @throws FileAlert\AccessDenied + */ + public function writeBytes($buf, $num = null) + { + throw new CryptoException\FileAccessDenied( + 'This is a read-only file handle.' + ); + } + + public function reset($i = 0) + { + $this->pos = $i; + \fseek($this->fp, $i, SEEK_SET); + } + + /** + * Calculate a hash of a file + * + * @return string + */ + public function getHash() + { + $init = $this->pos; + \fseek($this->fp, 0, SEEK_SET); + $h = \Sodium\crypto_generichash_init( + null, + \Sodium\CRYPTO_GENERICHASH_BYTES_MAX + ); + $i = 0; + for ($i = 0; $i < $this->stat['size']; $i += self::CHUNK) { + if (($i + self::CHUNK) > $this->stat['size']) { + $c = \fread($this->fp, ($this->stat['size'] - $i)); + } else { + $c = \fread($this->fp, self::CHUNK); + } + \Sodium\crypto_generichash_update($h, $c); + } + \fseek($this->fp, $init, SEEK_SET); + return \Sodium\crypto_generichash_final($h); + } + + /** + * Run-time test to prevent TOCTOU attacks (race conditions) + * + * @throws CryptoException\FileModified + * @return true + */ + public function toctouTest() + { + if (!\hash_equals($this->hash, $this->getHash())) { + throw new CryptoException\FileModified( + 'Read-only file has been modified since it was opened for reading' + ); + } + return true; + } +} \ No newline at end of file diff --git a/src/Util.php b/src/Util.php index 47c05ec..407ae17 100644 --- a/src/Util.php +++ b/src/Util.php @@ -21,8 +21,6 @@ final public static function readBytes($stream, $num) $fstat = \fstat($stream); $pos = \ftell($stream); if (($pos + $num) > $fstat['size']) { - \var_dump(['pos' => $pos, 'num' => $num, 'size' => $fstat['size']]); - exit; throw new \Exception('Out-of-bounds read'); } $buf = ''; diff --git a/test/unit/FileLazyTest.php b/test/unit/FileLazyTest.php index 7d1d6d0..61f38d1 100644 --- a/test/unit/FileLazyTest.php +++ b/test/unit/FileLazyTest.php @@ -169,24 +169,4 @@ public function testChecksum() $file ); } - - public function testArgFail() - { - try { - \touch(__DIR__.'/tmp/paragon_avatar.encrypted.png'); - \chmod(__DIR__.'/tmp/paragon_avatar.encrypted.png', 0777); - \touch(__DIR__.'/tmp/paragon_avatar.decrypted.png'); - \chmod(__DIR__.'/tmp/paragon_avatar.decrypted.png', 0777); - - $key = new EncryptionKey(\str_repeat('B', 32)); - File::encrypt( - __DIR__.'/tmp/paragon_avatar.png', - \fopen(__DIR__.'/tmp/paragon_avatar.encrypted.png', 'wb'), - $key - ); - $this->fail('We should be throwing an exception, not failing open.'); - } catch (\InvalidArgumentException $e) { - $this->assertTrue($e instanceof \InvalidArgumentException); - } - } } \ No newline at end of file diff --git a/test/unit/StreamTest.php b/test/unit/StreamTest.php new file mode 100644 index 0000000..f1aaaec --- /dev/null +++ b/test/unit/StreamTest.php @@ -0,0 +1,60 @@ +assertEquals( + $fileOne->getHash(), + $fileTwo->getHash() + ); + \fclose($fp); + } + + public function testFileRead() + { + $filename = \tempnam('/tmp', 'x'); + + $buf = \Sodium\randombytes_buf(65537); + \file_put_contents($filename, $buf); + + $fStream = new ReadOnlyFile($filename); + + $this->assertEquals( + $fStream->readBytes(65537), + $buf + ); + $fStream->reset(0); + + \file_put_contents( + $filename, + Util::safeSubstr($buf, 0, 32768) . 'x' . Util::safeSubstr($buf, 32768) + ); + + try { + $x = $fStream->readBytes(65537); + throw new \Exception('fail'); + } catch (CryptoException\FileModified $ex) { + $this->assertTrue( + $ex instanceof CryptoException\FileModified + ); + } + } +} \ No newline at end of file