[FFmpeg-devel] [PATCH] avcodec/apedec: use int64_t for FFABS

[Ganesh Ajjanagadde]

On Mon, Oct 12, 2015 at 11:43 AM, Michael Niedermayer
<michael at niedermayer.cc> wrote:
> On Mon, Oct 12, 2015 at 11:22:17AM -0400, Ganesh Ajjanagadde wrote:
>> On Mon, Oct 12, 2015 at 11:09 AM, Michael Niedermayer
>> <michael at niedermayer.cc> wrote:
>> > On Fri, Oct 09, 2015 at 01:48:10PM -0400, Ganesh Ajjanagadde wrote:
>> >> res, absres are currently int's, which on most platforms is 32 bits.
>> >> Unfortunately, data is untrusted, and on line 1267 res is manipulated
>> >> with data. Thus, res can take on INT32_MIN/INT_MIN with crafted data,
>> >> making FFABS on line 1282 unsafe.
>> >>
>> >> Once again, using FFNABS will make it less readable: logic is less
>> >> clear, diff is bigger, and there is scope for mistakes during the
>> >> expression rewrites.
>> >>
>> >> Tested with FATE.
>> >>
>> >> Signed-off-by: Ganesh Ajjanagadde <gajjanagadde at gmail.com>
>> >> ---
>> >>  libavcodec/apedec.c | 4 ++--
>> >>  1 file changed, 2 insertions(+), 2 deletions(-)
>> >>
>> >> diff --git a/libavcodec/apedec.c b/libavcodec/apedec.c
>> >> index fcccfbe..e46558e 100644
>> >> --- a/libavcodec/apedec.c
>> >> +++ b/libavcodec/apedec.c
>> >> @@ -1254,8 +1254,8 @@ static void init_filter(APEContext *ctx, APEFilter *f, int16_t *buf, int order)
>> >>  static void do_apply_filter(APEContext *ctx, int version, APEFilter *f,
>> >>                              int32_t *data, int count, int order, int fracbits)
>> >>  {
>> >> -    int res;
>> >> -    int absres;
>> >> +    int64_t res;
>> >> +    int64_t absres;
>> >
>> > this makes the function slower:
>> > before: 1636364 decicycles in doapply,     128 runs,      0 skips
>> > after:  1710778 decicycles in doapply,     128 runs,      0 skips
>> >
>> > (this is on x86-64, i assume its a bigger difference on 32bit)
>> >
>> > tested with:
>> > ./ffmpeg -i fate-suite//lossless-audio/luckynight-partial.ape -f null -
>>
>> Ok, but don't you agree that overflow is possible? int16 (essentially
>> the width after the scalar product) + int32 may not fit in int32. If
>> we want a lossless conversion, int64 must be used; there is no other
>> option AFAIK.
>
> the max output supported looks like 24 bits not 32
>
> also whatever the official decoder / encoder do is correct
> i dont know what they do, but for example if the encoder uses a
> int32 and that overflows then any lossless decoder must replicate that.

Ah, the joys of multimedia.

>
> so i cant say just from knowing that there is a overflow that it is
> neccessarily wrong, it would require a testcase or comparission to
> what the official encoder(/decoder) do
>
> also if >32bit is possible then av_clip_int16(res); and
> res & INT32_MIN look suspect
>
> i dont know what is correct, as i didnt check the official code nor
> have a testcase ...

I think this is a good one for the friendly fuzzers at Google (they
still seem to be helping us out), Can you contact them and see if they
can craft data that causes overflow issues? On any sane implementation
of integer overflow, this should not be a security vulnerability
capable of exploitation (these integers are not being used to index a
buffer for instance). Nevertheless, I think it is worthy of
investigation on their end.

>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> DNS cache poisoning attacks, popular search engine, Google internet authority
> dont be evil, please
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>