[BUG] one unsigned-integer-overflow bug

[agatah2333]

Reproduction steps

Issue Summary

A unsigned integer overflow bug was discovered in the Matter SDK while running the Self-modify program @Chapoly1305. The bug manifests when the mCurReadHandlerIdx counter reaches 0 and attempts to decrement, causing undefined behavior.

Environment

• Location: Matter SDK (ConnectedHomeIP)
• File: src/app/reporting/Engine.h
• Line: 117

Crash Analysis

The crash was detected by UndefinedBehaviorSanitizer (UBSAN) with the following error:

runtime error: unsigned integer overflow: 0 - 1 cannot be represented in type 'uint32_t'

Stack Trace Analysis

Key call stack elements:

chip::app::reporting::Engine::ResetReadHandlerTracker()
chip::app::InteractionModelEngine::OnDone()
chip::app::ReadHandler::Close()
chip::app::reporting::Engine::BuildAndSendSingleReportData()
chip::app::reporting::Engine::Run()

Root Cause

The bug occurs in the Engine::ResetReadHandlerTracker method when attempting to decrement mCurReadHandlerIdx which is an unsigned integer (uint32_t). When the value is 0, the decrement operation causes an overflow.

Problematic Code

// In Engine.h, line 117
--mCurReadHandlerIdx;

The issue arises because:

1. mCurReadHandlerIdx is declared as uint32_t
2. No bounds checking is performed before decrement
3. The counter can reach 0 during normal operation
4. Attempting to decrement 0 causes undefined behavior for unsigned integers

Impact

This bug can lead to:

1. Program crashes due to UBSAN detection
2. Potential memory corruption if UBSAN is not enabled
3. Inconsistent state in the reporting engine
4. Possible security implications due to integer overflow

Proposed Solutions

Short-term Fix

1. Add bounds checking before decrementing:

if (mCurReadHandlerIdx > 0) {
    --mCurReadHandlerIdx;
}

• Matter SDK Issue:

  connectedhomeip/src/app/reporting/Engine.h

  Line 117 in 9c8a552

  --mCurReadHandlerIdx;

Bug prevalence

each time

GitHub hash of the SDK that was being used

561d23d

Platform

core

Platform Version(s)

all versions

Anything else?

No response

[bzbarsky-apple]

and attempts to decrement, causing undefined behavior.

@agatah2333 How is this undefined behavior? Decrement for an unsigned type past 0 is very much defined by the C++ standard, last I checked. It turns into the max representable value of the type.

And since all uses of mCurReadHandlerIdx are either mod mReadHandlers.Allocated() or clamped to that value, there is no security issue.

This seems like a false positive, in terms of being a security issue, on part of UBSan (and it's odd that it claims undefined behavior when behavior is in fact defined....)

[agatah2333]

Thanks for your reply! When mCurReadHandlerIdx is zero, executing --mCurReadHandlerIdx; results in an unsigned integer underflow, setting its value to the maximum representable by uint32_t. While this behavior is defined in C++, it can lead to logical errors or unexpected behaviors, especially in indexing or counting contexts.

We reported this mCurReadHandlerIdx issue after encountering the "Unsigned Integer Overflow" in chip-all-clusters-app-fuzzing within chip::Encoding::BigEndian::BufferWriter::EndianPut(this=0x000055555ac33290, x=12, size=0) at BufferWriter.cpp:81:16. This issue has been patched by #35580, thanks to @Alami-Amine. Could you please let me know why this bug was patched in Matter 1.4 but not backported to Matter 1.3, and why it was considered necessary to patch this code? UBSan almost confused us as it reported so many positives.

[Alami-Amine]

Hello @agatah2333
The following quote in LLVM's UndefinedBehaviorSanitizer page states that "unsigned integer overflow" is NOT undefined.

-fsanitize=unsigned-integer-overflow: Unsigned integer overflow, where the result of an unsigned integer computation cannot be represented in its type. Unlike signed integer overflow, this is not undefined behavior, but it is often unintentional. This sanitizer does not check for lossy implicit conversions performed before such a computation (see -fsanitize=implicit-integer-conversion).

• Although I agree that it shouldn't make it into the code and should be fixed. However, I am not sure if it should be backported.

[andy31415]

I believe our change was to be able to enable UBSAN without what we consider "false positives" showing up. We have identified no security issue or bug due to this code.

@agatah2333 if you have a concrete example for this bug happening (as an exploit) please share it (probably via a CVE process because security issue). Otherwise wrapping behaviour on decrement is well defined behavior.