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

[Michael Niedermayer]

On Wed, Mar 08, 2017 at 07:31:27PM +0100, wm4 wrote:
> On Wed, 8 Mar 2017 19:03:21 +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:    
> > > > > > > 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
> 
> You have to use whatever is correct in each specific case. Using a
> number or sizeof() argument for new_side_data is simply an API
> violation in some cases, similar to e.g. using
> av_malloc(sizeof(AVFrame)). There are a few.
> 

> I don't know why you want to "check" these uses with FATE. As I've said
> in the other thread, that's like letting FATE check sizeof(AVFrame).
> 
> The right way is to check it in new_side_data, or have an API that is
> not so hard to use incorrectly. This has been discussed before, when
> we added new functions to add display mastering data or something
> similar in an ABI-safe way.
> 
> > 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
> 
> And FATE-checking the sizes is the wrong thing to do. At least for the
> side data types whose byte layouts are defined by the C ABI like
> MASTERING_DISPLAY_METADATA, not by something wire-like as for
> example the SKIP_SAMPLES ones.

wrong thread or you totally misunderstand me

what you originally said:
> It looks like this could lead to security issues, as side data readers
> will for example rely on side data allocation sizes to be as large as
> needed for correct operation.

And what i replied:
    [...]

    also size checks are needed for the case where a lib gets replaced by
    one with newer version that has a bigger struct.

Now fact is that for cases where you link to a lib with a differently
sized struct or more generally any side data (which is what was meant)
if its not using an allocator it needs a check in the code or something
else, thats what i meant and thats from where this bizare sub
discussion started where you seem to keep disagreeing about things i
did not say

Iam not talking about fate here, thats a seperate thing


> 
> > 
> > 
> > > 
> > > 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.
> 
> I don't see much simplicity in code duplication, putting code into
> public headers (which also means you have to make sure this
> compiles with C++), reducing compile times, and potentially exposing
> implementation details.

It can be in a private header, it doesnt need to be public


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

Why not whip the teacher when the pupil misbehaves? -- Diogenes of Sinope
-------------- 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/bd2fa94c/attachment.sig>