Skip to content

Commit

Permalink
Patch unsafe strlen in TLVWriter (#37065)
Browse files Browse the repository at this point in the history
* Patch unsafe strlen in TLVWriter

This is a vulnerability from Weave that was recently fixed. Apply
the patch to Matter TLVWriter as well. This avoids reading bad
pointers beyond stack-allocated memory.

> One of the PutString function overloads makes a call to strlen
> without safeguards. This has caused faults on several products when
> passing in uninitialized memory. While these call sites have been
> fixed with explicit initialization, we can also make the core
> library more secure. Use the container to determine a maximum length
> and avoid buffer overflow.

* Fix comment and apply clang-format

* Fixes for clang-tidy
  • Loading branch information
alexhqwang authored and pull[bot] committed Jan 24, 2025
1 parent 91673ab commit 2357657
Show file tree
Hide file tree
Showing 2 changed files with 34 additions and 4 deletions.
22 changes: 18 additions & 4 deletions src/lib/core/TLVWriter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -290,12 +290,26 @@ CHIP_ERROR TLVWriter::PutBytes(Tag tag, const uint8_t * buf, uint32_t len)

CHIP_ERROR TLVWriter::PutString(Tag tag, const char * buf)
{
size_t len = strlen(buf);
if (buf == nullptr)
return CHIP_ERROR_INVALID_ARGUMENT;
if (mMaxLen == 0)
return CHIP_ERROR_INCORRECT_STATE;

// Calculate length with a hard limit to prevent unbounded reads.
// Use mMaxLen instead of mRemainingLen to account for CircularTLVWriter.
// Note: Overrun is still possible if buf is not null-terminated, and this
// check cannot prevent all invalid memory reads.
size_t len = strnlen(buf, mMaxLen);

if (!CanCastTo<uint32_t>(len))
{
return CHIP_ERROR_INVALID_ARGUMENT;
}
return PutString(tag, buf, static_cast<uint32_t>(len));

uint32_t stringLen = static_cast<uint32_t>(len);

// Null terminator was not found within the allocated space.
if (stringLen == mMaxLen)
return CHIP_ERROR_BUFFER_TOO_SMALL;
return PutString(tag, buf, stringLen);
}

CHIP_ERROR TLVWriter::PutString(Tag tag, const char * buf, uint32_t len)
Expand Down
16 changes: 16 additions & 0 deletions src/lib/core/tests/TestTLV.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2605,6 +2605,22 @@ TEST_F(TestTLV, CheckCircularTLVBufferEdge)

TestEnd<TLVReader>(reader);
}

TEST_F(TestTLV, CheckTLVPutStringOverrun)
{
const size_t bufSize = 8;
uint8_t backingStore[bufSize] = {};

const char * badPointer = "Foo <segfault here>";

TLVWriter writer;

writer.Init(backingStore, bufSize);

CHIP_ERROR err = writer.PutString(ProfileTag(TestProfile_1, 1), badPointer);
EXPECT_EQ(err, CHIP_ERROR_BUFFER_TOO_SMALL);
}

TEST_F(TestTLV, CheckTLVPutStringF)
{
const size_t bufsize = 24;
Expand Down

0 comments on commit 2357657

Please sign in to comment.