Validate CRC32C checksum values before decoding

src/brpc/policy/crc32c_checksum.cpp

@@ -17,6 +17,8 @@
 
 #include "brpc/policy/crc32c_checksum.h"
 
+#include <cstring>
+
 #include "brpc/details/controller_private_accessor.h"
 #include "brpc/log.h"
 #include "butil/crc32c.h"
@@ -61,12 +63,15 @@ void Crc32cCompute(const ChecksumIn& in) {
 }
 
 bool Crc32cVerify(const ChecksumIn& in) {
-    uint32_t crc = ComputeCrc32c(in);
     auto& val = ControllerPrivateAccessor(const_cast<Controller*>(in.cntl))
                     .checksum_value();
-    CHECK_EQ(val.size(), sizeof(crc));
-    auto expected = *reinterpret_cast<const uint32_t*>(val.data());
+    uint32_t expected;
+    if (val.size() != sizeof(expected)) {
+        return false;
+    }
+    memcpy(&expected, val.data(), sizeof(expected));
     expected = butil::crc32c::Unmask(butil::NetToHost32(expected));
+    const uint32_t crc = ComputeCrc32c(in);
     RPC_VLOG << "Crc32cVerify crc=" << crc << " expected=" << expected;
     return crc == expected;
 }

test/brpc_checksum_unittest.cpp

@@ -27,6 +27,7 @@
 #include "brpc/details/controller_private_accessor.h"
 #include "brpc/policy/crc32c_checksum.h"
 #include "butil/iobuf.h"
+#include "butil/memory/scope_guard.h"
 #include "echo.pb.h"
 
 namespace {
@@ -50,6 +51,34 @@ TEST_F(ChecksumAttachmentTest, verify_succeeds_when_body_only) {
     EXPECT_TRUE(brpc::policy::Crc32cVerify(verify_in));
 }
 
+TEST_F(ChecksumAttachmentTest, verify_rejects_invalid_checksum_lengths) {
+    brpc::Controller cntl;
+    butil::IOBuf body;
+    body.append("request body");
+    brpc::ChecksumIn in{&body, &cntl, nullptr};
+    brpc::policy::Crc32cCompute(in);
+    brpc::ControllerPrivateAccessor accessor(&cntl);
+    const std::string valid = accessor.checksum_value();
+    ASSERT_EQ(sizeof(uint32_t), valid.size());
+
+    logging::StringSink logs;
+    {
+        logging::LogSink* previous = logging::SetLogSink(&logs);
+        BUTIL_SCOPE_EXIT { logging::SetLogSink(previous); };
+        for (size_t size = 0; size <= 8; ++size) {
+            if (size == valid.size()) {
+                continue;
+            }
+            const std::string invalid(size, '\0');
+            accessor.set_checksum_value(invalid.data(), invalid.size());
+            EXPECT_FALSE(brpc::policy::Crc32cVerify(in)) << "size=" << size;
+        }
+        accessor.set_checksum_value(valid.data(), valid.size());
+        EXPECT_TRUE(brpc::policy::Crc32cVerify(in));
+    }
+    EXPECT_EQ(std::string::npos, logs.find("Check failed: val.size()"));
+}
+
 TEST_F(ChecksumAttachmentTest, verify_succeeds_when_attachment_included) {
     brpc::Controller cntl;
     butil::IOBuf body;