Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix too large buffer size in VerifyStatusReport() #4765

Merged
merged 1 commit into from
Feb 9, 2021
Merged

Fix too large buffer size in VerifyStatusReport() #4765

merged 1 commit into from
Feb 9, 2021

Conversation

holbrookt
Copy link
Contributor

Problem

In TestBdxTransferSession.cpp, VerifyStatusReport(), a Reader object was being initialized with a length that exceeded the bounds of the PacketBuffer to read.

Summary of Changes

Subtract headerSize from the length parameter so that the Reader cannot read data beyond the bounds of the PacketBuffer

Fixes #4748

@mspang
Copy link
Contributor

mspang commented Feb 9, 2021

@kpschoedel

Can we do something like

Reader reader(msg)
reader.Skip(headerSize) ?

@kpschoedel
Copy link
Contributor

reader.Skip(headerSize) ?

BufferReader doesn't have Skip(), but it probably should.

@tcarmelveilleux
Copy link
Contributor

tcarmelveilleux commented Feb 9, 2021 •
edited
Loading

reader.Skip(headerSize) ?

BufferReader doesn't have Skip(), but it probably should.

I strongly feel like adding Skip()/Reset() is a root-cause fix that could avoid the risky size-arithmetic that is being introduced as part of the fix.

@holbrookt
Copy link
Contributor Author

reader.Skip(headerSize) ?

BufferReader doesn't have Skip(), but it probably should.

I strongly feel like adding Skip()/Reset() is a root-cause fix that could avoid the risky size-arithmetic that is being introduced as part of the fix.

I'm happy to add this as part of this PR

@mspang
Copy link
Contributor

mspang commented Feb 9, 2021

reader.Skip(headerSize) ?

BufferReader doesn't have Skip(), but it probably should.

I strongly feel like adding Skip()/Reset() is a root-cause fix that could avoid the risky size-arithmetic that is being introduced as part of the fix.

I'm happy to add this as part of this PR

Yes, please.

bzbarsky-apple
bzbarsky-apple approved these changes Feb 9, 2021
View reviewed changes
src/protocols/bdx/tests/TestBdxTransferSession.cpp
@@ -129,8 +129,13 @@ void VerifyStatusReport(nlTestSuite * inSuite, void * inContext, const System::P
NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
NL_TEST_ASSERT(inSuite, payloadHeader.GetProtocolID() == Protocols::kProtocol_Protocol_Common);
NL_TEST_ASSERT(inSuite, payloadHeader.GetMessageType() == static_cast<uint8_t>(Protocols::Common::MsgType::StatusReport));
if (headerSize > msg->DataLength())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In theory, this should have caused err to be an error.... I'm OK with keeping it, or ok with a comment saying we can assume headerSize is small enough because err == CHIP_NO_ERROR here.

Certainly non-test code makes that assumption.

src/protocols/bdx/tests/TestBdxTransferSession.cpp

Encoding::LittleEndian::Reader reader(msg->Start() + headerSize, msg->DataLength());
Encoding::LittleEndian::Reader reader(msg->Start() + headerSize, static_cast<uint16_t>(msg->DataLength() - headerSize));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably OK if we need to not change the state of msg. Otherwise we could msg->Consume(headerSize) or something and then ditch the arithmetic.

@andy31415 andy31415 merged commit ad2f20c into project-chip:master Feb 9, 2021
@holbrookt holbrookt mentioned this pull request Feb 9, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andy31415 andy31415 andy31415 approved these changes

@saurabhst saurabhst saurabhst approved these changes

@bzbarsky-apple bzbarsky-apple bzbarsky-apple approved these changes

@tcarmelveilleux tcarmelveilleux Awaiting requested review from tcarmelveilleux

@BroderickCarlin BroderickCarlin Awaiting requested review from BroderickCarlin

@chrisdecenzo chrisdecenzo Awaiting requested review from chrisdecenzo

@hawk248 hawk248 Awaiting requested review from hawk248

@jelderton jelderton Awaiting requested review from jelderton

@mspang mspang Awaiting requested review from mspang

@woody-apple woody-apple Awaiting requested review from woody-apple

@kpschoedel kpschoedel Awaiting requested review from kpschoedel

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

TestBdxTransferSession tries to read out of packetbuffer bounds
7 participants
@holbrookt @mspang @kpschoedel @tcarmelveilleux @andy31415 @saurabhst @bzbarsky-apple