[FFmpeg-devel] [PATCH 2/3] avformat: reject FFmpeg-style merged side data in raw packets

[wm4]

On Thu, 9 Mar 2017 12:00:38 +0100
Michael Niedermayer <michael at niedermayer.cc> wrote:

> On Thu, Mar 09, 2017 at 07:50:14AM +0100, wm4 wrote:
> > On Wed, 8 Mar 2017 23:34:39 +0100
> > Michael Niedermayer <michael at niedermayer.cc> wrote:
> >   
> > > On Wed, Mar 08, 2017 at 09:01:32PM +0100, wm4 wrote:  
> > > > On Wed, 8 Mar 2017 20:54:43 +0100
> > > > Michael Niedermayer <michael at niedermayer.cc> wrote:
> > > >     
> > > > > On Wed, Mar 08, 2017 at 07:32:26PM +0100, wm4 wrote:    
> > > > > > On Wed, 8 Mar 2017 19:20:15 +0100
> > > > > > Michael Niedermayer <michael at niedermayer.cc> wrote:
> > > > > >       
> > > > > > > On Wed, Mar 08, 2017 at 05:26:57PM +0100, wm4 wrote:      
> > > > > > > > On Wed, 8 Mar 2017 17:11:12 +0100
> > > > > > > > Michael Niedermayer <michael at niedermayer.cc> wrote:
> > > > > > > >         
> > > > > > > > > On Wed, Mar 08, 2017 at 04:06:20PM +0100, wm4 wrote:        
> > > > > > > > > > On Wed, 8 Mar 2017 15:36:25 +0100
> > > > > > > > > > Michael Niedermayer <michael at niedermayer.cc> wrote:
> > > > > > > > > >           
> > > > > > > > > > > On Wed, Mar 08, 2017 at 01:40:11PM +0100, wm4 wrote:          
> > > > > > > [...]      
> > > > > > > > >        
> > > > > > > > > >           
> > > > > > > > > > > also it may be interresting to disable this check for fuzzing so
> > > > > > > > > > > side data can be fuzzed in a wider range of cases and any past
> > > > > > > > > > > testcases that happen to use this can still be used for regression
> > > > > > > > > > > testing          
> > > > > > > > > > 
> > > > > > > > > > I think what you want is fault injection for memory errors, seems out
> > > > > > > > > > of scope here.          
> > > > > > > > > 
> > > > > > > > > no, i want fuzzing to continue to fuzz side data, it did so in the
> > > > > > > > > past and it should continue to do so.        
> > > > > > > > 
> > > > > > > > You can fuzz side data as much as you can fuzz AVFrame or
> > > > > > > > AVCodecContext. I believe randomly changing in-memory data structures
> > > > > > > > is referred to as fault injection, not fuzzing.        
> > > > > > > 
> > > > > > > it doesnt really matter what you call it, but it was done and the
> > > > > > > patch breaks it if theres no option to disable it or something else      
> > > > > > 
> > > > > > Well, you can't do it anymore. Why are you so afraid that a potential
> > > > > > error source is being eliminated?      
> > > > > 
> > > > > well its rather hidden, harder to test for not eliminated.    
> > > > 
> > > > How is it not eliminated?    
> > > 
> > > every side data we support can be generated by something otherwise  
> > 
> > Like what? You're not answering, just spouting confused stuff. What
> > kind of reply is this? 
> >   
> > > we wouldnt have that side data type  
> > 
> > ????????????????  
> 
> This is very basic really but lets elaborate
> for each side data type T
> possiblity A
> nothing uses side data type T
> 
> possiblity B
> something uses side data type T
> 
> 
> Its the same with a codec, either a codec is used in some case or
> its used in no case.
> 
> If something is used in no case then it has been eliminated as you
> describe.
> If somehing is still used in a case it has not been eliminated
> 
> If as you describe side data has been eliminated then you could
> remove side data as a whole from the source code.
> 
> If you cannot remove side data or a specific side data type from
> the source code then it has not been eliminated
> 
> your change removes one way for an attacker to set side data but
> by the fact that you dont remove any of the side data types its
> clear you are aware of that every is still in use in some code path.
> 
> a attacker may need to use a specific container format to set a
> specific side data type or may depend on a specific demuxer lib or
> application that allows him to set a side data type.
> 
> now if you remove every way to set side data for an attacker then
> you can remove that side data type as a whole from the code.
> Of course that removes whatever the side data is for.
> 
> Let me provide a specific example
> If a container suports changing extradata mid stream it will either
> be support or not.
> if any demuxer supports it then you have not eliminated the possiblity
> for an attacker
> 
> I hope writing a elaborate reply will not lead to this discussion
> to shift onto some unrelated detail

OK, but that has absolutely nothing to do with the issue at hand.

Of course side data can still be used after this patch!

Now if you could get back on topic...