Hi,
> On 12. Jun 2023, at 13:56, Zhao Zhili <quinkbl...@foxmail.com> wrote:
>
> From: Zhao Zhili <zhiliz...@tencent.com>
>
> mov_try_read_block() allocates 1MB at least, which can be more than
> enough. It was called when reading saiz box, which can appear
> periodically inside fmp4. This consumes a lot of memory.
>
> We can fix mov_try_read_block() by clamp 'block_size' with 'size'.
> However, the function is harmful than helpful. It avoids allocating
> large memory when the real data is small. Even in that case, if
> allocating large memory directly failed, it's fine to return ENOMEM;
> if allocating success and reading doesn't match the given size, it's
> fine to free and return AVERROR_INVALIDDATA. In other cases, it's a
> waste of CPU and memory.
>
> So I decided to remove the function, and replace it by call
> av_malloc() and avio_read() directly.
>
> mov_read_saiz() and mov_read_pssh() need more check, but they don't
> belong to this patch.
>
> Fixes #7641 and #9243.
>
> Signed-off-by: Zhao Zhili <zhiliz...@tencent.com>
> ---
> libavformat/mov.c | 63 +++++++++++++++++++----------------------------
> 1 file changed, 25 insertions(+), 38 deletions(-)
>
> diff --git a/libavformat/mov.c b/libavformat/mov.c
> index a8d004e02b..3d0969545a 100644
> --- a/libavformat/mov.c
> +++ b/libavformat/mov.c
> @@ -6649,38 +6649,6 @@ finish:
> return ret;
> }
>
> -/**
> - * Tries to read the given number of bytes from the stream and puts it in a
> - * newly allocated buffer. This reads in small chunks to avoid allocating
> large
> - * memory if the file contains an invalid/malicious size value.
I fail to see how your replacement code addresses the malicious size value case
that this function mitigated, see in more detail what I mean below…
> - */
> -static int mov_try_read_block(AVIOContext *pb, size_t size, uint8_t **data)
> -{
> - const unsigned int block_size = 1024 * 1024;
> - uint8_t *buffer = NULL;
> - unsigned int alloc_size = 0, offset = 0;
> - while (offset < size) {
> - unsigned int new_size =
> - alloc_size >= INT_MAX - block_size ? INT_MAX : alloc_size +
> block_size;
> - uint8_t *new_buffer = av_fast_realloc(buffer, &alloc_size, new_size);
> - unsigned int to_read = FFMIN(size, alloc_size) - offset;
> - if (!new_buffer) {
> - av_free(buffer);
> - return AVERROR(ENOMEM);
> - }
> - buffer = new_buffer;
> -
> - if (avio_read(pb, buffer + offset, to_read) != to_read) {
> - av_free(buffer);
> - return AVERROR_INVALIDDATA;
> - }
> - offset += to_read;
> - }
> -
> - *data = buffer;
> - return 0;
> -}
> -
> static int mov_read_saiz(MOVContext *c, AVIOContext *pb, MOVAtom atom)
> {
> MOVEncryptionIndex *encryption_index;
> @@ -6736,15 +6704,24 @@ static int mov_read_saiz(MOVContext *c, AVIOContext
> *pb, MOVAtom atom)
>
> encryption_index->auxiliary_info_default_size = avio_r8(pb);
> sample_count = avio_rb32(pb);
> - encryption_index->auxiliary_info_sample_count = sample_count;
>
> if (encryption_index->auxiliary_info_default_size == 0) {
> - ret = mov_try_read_block(pb, sample_count,
> &encryption_index->auxiliary_info_sizes);
> - if (ret < 0) {
> - av_log(c->fc, AV_LOG_ERROR, "Failed to read the auxiliary
> info\n");
> + encryption_index->auxiliary_info_sizes = av_malloc(sample_count);
> + if (!encryption_index->auxiliary_info_sizes)
> + return AVERROR(ENOMEM);
> +
> + ret = avio_read(pb, encryption_index->auxiliary_info_sizes,
> sample_count);
> + if (ret != sample_count) {
> + av_freep(&encryption_index->auxiliary_info_sizes);
> +
> + if (ret >= 0)
> + ret = AVERROR_INVALIDDATA;
> + av_log(c->fc, AV_LOG_ERROR, "Failed to read the auxiliary info,
> %s\n",
> + av_err2str(ret));
> return ret;
> }
> }
> + encryption_index->auxiliary_info_sample_count = sample_count;
>
> if (encryption_index->auxiliary_offsets_count) {
> return mov_parse_auxiliary_info(c, sc, pb, encryption_index);
> @@ -6913,9 +6890,19 @@ static int mov_read_pssh(MOVContext *c, AVIOContext
> *pb, MOVAtom atom)
> }
>
> extra_data_size = avio_rb32(pb);
> - ret = mov_try_read_block(pb, extra_data_size, &extra_data);
> - if (ret < 0)
> + extra_data = av_malloc(extra_data_size);
If I understand correctly you are now effectively passing a potentially
malicious size value directly to malloc, allowing an attacker to exhaust memory
with a crafted file.
> + if (!extra_data) {
> + ret = AVERROR(ENOMEM);
> goto finish;
> + }
> + ret = avio_read(pb, extra_data, extra_data_size);
> + if (ret != extra_data_size) {
> + av_free(extra_data);
> +
> + if (ret >= 0)
> + ret = AVERROR_INVALIDDATA;
> + goto finish;
> + }
>
> av_freep(&info->data); // malloc(0) may still allocate something.
> info->data = extra_data;
> --
> 2.25.1
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
