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

retrieveOptionalInfoString uses untrusted length for allocation #9009

Closed
bzbarsky-apple opened this issue Aug 13, 2021 · 0 comments · Fixed by #9249
Closed

retrieveOptionalInfoString uses untrusted length for allocation #9009

bzbarsky-apple opened this issue Aug 13, 2021 · 0 comments · Fixed by #9249
Assignees
@bzbarsky-apple

Comments

@bzbarsky-apple
Copy link
Contributor

Problem

retrieveOptionalInfoString does:

    const uint32_t valLength = reader.GetLength();
    chip::Platform::ScopedMemoryBuffer<char> value;
    value.Alloc(valLength + 1);

Proposed Solution

Don't even allocate. We're about to copy into an std::string anyway. Just grab the pointer to the in-tlv data (which will do length-verification at least to the extent of it not overflowing the tlv data itself) and use the 2-arg form of std::string::assign.
@bzbarsky-apple bzbarsky-apple self-assigned this Aug 13, 2021
bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Aug 25, 2021
@bzbarsky-apple
Add non-copying string and byte getters on a TLV reader.
d3382c6 
We have existing callsites that are doing what GetString/GetBytes say to do and call GetLength() to decide how big a buffer to allocate.  But the return value from GetLength() doesn't have to be sane, so we end up allocating buffers with attacker-controlled sizes.

The changes here are:

1) Add ContiguousBufferTLVReader that is always backed by a single
   buffer, so it can guarantee that the in-place view makes sense.

2) Use the new class in setup payload parsers, fixing various bugs in
   the process.

Fixes project-chip#9009
@bzbarsky-apple bzbarsky-apple mentioned this issue Aug 25, 2021
Merged
bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Aug 25, 2021
@bzbarsky-apple
Add non-copying string and byte getters on a TLV reader.
77d19a3 
We have existing callsites that are doing what GetString/GetBytes say to do and call GetLength() to decide how big a buffer to allocate.  But the return value from GetLength() doesn't have to be sane, so we end up allocating buffers with attacker-controlled sizes.

The changes here are:

1) Add ContiguousBufferTLVReader that is always backed by a single
   buffer, so it can guarantee that the in-place view makes sense.

2) Use the new class in setup payload parsers, fixing various bugs in
   the process.

Fixes project-chip#9009
bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Aug 26, 2021
@bzbarsky-apple
Add non-copying string and byte getters on a TLV reader.
3292d8a 
We have existing callsites that are doing what GetString/GetBytes say to do and call GetLength() to decide how big a buffer to allocate.  But the return value from GetLength() doesn't have to be sane, so we end up allocating buffers with attacker-controlled sizes.

The changes here are:

1) Add ContiguousBufferTLVReader that is always backed by a single
   buffer, so it can guarantee that the in-place view makes sense.

2) Use the new class in setup payload parsers, fixing various bugs in
   the process.

Fixes project-chip#9009
bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Aug 26, 2021
@bzbarsky-apple
Add non-copying string and byte getters on a TLV reader.
1b2393a 
We have existing callsites that are doing what GetString/GetBytes say to do and call GetLength() to decide how big a buffer to allocate.  But the return value from GetLength() doesn't have to be sane, so we end up allocating buffers with attacker-controlled sizes.

The changes here are:

1) Add ContiguousBufferTLVReader that is always backed by a single
   buffer, so it can guarantee that the in-place view makes sense.

2) Use the new class in setup payload parsers, fixing various bugs in
   the process.

Fixes project-chip#9009
bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Aug 26, 2021
@bzbarsky-apple
Add non-copying string and byte getters on a TLV reader.
578751c 
We have existing callsites that are doing what GetString/GetBytes say to do and call GetLength() to decide how big a buffer to allocate.  But the return value from GetLength() doesn't have to be sane, so we end up allocating buffers with attacker-controlled sizes.

The changes here are:

1) Add ContiguousBufferTLVReader that is always backed by a single
   buffer, so it can guarantee that the in-place view makes sense.

2) Use the new class in setup payload parsers, fixing various bugs in
   the process.

Fixes project-chip#9009
bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Aug 26, 2021
@bzbarsky-apple
Add non-copying string and byte getters on a TLV reader.
85ec9cc 
We have existing callsites that are doing what GetString/GetBytes say to do and call GetLength() to decide how big a buffer to allocate.  But the return value from GetLength() doesn't have to be sane, so we end up allocating buffers with attacker-controlled sizes.

The changes here are:

1) Add ContiguousBufferTLVReader that is always backed by a single
   buffer, so it can guarantee that the in-place view makes sense.

2) Use the new class in setup payload parsers, fixing various bugs in
   the process.

Fixes project-chip#9009
bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Aug 27, 2021
@bzbarsky-apple
Add non-copying string and byte getters on a TLV reader.
388cbed 
We have existing callsites that are doing what GetString/GetBytes say to do and call GetLength() to decide how big a buffer to allocate.  But the return value from GetLength() doesn't have to be sane, so we end up allocating buffers with attacker-controlled sizes.

The changes here are:

1) Add ContiguousBufferTLVReader that is always backed by a single
   buffer, so it can guarantee that the in-place view makes sense.

2) Use the new class in setup payload parsers, fixing various bugs in
   the process.

Fixes project-chip#9009
bzbarsky-apple added a commit that referenced this issue Aug 27, 2021
@bzbarsky-apple
Add non-copying string and byte getters on a TLV reader. (#9249)
f4c307f 
* Add non-copying string and byte getters on a TLV reader.

We have existing callsites that are doing what GetString/GetBytes say to do and call GetLength() to decide how big a buffer to allocate.  But the return value from GetLength() doesn't have to be sane, so we end up allocating buffers with attacker-controlled sizes.

The changes here are:

1) Add ContiguousBufferTLVReader that is always backed by a single
   buffer, so it can guarantee that the in-place view makes sense.

2) Use the new class in setup payload parsers, fixing various bugs in
   the process.

Fixes #9009

* Addressing review comments

* More review comments

* Bump Darwin build job bootstrap timeout to match other Darwin jobs.

10 minutes keeps timing out.
mkardous-silabs pushed a commit to mkardous-silabs/connectedhomeip that referenced this issue Sep 24, 2021
@bzbarsky-apple @mkardous-silabs
Add non-copying string and byte getters on a TLV reader. (project-chi…
17790c8 
…p#9249)

* Add non-copying string and byte getters on a TLV reader.

We have existing callsites that are doing what GetString/GetBytes say to do and call GetLength() to decide how big a buffer to allocate.  But the return value from GetLength() doesn't have to be sane, so we end up allocating buffers with attacker-controlled sizes.

The changes here are:

1) Add ContiguousBufferTLVReader that is always backed by a single
   buffer, so it can guarantee that the in-place view makes sense.

2) Use the new class in setup payload parsers, fixing various bugs in
   the process.

Fixes project-chip#9009

* Addressing review comments

* More review comments

* Bump Darwin build job bootstrap timeout to match other Darwin jobs.

10 minutes keeps timing out.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@bzbarsky-apple bzbarsky-apple

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add non-copying string and byte getters on a TLV reader. bzbarsky-apple/connectedhomeip
1 participant
@bzbarsky-apple