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

From: Zhao Zhili <zhilizhao@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@tencent.com>
---
 libavformat/mov.c | 63 +++++++++++++++++++----------------------------
 1 file changed, 25 insertions(+), 38 deletions(-)

Hi,

> On 12. Jun 2023, at 13:56, Zhao Zhili <quinkblack@foxmail.com> wrote:
> 
> From: Zhao Zhili <zhilizhao@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@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-request@ffmpeg.org with subject "unsubscribe".

> On Jun 12, 2023, at 20:06, Marvin Scholz (ePirat) <epirat07@gmail.com> wrote:
> 
> Hi,
> 
>> On 12. Jun 2023, at 13:56, Zhao Zhili <quinkblack@foxmail.com> wrote:
>> 
>> From: Zhao Zhili <zhilizhao@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@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.

1. If malloc failed, it doesn’t exhaust memory.

2. If malloc success and the real data is less than extra_data_size, the memory 
will be freed immediately.

3. Almost every box with undetermined length of entries have this issue.

4. We can check extra_data_size < atom.size (which make sense), but it doesn’t help
much in this case.

Now let consider the normal case when extra_data_size isn’t a malicious size, like
48. The original code will allocate 1M and only use 48 bytes. Like I said in the
commit message, we can limit block_size by

const unsigned int block_size = FF_MIN(1024 * 1024, size).

But the function itself is useless and hurt performance. And it has more issues: it
can clamp the size silently when alloc_size >= INT_MAX - block_size, then overwrite
old data by new data.

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);

> 
>> +    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-request@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-request@ffmpeg.org with subject "unsubscribe".