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
This commit is contained in:
jsteemann 2019-04-08 14:54:36 -07:00 committed by Facebook Github Bot
parent 0bb555630f
commit 313e877285
2 changed files with 19 additions and 0 deletions

View File

@ -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

18
env/env_encryption.cc vendored
View File

@ -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));