[FFmpeg-devel] [PATCH 1/3] Revert "avcodec: Add max_pixels options"

[wm4]

On Mon, 12 Dec 2016 21:00:19 +0100
Michael Niedermayer <michael at niedermayer.cc> wrote:

> On Mon, Dec 12, 2016 at 12:04:05PM +0100, Nicolas George wrote:
> > Le primidi 21 frimaire, an CCXXV, Michael Niedermayer a écrit :  
> > > You misunderstand
> > > 
> > > I want to find code that allocates too much memory where it should
> > > not.
> > > to give an example
> > > there was long ago some code like
> > > 
> > > len = read()
> > > for (i<len)
> > >     x= alloc()
> > >     x.whatever =read()
> > >     ...
> > > 
> > > Straight OOM here with a tiny input file.
> > > add a simple if(eof) in there and no OOM anymore
> > > this is just one example, code can look very diferent.
> > > 
> > > I want to find these cases and i want to fix them
> > > But what i get from the fuzzer is files with resolutions like
> > > 65123x3210 which go OOM because of valid but silly resolution.
> > > If i can limit the resolution then i can find the other issues
> > > which i can fix.
> > > If i cannot limit the resolution then i cannot fix the other issues
> > > as they are in a sea of OOMs from large resolution files
> > > 
> > > Nothing you can do at the OS level will get you this effect  
> > 
> > Thanks for explaining.
> > 
> > If I read this correctly, this option does not fix any security issue at
> > all, it only help you find other parts of the code that may contain
> > security issues. Am I right?  
> 
> its more complex
> there are really independant things this achievs
> 
> OOM is a security issue under some(1) circumstances, one can hit OOM due
> to too many pixels (or streams), this specific issue is fixed by the
> options

But the transcoding dies with the max_streams limit enforced too. On
the other hand, if your server doesn't run the ffmpeg transcoder process
in a sandbox, and that process dying due to OOM kills your server, and
in response makes your entire site unavailable AND causes some actual
security issue... uh, I don't know what to say. Just don't run a server
in this case.

> 
> completely independant of this, the option is needed to fuzz FFmpeg
> efficiently as andreas explained
> 
> and also completely independant of this, the option is needed to
> allow finding some fixable OOM bugs that i would like to fix

You can do that with a wrapper that makes malloc randomly fail.

> 
> (1) For example a server process that dies due to it. Even if restarted
> this can put load on the machine to be a effective was to DOS it
> 
> Also its certainly true that this does not fix every OOM issue but
> no bugfix that fixes a out of array read fixes every out of array read
> either

An array out of bounds read is a real issue that programmers try to
avoid. OOM on the other hand is a "normal" failure just as much as an
invalid file that can't be read.

> 
> 
> >   
> > > it is exceptionally unprofessional to publish testcases for CVEs
> > > before they have been fixed.
> > > Also more generally its the researchers choice/job to publish their
> > > work. If you belive it should be put in a ticket you should ask him
> > > not a 3rd party like me to do that.  
> > 
> > This is Free software, secrecy is not a good policy. "I have this patch
> > that fix a bug, but I can not show you the bug." Well, if the patch is
> > straightforward, we can accept it, but if the patch is not
> > straightforward, we need, collectively, to see the bug.
> > 
> > I can understand that if the bug is a critical 0-day exploit, some
> > leeway must be accepted. But "there is a file that triggers a crash" is
> > not enough by far.  
> 
> If it is neccessary to publish exploits then i belive all security
> support will end in FFmpeg and move elsewhere
> I cannot really speak for others but i belive that companies doing
> fuzzing and submit testcases will not submit them if that implies
> them being made public. Actually they can probably not even legally
> do that if they wanted it would massivly endanger their customers.
> 
> About this specific bug, as mentioned it has a CVE, it is on the public
> oss security list
> heres a link:
> http://www.openwall.com/lists/oss-security/2016/12/08/1

Causing this a vulnerability was already derided as nonsense.

Assuming it's a real OOM kill, and not a buggy OOM error handling path.
I can't really see this from the report.

> That seems more than adequate to understand this one reported case
> I can privatly share the sample with FFmpeg developers who work on
> writing an alterantive fix
> 
> [...]
> 
>