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

[Michael Niedermayer <michael@niedermayer.cc>]

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