[FFmpeg-devel] [PATCH] avcodec/jpeg2000dwt: Fix left shift of negative number

[Andreas Rheinhardt]

Tomas Härdin:
> tis 2022-09-27 klockan 13:40 +0200 skrev Andreas Rheinhardt:
>> Tomas Härdin:
>>> tis 2022-09-27 klockan 01:11 +0200 skrev Andreas Rheinhardt:
>>>> Fixes the j2k-dwt FATE-test; also fixes #9945.
>>>> (I don't know whether the multiplication can overflow.)
>>>
>>> The 5/3 transform is used in lossless mode and therefore shouldn't
>>> overflow for normal use cases. But someone can of course craft a
>>> malicious file
>>>
>>>>
>>>> Signed-off-by: Andreas Rheinhardt
>>>> <andreas.rheinhardt at outlook.com>
>>>> ---
>>>>  libavcodec/jpeg2000dwt.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/libavcodec/jpeg2000dwt.c b/libavcodec/jpeg2000dwt.c
>>>> index f2da7307c4..34e33553f7 100644
>>>> --- a/libavcodec/jpeg2000dwt.c
>>>> +++ b/libavcodec/jpeg2000dwt.c
>>>> @@ -81,7 +81,7 @@ static void sd_1d53(int *p, int i0, int i1)
>>>>  
>>>>      if (i1 <= i0 + 1) {
>>>>          if (i0 == 1)
>>>> -            p[1] <<= 1;
>>>> +            p[1] *= 2;
>>>
>>> To trigger an actual overflow here you need enough coefficient bits
>>> and
>>> enough decomposition levels, meaning also huge resolution.
>>> Resolution
>>> is capped at what 32k x 32k currently? That means you need 17-bit
>>> coefficients at the lowest levels to get over INT_MAX. I'm not
>>> actually
>>> sure what the limits for that in jpeg2000 is, but 12-bit lossless
>>> would
>>> certaily hit these levels at 5 or more decomp levels. I have
>>> samples
>>> that use 6, and it's easy to generate ones that have even more.
>>>
>>
>> FYI: This is not triggered by an actual jpeg2000 sample (not even a
>> malicious one), this is triggered by the jpeg2000dwt test tool
> 
> Yeah, I had the test uncover some interesting bugs on my end when
> developing, that probably don't happen with real files. But malicious
> files potentially triggering UB is something we shouldn't ignore
> 
>>> To be really safe we'd need to use something like
>>> https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html
>>> and maybe define fallbacks for other compilers.
>>>
>>
>> Take a look at av_sat_add64_c() and similar functions.
> 
> We don't need saturation here, only that the behavior is not undefined.

Does this mean that this patch is ok?

> Wrapping around is fine. The only place where saturation is performed
> is when converting decoded and idwt'd coefficients to actual pixel data
> in write_frame_*()
> 
> It's possible that for sufficiently large 16-bit frames with enough
> decomposition levels that "lossless" encoding is not actually lossless
> unless the encoder uses 64-bit integers.
> 
> j2kenc supports RGB48, nreslevels=7, which can run into this problem at
> resolutions as low as 255x255 I think.
>