[FFmpeg-devel] [PATCH 2/2] lavu/tests/opts: add tests for filepath options

[Michael Niedermayer]

On Tue, Mar 08, 2022 at 08:47:17AM +0100, J. Dekker wrote:
> 
> 
> On 5 Mar 2022, at 20:16, Michael Niedermayer wrote:
> 
> > On Fri, Mar 04, 2022 at 04:03:07PM +0100, Niklas Haas wrote:
> >> From: Niklas Haas <git at haasn.dev>
> >>
> >> Using the venerable HEADER.txt as a small file to load.
> >> ---
> >>  libavutil/tests/opt.c    | 38 +++++++++++++++++++++++++++++++++++++-
> >>  tests/fate/libavutil.mak |  2 +-
> >>  tests/ref/fate/opt       |  4 ++++
> >>  3 files changed, 42 insertions(+), 2 deletions(-)
> >
> > Please add tests which tries to load
> > id_rsa
> > ~/.ssh/id_rsa
> > shadow
> > /etc/shadow
> > .bash_history
> > ...
> >
> > The idea here is of course that such attempts fail
> 
> There is absolutely no way we can or should try to implement a path based blacklist.

did i ask for one ?
what i asked for is that you write an exploit to show that it fails.


> Untrusted inputs should be sanitised externally by whichever script is being used to call ffmpeg.

my suggestion isnt really affected by this, please implement a test of this
you can put a script around the call to ffmpeg in the fate test but show that
it achieves the security.

If you cannot do this, then please do not suggest that this is the way
to sanitize untrusted data.


> 
> > Also document the security implications of this feature in
> > doc/APIchanges / release notes if there is a security implication
> >
> > Adjusting the parameters of most components could previously
> > not read arbitrary files so a application could previously
> > pass a string from a untrusted user to it.
> > If this changes it needs to be justfied and documented
> > If it doesnt change and its still safe that should be documented.
> > If it depends on whitelists and callbacks that should be actually implemented
> > in ffmpeg and the relevant examples
> >
> > And i do like this feature, if it can be done without security issues
> 
> There aren't any extra security implications here, if a user is allowed to specify filter arguments themselves then they can already use the movie/amovie filter etc. This new option is just a way to unify the way in which filters which already (and will) require to load files can do so.

hmm

So above you say "Untrusted inputs should be sanitised externally by whichever script is being used to call ffmpeg."
and that script now lets say blocks movie and amovie (and others)
before your patch thats safe, afterwards its not
how would the developer know that a git pull --rebase requires him
to rewrite that script? 

Or maybe the script has a list of safe filters and safe arguments, maybe
scale, crop, and a few others with width/height and so on
such script needs an update too if the binary option gains the ability
to read arbitrary files.

thx

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

Does the universe only have a finite lifespan? No, its going to go on
forever, its just that you wont like living in it. -- Hiranya Peiri
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20220308/c8c3ab7b/attachment.sig>