[FFmpeg-devel] [PATCH 4/4] avcodec/apedec: Fix multiple integer overflows in filter_3800()

[Reimar Döffinger]

On 16.06.2019, at 17:12, Paul B Mahol <onemda at gmail.com> wrote:

> On 6/16/19, Reimar Döffinger <Reimar.Doeffinger at gmx.de> wrote:
>> 
>> 
>> On 16.06.2019, at 12:30, Lynne <dev at lynne.ee> wrote:
>> 
>>> Jun 16, 2019, 10:57 AM by michael at niedermayer.cc:
>>> 
>>>> Fixes: left shift of negative value -4
>>>> Fixes: signed integer overflow: -15091694 * 167 cannot be represented in
>>>> type 'int'
>>>> Fixes: signed integer overflow: 1898547155 + 453967445 cannot be
>>>> represented in type 'int'
>>>> Fixes:
>>>> 15258/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_APE_fuzzer-5759095564402688
>>>> 
>>>> 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/apedec.c | 16 ++++++++--------
>>>> 1 file changed, 8 insertions(+), 8 deletions(-)
>>>> 
>>>> diff --git a/libavcodec/apedec.c b/libavcodec/apedec.c
>>>> index 61ebfdafd5..f1bfc634c2 100644
>>>> --- a/libavcodec/apedec.c
>>>> +++ b/libavcodec/apedec.c
>>>> @@ -859,22 +859,22 @@ static av_always_inline int
>>>> filter_3800(APEPredictor *p,
>>>> return predictionA;
>>>> }
>>>> d2 =  p->buf[delayA];
>>>> -    d1 = (p->buf[delayA] - p->buf[delayA - 1]) << 1;
>>>> -    d0 =  p->buf[delayA] + ((p->buf[delayA - 2] - p->buf[delayA - 1]) <<
>>>> 3);
>>>> +    d1 = (p->buf[delayA] - p->buf[delayA - 1]) * 2;
>>>> +    d0 =  p->buf[delayA] + ((p->buf[delayA - 2] - p->buf[delayA - 1]) *
>>>> 8);
>>>> d3 =  p->buf[delayB] * 2 - p->buf[delayB - 1];
>>>> d4 =  p->buf[delayB];
>>>> 
>>>> -    predictionA = d0 * p->coeffsA[filter][0] +
>>>> -                  d1 * p->coeffsA[filter][1] +
>>>> -                  d2 * p->coeffsA[filter][2];
>>>> +    predictionA = d0 * (unsigned)p->coeffsA[filter][0] +
>>>> +                  d1 * (unsigned)p->coeffsA[filter][1] +
>>>> +                  d2 * (unsigned)p->coeffsA[filter][2];
>>>> 
>>>> sign = APESIGN(decoded);
>>>> p->coeffsA[filter][0] += (((d0 >> 30) & 2) - 1) * sign;
>>>> p->coeffsA[filter][1] += (((d1 >> 28) & 8) - 4) * sign;
>>>> p->coeffsA[filter][2] += (((d2 >> 28) & 8) - 4) * sign;
>>>> 
>>>> -    predictionB = d3 * p->coeffsB[filter][0] -
>>>> -                  d4 * p->coeffsB[filter][1];
>>>> +    predictionB = d3 * (unsigned)p->coeffsB[filter][0] -
>>>> +                  d4 * (unsigned)p->coeffsB[filter][1];
>>>> p->lastA[filter] = decoded + (predictionA >> 11);
>>>> sign = APESIGN(p->lastA[filter]);
>>>> p->coeffsB[filter][0] += (((d3 >> 29) & 4) - 2) * sign;
>>>> @@ -902,7 +902,7 @@ static void long_filter_high_3800(int32_t *buffer,
>>>> int order, int shift, int len
>>>> dotprod = 0;
>>>> sign = APESIGN(buffer[i]);
>>>> for (j = 0; j < order; j++) {
>>>> -            dotprod += delay[j] * coeffs[j];
>>>> +            dotprod += delay[j] * (unsigned)coeffs[j];
>>>> coeffs[j] += ((delay[j] >> 31) | 1) * sign;
>>>> }
>>>> buffer[i] -= dotprod >> shift;
>>>> 
>>> 
>>> This code is DSP, overflows should be allowed in case input is corrupt.
>> 
>> Then get the C standard changed or use a different language.
>> But in C overflows on signed types absolutely are not Ok.
> 
> I disagree, overflows in DSP are normal.

It's simply incorrect code.
Yes, you will probably get lucky for a long time.
But there are ISAs that trap on overflow, and gcc can trap on overflow.
That already makes these cases a denial-of-service issue.
However it gets far worse than that.
All it takes is for code to do something like
if (x < 2048) some_array[x] = y;
(lots of code followed by DSP code doing)
x * (1 << 24)

and now the compiler can and actually might really remove the if condition and you have an exploitable buffer overflow.
People have been bitten by such issues often enough (because due to LTO compiler optimizations of this kind have potentially unlimited scope, whereas reviewer have almost not chance of catching something this subtle) that such assumptions violating the C standard should be avoided.
(besides the more immediate point that fuzzing does catch quite real issues, and leaving such well-maybe-it's-ok cases in prevents finding real issues - on its own it might be an argument to change to tools, but due to the above it is reasonable to consider the tools doing the right thing).