bahman/scylla
1
0
Fork 0
Code Issues Pull Requests Actions 3 Packages Projects Releases Wiki Activity

Merge 'encrypted_file_impl: Check for reads on or past actual file length in transform' from Calle Wilund

Browse Source 
Fixes #22236

If reading a file and not stopping on block bounds returned by `size()`, we could allow reading from (_file_size+<1-15>) (if crossing block boundary) and try to decrypt this buffer (last one).

Simplest example:
Actual data size: 4095
Physical file size: 4095 + key block size (typically 16)
Read from 4096: -> 15 bytes (padding) -> transform return `_file_size` - `read offset` -> wraparound -> rather larger number than we expected (not to mention the data in question is junk/zero).

Check on last block in `transform` would wrap around size due to us being >= file size (l).
Just do an early bounds check and return zero if we're past the actual data limit.

Closes scylladb/scylladb#22395

* github.com:scylladb/scylladb:
  encrypted_file_test: Test reads beyond decrypted file length
  encrypted_file_impl: Check for reads on or past actual file length in transform
This commit is contained in:
Botond Dénes
2025-01-29 20:09:32 +02:00
parent 2a6445343c 2fb95e4e2f
commit 5dd6fcfe6f
2 changed files with 142 additions and 2 deletions
Download Patch File Download Diff File Expand all files Collapse all files

25
ent/encryption/encrypted_file_impl.cc
View File

@@ -223,7 +223,18 @@ size_t encrypted_file_impl::transform(uint64_t pos, const void* buffer, size_t l
throw std::invalid_argument("Output data not aligned");
}
_key->transform_unpadded(m, i + off, align_down(rem, b), o + off, iv.data());
return l - pos;
// #22236 - ensure we don't wrap numbers here.
// If reading past actual end of file (_file_length), we can be decoding
// 1-<key block size> bytes here, that are at the boundary of last
// (fake) block of the file.
// Example:
// File data size: 4095 bytes
// Physical file size: 4095 + 16 (assume 16 bytes key block)
// Read 0:4096 -> 4095 bytes
// If caller now ignores this and just reads 4096 (or more)
// bytes at next block (4096), we read 15 bytes and decode.
// But would be past _file_length -> ensure we return zero here.
return std::max(l, pos) - pos;
}
_key->transform_unpadded(m, i + off, block_size, o + off, iv.data());
}
@@ -271,6 +282,9 @@ future<size_t> encrypted_file_impl::write_dma(uint64_t pos, std::vector<iovec> i
future<size_t> encrypted_file_impl::read_dma(uint64_t pos, void* buffer, size_t len, io_intent* intent) {
assert(!(len & (block_size - 1)));
return verify_file_length().then([this, pos, buffer, len, intent] {
if (pos >= *_file_length) {
return make_ready_future<size_t>(0);
}
return _file.dma_read(pos, buffer, len, intent).then([this, pos, buffer](size_t len) {
return transform(pos, buffer, len, buffer, mode::decrypt);
});
@@ -279,6 +293,9 @@ future<size_t> encrypted_file_impl::read_dma(uint64_t pos, void* buffer, size_t
future<size_t> encrypted_file_impl::read_dma(uint64_t pos, std::vector<iovec> iov, io_intent* intent) {
return verify_file_length().then([this, pos, iov = std::move(iov), intent]() mutable {
if (pos >= *_file_length) {
return make_ready_future<size_t>(0);
}
auto f = _file.dma_read(pos, iov, intent);
return f.then([this, pos, iov = std::move(iov)](size_t len) mutable {
size_t off = 0;
@@ -292,6 +309,9 @@ future<size_t> encrypted_file_impl::read_dma(uint64_t pos, std::vector<iovec> io
future<temporary_buffer<uint8_t>> encrypted_file_impl::dma_read_bulk(uint64_t offset, size_t range_size, io_intent* intent) {
return verify_file_length().then([this, offset, range_size, intent]() mutable {
if (offset >= *_file_length) {
return make_ready_future<temporary_buffer<uint8_t>>();
}
auto front = offset & (block_size - 1);
offset -= front;
range_size += front;
@@ -305,7 +325,8 @@ future<temporary_buffer<uint8_t>> encrypted_file_impl::dma_read_bulk(uint64_t of
auto s = transform(offset, result.get(), result.size(), result.get_write(), mode::decrypt);
// never give back more than asked for.
result.trim(std::min(s, range_size));
result.trim_front(front);
// #22236 - ensure we don't overtrim if we get a short read.
result.trim_front(std::min(front, result.size()));
return result;
});
});

119
test/boost/encrypted_file_test.cc
View File

@@ -15,12 +15,14 @@
#include <seastar/core/seastar.hh>
#include <seastar/core/shared_ptr.hh>
#include <seastar/core/thread.hh>
#include <seastar/core/fstream.hh>
#include <seastar/testing/test_case.hh>
#include "ent/encryption/encryption.hh"
#include "ent/encryption/symmetric_key.hh"
#include "ent/encryption/encrypted_file_impl.hh"
#include "test/lib/log.hh"
#include "test/lib/tmpdir.hh"
#include "test/lib/random_utils.hh"
#include "test/lib/exception_utils.hh"
@@ -181,6 +183,66 @@ SEASTAR_TEST_CASE(test_short) {
}
}
SEASTAR_TEST_CASE(test_read_across_size_boundary) {
auto name = "test_read_across_size_boundary";
auto [dst, k] = co_await make_file(name, open_flags::rw|open_flags::create);
auto size = dst.disk_write_dma_alignment() - 1;
co_await dst.truncate(size);
co_await dst.close();
auto [f, _] = co_await make_file(name, open_flags::ro, k);
auto a = f.disk_write_dma_alignment();
auto m = f.memory_dma_alignment();
auto buf = temporary_buffer<char>::aligned(m, a);
auto n = co_await f.dma_read(0, buf.get_write(), buf.size());
auto buf2 = temporary_buffer<char>::aligned(m, a);
auto n2 = co_await f.dma_read(a, buf2.get_write(), buf2.size());
auto buf3 = temporary_buffer<char>::aligned(m, a);
std::vector<iovec> iov({{buf3.get_write(), buf3.size()}});
auto n3 = co_await f.dma_read(a, std::move(iov));
auto buf4 = co_await f.dma_read_bulk<char>(a, size_t(a));
co_await f.close();
BOOST_REQUIRE_EQUAL(size, n);
buf.trim(n);
for (auto c : buf) {
BOOST_REQUIRE_EQUAL(c, 0);
}
BOOST_REQUIRE_EQUAL(0, n2);
BOOST_REQUIRE_EQUAL(0, n3);
BOOST_REQUIRE_EQUAL(0, buf4.size());
}
static future<> test_read_across_size_boundary_unaligned_helper(int64_t size_off, int64_t read_off) {
auto name = "test_read_across_size_boundary_unaligned";
auto [dst, k] = co_await make_file(name, open_flags::rw|open_flags::create);
auto size = dst.disk_write_dma_alignment() + size_off;
co_await dst.truncate(size);
co_await dst.close();
auto [f, k2] = co_await make_file(name, open_flags::ro, k);
auto buf = co_await f.dma_read_bulk<char>(f.disk_write_dma_alignment() + read_off, size_t(f.disk_write_dma_alignment()));
co_await f.close();
BOOST_REQUIRE_EQUAL(0, buf.size());
}
SEASTAR_TEST_CASE(test_read_across_size_boundary_unaligned) {
co_await test_read_across_size_boundary_unaligned_helper(-1, 1);
}
SEASTAR_TEST_CASE(test_read_across_size_boundary_unaligned2) {
co_await test_read_across_size_boundary_unaligned_helper(-2, -1);
}
SEASTAR_TEST_CASE(test_truncating_empty) {
auto name = "test_truncating_empty";
auto t = co_await make_file(name, open_flags::rw|open_flags::create);
@@ -263,3 +325,60 @@ SEASTAR_TEST_CASE(test_truncating_extend) {
co_await f.close();
}
// Reproducer for https://github.com/scylladb/scylladb/issues/22236
SEASTAR_TEST_CASE(test_read_from_padding) {
key_info kinfo {"AES/CBC/PKCSPadding", 128};
shared_ptr<symmetric_key> k = make_shared<symmetric_key>(kinfo);
testlog.info("Created symmetric key: info={} key={} ", k->info(), k->key());
size_t block_size;
size_t buf_size;
constexpr auto& filename = "encrypted_file";
const auto& filepath = dir.path() / filename;
testlog.info("Creating encrypted file {}", filepath.string());
{
auto [file, _] = co_await make_file(filename, open_flags::create | open_flags::wo, k);
auto ostream = co_await make_file_output_stream(file);
block_size = file.disk_write_dma_alignment();
buf_size = block_size - 1;
auto wbuf = seastar::temporary_buffer<char>::aligned(file.memory_dma_alignment(), buf_size);
co_await ostream.write(wbuf.get(), wbuf.size());
testlog.info("Wrote {} bytes to encrypted file {}", wbuf.size(), filepath.string());
co_await ostream.close();
testlog.info("Length of {}: {} bytes", filename, co_await file.size());
}
testlog.info("Testing DMA reads from padding area of file {}", filepath.string());
{
auto [file, _] = co_await make_file(filename, open_flags::ro, k);
// Triggering the bug requires reading from the padding area:
// `buf_size < read_pos < file.size()`
//
// For `dma_read()`, we have the additional requirement that `read_pos` must be aligned.
// For `dma_read_bulk()`, it doesn't have to.
uint64_t read_pos = block_size;
size_t read_len = block_size;
auto rbuf = seastar::temporary_buffer<char>::aligned(file.memory_dma_alignment(), read_len);
std::vector<iovec> iov {{static_cast<void*>(rbuf.get_write()), rbuf.size()}};
auto res = co_await file.dma_read_bulk<char>(read_pos, read_len);
BOOST_CHECK_MESSAGE(res.size() == 0, seastar::format(
"Bulk DMA read on pos {}, len {}: returned {} bytes instead of zero", read_pos, read_len, res.size()));
auto res_len = co_await file.dma_read(read_pos, iov);
BOOST_CHECK_MESSAGE(res_len == 0, seastar::format(
"IOV DMA read on pos {}, len {}: returned {} bytes instead of zero", read_pos, read_len, res_len));
res_len = co_await file.dma_read<char>(read_pos, rbuf.get_write(), read_len);
BOOST_CHECK_MESSAGE(res_len == 0, seastar::format(
"DMA read on pos {}, len {}: returned {} bytes instead of zero", read_pos, read_len, res_len));
co_await file.close();
}
}