This is an automated email from the ASF dual-hosted git repository.
git-hulk pushed a commit to branch unstable
in repository https://gitbox.apache.org/repos/asf/kvrocks.git
The following commit(s) were added to refs/heads/unstable by this push:
new 634ed15df fix(rdb): reject over-large declared lengths when loading
RDB payloads (#3550)
634ed15df is described below
commit 634ed15dfd9249cfb57a52541dddba05b377df0a
Author: hulk <[email protected]>
AuthorDate: Sat Jul 11 17:30:21 2026 +0800
fix(rdb): reject over-large declared lengths when loading RDB payloads
(#3550)
The RDB load path sized buffers from an attacker-declared 64-bit length
before checking the stream actually holds that many bytes. A write-level
client could send a `RESTORE` payload declaring a huge length and make
the server allocate it up front and crash (OOM) — via both the plain
string path (`read_string.resize(len)`) and the LZF path (`out_buf(len,
0)`).
Add `RdbStream::EnsureRemainingBytes()` and validate every length that
drives a read or allocation against the bytes still remaining in the
stream, so a value can never be declared larger than the input that
carries it. The LZF decompressed size may legitimately exceed the
remaining stream, so it is additionally capped against
`proto-max-bulk-len`.
Add gocase regression tests for both the plain-string and LZF-encoded
`RESTORE` payloads: the server now returns an error and stays up.
---
src/common/rdb_stream.cc | 9 +++++
src/common/rdb_stream.h | 21 +++++++++++
src/storage/rdb/rdb.cc | 15 ++++++++
tests/gocase/unit/restore/restore_test.go | 58 ++++++++++++++++++++++++++-----
tests/gocase/util/crc64.go | 32 +++++++++++++++++
5 files changed, 127 insertions(+), 8 deletions(-)
diff --git a/src/common/rdb_stream.cc b/src/common/rdb_stream.cc
index 5a4fb9be3..6f7324629 100644
--- a/src/common/rdb_stream.cc
+++ b/src/common/rdb_stream.cc
@@ -20,6 +20,9 @@
#include "rdb_stream.h"
+#include <filesystem>
+#include <system_error>
+
#include "fmt/format.h"
#include "vendor/crc64.h"
@@ -52,6 +55,12 @@ Status RdbFileStream::Open() {
return {Status::NotOK, fmt::format("failed to open rdb file: '{}': {}",
file_name_, strerror(errno))};
}
+ std::error_code ec;
+ file_size_ = std::filesystem::file_size(file_name_, ec);
+ if (ec) {
+ return {Status::NotOK, fmt::format("failed to get the size of rdb file:
'{}': {}", file_name_, ec.message())};
+ }
+
return Status::OK();
}
diff --git a/src/common/rdb_stream.h b/src/common/rdb_stream.h
index eb2bc8eca..a4270226a 100644
--- a/src/common/rdb_stream.h
+++ b/src/common/rdb_stream.h
@@ -35,6 +35,10 @@ class RdbStream {
virtual Status Read(char *buf, size_t len) = 0;
virtual Status Write(const char *buf, size_t len) = 0;
virtual StatusOr<uint64_t> GetCheckSum() const = 0;
+ // Rejects a declared object length that exceeds the bytes still available in
+ // the stream, before that length is used to allocate memory: a value can
never
+ // be larger than the input that carries it.
+ virtual Status EnsureRemainingBytes(uint64_t n) const = 0;
StatusOr<uint8_t> ReadByte() {
uint8_t value = 0;
auto s = Read(reinterpret_cast<char *>(&value), 1);
@@ -55,6 +59,14 @@ class RdbStringStream : public RdbStream {
Status Read(char *buf, size_t len) override;
Status Write(const char *buf, size_t len) override;
StatusOr<uint64_t> GetCheckSum() const override;
+ Status EnsureRemainingBytes(uint64_t n) const override {
+ uint64_t remaining = pos_ < input_.size() ? input_.size() - pos_ : 0;
+ if (n > remaining) {
+ return {Status::NotOK,
+ fmt::format("invalid RDB payload: required {} bytes exceeds the
remaining {} bytes", n, remaining)};
+ }
+ return Status::OK();
+ }
std::string &GetInput() { return input_; }
private:
@@ -80,6 +92,14 @@ class RdbFileStream : public RdbStream {
memrev64ifbe(&crc);
return crc;
}
+ Status EnsureRemainingBytes(uint64_t n) const override {
+ uint64_t remaining = file_size_ > total_read_bytes_ ? file_size_ -
total_read_bytes_ : 0;
+ if (n > remaining) {
+ return {Status::NotOK,
+ fmt::format("invalid RDB payload: required {} bytes exceeds the
remaining {} bytes", n, remaining)};
+ }
+ return Status::OK();
+ }
private:
std::ifstream ifs_;
@@ -87,4 +107,5 @@ class RdbFileStream : public RdbStream {
uint64_t check_sum_;
size_t total_read_bytes_;
size_t max_read_chunk_size_; // maximum single read chunk size
+ uint64_t file_size_ = 0; // total size of the rdb file, set in Open()
};
diff --git a/src/storage/rdb/rdb.cc b/src/storage/rdb/rdb.cc
index 5adf48f05..4d01bed3c 100644
--- a/src/storage/rdb/rdb.cc
+++ b/src/storage/rdb/rdb.cc
@@ -141,6 +141,15 @@ StatusOr<std::string> RDB::LoadStringObject() { return
loadEncodedString(); }
StatusOr<std::string> RDB::loadLzfString() {
auto compression_len = GET_OR_RET(loadObjectLen(nullptr));
auto len = GET_OR_RET(loadObjectLen(nullptr));
+ // The compressed bytes must actually be present in the stream.
+ GET_OR_RET(stream_->EnsureRemainingBytes(compression_len));
+ // The decompressed size can legitimately exceed the remaining stream (that's
+ // what compression does), so it can't be bounded by the stream; cap it
against
+ // proto-max-bulk-len so a tiny payload can't declare a huge output
allocation.
+ if (uint64_t max_len = storage_->GetConfig()->proto_max_bulk_len; len >
max_len) {
+ return {Status::NotOK, fmt::format("invalid RDB payload: required {} bytes
exceeds the proto-max-bulk-len limit {}",
+ len, max_len)};
+ }
std::string out_buf(len, 0);
std::vector<char> vec(compression_len);
GET_OR_RET(stream_->Read(vec.data(), compression_len));
@@ -180,6 +189,7 @@ StatusOr<std::string> RDB::loadEncodedString() {
if (len == 0) {
return "";
}
+ GET_OR_RET(stream_->EnsureRemainingBytes(len));
std::string read_string;
read_string.resize(len);
GET_OR_RET(stream_->Read(read_string.data(), len));
@@ -192,6 +202,7 @@ StatusOr<std::vector<std::string>>
RDB::LoadListWithQuickList(int type) {
if (len == 0) {
return list;
}
+ GET_OR_RET(stream_->EnsureRemainingBytes(len));
uint64_t container = QuickListNodeContainerPacked;
for (size_t i = 0; i < len; i++) {
@@ -228,6 +239,7 @@ StatusOr<std::vector<std::string>> RDB::LoadListObject() {
if (len == 0) {
return list;
}
+ GET_OR_RET(stream_->EnsureRemainingBytes(len));
for (size_t i = 0; i < len; i++) {
auto element = GET_OR_RET(loadEncodedString());
list.push_back(std::move(element));
@@ -247,6 +259,7 @@ StatusOr<std::vector<std::string>> RDB::LoadSetObject() {
if (len == 0) {
return set;
}
+ GET_OR_RET(stream_->EnsureRemainingBytes(len));
for (size_t i = 0; i < len; i++) {
auto element = GET_OR_RET(LoadStringObject());
set.push_back(std::move(element));
@@ -272,6 +285,7 @@ StatusOr<std::map<std::string, std::string>>
RDB::LoadHashObject() {
if (len == 0) {
return hash;
}
+ GET_OR_RET(stream_->EnsureRemainingBytes(len));
for (size_t i = 0; i < len; i++) {
auto field = GET_OR_RET(LoadStringObject());
@@ -344,6 +358,7 @@ StatusOr<std::vector<MemberScore>> RDB::LoadZSetObject(int
type) {
if (len == 0) {
return zset;
}
+ GET_OR_RET(stream_->EnsureRemainingBytes(len));
for (size_t i = 0; i < len; i++) {
auto member = GET_OR_RET(LoadStringObject());
diff --git a/tests/gocase/unit/restore/restore_test.go
b/tests/gocase/unit/restore/restore_test.go
index d92cc39a0..1c46e4a2c 100644
--- a/tests/gocase/unit/restore/restore_test.go
+++ b/tests/gocase/unit/restore/restore_test.go
@@ -22,7 +22,6 @@ package restore
import (
"context"
"encoding/binary"
- "hash/crc64"
"testing"
"time"
@@ -251,12 +250,6 @@ func TestRestore_Set(t *testing.T) {
}
func TestRestoreRejectsInvalidIntSetLength(t *testing.T) {
- // Redis CRC64 reflected polynomial, used by the DUMP/RESTORE payload
footer.
- redisCRC64Table := crc64.MakeTable(0x95ac9329ac4bc9b5)
- redisCRC64 := func(data []byte) uint64 {
- return ^crc64.Update(^uint64(0), redisCRC64Table, data)
- }
-
srv := util.StartServer(t, map[string]string{})
defer srv.Close()
@@ -271,7 +264,7 @@ func TestRestoreRejectsInvalidIntSetLength(t *testing.T) {
body = binary.LittleEndian.AppendUint32(body, 0x20000000)
// RDB version 11 followed by the Redis CRC64 checksum.
body = binary.LittleEndian.AppendUint16(body, 11)
- value := string(binary.LittleEndian.AppendUint64(body,
redisCRC64(body)))
+ value := string(binary.LittleEndian.AppendUint64(body,
util.RedisCRC64(body)))
restoreCtx, cancel := context.WithTimeout(ctx, 30*time.Second)
defer cancel()
@@ -280,6 +273,55 @@ func TestRestoreRejectsInvalidIntSetLength(t *testing.T) {
require.NoError(t, rdb.Ping(ctx).Err())
}
+func TestRestoreRejectsOversizedLength(t *testing.T) {
+ srv := util.StartServer(t, map[string]string{})
+ defer srv.Close()
+
+ ctx := context.Background()
+ rdb := srv.NewClient()
+ defer func() { require.NoError(t, rdb.Close()) }()
+
+ // RDBTypeString declaring a 1 TiB length with no actual string bytes.
+ body := []byte{0x00, 0x81} // RDBTypeString, RDB64BitLen
+ body = binary.BigEndian.AppendUint64(body, 1<<40)
+ body = binary.LittleEndian.AppendUint16(body, 11) // RDB version
+ value := string(binary.LittleEndian.AppendUint64(body,
util.RedisCRC64(body)))
+
+ restoreCtx, cancel := context.WithTimeout(ctx, 30*time.Second)
+ defer cancel()
+ require.ErrorContains(t, rdb.Restore(restoreCtx, util.RandString(32,
64, util.Alpha), 0, value).Err(),
+ "invalid RDB payload")
+ require.NoError(t, rdb.Ping(ctx).Err())
+}
+
+func TestRestoreRejectsOversizedLzfLength(t *testing.T) {
+ srv := util.StartServer(t, map[string]string{})
+ defer srv.Close()
+
+ ctx := context.Background()
+ rdb := srv.NewClient()
+ defer func() { require.NoError(t, rdb.Close()) }()
+
+ // LZF-encoded RDBTypeString: a 1-byte compressed body declaring a 1 TiB
+ // decompressed size, which the stream bound alone can't catch.
+ body := []byte{
+ 0x00, // RDBTypeString
+ 0xc3, // RDBEncVal | RDBEncLzf -> LZF-encoded string
+ 0x01, // compressed length = 1
+ 0x81, // RDB64BitLen marker for the decompressed length
+ }
+ body = binary.BigEndian.AppendUint64(body, 1<<40) // decompressed
length = 1 TiB
+ body = append(body, 0x00) // 1 byte of
compressed data
+ body = binary.LittleEndian.AppendUint16(body, 11) // RDB version
+ value := string(binary.LittleEndian.AppendUint64(body,
util.RedisCRC64(body)))
+
+ restoreCtx, cancel := context.WithTimeout(ctx, 30*time.Second)
+ defer cancel()
+ require.ErrorContains(t, rdb.Restore(restoreCtx, util.RandString(32,
64, util.Alpha), 0, value).Err(),
+ "invalid RDB payload")
+ require.NoError(t, rdb.Ping(ctx).Err())
+}
+
func TestRestoreWithTTL(t *testing.T) {
srv := util.StartServer(t, map[string]string{})
defer srv.Close()
diff --git a/tests/gocase/util/crc64.go b/tests/gocase/util/crc64.go
new file mode 100644
index 000000000..744915d9f
--- /dev/null
+++ b/tests/gocase/util/crc64.go
@@ -0,0 +1,32 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package util
+
+import "hash/crc64"
+
+// redisCRC64Table is the reflected CRC-64 (Jones) table Redis uses for the
+// DUMP/RESTORE payload footer.
+var redisCRC64Table = crc64.MakeTable(0x95ac9329ac4bc9b5)
+
+// RedisCRC64 computes the CRC-64 checksum that Redis appends to a DUMP/RESTORE
+// payload, matching the checksum kvrocks verifies when loading the payload.
+func RedisCRC64(data []byte) uint64 {
+ return ^crc64.Update(^uint64(0), redisCRC64Table, data)
+}
