[FFmpeg-devel] [PATCH] add av_enable_strict_whitelists()

[Michael Niedermayer]

On Sun, Oct 26, 2014 at 12:16:26AM +0200, Nicolas George wrote:
> Le quartidi 4 brumaire, an CCXXIII, Michael Niedermayer a écrit :
> > This fixes the issue that a not set or not forwarded whitelist
> > would allow to bypass it.
> 
> I am a bit worried by the sheer amount of extra code and API this simple
> feature requires. Maybe it is a sign that it was not the best approach to
> begin with.
> 
> Also, I did not say at the time, but I believe using a string to hold a list
> like that is quite ugly.
> 

> Unless I am mistaken, the core of the problem resides in the existence of
> global variables in the library, and this is not the only instance of this
> issue: remember people who want to free the global mutex manager, not
> understanding that "still reachable" is not a memory leak.
> 
> There is an idea that I have been nursing for some time: moving all these
> global variables to a structure, and start passing it around everywhere.
> Most of the functions that access the global variables already have some
> kind of context where the pointer can be stored, so it would only require
> changing a few API entry points.
> 
> That may look somewhat like that:
> 
> AVCodecInstance *libavcodec = avcodec_library_create(OPTIONS);
> AVFormatInstance *libavformat = avformat_library_create(libavformat, OPTIONS);
> 
> avformat_instance_open_input(libavformat, &ctx, filename, NULL, NULL);
> 
> Then the whitelist feature comes for free: use the options to disable
> registering all the codecs, then manually register the ones needed.
> 
> Of course, for compatibility, a global instance need to be inited if the
> legacy entry points are used, but it becomes much easier to test extensively
> that modern API usage will not touch them.
> 
> Of course, that requires more work, and the whitelist feature will not be
> available immediately, but in the long run I believe it would be greatly
> beneficial.
> 
> What do people think of that?

about whitelists, there are 2 use cases / features

The first is to globally within a process limit the decoders and
demuxers which can be used. This is to limit the attack surface a
attacker has, for this to really work well other libs, plugins
using their own local contexts and so on should be limited as well.
This can currently be achived by disabling decoders and demuxers at
compile time only, but the register list patch i posted previously
would allow to do this at runtime

The second is to limit a specific set of contexts, for example ones
used to demux and decode a remote url that would be intended for
for example the flashplayer, the set of demuxers and decoders for this
could be very limited wihout any loss of functionality as the original
flash player only supports a small set of formats, thus nicely
reducing the attack surface for an attacker.
This can currently be done with the whitelists but has the issue that
forgetting to pass a whitelist somewhere in the depth of one of the
libs could be a security issue.
The patch here fixes this weakness.
The whitelists could be passed around using a different system
like what you suggest, still this patch here would none the
less be needed to protect against the case where the context is not
copied/forwarded correctly
Thus your system probably is nicer than the whitelists as they are now
but it has the same problem and need for a global switch to enable
security
something that in plain english globally says "everyone received a
whitelist, if you didnt then theres a bug dont open anything for
saftey" and of course this implies that the default must be "nothing"
for the whitelists not "all" because otherwise a context opened with
defaults would bypass this and such a thing is easily made by mistake
like in a demuxer opening another demuxer

Also i think its best if we move the whitelists in the structs you
suggest when they are finished and in git but not to delay/block
work on them till then.

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Concerning the gods, I have no means of knowing whether they exist or not
or of what sort they may be, because of the obscurity of the subject, and
the brevity of human life -- Protagoras
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20141026/39dd4b81/attachment.asc>