[FFmpeg-devel] [PATCH 0/3] Finish new iteration APIs

[Michael Niedermayer]

On Tue, Feb 20, 2018 at 04:44:13PM +0000, Rostislav Pehlivanov wrote:
> On 20 February 2018 at 16:30, Michael Niedermayer <michael at niedermayer.cc>
> wrote:
> 
> > On Tue, Feb 20, 2018 at 10:17:02AM -0300, James Almer wrote:
> > > On 2/20/2018 9:21 AM, wm4 wrote:
> > > > On Tue, 20 Feb 2018 08:47:51 -0300
> > > > James Almer <jamrial at gmail.com> wrote:
> > > >
> > > >>> It has fully achieved its objectives. There's no more visible global
> > > >>> mutable state, and the actual global mutable state has been reduced
> > to
> > > >>> less than 5 codec entries.
> > > >>
> > > >> It's true that after the deprecation period they will effectively be
> > the
> > > >> only ones still mutable, but shouldn't the objective be to cover them
> > all?
> > > >
> > > > That would be nice. But this has been discussed before. No kind of
> > > > different registration API could fix this issue either, unless we start
> > > > mallocing AVCodec structs and require the user to free them, or
> > > > something equally absurd. The proper solution for this specific issue
> > > > would be making a new API that somehow replaces AVCodec.pix_fmts.
> > > >
> > > >>>
> > > >>> Why are we discussing this _again_?
> > > >>
> > > >> Because a drawback has been found, namely that link time optimization
> > > >> using static libraries can't remove "non registered" modules anymore,
> > > >> and now depends fully on what's enabled at configure time.
> > > >> Ideally, a better registration based API that follows what i described
> > > >> above should be designed with care.
> > > >
> > > > Oh yeah, bikeshed attack by Michael. As it was said a few thousand
> > times
> > > > already, public API users could NOT use the registration stuff to
> > > > register only specific components at runtime. So they have to use the
> > > > register_all variants, which pull in _all_ components even with static
> > > > linking.
> > >
> > > Oh, i assumed it was possible to use avcodec_register() on a manual
> > > basis in a proper API usage scenario, but i see now that's not the case.
> >
> > of course its possible to use avcodec_register() on a manual basis in a
> > proper API usage scenario, why would it not be ?
> >
> > why do you think this function exists or was written ?
> >
> > is it the ff_* symbols you are thinking of ?
> > This is a problem for an existing API it is not a problem for a new API as
> > we can change the symbols if they are intended to be used for individual
> > component registration.
> > The whole discussion is about designing a new API. So any limitation of
> > an existing API can be changed.
> >
> > There also should be no need to call register_all in the existing API,
> > and there cannot be such a problem in a new design because it would be
> > a new design you wouldnt design it to "not work".
> >
> >
> > >
> > > Nevermind then, my argument was focused on preventing losing some link
> > > time optimization for static libraries assuming proper API usage.
> > >
> > > >
> > > > And that can't be made with dynamic linking either. If you statically
> > > > link anyway, you probably control everything, and you can configure
> > this
> > > > stuff at compile time.
> > > >
> > > > Then I guess there's this very special case where a fuzzer statically
> > > > links to libavcodec, and registers only a few components (technically
> > > > violating the API and by guessing the component symbol name), and
> > > > without calling the register_all functions. This would make the
> > > > resulting executable much smaller, which is apparently important
> > > > because Google (who runs the fuzzers for their oss-fuzz project) is too
> > > > poor (?) to host all the statically linked fuzzers, _or_ the whitelist
> > > > stuff is not enough to trick the fuzzer into not trying to fuzz the
> > > > wrong code. In addition, it's apparently infeasible to just build
> > > > every fuzzer with a separate libavcodec. (Not sure about the details,
> > it
> > > > was something like this.)
> > > >
> > > > Those requirements are really quite nebulous. I don't know why we even
> > > > should care - surely whoever does this will find a solution that works
> > > > for them. For example they could apply a small patch that makes the
> > > > codec_list[] symbol non-static non-const, which would allow them to
> > > > overwrite it (i.e. deleting unneeded entries). It's a really simple
> > > > solution that took me 5 minutes to come up with.
> > >
> > > Something like this is probably the best solution for the fuzzer issue,
> > yes.
> >
> > This basically says one should fork ffmpeg if one has problems with the
> > new API
> >
> > Thats a very chilling response to be honest in a discussion about that APIs
> > design. More so as this is at a time we still can change the API.
> >
> > and anyone who wants to only register a subset of components (due to
> > security)
> > has to either link to a seperate binary (which is  disallowed in some
> > major distributions which mandate shared libs without exception IIRC so
> > that
> > sounds great but simple isnt possible)
> > or just not use FFmpeg or fork it or use a fork or just forget about only
> > registering a subset.
> >
> 
> You bring up the security argument but I don't see anything here which
> would compromise it. _All_ the executable code for all codecs is still in
> the library. Restricting the codecs you get during init would do nothing
> for security if the attacker can still call any function.

yes but that assumtation is not true
"if the attacker can still call any function."  <-- if thats the case the
attacker will call some more interresting functions like system()

The attacker can in general feed any data to the victim. Like a file, specific
headers, packets, timestamps, ...

All this goes to the user Application which then sets up codecs, filters,
formats, opens them and feeds the attackers headers and packets to them.

Now case A, all components are registered.
In this case any mistake in any piece of the codepath from the user app
choosing the format, the codecs, the filters, and the libs choosing codecs
from the file headers, parsing various headers and strings, maybe referenced
files hls to mov references, and so on must block opening unintended components.
Yes it should be safe with whitelists set.

Now case B, only components actually required are registered.
In this case any mistake in any piece of the codepath from the user app
choosing the format, the codecs, the filters, and the libs choosing codecs
from the file headers, parsing various headers and strings, maybe referenced
files hls to mov references, and so on has a much smaller impact as theres
no easy way the code can access a component thats not registered.

This is an additional saftey layer.
Similar to how file access rights, seperatwe users a virtual machine and an
application not allowing access to unintended things in the first place are
redundant layers of security.
And as i already bring this comparission up, reality shows us that multiple
layers like this still can get broken through sometimes, so its not just
paranoia sadly that an extra layer makes sense

also a different case thats similar. A sys admin that cares about
security disabled daemons he does not need, he doesnt just live by
the firewall blocking them. the firewall is like our whitelists,
it should block things but its still better if its not running at all
in the first place.

I hope this explains my security concern about this better

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

During times of universal deceit, telling the truth becomes a
revolutionary act. -- George Orwell
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20180220/a5bf0b92/attachment.sig>