fix reading encrypted files beyond file boundaries (#5160)

Summary:
This fix should help reading from encrypted files if the file-to-be-read
is smaller than expected. For example, when using the encrypted env and
making it read a journal file of exactly 0 bytes size, the encrypted env
code crashes with SIGSEGV in its Decrypt function, as there is no check
if the read attempts to read over the file's boundaries (as specified
originally by the `dataSize` parameter).

The most important problem this patch addresses is however that there is
no size underlow check in `CTREncryptionProvider::CreateCipherStream`:

The stream to be read will be initialized to a size of always
`prefix.size() - (2 * blockSize)`. If the prefix however is smaller than
twice the block size, this will obviously assume a _very_ large stream
and read over the bounds. The patch adds a check here as follows:

    // If the prefix is smaller than twice the block size, we would below read a
    // very large chunk of the file (and very likely read over the bounds)
    assert(prefix.size() >= 2 * blockSize);
    if (prefix.size() < 2 * blockSize) {
      return Status::Corruption("Unable to read from file " + fname + ": read attempt would read beyond file bounds");
    }

so embedders can catch the error in their release builds.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/5160

Differential Revision: D14834633

Pulled By: sagar0

fbshipit-source-id: 47aa39a6db8977252cede054c7eb9a663b9a3484

diff --git a/HISTORY.md b/HISTORY.md
index 6c5464d5010ca0ed31c76e8254654348b6df2c2d..f2bdcda11ba1af73961fc25dbe6c5f772fa41eed 100644 (file)
--- a/HISTORY.md
+++ b/HISTORY.md
@@ -6,6 +6,7 @@
 ### Public API Change
 ### Bug Fixes
 * Fix a bug in 2PC where a sequence of txn prepare, memtable flush, and crash could result in losing the prepared transaction.
+* Fix a bug in Encryption Env which could cause encrypted files to be read beyond file boundaries.
 
 ## 6.1.0 (3/27/2019)
 ### New Features

diff --git a/env/env_encryption.cc b/env/env_encryption.cc
index 1f2bfa7e527eb6f0d39f133795c8d6c46ce6ea0e..aa59e66357ae22e95f52fe93bea5126696600db4 100644 (file)
--- a/env/env_encryption.cc
+++ b/env/env_encryption.cc
@@ -8,6 +8,7 @@
 #include <algorithm>
 #include <cctype>
 #include <iostream>
+#include <cassert>
 
 #include "rocksdb/env_encryption.h"
 #include "util/aligned_buffer.h"
@@ -733,6 +734,8 @@ Status BlockAccessCipherStream::Decrypt(uint64_t fileOffset, char *data, size_t
   std::string scratch;
   AllocateScratch(scratch);
 
+  assert(fileOffset < dataSize);
+
   // Decrypt individual blocks.
   while (1) {
     char *block = data;
@@ -756,6 +759,14 @@ Status BlockAccessCipherStream::Decrypt(uint64_t fileOffset, char *data, size_t
       // Copy decrypted data back to `data`.
       memmove(data, block + blockOffset, n);
     }
+
+    // Simply decrementing dataSize by n could cause it to underflow,
+    // which will very likely make it read over the original bounds later
+    assert(dataSize >= n);
+    if (dataSize < n) {
+      return Status::Corruption("Cannot decrypt data at given offset");
+    }
+
     dataSize -= n;
     if (dataSize == 0) {
       return Status::OK();
@@ -882,6 +893,13 @@ Status CTREncryptionProvider::CreateCipherStream(
   Slice iv;
   decodeCTRParameters(prefix.data(), blockSize, initialCounter, iv);
 
+  // If the prefix is smaller than twice the block size, we would below read a
+  // very large chunk of the file (and very likely read over the bounds)
+  assert(prefix.size() >= 2 * blockSize);
+  if (prefix.size() < 2 * blockSize) {
+    return Status::Corruption("Unable to read from file " + fname + ": read attempt would read beyond file bounds");
+  }
+
   // Decrypt the encrypted part of the prefix, starting from block 2 (block 0, 1 with initial counter & IV are unencrypted)
   CTRCipherStream cipherStream(cipher_, iv.data(), initialCounter);
   auto status = cipherStream.Decrypt(0, (char*)prefix.data() + (2 * blockSize), prefix.size() - (2 * blockSize));