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.

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

decoder: add slice len bounds checks #6

Merged
merged 1 commit into from Aug 20, 2022

Conversation

riptl
Copy link
Contributor

@riptl riptl commented Aug 20, 2022

The decoder performs memory allocations based on untrusted length inputs while decoding slices.

This patch adds an upper constraint to the slice element count before calling runtime.MakeSlice in decoder.

It also fixes an integer overflow bug that could result in negative slice lengths.

Rationale: We assume each slice element is at least 1 byte large 1 Thus, for a valid encoding, the number of bytes remaining must be greater or equal to the number of slice elements. If there are less bytes remaining than the slice element count indicates, we would eventually try to read past the buffer and trip on io.ErrUnexpectedEOF. Thus we can safely bail early without actually deserializing slice elements.

1 A slice element can reasonably only be zero bytes if all other elements are also zero bytes (ignoring custom UnmarshalWithDecoder). Zero size slices are rare and should be represented as a single varint instead anyways.

Fixes gagliardetto/solana-go#95

@gagliardetto
Copy link
Owner

Inside decoder_test.go on line 602, the line should be assert.EqualError(t, err, "unexpected EOF") (because the erro changed).

@gagliardetto
Copy link
Owner

What about also adding some size evaluation to the check?

Negative Example:

  • readLen: 1 million
  • check if there's at least 1 million bytes left: true, there is at least that amount remaining.
  • allocate a slice of 1 million structs.

@gagliardetto
Copy link
Owner

Or maybe there's an "append" in reflect that would avoid preallocating?

@riptl
Copy link
Contributor Author

riptl commented Aug 20, 2022

@gagliardetto Thanks, I fixed the test. I also found an unrelatived integer overflow bug that resulted in negative length slices and fixed that.

@gagliardetto
Copy link
Owner

gagliardetto commented Aug 20, 2022

I think we can leave go 1.16 behind, and add go 1.19 instead

@riptl
Copy link
Contributor Author

riptl commented Aug 20, 2022

variant.go got accidentally changed by go fmt

@gagliardetto gagliardetto merged commit 6096313 into gagliardetto:master Aug 20, 2022
@gagliardetto
Copy link
Owner

Thanks!

@riptl riptl deleted the decoder-slice-bounds branch August 21, 2022 06:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

gagliardetto/binary have some problem
2 participants