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

[Lynne]

Jul 25, 2019, 4:47 PM by michael at niedermayer.cc:

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

I told you its up to you to find them. I am sorry if I was rude however none of your timeout patches have shown any lenience, regardless of how short of a timeout they addressed.
As such, I can't help but feel like you've never thought of what the side effects of the patches were. Just that the fuzzer's random opinion on what a timeout is was more important.



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

Stop trying to involve undefined behavior to derail discussion, I asked you a series of questions regarding the timeouts.
I disagree with the fuzzer's opinion on what's considered a timeout, and with your fixes of those so-called issues.
I don't think that patches which fix timeouts less than 3 seconds or so are worth fixing for the trouble it is to prove they don't affect anything else, unless its plainly obvious. And I don't think timeout fixes should be affected by any options, like the damage percentage one. That's 100% clearly just shutting the fuzzer up, and absolutely nothing else, since that's not something anyone sets ever.
Let the fuzzer publish those 3-second "timeouts", big deal. As I've said before, there are much more high profile vulnerabilities that can't be fixed without breaking some files which pretty much everyone knows about, yet no exploits have been made for.
As for the fuzzer not finding more timeouts, that's the fuzzer's problem, not ours. It shouldn't have called wolf on all little timeouts it encounters. Or it should filter that "timeout", like Coverity when it makes mistakes. Though unlike Coverity, the fuzzer is a black box that's unfixable, and one more point against it as a tool that shouldn't be trusted.
Hence I am asking you to exercise discretion when writing and posting those patches and filter insignificant ones.