[FFmpeg-devel] [PATCH 1/4] avformat/iamf_parse: Check extradata size

[James Almer]

On 6/23/2025 12:14 PM, James Almer wrote:
> On 6/23/2025 9:57 AM, Andreas Rheinhardt wrote:
>> James Almer:
>>> On 6/23/2025 9:44 AM, Michael Niedermayer wrote:
>>>> On Fri, Jun 20, 2025 at 12:28:13AM +0200, Andreas Rheinhardt wrote:
>>>>> Michael Niedermayer:
>>>>>> Fixes: Assertion n>=0 && n<=32 failed at ./libavcodec/get_bits.h:406
>>>>>> Fixes: 398527871/clusterfuzz-testcase-minimized-
>>>>>> ffmpeg_dem_IAMF_fuzzer-6602025714647040
>>>>>>
>>>>>> Found-by: continuous fuzzing process https://github.com/google/oss-
>>>>>> fuzz/tree/master/projects/ffmpeg
>>>>>> Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
>>>>>> ---
>>>>>>    libavformat/iamf_parse.c | 2 ++
>>>>>>    1 file changed, 2 insertions(+)
>>>>>>
>>>>>> diff --git a/libavformat/iamf_parse.c b/libavformat/iamf_parse.c
>>>>>> index 71497876ac3..330e01733dd 100644
>>>>>> --- a/libavformat/iamf_parse.c
>>>>>> +++ b/libavformat/iamf_parse.c
>>>>>> @@ -305,6 +305,8 @@ static int update_extradata(AVCodecParameters
>>>>>> *codecpar)
>>>>>>            skip_bits(&gb, 4);
>>>>>>            put_bits(&pb, 4, codecpar->ch_layout.nb_channels); // set
>>>>>> channel config
>>>>>>            ret = put_bits_left(&pb);
>>>>>> +        if (ret < 0)
>>>>>> +            return AVERROR_INVALIDDATA;
>>>>>>            while (ret >= 32) {
>>>>>>               put_bits32(&pb, get_bits_long(&gb, 32));
>>>>>>               ret -= 32;
>>>>>
>>>>> There is only one way for put_bits_left() to return a negative 
>>>>> value: If
>>>>> there is more data in the internal buffer than can be written out. And
>>>>> this scenario is already a violation of the PutBit API. Given that the
>>>>> size of the internal buffer depends upon the arch, it could be that 
>>>>> one
>>>>> would have already hit an assert in case one is not using x64. In 
>>>>> other
>>>>> words, your check is too late.
>>>>
>>>> the patches puprose was mainly to show that
>>>> 3f9420132441345b7ccd57001f230bb98f655696
>>>> was insufficient to fix 398527871
>>>>
>>>> I do not expect my patch would be the correct solution even if the
>>>> check is done earlier. IAMF is cursed
>>>
>>> Does increasing buf from 6 bytes to 8 or more fix it? I see putbits may
>>> do an AV_W*64(), so six bytes sounds like it was never safe.
>>>
>> That is only executed when the internal bit buffer is full; you will
>> never reach it on x64. The problem is that you initialize the put bits
>> buffer with FFMIN(codecpar->extradata_size, sizeof(buf)) instead of
>> sizeof(buf). If this were not so, there would always be bits left.
>> But this only fixes the API violations, it does not guarantee that the
>> written data is actually correct. What is actually in the data that gets
>> written in the loop?
> The remaining bits in the GetBitContext buffer in order to fill the 
> PutBitContext buffer.
> 
> The following probably fixes it, based on what you said.
> 
>> diff --git a/libavformat/iamf_parse.c b/libavformat/iamf_parse.c
>> index 71497876ac..19c549d4ac 100644
>> --- a/libavformat/iamf_parse.c
>> +++ b/libavformat/iamf_parse.c
>> @@ -288,7 +288,7 @@ static int update_extradata(AVCodecParameters 
>> *codecpar)
>>          uint8_t buf[6];
>>          int size = FFMIN(codecpar->extradata_size, sizeof(buf));
>>
>> -        init_put_bits(&pb, buf, size);
>> +        init_put_bits(&pb, buf, sizeof(buf));
>>          ret = init_get_bits8(&gb, codecpar->extradata, size);
>>          if (ret < 0)
>>              return ret;
>> @@ -312,6 +312,9 @@ static int update_extradata(AVCodecParameters 
>> *codecpar)
>>          put_bits(&pb, ret, get_bits_long(&gb, ret));
>>          flush_put_bits(&pb);
>>
>> +        if (get_bits_left(&gb) < 0)
>> +           return AVERROR_INVALIDDATA;
>> +
>>          memcpy(codecpar->extradata, buf, put_bytes_output(&pb));
>>          break;
>>      }

Actually no, that broke FATE.

This maybe:

> diff --git a/libavformat/iamf_parse.c b/libavformat/iamf_parse.c
> index 71497876ac..0bd4696b83 100644
> --- a/libavformat/iamf_parse.c
> +++ b/libavformat/iamf_parse.c
> @@ -288,7 +288,7 @@ static int update_extradata(AVCodecParameters *codecpar)
>          uint8_t buf[6];
>          int size = FFMIN(codecpar->extradata_size, sizeof(buf));
> 
> -        init_put_bits(&pb, buf, size);
> +        init_put_bits(&pb, buf, sizeof(buf));
>          ret = init_get_bits8(&gb, codecpar->extradata, size);
>          if (ret < 0)
>              return ret;
> @@ -304,7 +304,10 @@ static int update_extradata(AVCodecParameters *codecpar)
> 
>          skip_bits(&gb, 4);
>          put_bits(&pb, 4, codecpar->ch_layout.nb_channels); // set channel config
> -        ret = put_bits_left(&pb);
> +        ret = get_bits_left(&gb);
> +        if (ret < 0)
> +            return AVERROR_INVALIDDATA;
> +        ret = FFMIN(ret, put_bits_left(&pb));
>          while (ret >= 32) {
>             put_bits32(&pb, get_bits_long(&gb, 32));
>             ret -= 32;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature.asc
Type: application/pgp-signature
Size: 495 bytes
Desc: OpenPGP digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20250623/bc9d50e9/attachment.sig>