| Message ID | 20220304150307.61769-2-ffmpeg@haasn.xyz |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/2] lavu: add syntax for loading AV_OPT_TYPE_BINARY from files | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_aarch64_jetson | success | Make finished |
| andriy/make_fate_aarch64_jetson | success | Make fate finished |
On Fri, Mar 04, 2022 at 04:03:07PM +0100, Niklas Haas wrote: > From: Niklas Haas <git@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 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 thx [...]
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@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. Untrusted inputs should be sanitised externally by whichever script is being used to call ffmpeg. > 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.
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@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 [...]
diff --git a/libavutil/tests/opt.c b/libavutil/tests/opt.c index e6ea892373..e3d4edee4e 100644 --- a/libavutil/tests/opt.c +++ b/libavutil/tests/opt.c @@ -113,7 +113,7 @@ static void log_callback_help(void *ptr, int level, const char *fmt, va_list vl) vfprintf(stdout, fmt, vl); } -int main(void) +int main(int argc, const char **argv) { int i; @@ -355,5 +355,41 @@ int main(void) av_opt_free(&test_ctx); } + printf("\nTesting av_opt_set_filepath()\n"); + { + TestContext test_ctx = { 0 }; + const char *samples; + char buf[256]; + uint8_t *value; + int ret = AVERROR_BUG; + + if (argc < 2) + return 1; + samples = argv[1]; + + test_ctx.class = &test_class; + av_opt_set_defaults(&test_ctx); + av_log_set_level(AV_LOG_QUIET); + + if (snprintf(buf, sizeof(buf), "@%s/HEADER.txt", samples) >= sizeof(buf)) + return 1; + + ret = av_opt_set_filepath(&test_ctx, "bin", &buf[1], 0); + if (!ret) + ret = av_opt_get(&test_ctx, "bin", 0, &value); + printf("av_opt_set_filepath: bin='%s'\n", + ret ? av_err2str(ret) : (char *) value); + av_free(value); + + ret = av_opt_set(&test_ctx, "bin", buf, 0); + if (!ret) + ret = av_opt_get(&test_ctx, "bin", 0, &value); + printf("av_opt_set: bin='%s'\n", + ret ? av_err2str(ret) : (char *) value); + av_free(value); + + av_opt_free(&test_ctx); + } + return 0; } diff --git a/tests/fate/libavutil.mak b/tests/fate/libavutil.mak index 1ec9ed00ad..587aa810cd 100644 --- a/tests/fate/libavutil.mak +++ b/tests/fate/libavutil.mak @@ -164,7 +164,7 @@ fate-tea: CMD = run libavutil/tests/tea$(EXESUF) FATE_LIBAVUTIL += fate-opt fate-opt: libavutil/tests/opt$(EXESUF) -fate-opt: CMD = run libavutil/tests/opt$(EXESUF) +fate-opt: CMD = run libavutil/tests/opt$(EXESUF) $(TARGET_SAMPLES) FATE_LIBAVUTIL += $(FATE_LIBAVUTIL-yes) FATE-$(CONFIG_AVUTIL) += $(FATE_LIBAVUTIL) diff --git a/tests/ref/fate/opt b/tests/ref/fate/opt index aac3fa0e7e..341693d097 100644 --- a/tests/ref/fate/opt +++ b/tests/ref/fate/opt @@ -417,3 +417,7 @@ Setting options string 'a_very_long_option_name_that_will_need_to_be_ellipsized_ Setting 'a_very_long_option_name_that_will_need_to_be_ellipsized_around_here' to value '42' Option 'a_very_long_option_name_that_will_need_to_be_ellipsized_around_here' not found Error 'a_very_long_option_name_that_will_need_to_be_ellipsized_around_here=42' + +Testing av_opt_set_filepath() +av_opt_set_filepath: bin='46415445202D2046466D706567204175746F6D617465642054657374696E6720456E7669726F6E6D656E740A687474703A2F2F666174652E6D756C74696D656469612E63782F0A0A' +av_opt_set: bin='46415445202D2046466D706567204175746F6D617465642054657374696E6720456E7669726F6E6D656E740A687474703A2F2F666174652E6D756C74696D656469612E63782F0A0A'
From: Niklas Haas <git@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(-)