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

Michael Niedermayer michael at niedermayer.cc
Wed Mar 8 20:03:21 EET 2017


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

you talk about something else here.

fact is the allocated side data uses hardcoded size values often
anyone can look at
git grep -A3 new_side_data

theres is sizeof() use and there are litteral numbers also

if these ever change, checks on the size become needed
which was the original thing i meant in this sub argumentation.
checks are needed, or something else in their place
is needed in that case



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

> > 
> > 
> > [...]
> > 
> > > >   
> > > > > +            && 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.

i think i explained this but ill try again all more verbosly

using a functiom to check for splitable sidedata instead of
spliting the side data is cleaner as we dont want to split it we just
want to check if theres any.

Its simpler as we dont have to deal with errors from the split code
and also dont need to deal with the case that split happened.

its faster as a static inline function as it would be inlined

its easier to backport as theres no new symbol added to another lib
on which libavformat depends. (that is if its static inline vs non
static)
If we add a new symbol we should bump minor but we cannot do that in
many past releases due to master using the next minor.Its easier thus
with no new symbol.


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

The misfortune of the wise is better than the prosperity of the fool.
-- Epicurus
-------------- 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/0f0bb143/attachment.sig>


More information about the ffmpeg-devel mailing list