diff --git a/src/ffmpeg.imageio/ffmpeginput.cpp b/src/ffmpeg.imageio/ffmpeginput.cpp index ba6a3a8b12..165d81fb77 100644 --- a/src/ffmpeg.imageio/ffmpeginput.cpp +++ b/src/ffmpeg.imageio/ffmpeginput.cpp @@ -74,13 +74,37 @@ receive_frame(AVCodecContext* avctx, AVFrame* picture, AVPacket* avpkt) #include +#include +#include #include +#include +#include #include #include OIIO_PLUGIN_NAMESPACE_BEGIN +// Timestamps, frame rates and time bases all come from the file, so the +// arithmetic below can produce NaN, infinity, or values outside the integer +// range. Convert through these helpers to keep that out of undefined +// behavior. safe_int64's range is well under the type's limits so that a sum +// of two results can't overflow either. +static int64_t +safe_int64(double v) +{ + return std::isfinite(v) ? int64_t(clamp(v, -4.0e18, 4.0e18)) : 0; +} + + +static int +safe_int(double v) +{ + return std::isfinite(v) ? int(clamp(v, double(INT_MIN), double(INT_MAX))) + : 0; +} + + class FFmpegInput final : public ImageInput { public: FFmpegInput(); @@ -120,6 +144,7 @@ class FFmpegInput final : public ImageInput { AVFrame* m_frame = nullptr; AVFrame* m_rgb_frame = nullptr; size_t m_stride; // scanline width in bytes, a.k.a. scanline stride + AVPixelFormat m_decoded_pix_format; // what the decoder promised at open AVPixelFormat m_dst_pix_format; SwsContext* m_sws_rgb_context = nullptr; AVRational m_frame_rate; @@ -133,19 +158,21 @@ class FFmpegInput final : public ImageInput { bool m_offset_time; bool m_codec_cap_delay; bool m_read_frame; + bool m_frame_valid; // did the last read_frame() actually decode? int64_t m_start_time; // init to initialize state void init(void) { m_filename.clear(); - m_format_context = nullptr; - m_codec_context = nullptr; - m_codec = nullptr; - m_frame = nullptr; - m_rgb_frame = nullptr; - m_sws_rgb_context = nullptr; - m_stride = 0; + m_format_context = nullptr; + m_codec_context = nullptr; + m_codec = nullptr; + m_frame = nullptr; + m_rgb_frame = nullptr; + m_sws_rgb_context = nullptr; + m_stride = 0; + m_decoded_pix_format = AV_PIX_FMT_NONE; m_rgb_buffer.clear(); m_video_indexes.clear(); m_video_stream = -1; @@ -155,6 +182,7 @@ class FFmpegInput final : public ImageInput { m_last_decoded_pos = 0; m_offset_time = true; m_read_frame = false; + m_frame_valid = false; m_codec_cap_delay = false; m_subimage = 0; m_start_time = 0; @@ -315,31 +343,48 @@ FFmpegInput::open(const std::string& name, ImageSpec& spec) AVStream* stream = m_format_context->streams[m_video_stream]; m_frame_rate = av_guess_frame_rate(m_format_context, stream, NULL); - m_frames = stream->nb_frames; + // nb_frames and start_time come from the file. Clamp them to a range the + // rest of this reader can handle: subimage indices are int, and an + // unset start_time (AV_NOPTS_VALUE == INT64_MIN) would blow up the frame + // arithmetic in read_frame(). + m_frames = OIIO::clamp(stream->nb_frames, int64_t(0), int64_t(INT_MAX)); m_start_time = stream->start_time; + if (m_start_time == int64_t(AV_NOPTS_VALUE)) + m_start_time = 0; if (!m_frames) { seek(0); AVPacket pkt; av_init_packet(&pkt); av_read_frame(m_format_context, &pkt); int64_t first_pts = pkt.pts; - int64_t max_pts = 0; + if (first_pts == int64_t(AV_NOPTS_VALUE)) + first_pts = 0; + int64_t max_pts = 0; av_packet_unref(&pkt); //because seek(int) uses m_format_context seek(1 << 29); av_init_packet(&pkt); //Is this needed? while (stream && av_read_frame(m_format_context, &pkt) >= 0) { - int64_t current_pts = static_cast( - av_q2d(stream->time_base) * (pkt.pts - first_pts) * fps()); + // Do the difference in double: both timestamps are untrusted and + // subtracting them as int64 can overflow. + int64_t current_pts = safe_int64( + av_q2d(stream->time_base) + * (double(pkt.pts) - double(first_pts)) * fps()); if (current_pts > max_pts) { max_pts = current_pts + 1; } av_packet_unref(&pkt); //Always free before format_context usage } - m_frames = max_pts; + m_frames = std::min(max_pts, int64_t(INT_MAX)); } m_frame = av_frame_alloc(); m_rgb_frame = av_frame_alloc(); + if (!m_frame || !m_rgb_frame) { + errorfmt("\"{}\" could not allocate FFmpeg frame", file_name); + close(); + return false; + } + m_decoded_pix_format = m_codec_context->pix_fmt; AVPixelFormat src_pix_format; switch (m_codec_context->pix_fmt) { // deprecation warning for YUV formats case AV_PIX_FMT_YUVJ420P: src_pix_format = AV_PIX_FMT_YUV420P; break; @@ -418,6 +463,7 @@ FFmpegInput::open(const std::string& name, ImageSpec& spec) case AV_PIX_FMT_GRAY8: case AV_PIX_FMT_MONOWHITE: case AV_PIX_FMT_MONOBLACK: + nchannels = 1; datatype = TypeUInt8; m_dst_pix_format = AV_PIX_FMT_GRAY8; break; @@ -430,6 +476,7 @@ FFmpegInput::open(const std::string& name, ImageSpec& spec) case AV_PIX_FMT_GRAY12LE: case AV_PIX_FMT_GRAY16BE: case AV_PIX_FMT_GRAY16LE: + nchannels = 1; datatype = TypeUInt16; m_dst_pix_format = AV_PIX_FMT_GRAY16; break; @@ -496,8 +543,16 @@ FFmpegInput::open(const std::string& name, ImageSpec& spec) default: break; } - m_spec = ImageSpec(m_codec_context->width, m_codec_context->height, - nchannels, datatype); + m_spec = ImageSpec(m_codec_context->width, m_codec_context->height, + nchannels, datatype); + // The frame dimensions come from the container/codec headers, which are + // untrusted. FFmpeg caps the pixel count at only about 2.7e8, which still + // leaves room for a tiny file to declare a multi-GB frame. + if (!check_open(m_spec, { 0, 1 << 16, 0, 1 << 16, 0, 1, 0, 4 }) + || !check_compression_ratio(m_spec, Filesystem::file_size(name))) { + close(); + return false; + } m_stride = (size_t)(m_spec.scanline_bytes()); int rgb_buffer_size @@ -592,8 +647,9 @@ FFmpegInput::seek_subimage(int subimage, int miplevel) if (subimage == m_subimage) { return true; } - m_subimage = subimage; - m_read_frame = false; + m_subimage = subimage; + m_read_frame = false; + m_frame_valid = false; return true; } @@ -633,6 +689,14 @@ FFmpegInput::read_native_scanline(int subimage, int miplevel, int y, int /*z*/, if (!m_read_frame) { read_frame(m_subimage); } + if (!m_frame_valid) { + // Nothing was decoded for this subimage. Without this check we would + // hand back whatever the previously decoded frame left in the buffer. + if (!has_error()) + errorfmt("Could not decode frame {} of \"{}\"", m_subimage, + m_filename); + return false; + } if (m_spec.format == TypeUInt8 || m_spec.nchannels == 1) { if (m_rgb_frame->data[0]) { memcpy(data, m_rgb_frame->data[0] + y * m_rgb_frame->linesize[0], @@ -676,8 +740,8 @@ FFmpegInput::close(void) if (m_codec_context) avcodec_free_context(&m_codec_context); if (m_format_context) { + // Frees the context and everything it owns, and nulls the pointer. avformat_close_input(&m_format_context); - av_free(m_format_context); // will free m_codec and m_codec_context } if (m_frame) av_frame_free(&m_frame); // free after close input @@ -697,61 +761,87 @@ FFmpegInput::read_frame(int frame) if (m_last_decoded_pos + 1 != frame) { seek(frame); } - AVPacket pkt; - int finished = 0; - int ret = 0; - while ((ret = av_read_frame(m_format_context, &pkt)) == 0 - || m_codec_cap_delay) { - if (ret == AVERROR_EOF) { - break; + m_read_frame = true; + m_frame_valid = false; + // Allocate the packet rather than using a stack AVPacket: av_read_frame + // is not guaranteed to initialize it on failure, and the flush path below + // would then unref an uninitialized packet. + AVPacket* pkt = av_packet_alloc(); + if (!pkt) { + errorfmt("Could not allocate FFmpeg packet"); + return; + } + bool flushing = false; + while (true) { + int ret = av_read_frame(m_format_context, pkt); + if (ret < 0) { + if (!m_codec_cap_delay || ret == AVERROR_EOF) + break; + // The codec buffers delayed frames, so keep going with flush + // packets (data == null, size == 0), but stop as soon as the + // decoder runs dry -- otherwise a stream that keeps returning + // the same error would spin here forever. + flushing = true; + av_packet_unref(pkt); + pkt->stream_index = m_video_stream; } - if (pkt.stream_index == m_video_stream) { - if (ret < 0 && m_codec_cap_delay) { - pkt.data = NULL; - pkt.size = 0; - } - - finished = receive_frame(m_codec_context, m_frame, &pkt); + if (pkt->stream_index == m_video_stream) { + int finished = receive_frame(m_codec_context, m_frame, pkt); + if (flushing && !finished) + break; double pts = 0; if (static_cast(m_frame->pts) != int64_t(AV_NOPTS_VALUE)) { pts = av_q2d( m_format_context->streams[m_video_stream]->time_base) - * m_frame->pts; + * double(m_frame->pts); } - int current_frame = int((pts - m_start_time) * fps() + 0.5f); //??? + int current_frame = safe_int((pts - double(m_start_time)) * fps() + + 0.5); //current_frame = m_frame->display_picture_number; m_last_search_pos = current_frame; if (current_frame == frame && finished) { + // A decoder may change frame geometry or pixel format + // mid-stream, but the scaling context and RGB buffer were + // sized from the header when we opened the file. + if (m_frame->width != m_spec.width + || m_frame->height != m_spec.height + || m_frame->format != m_decoded_pix_format) { + errorfmt("\"{}\" frame {} does not match the {}x{} format " + "declared by the header", + m_filename, frame, m_spec.width, m_spec.height); + break; + } + // Use the spec dimensions, which are what m_rgb_buffer and + // the scaling context were sized for -- the decoder can move + // m_codec_context->width/height out from under us. int fill_ret = avpicture_fill(m_rgb_frame, &m_rgb_buffer[0], - m_dst_pix_format, - m_codec_context->width, - m_codec_context->height); + m_dst_pix_format, m_spec.width, + m_spec.height); if (fill_ret < 0) { errorfmt("Error filling FFmpeg RGB frame"); - av_packet_unref(&pkt); break; } - int scale_ret = sws_scale( - m_sws_rgb_context, - static_cast(m_frame->data), - m_frame->linesize, 0, m_codec_context->height, - m_rgb_frame->data, m_rgb_frame->linesize); + int scale_ret = sws_scale(m_sws_rgb_context, + static_cast( + m_frame->data), + m_frame->linesize, 0, m_spec.height, + m_rgb_frame->data, + m_rgb_frame->linesize); if (scale_ret <= 0) { errorfmt("Error converting FFmpeg frame"); - av_packet_unref(&pkt); break; } m_last_decoded_pos = current_frame; - av_packet_unref(&pkt); + m_frame_valid = true; break; } } - av_packet_unref(&pkt); + av_packet_unref(pkt); } - m_read_frame = true; + av_packet_free(&pkt); } @@ -791,15 +881,19 @@ FFmpegInput::seek(int frame) int64_t FFmpegInput::time_stamp(int frame) const { - int64_t timestamp = static_cast( - (static_cast(frame) - / (fps() - * av_q2d(m_format_context->streams[m_video_stream]->time_base)))); + // A corrupt header can give us a zero or degenerate time base, which + // would make the divisions below produce inf/NaN. + double time_base = av_q2d( + m_format_context->streams[m_video_stream]->time_base); + double scale = fps() * time_base; + if (!(scale > 0) || !(time_base > 0)) + return 0; + int64_t timestamp = safe_int64(static_cast(frame) / scale); if (static_cast(m_format_context->start_time) != int64_t(AV_NOPTS_VALUE)) { - timestamp += static_cast( + timestamp += safe_int64( static_cast(m_format_context->start_time) * AV_TIME_BASE - / av_q2d(m_format_context->streams[m_video_stream]->time_base)); + / time_base); } return timestamp; } diff --git a/testsuite/ffmpeg/ref/out-ffmpeg6.1.txt b/testsuite/ffmpeg/ref/out-ffmpeg6.1.txt index 1f1258b343..8d09187e79 100644 --- a/testsuite/ffmpeg/ref/out-ffmpeg6.1.txt +++ b/testsuite/ffmpeg/ref/out-ffmpeg6.1.txt @@ -110,3 +110,17 @@ ref/vp9_rec2100_pq.mkv : 384 x 216, 3 channel, uint10 FFmpeg movie oiio:ColorSpace: "pq_rec2020_display" oiio:Movie: 1 oiio:subimages: 2 +oiiotool WARNING : oiiotool produced no output. Did you forget -o? +src/gray8.avi : 128 x 64, 1 channel, uint8 FFmpeg movie +oiiotool WARNING : oiiotool produced no output. Did you forget -o? +src/gray16.avi : 128 x 64, 1 channel, uint16 FFmpeg movie +oiiotool WARNING : oiiotool produced no output. Did you forget -o? +oiiotool ERROR: read : "src/bomb-16384x9000.avi": FFmpeg movie header claims a 1125 MB image from a 6592 byte file; probably a corrupt or malicious header +Full command line was: +> oiiotool --info --hash src/bomb-16384x9000.avi +oiiotool ERROR: read : "src/truncated.avi" could not open input +Full command line was: +> oiiotool --info --hash src/truncated.avi +src/resolution-change.mkv : 320 x 240, 3 channel, uint8 FFmpeg movie (4 subimages) + SHA-1: "src/resolution-change.mkv" frame 0 does not match the 320x240 format declared by the header +iinfo ERROR: "src/resolution-change.mkv" : "src/resolution-change.mkv" frame 0 does not match the 320x240 format declared by the header diff --git a/testsuite/ffmpeg/ref/out-ffmpeg8.0.txt b/testsuite/ffmpeg/ref/out-ffmpeg8.0.txt index 28084a22e3..022f360f6a 100644 --- a/testsuite/ffmpeg/ref/out-ffmpeg8.0.txt +++ b/testsuite/ffmpeg/ref/out-ffmpeg8.0.txt @@ -110,3 +110,17 @@ ref/vp9_rec2100_pq.mkv : 384 x 216, 3 channel, uint10 FFmpeg movie oiio:ColorSpace: "pq_rec2020_display" oiio:Movie: 1 oiio:subimages: 2 +oiiotool WARNING : oiiotool produced no output. Did you forget -o? +src/gray8.avi : 128 x 64, 1 channel, uint8 FFmpeg movie +oiiotool WARNING : oiiotool produced no output. Did you forget -o? +src/gray16.avi : 128 x 64, 1 channel, uint16 FFmpeg movie +oiiotool WARNING : oiiotool produced no output. Did you forget -o? +oiiotool ERROR: read : "src/bomb-16384x9000.avi": FFmpeg movie header claims a 1125 MB image from a 6592 byte file; probably a corrupt or malicious header +Full command line was: +> oiiotool --info --hash src/bomb-16384x9000.avi +oiiotool ERROR: read : "src/truncated.avi" could not open input +Full command line was: +> oiiotool --info --hash src/truncated.avi +src/resolution-change.mkv : 320 x 240, 3 channel, uint8 FFmpeg movie (4 subimages) + SHA-1: "src/resolution-change.mkv" frame 0 does not match the 320x240 format declared by the header +iinfo ERROR: "src/resolution-change.mkv" : "src/resolution-change.mkv" frame 0 does not match the 320x240 format declared by the header diff --git a/testsuite/ffmpeg/ref/out-ffmpeg8.1.txt b/testsuite/ffmpeg/ref/out-ffmpeg8.1.txt index 0a9eef7cb9..e5bcc54445 100644 --- a/testsuite/ffmpeg/ref/out-ffmpeg8.1.txt +++ b/testsuite/ffmpeg/ref/out-ffmpeg8.1.txt @@ -110,3 +110,17 @@ ref/vp9_rec2100_pq.mkv : 384 x 216, 3 channel, uint10 FFmpeg movie oiio:ColorSpace: "pq_rec2020_display" oiio:Movie: 1 oiio:subimages: 2 +oiiotool WARNING : oiiotool produced no output. Did you forget -o? +src/gray8.avi : 128 x 64, 1 channel, uint8 FFmpeg movie +oiiotool WARNING : oiiotool produced no output. Did you forget -o? +src/gray16.avi : 128 x 64, 1 channel, uint16 FFmpeg movie +oiiotool WARNING : oiiotool produced no output. Did you forget -o? +oiiotool ERROR: read : "src/bomb-16384x9000.avi": FFmpeg movie header claims a 1125 MB image from a 6592 byte file; probably a corrupt or malicious header +Full command line was: +> oiiotool --info --hash src/bomb-16384x9000.avi +oiiotool ERROR: read : "src/truncated.avi" could not open input +Full command line was: +> oiiotool --info --hash src/truncated.avi +src/resolution-change.mkv : 320 x 240, 3 channel, uint8 FFmpeg movie (4 subimages) + SHA-1: "src/resolution-change.mkv" frame 0 does not match the 320x240 format declared by the header +iinfo ERROR: "src/resolution-change.mkv" : "src/resolution-change.mkv" frame 0 does not match the 320x240 format declared by the header diff --git a/testsuite/ffmpeg/run.py b/testsuite/ffmpeg/run.py index eca172516c..0522a96e29 100755 --- a/testsuite/ffmpeg/run.py +++ b/testsuite/ffmpeg/run.py @@ -9,6 +9,31 @@ for f in files: command = command + info_command (os.path.join(imagedir, f)) +# Capture stderr too, so the errors from the malformed-input reads below land +# in out.txt and are checked against the ref. +redirect = " >> out.txt 2>&1 " + # Regression test for narrow 10-bit VP9 input that used to make swscale write # past OIIO's destination buffer during pixel reads. command = command + oiiotool ("src/ffmpeg-width1-gbrp16.mkv --hash") + +# Grayscale movies. These used to be described as 3-channel even though the +# frames were requested from swscale as GRAY8/GRAY16, so every scanline copy +# ran past the end of the decoded row. Check the channel count, then read the +# pixels to make sure the copy stays in bounds. (No hash: swscale output is +# not bit-identical across FFmpeg versions.) See src/make_malformed_movies.py. +for f in [ "gray8.avi", "gray16.avi" ]: + command = command + info_command ("src/" + f, verbose=False, hash=False) + command = command + oiiotool ("src/" + f + " --hash") + +# Hostile/truncated input must be rejected cleanly, with no crash and no +# over-allocation ahead of the rejection. +for f in [ "bomb-16384x9000.avi", "truncated.avi" ]: + command = command + info_command ("src/" + f, verbose=False, + failureok=True) + +# A stream whose frames decode smaller than the header claims. swscale used +# to read past the end of those frames; the read must fail instead. (iinfo, +# because oiiotool's --hash exits 0 on a failed read.) +command = command + info_command ("src/resolution-change.mkv", verbose=False, + failureok=True, info_program="iinfo") diff --git a/testsuite/ffmpeg/src/bomb-16384x9000.avi b/testsuite/ffmpeg/src/bomb-16384x9000.avi new file mode 100644 index 0000000000..654bda29d3 Binary files /dev/null and b/testsuite/ffmpeg/src/bomb-16384x9000.avi differ diff --git a/testsuite/ffmpeg/src/gray16.avi b/testsuite/ffmpeg/src/gray16.avi new file mode 100644 index 0000000000..5fd43e5443 Binary files /dev/null and b/testsuite/ffmpeg/src/gray16.avi differ diff --git a/testsuite/ffmpeg/src/gray8.avi b/testsuite/ffmpeg/src/gray8.avi new file mode 100644 index 0000000000..13ed527092 Binary files /dev/null and b/testsuite/ffmpeg/src/gray8.avi differ diff --git a/testsuite/ffmpeg/src/make_malformed_movies.py b/testsuite/ffmpeg/src/make_malformed_movies.py new file mode 100644 index 0000000000..5d04572c78 --- /dev/null +++ b/testsuite/ffmpeg/src/make_malformed_movies.py @@ -0,0 +1,82 @@ +#!/usr/bin/env python3 +# Copyright Contributors to the OpenImageIO project. +# SPDX-License-Identifier: Apache-2.0 +# https://github.com/AcademySoftwareFoundation/OpenImageIO + +# Generate the small movie fixtures used by the ffmpeg security-audit +# regression tests. Requires the `ffmpeg` command line tool; the generated +# files are committed, so this only needs to be re-run if a fixture changes. +# +# gray8.avi valid 8-bit grayscale movie +# gray16.avi valid 16-bit grayscale movie +# bomb-16384x9000.avi tiny file whose header declares a 1.1 GB frame +# truncated.avi valid header followed by a chopped-off stream +# resolution-change.mkv frames smaller than the size the header declares +# +# The two grayscale movies are the regression case for a heap overread: the +# reader asked FFmpeg for GRAY8/GRAY16 frames but described them in the +# ImageSpec as 3-channel, so each scanline copy ran three times past the end +# of the decoded row. +# +# AVI keeps the frame dimensions in two places, the main header (avih) and +# the stream format header (strf, a BITMAPINFOHEADER). Patching both is +# enough to make a small file claim an arbitrary frame size, since ffv1 takes +# its dimensions from the container. + +import os +import struct +import subprocess +import sys + + +def ffmpeg(pix_fmt, size, out): + subprocess.run(["ffmpeg", "-y", "-loglevel", "error", "-f", "lavfi", "-i", + "testsrc=size=%s:rate=1:duration=1" % size, + "-pix_fmt", pix_fmt, "-c:v", "ffv1", "-frames:v", "1", + out], check=True) + + +def patch_dimensions(src, dst, width, height): + d = bytearray(open(src, "rb").read()) + avih = d.find(b"avih") + strf = d.find(b"strf") + if avih < 0 or strf < 0: + sys.exit("could not locate AVI headers in " + src) + # avih: dwWidth/dwHeight are the 9th and 10th uint32 of the structure. + d[avih + 40:avih + 44] = struct.pack("