fix(ffmpeg): heap reads after the end, bad packet, checks at open - #5406
Open
lgritz wants to merge 1 commit into
Open
fix(ffmpeg): heap reads after the end, bad packet, checks at open#5406lgritz wants to merge 1 commit into
lgritz wants to merge 1 commit into
Conversation
This is a security audit of the FFmpeg reader (specs/002-format-security-audit). Gray movies read heap memory after the end of the buffer. The GRAY8 and GRAY16 code asked swscale for a gray frame with one plane, but it left nchannels at 3. Each scanline copy then read three times too many bytes. Now nchannels is 1. This was the intent when we added the gray code (AcademySoftwareFoundation#2349). It also makes 16-bit gray files readable, which they were not before. A decoder can change the frame size in the middle of a stream. But we make the scale context and the RGB buffer one time, from the header. A smaller frame thus caused a read after its end. Now read_frame() refuses a frame whose size or pixel format is different from the ImageSpec. It also scales from the spec. Nothing examined the frame size at open. A file of 6 KB can declare a frame of 1.1 GB, and cause an allocation of 2.4 GB. Now check_open() and check_compression_ratio() refuse such a file. The ratio floor stays at 1 GB, because a correct movie with one frame can reach a ratio of 13000:1. read_frame() gave an uninitialized AVPacket to av_read_frame(), which does not initialize it when it fails. The code then read bad values from the packet, and it could continue forever on a bad stream. Now the code uses av_packet_alloc(), and it stops when the decoder is empty. There are smaller changes. Examine the result of av_frame_alloc(). Correct a bad nb_frames and start_time. Protect time_stamp() against a time base of zero. Do the frame-count arithmetic in double, because two int64 times can overflow. Report a decode failure, and do not give back the last good frame. Remove an unnecessary av_free() after avformat_close_input(). New fixtures come from testsuite/ffmpeg/src/make_malformed_movies.py: the gray8 and gray16 tests, a decompression bomb, a truncated stream, and a change of size in the middle of a stream. Assisted-by: Claude Code / claude-opus-5 Signed-off-by: Larry Gritz <lg@larrygritz.com>
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
1 participant
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.
This is from an audit of the FFmpeg reader, looking for bugs and security issues.
Grayscale movies read heap memory after the end of the buffer. The GRAY8 and GRAY16 code asked swscale for a gray frame with one plane, but it left nchannels at 3. Each scanline copy then read three times too many bytes. Now nchannels is 1. This was the intent when we added the gray code (#2349). It also makes 16-bit gray files readable, which they were not before.
A decoder can change the frame size in the middle of a stream. But we make the scale context and the RGB buffer one time, from the header. A smaller frame thus caused a read after its end. Now read_frame() refuses a frame whose size or pixel format is different from the ImageSpec. It also scales from the spec.
Nothing examined the frame size at open. A file of 6 KB can declare a frame of 1.1 GB, and cause an allocation of 2.4 GB. Now check_open() and check_compression_ratio() refuse such a file. The ratio floor stays at 1 GB, because a correct movie with one frame can reach a ratio of 13000:1.
read_frame() gave an uninitialized AVPacket to av_read_frame(), which does not initialize it when it fails. The code then read bad values from the packet, and it could continue forever on a bad stream. Now the code uses av_packet_alloc(), and it stops when the decoder is empty.
There are smaller changes. Examine the result of av_frame_alloc(). Correct a bad nb_frames and start_time. Protect time_stamp() against a time base of zero. Do the frame-count arithmetic in double, because two int64 times can overflow. Report a decode failure, and do not give back the last good frame. Remove an unnecessary av_free() after avformat_close_input().
New test fixtures come from testsuite/ffmpeg/src/make_malformed_movies.py: the gray8 and gray16 tests, a decompression bomb, a truncated stream, and a change of size in the middle of a stream.
Assisted-by: Claude Code / claude-opus-5