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

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

[Andreas Rheinhardt]

Fixes the j2k-dwt FATE-test; also fixes #9945.
(I don't know whether the multiplication can overflow.)

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@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;
         return;
     }
 
-- 
2.34.1

_______________________________________________
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] avcodec/jpeg2000dwt: Fix left shift of negative number

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

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.

This is part of why I've been harping about formal verification.

/Tomas

_______________________________________________
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] avcodec/jpeg2000dwt: Fix left shift of negative number

[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@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
(libavcodec/tests/jpeg2000dwt.c). It might entirely be possible that the
tool tests something that can't happen during decoding (and not only
during decoding of spec-compliant files).

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

> This is part of why I've been harping about formal verification.
> 

_______________________________________________
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] avcodec/jpeg2000dwt: Fix left shift of negative number

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

/Tomas

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

_______________________________________________
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] avcodec/jpeg2000dwt: Fix left shift of negative number

[Tomas Härdin]

tis 2022-09-27 klockan 21:20 +0200 skrev 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@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?

Sure, it's better than before

/Tomas

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