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

Michael Niedermayer michael at niedermayer.cc
Wed Mar 8 18:11:12 EET 2017


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.



[...]

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


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

side data is in libav maybe native structs, in FFmpeg it was bytestream
like packet data[], thats at least how i viewed it.



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

He who knows, does not speak. He who speaks, does not know. -- Lao Tsu
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170308/1ce78367/attachment.sig>


More information about the ffmpeg-devel mailing list