[FFmpeg-devel] [PATCH 1/4] avcodec/jpeg2000htdec: Check M_b / magp before using it in a shift

[Michael Niedermayer]

On Wed, Mar 27, 2024 at 11:13:48AM +0100, Tomas Härdin wrote:
> mån 2024-03-25 klockan 21:04 +0100 skrev Michael Niedermayer:
> > On Mon, Mar 25, 2024 at 08:13:13PM +0100, Michael Niedermayer wrote:
> > > On Thu, Mar 21, 2024 at 04:07:14PM +0100, Tomas Härdin wrote:
> > > > ons 2024-03-20 klockan 21:35 +0100 skrev Tomas Härdin:
> > > > > ons 2024-03-20 klockan 14:12 +0100 skrev Michael Niedermayer:
> > > > > > On Wed, Mar 20, 2024 at 12:20:11PM +0100, Tomas Härdin wrote:
> > > > > > > ons 2024-03-20 klockan 03:59 +0100 skrev Michael
> > > > > > > Niedermayer:
> > > > > > > > Fixes: shift exponent -1 is negative
> > > > > > > > Fixes: 65378/clusterfuzz-testcase-minimized-
> > > > > > > > ffmpeg_AV_CODEC_ID_JPEG2000_fuzzer-5457678193197056
> > > > > > > > 
> > > > > > > > Found-by: continuous fuzzing process
> > > > > > > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> > > > > > > > Signed-off-by: Michael Niedermayer
> > > > > > > > <michael at niedermayer.cc>
> > > > > > > > ---
> > > > > > > >  libavcodec/jpeg2000htdec.c | 3 +++
> > > > > > > >  1 file changed, 3 insertions(+)
> > > > > > > > 
> > > > > > > > diff --git a/libavcodec/jpeg2000htdec.c
> > > > > > > > b/libavcodec/jpeg2000htdec.c
> > > > > > > > index 6b9898d3ff..0b94bb5da2 100644
> > > > > > > > --- a/libavcodec/jpeg2000htdec.c
> > > > > > > > +++ b/libavcodec/jpeg2000htdec.c
> > > > > > > > @@ -1193,6 +1193,9 @@ ff_jpeg2000_decode_htj2k(const
> > > > > > > > Jpeg2000DecoderContext *s, Jpeg2000CodingStyle *c
> > > > > > > >  
> > > > > > > >      int32_t M_b = magp;
> > > > > > > >  
> > > > > > > > +    if (magp >= 31)
> > > > > > > > +        return AVERROR_INVALIDDATA;
> > > > > > > 
> > > > > > > This isn't where the error is, assuming it even is an
> > > > > > > error. It's
> > > > > > > either expn or nguardbits that are wrong, and they should
> > > > > > > be
> > > > > > > detected
> > > > > > > and reported as such in jpeg2000dec.c. Checking this in
> > > > > > > every
> > > > > > > call
> > > > > > > to
> > > > > > > ff_jpeg2000_decode_htj2k() is wasteful.
> > > > > > > 
> > > > > > > nguardbits can be 0..7 and expn can be 0..31. Table A.11
> > > > > > > indicates
> > > > > > > that
> > > > > > > Ssize can be up to 38 bits, so M_b >= 31 is in fact
> > > > > > > perfectly
> > > > > > > valid.
> > > > > > 
> > > > > > > A
> > > > > > > more appropriate error might be AVERROR_PATCHWELCOME.
> > > > > > 
> > > > > > indeed, i will change it to AVERROR_PATCHWELCOME
> > > > > 
> > > > > Please also move it further up so as to not waste cycles
> > > > > checking it
> > > > > every time
> > > > 
> > > > To be more precise, get_qcx() looks like the proper place for it
> > > 
> > > will apply with teh check moved there
> > 
> > the values that are causing undefined behavior for htj2k are used in
> > normal
> > j2k knowing which type of j2k we have seems decided by COC/COD/COX
> > 
> > so i dont think we can check in QCX, because a later COX could
> > make it both invalid or valid
> > and we cannot check in COX as a later QCX can similarly change it
> 
> That all calls get_qcx().

yes


> If you git grep for nguardbits you'll see
> it's only ever set there when decoding, and similarly with expn.

yes


> Coding
> style and quantization style are not the same thing.

yes


but still, you can try to add a check the values for both nguardbits
and expn which lead to undefined shifts in ff_jpeg2000_decode_htj2k()
are used in normal jpeg2000 and break these samples

because the same get_qcx() is used both for "normal" jpeg2000 and htj2k
so the check needs to know if its htj2k

inside ff_jpeg2000_decode_htj2k() thats obvious, in get_qcx() its not
one can use the coding style to tell them apart but thats not trivial
with the various ways to slice and dice things
and it might not be set when get_qcx() is run

if iam missing something please tell me how to know in get_qcx() that
teh data is only used for ht2jk
(Its very possible iam missing something given i did not read the hundreads
 of pages of spec, and mostly assume that its as insane as possible)

thx

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

If you think the mosad wants you dead since a long time then you are either
wrong or dead since a long time.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20240328/67509eea/attachment.sig>