[FFmpeg-devel] [PATCH] avformat/mov: fix overallocation when reading pssh/saiz
"zhilizhao(赵志立)"
quinkblack at foxmail.com
Mon Jun 12 15:28:09 EEST 2023
> On Jun 12, 2023, at 20:06, Marvin Scholz (ePirat) <epirat07 at gmail.com> wrote:
>
> 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.
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 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".
> _______________________________________________
> 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".
More information about the ffmpeg-devel
mailing list