[FFmpeg-devel] [PATCH 2/3] avcodec/lagarith: Optimize case with singleton probability distribution

[Paul B Mahol]

On 12/25/18, Paul B Mahol <onemda at gmail.com> wrote:
> On 12/25/18, Michael Niedermayer <michael at niedermayer.cc> wrote:
>> On Mon, Dec 24, 2018 at 11:54:31PM +0000, Kieran Kunhya wrote:
>>> >
>>> > commit 0ca7a8deeffd33e05ae15a447259b32b6678c727 (HEAD -> master)
>>> > Author: Michael Niedermayer <michael at niedermayer.cc>
>>> > Date:   Mon Dec 24 01:14:50 2018 +0100
>>> >
>>> >     avcodec/lagarith: Optimize case with singleton probability
>>> > distribution
>>> >
>>> >     Fixes: Timeout
>>> >     Fixes:
>>> > 10554/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_LAGARITH_fuzzer-5739938067251200
>>> >
>>> >     In case of a Denial of Service attack, the attacker wants to
>>> > maximize
>>> > the load on the target
>>> >     per byte transmitted from the attacker.
>>> >     For such a DoS attack it is best for the attacker to setup the
>>> > probabilities so that the
>>> >     arithmetic decoder does not advance in the bytestream that way the
>>> > attacker only needs to
>>> >     transmit the initial bytes and header for an arbitrary large
>>> > frame.
>>> >     This patch here optimizes this codepath and avoids executing the
>>> > arithmetic decoder more than
>>> >     once. It thus reduces the load causes by this codepath on the
>>> > target.
>>> >     We also could completely disallow this codepath but it appears
>>> > such
>>> > odd probability
>>> >     distributions are not invalid.
>>> >
>>> >     Found-by: continuous fuzzing process
>>> > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
>>> >     Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
>>> >
>>>
>>> This is a nonsense argument, a user could send a frame that was
>>> 99999999x99999999 in dimensions, would have the same effect.
>>
>> This suggested attack would not work, a user wanting to minimize these
>> DoS issues would have set AVCodecContext.max_pixels which would block
>> this.
>>
>>
>>> The calling application should manage timeouts themselves in a sandbox
>>> or
>>> container or similar.
>>
>> Its always possible and also a very good idea to have a 2nd line of
>> defense
>> like a sandbox / VM, ... as you suggest here, I did and do agree here.
>> And also a 3rd line of defense, ...
>>
>> But this doesnt mean we should not attempt to fix or mitigate
>> security (or other) issues directly in the code.
>>
>> I think the point you are raising has been raised previously, so let me
>> argue a little broader here and not specific to just what you suggest.
>>
>> If you compare a native fix in the code with a fix by a timeout, a
>> fix by a timeout causes:
>> * The whole process to be killed, so any application using libavcodec
>>   would basically "crash" and would not neccessarily save its state,
>>   flush out buffers, write any trailers or do proper protocol shutdowns
>>   or save any unsafed data. This is a outcome that should be minimized
>> * Using a timeout as the main way to block DoS is difficult as there is
>>   often no good timeout value. Its not unexpected that a system may need
>> to
>>   support processing large videos taking several hours, thats a long
>>   time for a file of a hundread bytes in a DoS attack
>>   100bytes from an attacker could cause the same load as 1000000000 bytes
>> from
>>   a user.
>> * Worst maybe is that a tight timeout likely makes the system more
>> vulnerable
>>   not less. because during an attack all the processes would likely slow
>>   down and real users would be pushed into the timeout even if the system
>>   without the user processes timeouting would still function correctly
>>
>> Iam sure there are more arguments but above are the ones that came to my
>> mind
>> ATM.
>>
>>>
>>> Merry Xmas.
>>
>> Merry Xmas as well!
>>
>> [...]
>> --
>> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>>
>> Those who would give up essential Liberty, to purchase a little
>> temporary Safety, deserve neither Liberty nor Safety -- Benjamin Franklin
>>
>
> This is still missing numbers/statistics.
>

And comments explaining added code.