[FFmpeg-devel] [PATCH] avformat/mov: fix overallocation when reading pssh/saiz

[Marvin Scholz (ePirat)]

Hi,

> On 12. Jun 2023, at 13:56, Zhao Zhili <quinkblack at foxmail.com> wrote:
> 
> From: Zhao Zhili <zhilizhao at 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 <zhilizhao at 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 at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".