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

[Michael Niedermayer]

On Thu, Jul 25, 2019 at 05:55:02PM +0200, Paul B Mahol wrote:
> 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.

A maximum pixel count of 16M pixels that is 4k by 4k is part of
the interface code to the fuzzer since its initial commit into ffmpeg in
january 2017

Thanks

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Asymptotically faster algorithms should always be preferred if you have
asymptotical amounts of data
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20190726/65b3a9e8/attachment.sig>