From 313e8772856524d1a05419b11e4837ed4030427b Mon Sep 17 00:00:00 2001
From: jsteemann <jan@arangodb.com>
Date: Mon, 8 Apr 2019 14:54:36 -0700
Subject: [PATCH] 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
---
 HISTORY.md            |  1 +
 env/env_encryption.cc | 18 ++++++++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/HISTORY.md b/HISTORY.md
index 6c5464d50..f2bdcda11 100644
--- 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 1f2bfa7e5..aa59e6635 100644
--- 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));