Boyscouting, add TOCTOU tests against ftell/fstat

src/Stream/MutableFile.php

@@ -5,6 +5,9 @@
 use \ParagonIE\Halite\Alerts as CryptoException;
 use \ParagonIE\Halite\Util;
 
+/**
+ * Contrast with ReadOnlyFile: does not prevent race conditions by itself
+ */
 class MutableFile implements StreamInterface
 {
     const CHUNK = 8192; // PHP's fread() buffer is set to 8192 by default
@@ -57,7 +60,6 @@ public function readBytes($num)
             $readSize = Util::safeStrlen($read);
             $this->pos += $readSize;
             $remaining -= $readSize;
-            \var_dump($remaining);
         } while ($remaining > 0);
         return $buf;
     }

src/Stream/ReadOnlyFile.php

@@ -29,7 +29,7 @@ public function __construct($file)
     }
 
     /**
-     * Where are we in the buffeR?
+     * Where are we in the buffer?
      * 
      * @return int
      */
@@ -48,13 +48,19 @@ public function getSize()
     }
 
     /**
-     * Read from a stream; prevent partial reads
+     * Read from a stream; prevent partial reads (also uses run-time testing to
+     * prevent partial reads -- you can turn this off if you need performance
+     * and aren't concerned about race condition attacks, but this isn't a
+     * decision to make lightly!)
      * 
      * @param int $num
+     * @param boolean $skipTests Only set this to TRUE if you're absolutely sure
+     *                           that you don't want to defend against TOCTOU /
+     *                           race condition attacks on the filesystem!
      * @return string
      * @throws FileAlert\AccessDenied
      */
-    public function readBytes($num)
+    public function readBytes($num, $skipTests = false)
     {
         if ($num <= 0) {
             throw new \Exception('num < 0');
@@ -64,7 +70,9 @@ public function readBytes($num)
         }
         $buf = '';
         $remaining = $num;
-        $this->toctouTest();
+        if (!$skipTests) {
+            $this->toctouTest();
+        }
         do {
             if ($remaining <= 0) {
                 break;
@@ -94,39 +102,56 @@ public function remainingBytes()
     }
 
     /**
-     * Write to a stream; prevent partial writes
+     * This is a meaningless operation for a Read-Only File!
      * 
      * @param string $buf
      * @param int $num (number of bytes)
      * @throws FileAlert\AccessDenied
      */
     public function writeBytes($buf, $num = null)
     {
+        unset($buf);
+        unset($num);
         throw new CryptoException\FileAccessDenied(
             'This is a read-only file handle.'
         );
     }
 
+    /**
+     * Set the current cursor position to 
+     * 
+     * @param int $i
+     * 
+     * @return boolean
+     * 
+     * @throws \ParagonIE\Halite\Alerts\CannotPerformOperation
+     */
     public function reset($i = 0)
     {
         $this->pos = $i;
-        \fseek($this->fp, $i, SEEK_SET);
+        if (\fseek($this->fp, $i, SEEK_SET) === 0) {
+            return true;
+        }
+        throw new CryptoException\CannotPerformOperation(
+            'fseek() failed'
+        );
     }
 
     /**
-     * Calculate a hash of a file
+     * Calculate a BLAKE2b hash of a file
      * 
      * @return string
      */
     public function getHash()
     {
         $init = $this->pos;
         \fseek($this->fp, 0, SEEK_SET);
+
+        // Create a hash context:
         $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));
@@ -135,23 +160,51 @@ public function getHash()
             }
             \Sodium\crypto_generichash_update($h, $c);
         }
+        // Reset the file pointer's internal cursor to where it was:
         \fseek($this->fp, $init, SEEK_SET);
         return \Sodium\crypto_generichash_final($h);
     }
 
     /**
-     * Run-time test to prevent TOCTOU attacks (race conditions)
+     * Run-time test to prevent TOCTOU attacks (race conditions) through
+     * verifying that the hash matches and the current cursor position/file
+     * size matches their values when the file was first opened.
      * 
      * @throws CryptoException\FileModified
      * @return true
      */
     public function toctouTest()
     {
+        // Pre-hash checks
+        $this->internalTest();
+        // Is the hash we had at first access the same one we have now?
         if (!\hash_equals($this->hash, $this->getHash())) {
             throw new CryptoException\FileModified(
                 'Read-only file has been modified since it was opened for reading'
             );
         }
+        // Post-hash checks
+        $this->internalTest();
         return true;
     }
+
+    /**
+     * Verify that ftell() and fstat() return the values we expect
+     * 
+     * @throws CryptoException\FileModified
+     */
+    private function internalTest()
+    {
+        if (\ftell($this->fp) !== $this->pos) {
+            throw new CryptoException\FileModified(
+                'Read-only file has been modified since it was opened for reading'
+            );
+        }
+        $stat = \fstat($this->fp);
+        if ($stat['size'] !== $this->stat['size']) {
+            throw new CryptoException\FileModified(
+                'Read-only file has been modified since it was opened for reading'
+            );
+        }
+    }
 }