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

CASE allocates memory based on wire-provided sizes without sound bounds #8924

Closed
tcarmelveilleux opened this issue Aug 12, 2021 · 3 comments · Fixed by #17296
Closed

CASE allocates memory based on wire-provided sizes without sound bounds #8924

tcarmelveilleux opened this issue Aug 12, 2021 · 3 comments · Fixed by #17296
Assignees
@tcarmelveilleux
Labels
memory secure channel security V1.0

Comments

@tcarmelveilleux
Copy link
Contributor

Problem

In CASESession.cpp, there exists several usage of the following pattern:

VerifyOrExit(msg_R2_Encrypted.Alloc(tlvReader.GetLength()), err = CHIP_ERROR_NO_MEMORY);

This assumes in many places that tlvReader.GetLength() is not a huge value (or contextually large value), even though only certain values are permissible.

Proposed Solution

All usage of tlvReader.GetLength() for allocations should be audited and each one should be validated against a valid upper bound for the size of the value being read, within practical limits.
@tcarmelveilleux tcarmelveilleux self-assigned this Aug 12, 2021
tcarmelveilleux added a commit to tcarmelveilleux/connectedhomeip that referenced this issue Aug 12, 2021
@tcarmelveilleux
Properly set CASE salt span sizes and CASE cleanups
b5f61df 
- Caller and callee code for Sigma2/Sigma3 salt
  generation need to stay in sync for correct
  usage. If salt buffer grows, but output doesn't
  it can cause garbage data to be used since the
  salt output MutableByteSpan is assumed correct-sized
  rather than ensured to be of correct size.
- This change makes use of the computed final size
  to make sure it stays in sync.
- This change also cleans-up:
  - Random value buffers on heap now on stack
  - Opcert buffers reduced on stack
  - No longer trusting some sizes on wire where
    a max buffer size on stack is used
  - De-uint16-ify several variables
  - Reduce scope of HKDF for SR2K/SR3K
  - Remove some superfluous size variables

Testing done: cert tests and unit tests run successfully

Fixes: project-chip#8913
Issue: project-chip#8924
@tcarmelveilleux tcarmelveilleux mentioned this issue Aug 12, 2021
Merged
andy31415 pushed a commit that referenced this issue Aug 13, 2021
@tcarmelveilleux @restyled-commits
Properly set CASE salt span sizes and CASE cleanups (#8929)
13e94ef 
* Properly set CASE salt span sizes and CASE cleanups

- Caller and callee code for Sigma2/Sigma3 salt
  generation need to stay in sync for correct
  usage. If salt buffer grows, but output doesn't
  it can cause garbage data to be used since the
  salt output MutableByteSpan is assumed correct-sized
  rather than ensured to be of correct size.
- This change makes use of the computed final size
  to make sure it stays in sync.
- This change also cleans-up:
  - Random value buffers on heap now on stack
  - Opcert buffers reduced on stack
  - No longer trusting some sizes on wire where
    a max buffer size on stack is used
  - De-uint16-ify several variables
  - Reduce scope of HKDF for SR2K/SR3K
  - Remove some superfluous size variables

Testing done: cert tests and unit tests run successfully

Fixes: #8913
Issue: #8924

* Restyled by clang-format

* Restyled by clang-format

* Add comment per @msandstedt

Co-authored-by: Restyled.io <[email protected]>
@bzbarsky-apple bzbarsky-apple mentioned this issue Aug 13, 2021
Closed
nikita-s-wrk pushed a commit to nikita-s-wrk/connectedhomeip that referenced this issue Sep 23, 2021
@tcarmelveilleux @restyled-commits
Properly set CASE salt span sizes and CASE cleanups (project-chip#8929)
2321ded 
* Properly set CASE salt span sizes and CASE cleanups

- Caller and callee code for Sigma2/Sigma3 salt
  generation need to stay in sync for correct
  usage. If salt buffer grows, but output doesn't
  it can cause garbage data to be used since the
  salt output MutableByteSpan is assumed correct-sized
  rather than ensured to be of correct size.
- This change makes use of the computed final size
  to make sure it stays in sync.
- This change also cleans-up:
  - Random value buffers on heap now on stack
  - Opcert buffers reduced on stack
  - No longer trusting some sizes on wire where
    a max buffer size on stack is used
  - De-uint16-ify several variables
  - Reduce scope of HKDF for SR2K/SR3K
  - Remove some superfluous size variables

Testing done: cert tests and unit tests run successfully

Fixes: project-chip#8913
Issue: project-chip#8924

* Restyled by clang-format

* Restyled by clang-format

* Add comment per @msandstedt

Co-authored-by: Restyled.io <[email protected]>
@holbrookt
Copy link
Contributor

@tcarmelveilleux I see that a few PRs were merged referencing this. Is the issue fixed?

@woody-apple
Copy link
Contributor

Marking as closed, please re-open if still an issue.

@bzbarsky-apple
Copy link
Contributor

This is still an issue, as far as I can see. For example, CASESession::HandleSigma2 still does this:

    SuccessOrExit(err = tlvReader.Next());
    VerifyOrExit(TLV::TagNumFromTag(tlvReader.GetTag()) == ++decodeTagIdSeq, err = CHIP_ERROR_INVALID_TLV_TAG);
    VerifyOrExit(msg_R2_Encrypted.Alloc(tlvReader.GetLength()), err = CHIP_ERROR_NO_MEMORY);

tcarmelveilleux added a commit to tcarmelveilleux/connectedhomeip that referenced this issue Apr 12, 2022
@tcarmelveilleux
Stop allocating CASE buffer based on wire sizes
abe32d7 
- CASE was using direct lengths of TLV values to allocate heap values,
  without validations.
- Added validations on lengths of Sigma2/3 TBE, with some room to grow
- Updated tag order validation logic to be more explicit and easier to
  read.

Fixes project-chip#8924

Test done:
- Cert tests still pass
- Unit tests still pass
tcarmelveilleux added a commit to tcarmelveilleux/connectedhomeip that referenced this issue Apr 12, 2022
@tcarmelveilleux
Stop allocating CASE buffer based on wire sizes
5c5a308 
- CASE was using direct lengths of TLV values to allocate heap values,
  without validations.
- Added validations on lengths of Sigma2/3 TBE, with some room to grow
- Updated tag order validation logic to be more explicit and easier to
  read.

Fixes project-chip#8924

Test done:
- Cert tests still pass
- Unit tests still pass
@tcarmelveilleux tcarmelveilleux mentioned this issue Apr 12, 2022
Merged
tcarmelveilleux added a commit that referenced this issue Apr 13, 2022
@tcarmelveilleux @restyled-commits
Stop allocating CASE buffer based on wire sizes (#17296)
213869f 
* Stop allocating CASE buffer based on wire sizes

- CASE was using direct lengths of TLV values to allocate heap values,
  without validations.
- Added validations on lengths of Sigma2/3 TBE, with some room to grow
- Updated tag order validation logic to be more explicit and easier to
  read.

Fixes #8924

Test done:
- Cert tests still pass
- Unit tests still pass

* Restyled by clang-format

Co-authored-by: Restyled.io <[email protected]>
andrei-menzopol pushed a commit to andrei-menzopol/connectedhomeip that referenced this issue Apr 14, 2022
@tcarmelveilleux @restyled-commits @andrei-menzopol
Stop allocating CASE buffer based on wire sizes (project-chip#17296)
8f613f6 
* Stop allocating CASE buffer based on wire sizes

- CASE was using direct lengths of TLV values to allocate heap values,
  without validations.
- Added validations on lengths of Sigma2/3 TBE, with some room to grow
- Updated tag order validation logic to be more explicit and easier to
  read.

Fixes project-chip#8924

Test done:
- Cert tests still pass
- Unit tests still pass

* Restyled by clang-format

Co-authored-by: Restyled.io <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@tcarmelveilleux tcarmelveilleux

Labels
memory secure channel security V1.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Stop allocating CASE buffer based on wire sizes tcarmelveilleux/connectedhomeip
5 participants
@tcarmelveilleux @andy31415 @holbrookt @woody-apple @bzbarsky-apple