[FFmpeg-devel] [PATCH] avcodec/rl2: set dimensions

[Paul B Mahol]

On 7/25/19, Michael Niedermayer <michael at niedermayer.cc> wrote:
> On Wed, Jul 24, 2019 at 02:42:24PM +0200, Lynne wrote:
>> Jul 24, 2019, 11:08 AM by michael at niedermayer.cc:
>>
>> >
>> > What did you expect ? IIRC you have asked for whole classes of security
>> > issues to be not fixed.
>> >
>> > Something like that would require a vote and majority of developers.
>> >
>>
>> The way I interpret this: "Of course I ignored you, you're mental!",
>> doesn't help. And I don't think its just me.
>
> You are reading something into this that i have never meant or written
>
>
>> And you do remember incorrectly in saying that I want this whole class of
>> security issues not fixed. In this thread I specifically raised the issue
>> of what is considered to be a security issue by asking whether a speedup
>> of failing to decode from 2 to 0.4 seconds is considered such or what's
>> considered acceptable in general.
>> And I think I'll disagree that it is. 16 seconds to 2 seconds I can
>> accept, but not 2 to 0.4.
>
> These durations are the testcases found by the fuzzer, they say nothing
> about
> what the worst case for an issue is.
> The fuzzer builds a testcase trying to exceed a timeout it stops trying to
> "improve" it once it found something that takes a few seconds.
>
> You can in general make these cases significantly longer running.
>
> The reason why the fuzzer doesnt produce hour or day long timeouts is just
> because
> it doesnt search for anything longer than a few seconds.
>
>
>>
>>
>>
>> >> These patches affect decoding of real world broken files in favor of
>> >> fixing specially crafted fuzzed files.
>> >>
>> >
>> > Iam happy to look into such cases. Can you provide me with such
>> > "real world broken files"?
>> > Its not intended to worsen the output from such files
>> >
>>
>> Simple logical analysis, "if file is somewhat broken, don't try decoding"
>> does very well indicate that it won't only apply for _this_ broken file,
>> but in general.
>> Thus, this is for you to prove. I've said it before that otherwise its a
>> burden to other developers to have to screen all of these patches.
>
> The changes i do in general, i think about potential effects on
> slightly broken files and try to test with what iam able to find as
> matching
> input material.
> I find it a bit rude from you that you assume i would not already consider
> this
> case.
> Do i never make a mistake ? well i wish so but iam a human. So again if you
> know of specific cases where theres a problem, tell me about them please
>
>
>>
>>
>>
>> >> Sure, protecting against ddos attacts is important, but not important
>> >> enough to make decoders give up early and return nothing. Especially in
>> >> cases where the timeout speedup is of the order of 2s to 400ms.
>> >> Yet in all of those timeout patches all you've cared about is shutting
>> >> up the tool. You've never once shown any figures if this could affect
>> >> decoding, because its a lot harder than just showing they fix something
>> >> some tool calls a timeout and forgetting about it. You haven't even
>> >> commented on this when I asked you on IRC.
>> >> You also sneak this type of patches in when there's an overflow later
>> >> on during decoding, which is completely incorrect in almost all cases,
>> >> for the same reason above.
>> >>
>> >
>> > if you know of issues in a patch or commit you should report this
>> > during patch review or as soon as you find out about the issue
>> > as a reply to that patch or commit or as mail to the author.
>> >
>>
>> That's what I'm doing.
>> That aside, you've completely ignored my statements on what's considered
>> acceptable, showing figures, and sneaking this type of patches to fix
>> undefined behavior.
>> Making your reply a simple refutation, rather than addressing anything
>> I've said.
>> So I'm asking you again, what is considered a security issue and what is
>> considered acceptable? And what is considered not a security issue but a
>> complaint from an overzealous automated tool.
>
> undefined behavior is unacceptable. Its not allowed in C. It doesnt matter
> here if its a security issue or not.
>
> Timeouts can in general be used for denial of service attacks. While this
> is less critical than many other security issues it is a security issue.
> Also for Timeouts many point to bugs, to missing end of input checks, to
> missing
> checks in or before loops, to missing EOF checks, to missing checks that
> the input actually contains enough data resembling a fraction of the
> smallest
> half valid frame.
>
> We can spend many hours and days arguing if a issue is critical enough to
> be
> a security issue. maybe the one we would look at is not but then i still
> would
> try to fix it.
> If we dont fix one it will block the fuzzer from finding another timeout
> issue in the same decoder. And that one could be security relevant.
> So fixing as many as possible is the awnser here too
>
> About the fuzzer, if it reports a timeout then there is a timeout,
> if it reports undefined behavior then there is undefined behavior.
> Always ? no, it contained bugs but that is rather uncommon.
>
> Also the fuzzer has no mercy with you, you dont fix some issue, it will be
> made public and security researchers will look into how to exploit it
> eventually. If you didnt have the time or manpower, or deadlocked
> yourself with arguments the fuzzer doesnt care it has a clock and when
> thats
> up it publishes all details of an issue.


Correct fix for fuzzer is to not fuzz incredible big video sizes.

>
> Thanks
>
> [...]
>
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> If you think the mosad wants you dead since a long time then you are either
> wrong or dead since a long time.
>