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

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

[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@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;
-- 
2.49.0

_______________________________________________
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] [PATCH 2/4] tools/target_dec_fuzzer: Adjust RV60 threshold

[Michael Niedermayer]

Fixes: Timeout
Fixes: 410324670/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_RV60_fuzzer-5697706586865664

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 tools/target_dec_fuzzer.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/target_dec_fuzzer.c b/tools/target_dec_fuzzer.c
index 9e4d5bd64b6..f5f41bdb247 100644
--- a/tools/target_dec_fuzzer.c
+++ b/tools/target_dec_fuzzer.c
@@ -292,6 +292,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) {
     case AV_CODEC_ID_RASC:        maxpixels  /= 256;   break;
     case AV_CODEC_ID_RTV1:        maxpixels  /= 16;    break;
     case AV_CODEC_ID_RV30:        maxpixels  /= 256;   break;
+    case AV_CODEC_ID_RV60:        maxpixels  /= 256;   break;
     case AV_CODEC_ID_SANM:        maxpixels  /= 16;    break;
     case AV_CODEC_ID_SCPR:        maxpixels  /= 32;    break;
     case AV_CODEC_ID_SCREENPRESSO:maxpixels  /= 64;    break;
-- 
2.49.0

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

Re: [FFmpeg-devel] [PATCH 2/4] tools/target_dec_fuzzer: Adjust RV60 threshold

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 720 bytes --]

On Thu, Jun 19, 2025 at 05:04:29AM +0200, Michael Niedermayer wrote:
> Fixes: Timeout
> Fixes: 410324670/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_RV60_fuzzer-5697706586865664
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  tools/target_dec_fuzzer.c | 1 +
>  1 file changed, 1 insertion(+)

will apply

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Whats the most studid thing your enemy could do ? Blow himself up
Whats the most studid thing you could do ? Give up your rights and
freedom because your enemy blew himself up.


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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] [PATCH 3/4] avcodec/sanm: Check w, h for subversion < 2

[Michael Niedermayer]

Fixes: 410609432/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_SANM_fuzzer-4935159201988608
Fixes: out of array access

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/sanm.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/libavcodec/sanm.c b/libavcodec/sanm.c
index 642ef02bff2..02bb78859a8 100644
--- a/libavcodec/sanm.c
+++ b/libavcodec/sanm.c
@@ -1670,6 +1670,8 @@ static int process_frame_obj(SANMVideoContext *ctx, GetByteContext *gb)
             /* Rebel Assault 1: 384x242 internal size */
             xres = 384;
             yres = 242;
+            if (w > xres || h > yres)
+                return AVERROR_INVALIDDATA;
             ctx->have_dimensions = 1;
         } else if (codec == 37 || codec == 47 || codec == 48) {
             /* these codecs work on full frames, trust their dimensions */
-- 
2.49.0

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

Re: [FFmpeg-devel] [PATCH 3/4] avcodec/sanm: Check w, h for subversion < 2

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 689 bytes --]

Hi Manuel

On Thu, Jun 19, 2025 at 05:04:30AM +0200, Michael Niedermayer wrote:
> Fixes: 410609432/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_SANM_fuzzer-4935159201988608
> Fixes: out of array access
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/sanm.c | 2 ++
>  1 file changed, 2 insertions(+)

is this and the next patch correct ?
please review

thx

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The worst form of inequality is to try to make unequal things equal.
-- Aristotle

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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] [PATCH 4/4] avcodec/sanm: Check thet left/top is within the w/h

[Michael Niedermayer]

Fixes: 423673969/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_SANM_fuzzer-5466731806261248
Fixes: out of array access

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/sanm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavcodec/sanm.c b/libavcodec/sanm.c
index 02bb78859a8..df248ec3547 100644
--- a/libavcodec/sanm.c
+++ b/libavcodec/sanm.c
@@ -1649,7 +1649,7 @@ static int process_frame_obj(SANMVideoContext *ctx, GetByteContext *gb)
     bytestream2_skip(gb, 2);
     parm2 = bytestream2_get_le16u(gb);
 
-    if (w < 1 || h < 1 || w > 800 || h > 600 || left > 800 || top > 600) {
+    if (w < 1 || h < 1 || w > 800 || h > 600 || left >= w || top >= h || left < 0 || top < 0) {
         av_log(ctx->avctx, AV_LOG_WARNING,
                "ignoring invalid fobj dimensions: c%d %d %d @ %d %d\n",
                codec, w, h, left, top);
-- 
2.49.0

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

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

[Andreas Rheinhardt]

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

- Andreas

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

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

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 1941 bytes --]

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@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

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

You can kill me, but you cannot change the truth.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

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

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

[James Almer]

[-- Attachment #1.1.1: Type: text/plain, Size: 2011 bytes --]

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


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

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

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

[Andreas Rheinhardt]

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@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?

- Andreas

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

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

[James Almer]

[-- Attachment #1.1.1: Type: text/plain, Size: 3828 bytes --]

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@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;
>      }



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

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

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

[James Almer]

[-- Attachment #1.1.1: Type: text/plain, Size: 5240 bytes --]

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



[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

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

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

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 2717 bytes --]

On Mon, Jun 23, 2025 at 09:47:55AM -0300, James Almer wrote:
> 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@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.

i set buffer to 1000 and it still fails with put_bits_left()
returning -19

what will fix this specific case is:
-        if (ret == 0x0f)
-            put_bits(&pb, 24, get_bits(&gb, 24));
+        if (ret == 0x0f) {
+            if (get_bits_left(&gb) >= 24+4+4)
+                put_bits(&pb, 24, get_bits(&gb, 24));
+        }


thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

What does censorship reveal? It reveals fear. -- Julian Assange

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

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

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

[Michael Niedermayer]

[-- Attachment #1.1: Type: text/plain, Size: 2926 bytes --]

On Mon, Jun 23, 2025 at 04:19:52PM +0200, Michael Niedermayer wrote:
> On Mon, Jun 23, 2025 at 09:47:55AM -0300, James Almer wrote:
> > 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@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.
> 
> i set buffer to 1000 and it still fails with put_bits_left()
> returning -19
> 
> what will fix this specific case is:
> -        if (ret == 0x0f)
> -            put_bits(&pb, 24, get_bits(&gb, 24));
> +        if (ret == 0x0f) {
> +            if (get_bits_left(&gb) >= 24+4+4)

this should be only one +4

thx

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Those who would give up essential Liberty, to purchase a little
temporary Safety, deserve neither Liberty nor Safety -- Benjamin Franklin

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

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