[FFmpeg-devel] [PATCH 2/2] avformat: add protocol_whitelist

[Michael Niedermayer]

On Sun, Jan 24, 2016 at 12:47:59PM +0100, Nicolas George wrote:
> Le quintidi 5 pluviôse, an CCXXIV, Michael Niedermayer a écrit :
> > --- a/libavformat/avio.h
> > +++ b/libavformat/avio.h
> > @@ -595,6 +595,10 @@ int avio_open(AVIOContext **s, const char *url, int flags);
> >  int avio_open2(AVIOContext **s, const char *url, int flags,
> >                 const AVIOInterruptCB *int_cb, AVDictionary **options);
> >  
> > +int avio_open_whitelist(AVIOContext **s, const char *url, int flags,
> > +                         const AVIOInterruptCB *int_cb, AVDictionary **options,
> > +                         const char *whitelist);
> > +
> 
> Please no! Adding new arguments to a function that already has too many and

the argument is added, not out of strict need, (there already is a
AVDictionary that can be used) but to make it clear that the author
set the whitelist correctly. Also it simplifies the code compared
to using the AVDictionary.

security relevant code should be clear and explicit so that a
mistake in the code can easily be spotted. Its very hard to spot that
a passed dictionary doesnt carry/lost the whitelist, easy to spot that
a argument is missed and set to NULL


> using a string as a structured data structure: two things that are already
> present way too much in the code base and should be avoided for future
> design.
> 
> I suggest to put it in a structure, maybe AVIOSettings (or directly
> AVGlobalSettings), as an array of protocols, and with int_cb while we are at
> it:
> 
> typedef struct AVIOSettings {
>     AVIOInterruptCB *int_cb;
>     struct URLProtocol **protocols;
>     unsigned nb_protocols;
> }

we use string whitelists for codecs and formats, using a differnet
system for protocols would be inconsistent at least

The struct suggested has a few flaws as well
URLProtocols are not public so lists of pointers to them in a "AV*"
that is public struct might cause problems

also if the same is done for codecs and formats you have 3 such
structs with the need for 3 sets of allocators, deallocators and
probably a few helper functions for each to construct them.
then new function prototypes could be needed for
encoder, decoder, audio, video, subtitles, muxers, demuxers and
protocols to pass in these structs. (depending on details of the API)
also to maintain support for the existng codec/format whitelists
convertion and merging code would be needed

this is alot of complexity

another problem of the struct is that depending on from which lib
the protocols are set the same protocol may have unequal pointers

which system do people prefer ?
do we have a volunteer to implement a struct based system ?

do people want the string based solution to be applied till then
or to not have this security feature until then ?


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

What does censorship reveal? It reveals fear. -- Julian Assange
-------------- 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/20160124/ca383982/attachment.sig>