bound exp-golomb suffix read to bitstream in avc_nalu_read_uev#3334
Open
sahvx655-wq wants to merge 1 commit into
Open
bound exp-golomb suffix read to bitstream in avc_nalu_read_uev#3334sahvx655-wq wants to merge 1 commit into
sahvx655-wq wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
avc_nalu_read_uev decodes an Exp-Golomb ue(v) in two passes. The first loop counts leading-zero bits and correctly stops on stream->empty(), but the second loop then reads leadingZeroBits suffix bits back to back with no such check, and BitStream::read_bit() dereferences its cursor unconditionally (its own comment notes empty() must be checked first). A sequence parameter set whose bitstream ends inside or just after the leading-zero run therefore makes the suffix loop walk off the end of the rbsp buffer. I traced it with a guard-page harness: handing the function a one-byte all-zero bitstream empties the stream in the first loop, leadingZeroBits comes out as 7, and the suffix loop first read_bit() lands on the following PROT_NONE page and takes a SIGBUS. The same bytes reach here from the network via AVCDecoderConfigurationRecord::Create then ParseSPS when an AVC sequence header is parsed.
The fix re-checks stream->empty() inside the suffix loop and returns -1 when the data runs out, matching what the leading-zero loop and the sibling avc_nalu_read_bit already do and what every caller already treats as a decode failure. Keeping the bound in the reader covers every Exp-Golomb field in the SPS rather than asking ParseSPS to pre-measure each one. Left as is this is an out-of-bounds read of attacker-controlled length (up to ~30 bits past the buffer) driven by a publisher-supplied sequence header, leaking adjacent memory into the decoded value or crashing the parser.