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

["\"zhilizhao(赵志立)\"" <quinkblack@foxmail.com>]

> 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".

_______________________________________________
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".