commit 6fd03fbf98ffe6d7713cc9fa1fe9ef8c34514163
author Kasun Athukorala <kasunath@google.com> Tue Jul 15 05:19:39 2025 +0000
committer Kasun Athukorala <kasunath@google.com> Tue Jul 15 17:20:45 2025 +0000
tree b48d2ff73542fb5844393b39d905a8d961ae17a6
parent 485044b19c85e4c50df34fd508d49838c0f5ee67

Check SFLV tuple is within stream before decoding

If the SFLV tuple being decoded goes out of the stream length
it could lead to a heap overflow. This patch adds a check to
prevent it.

Tested:
Unit tested

Signed-off-by: Kasun Athukorala <kasunath@google.com>
Change-Id: Ie018d4f3614603efc6ed8b93fa0077c612d9b481

src/bej_decoder_core.c

diff --git a/src/bej_decoder_core.c b/src/bej_decoder_core.c
index 2536ed1..43f5b69 100644
--- a/src/bej_decoder_core.c
+++ b/src/bej_decoder_core.c
@@ -90,7 +90,7 @@
  * params->state.encodedSubStream pointing to the start of the encoded stream
  * and params->state.encodedStreamOffset pointing to the current bejTuple.
  */
-static void bejInitSFLVStruct(struct BejHandleTypeFuncParam* params)
+static bool bejInitSFLVStruct(struct BejHandleTypeFuncParam* params)
 {
     struct BejSFLVOffset localOffset;
     // Get offsets of different SFLV fields with respect to start of the encoded
@@ -109,9 +109,30 @@
     sflv->format = *(struct BejTupleF*)(params->state.encodedSubStream +
                                         localOffset.formatOffset);
     sflv->valueLength = valueLength;
-    sflv->valueEndOffset = params->state.encodedStreamOffset +
-                           localOffset.valueOffset + valueLength;
+
+    if ((UINT32_MAX - localOffset.valueOffset) <
+        params->state.encodedStreamOffset)
+    {
+        fprintf(stderr,
+                "Overflow when adding encodedStreamOffset and valueOffset\n");
+        return false;
+    }
+
+    uint32_t valueStartLocation =
+        params->state.encodedStreamOffset + localOffset.valueOffset;
+
+    if ((UINT32_MAX - valueStartLocation) < valueLength)
+    {
+        fprintf(
+            stderr,
+            "Overflow when adding valueLength to encodedStreamOffset + valueOffset\n");
+        return false;
+    }
+
+    // Offset to the location soon after the value
+    sflv->valueEndOffset = valueStartLocation + valueLength;
     sflv->value = params->state.encodedSubStream + localOffset.valueOffset;
+    return true;
 }
 
 /**
@@ -733,7 +754,21 @@
         // Go to the next encoded segment in the encoded stream.
         params.state.encodedSubStream =
             enStream + params.state.encodedStreamOffset;
-        bejInitSFLVStruct(&params);
+        if (!bejInitSFLVStruct(&params))
+        {
+            return bejErrorInvalidSize;
+        }
+
+        // Make sure that the next value segment (SFLV) is within the streamLen
+        if (params.sflv.valueEndOffset > streamLen)
+        {
+            fprintf(
+                stderr,
+                "Value goes beyond stream length. SFLV Offset: %u, valueEndOffset: %u, streamLen: %u\n",
+                params.state.encodedStreamOffset, params.sflv.valueEndOffset,
+                streamLen);
+            return bejErrorInvalidSize;
+        }
 
         if (params.sflv.format.readOnlyProperty)
         {

test/bej_decoder_test.cpp

diff --git a/test/bej_decoder_test.cpp b/test/bej_decoder_test.cpp
index 7869dfb..cf76d2b 100644
--- a/test/bej_decoder_test.cpp
+++ b/test/bej_decoder_test.cpp
@@ -82,6 +82,25 @@
     EXPECT_TRUE(jsonDecoded.dump() == inputsOrErr->expectedJson.dump());
 }
 
+/**
+ * TODO: Add more test cases.
+ * - Test Enums inside array elemets
+ * - Array inside an array: is this a valid case?
+ * - Real numbers with exponent part
+ * - Every type inside an array.
+ */
+INSTANTIATE_TEST_SUITE_P(
+    , BejDecoderTest,
+    testing::ValuesIn<BejDecoderTestParams>({
+        {"DriveOEM", driveOemTestFiles},
+        {"Circuit", circuitTestFiles},
+        {"Storage", storageTestFiles},
+        {"DummySimple", dummySimpleTestFiles},
+    }),
+    [](const testing::TestParamInfo<BejDecoderTest::ParamType>& info) {
+        return info.param.testName;
+    });
+
 TEST(BejDecoderSecurityTest, MaxOperationsLimit)
 {
     auto inputsOrErr = loadInputs(dummySimpleTestFiles);
@@ -232,23 +251,35 @@
                 bejErrorInvalidSize);
 }
 
-/**
- * TODO: Add more test cases.
- * - Test Enums inside array elemets
- * - Array inside an array: is this a valid case?
- * - Real numbers with exponent part
- * - Every type inside an array.
- */
-INSTANTIATE_TEST_SUITE_P(
-    , BejDecoderTest,
-    testing::ValuesIn<BejDecoderTestParams>({
-        {"DriveOEM", driveOemTestFiles},
-        {"Circuit", circuitTestFiles},
-        {"Storage", storageTestFiles},
-        {"DummySimple", dummySimpleTestFiles},
-    }),
-    [](const testing::TestParamInfo<BejDecoderTest::ParamType>& info) {
-        return info.param.testName;
-    });
+TEST(BejDecoderSecurityTest, ValueBeyondStreamLength)
+{
+    auto inputsOrErr = loadInputs(dummySimpleTestFiles);
+    ASSERT_TRUE(inputsOrErr);
+
+    BejDictionaries dictionaries = {
+        .schemaDictionary = inputsOrErr->schemaDictionary,
+        .annotationDictionary = inputsOrErr->annotationDictionary,
+        .errorDictionary = inputsOrErr->errorDictionary,
+    };
+
+    auto root = std::make_unique<RedfishPropertyParent>();
+    bejTreeInitSet(root.get(), "DummySimple");
+
+    auto intProp = std::make_unique<RedfishPropertyLeafInt>();
+    bejTreeAddInteger(root.get(), intProp.get(), "SampleIntegerProperty", 123);
+
+    libbej::BejEncoderJson encoder;
+    encoder.encode(&dictionaries, bejMajorSchemaClass, root.get());
+    std::vector<uint8_t> outputBuffer = encoder.getOutput();
+
+    // Tamper with the encoded stream to simulate a value extending beyond the
+    // stream length. This stream only has an integer 0x7b. The value before it
+    // is the length tuple.
+    outputBuffer[outputBuffer.size() - 2] = 0x05;
+
+    BejDecoderJson decoder;
+    EXPECT_THAT(decoder.decode(dictionaries, std::span(outputBuffer)),
+                bejErrorInvalidSize);
+}
 
 } // namespace libbej