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

wm4 nfxjfg at googlemail.com
Wed Mar 8 18:26:57 EET 2017

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:  
> > > > It looks like this could lead to security issues, as side data readers    
> [...]
> > > also size checks are needed for the case where a lib gets replaced by
> > > one with newer version that has a bigger struct.  
> > 
> > Oh really? We never do this. Normal API structs are also considered
> > appendable, so compiling against a newer API and then linking against
> > an older version doesn't work. This is exactly the same case.  
> no its not
> what you call normal structs are allocated by an allocator that is
> part of the lib that defines it, the struct, and lib dependancies
> ensure that its new. Any allocated struct as a result is large
> enough though possibly not every field is set
> with side data the code using it sets the size explicitly that makes
> the size generally hardcoded in the lib using the code theres no
> longer a common allocator (some exceptions exist).
> The size a lib allocates that way is the
> compile time sizeof() which may differ from another lib
> and side data can be passed in both directions between libs not just
> in the direction of their dependancy
> so you can end up with a smaller side data and that means you have to
> do checks.

This is wrong. side data which has structs have corresponding functions
to get their allocation size. Of course that's all very error prone and
hard to use correctly and some were added only recently because the
API had holes, but that's how the libav* APIs are for now.

Besides, manually checking struct sizes/allocations makes for an even
worse ABI compatibility concept than FFmpeg currently follows. (Worse
as in ease of use and accidental incompatibilities and UB triggered as

> [...]
> > >   
> > > > +            && av_packet_split_side_data(pkt) == 1) {    
> > > 
> > > this could fail with ENOMEM which complicates things  
> > 
> > I can add a check for ENOMEM too. This should be the only thing that
> > looks like a failure, but could work in a separate call (like the one
> > on the libavcodec side).
> >   
> > >   
> > > if we do block such packets, its prbobably better to add a new
> > > static inline function to a header that checks if a packet has
> > > splitable side data and use that in av_packet_split_side_data() and
> > > here  
> > 
> > Not sure what you mean here.
> >   
> > > Iam suggesting "static inline" here to make backporting easier so no
> > > new symbol is added to libavcodec that is needed by libavformat  
> > 
> > What's the purpose?  
> Its simpler, cleaner and faster

I mean of that function, or why we should care about symbols present.

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

> side data is in libav maybe native structs, in FFmpeg it was bytestream

No. FFmpeg has at least 1 struct-defined side data that Libav doesn't
have at all (mastering display data).

> like packet data[], thats at least how i viewed it.

You may have viewed it this way, but that's not how it actually is.
Like I said in my another reply, even FFmpeg code occasionally uses
side data without checking the side data size.

Also, in Libav side data contents cannot come directly from raw
files, and thus has potentially fewer security issues than FFmpeg.

More information about the ffmpeg-devel mailing list