From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by master.gitmailbox.com (Postfix) with ESMTP id 4493C45001 for ; Mon, 19 Dec 2022 13:30:20 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 85C4F68B4C8; Mon, 19 Dec 2022 15:30:17 +0200 (EET) Received: from nef.ens.fr (nef2.ens.fr [129.199.96.40]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id F411768B32D for ; Mon, 19 Dec 2022 15:30:10 +0200 (EET) X-ENS-nef-client: 129.199.129.80 ( name = phare.normalesup.org ) Received: from phare.normalesup.org (phare.normalesup.org [129.199.129.80]) by nef.ens.fr (8.14.4/1.01.28121999) with ESMTP id 2BJDU9p4001112 ; Mon, 19 Dec 2022 14:30:09 +0100 Received: by phare.normalesup.org (Postfix, from userid 1001) id 4A708EB5B7; Mon, 19 Dec 2022 14:30:09 +0100 (CET) Date: Mon, 19 Dec 2022 14:30:09 +0100 From: Nicolas George To: FFmpeg development discussions and patches Message-ID: References: <7407e74b181e4e00a7b7104fb63cf56a@huawei.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <7407e74b181e4e00a7b7104fb63cf56a@huawei.com> X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.4.3 (nef.ens.fr [129.199.96.32]); Mon, 19 Dec 2022 14:30:09 +0100 (CET) Subject: Re: [FFmpeg-devel] [PATCH] fftools/ffmpeg_ffplay_ffprobe_cmdutils: add -mask_url to replace the protocol address in the command with the asterisk (*) X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Cc: "wangqinghua \(I\)" Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: Wujian(Chin) (12022-12-19): > I have modified the issues. Please review it again. Thank you. > > If the protocol address contains the user name and password, The ps -ef command exposes plaintext. Spurious comma or capital. > The -mask_url parameter option is added to replace the protocol address in the command line with the asterisk (*). > Because other users can run the ps -ef command to view sensitive information such as the user name and password > in the protocol address, which is insecure. Please wrap to 60-72 characters. > > Signed-off-by: wujian_nanjing > --- > doc/ffmpeg.texi | 9 +++++++++ > doc/ffplay.texi | 8 ++++++++ > doc/ffprobe.texi | 9 +++++++++ > fftools/cmdutils.c | 47 +++++++++++++++++++++++++++++++++++++++++++---- > fftools/cmdutils.h | 15 +++++++++++++++ > fftools/ffmpeg.c | 16 +++++++++++++--- > fftools/ffplay.c | 15 +++++++++++++-- > fftools/ffprobe.c | 18 ++++++++++++++---- > 8 files changed, 124 insertions(+), 13 deletions(-) > > diff --git a/doc/ffmpeg.texi b/doc/ffmpeg.texi > index 0367930..1f6cb33 100644 > --- a/doc/ffmpeg.texi > +++ b/doc/ffmpeg.texi > --- a/doc/ffplay.texi > +++ b/doc/ffplay.texi > --- a/doc/ffprobe.texi > +++ b/doc/ffprobe.texi The place for common options is doc/fftools-common-opts.texi. > diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c > index a1de621..c35d7e1 100644 > --- a/fftools/cmdutils.c > +++ b/fftools/cmdutils.c > @@ -61,6 +61,40 @@ AVDictionary *format_opts, *codec_opts; > > int hide_banner = 0; > > +void param_masking(int argc, char **argv) { Functions name in ...ing do not seem idiomatic to me. The style for the brace is off. > + int i, j; > + for (i = 1; i < argc; i++) { > + char *match = strstr(argv[i], "://"); > + if (match) { > + int total = strlen(argv[i]); > + for (j = 0; j < total; j++) { > + argv[i][j] = '*'; > + } Masking the whole URL seems too much. Logins and passwords are introduced by the @ character. > + } > + } > +} > + > +char **copy_argv(int argc, char **argv) { The brace is off here too. > + char **argv2; > + argv2 = av_mallocz(argc * sizeof(char *)); sizeof(*argv2) > + if (!argv2) > + exit_program(1); Error message. > + > + for (int i = 0; i < argc; i++) { > + int length = strlen(argv[i]) + 1; > + argv2[i] = av_mallocz(length * sizeof(char *)); > + if (!argv2[i]) > + exit_program(1); > + memcpy(argv2[i], argv[i], length - 1); > + } > + return argv2; > +} > + > +void free_pp(int argc, char **argv) { The brace is off too. This function is called only from ffprobe, looks wrong. > + for (int i = 0; i < argc; i++) > + av_free(argv[i]); > + av_free(argv); > +} > void uninit_opts(void) > { > av_dict_free(&swr_opts); > @@ -215,13 +249,13 @@ static void prepare_app_arguments(int *argc_ptr, char ***argv_ptr) > if (win32_argv_utf8) { > *argc_ptr = win32_argc; > *argv_ptr = win32_argv_utf8; > - return; > + goto end; We only use goto for error processing. > } > > win32_argc = 0; > argv_w = CommandLineToArgvW(GetCommandLineW(), &win32_argc); > if (win32_argc <= 0 || !argv_w) > - return; > + goto end; > > /* determine the UTF-8 buffer size (including NULL-termination symbols) */ > for (i = 0; i < win32_argc; i++) > @@ -232,7 +266,7 @@ static void prepare_app_arguments(int *argc_ptr, char ***argv_ptr) > argstr_flat = (char *)win32_argv_utf8 + sizeof(char *) * (win32_argc + 1); > if (!win32_argv_utf8) { > LocalFree(argv_w); > - return; > + goto end; > } > > for (i = 0; i < win32_argc; i++) { > @@ -243,9 +277,14 @@ static void prepare_app_arguments(int *argc_ptr, char ***argv_ptr) > } > win32_argv_utf8[i] = NULL; > LocalFree(argv_w); > - > *argc_ptr = win32_argc; > *argv_ptr = win32_argv_utf8; > +end: > + if (*argc_ptr > 1 && !strcmp((*argv_ptr)[1], "-mask_url")) { > + (*argv_ptr)[1] = (*argv_ptr)[0]; > + (*argc_ptr)--; > + (*argv_ptr)++; > + } > } > #else > static inline void prepare_app_arguments(int *argc_ptr, char ***argv_ptr) > diff --git a/fftools/cmdutils.h b/fftools/cmdutils.h > index 4496221..ce4c1db 100644 > --- a/fftools/cmdutils.h > +++ b/fftools/cmdutils.h > @@ -50,6 +50,21 @@ extern AVDictionary *format_opts, *codec_opts; > extern int hide_banner; > > /** > + * Using to masking sensitive info. > + */ > +void param_masking(int argc, char **argv); > + > +/** > + * Using to copy ori argv. > + */ > +char **copy_argv(int argc, char **argv); > + > +/** > + * Free ** > + */ > +void free_pp(int argc, char **argv); > + > +/** > * Register a program-specific cleanup routine. > */ > void register_exit(void (*cb)(int ret)); > diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c > index 881d6f0..fccbde9 100644 > --- a/fftools/ffmpeg.c > +++ b/fftools/ffmpeg.c > @@ -3865,9 +3865,9 @@ static int64_t getmaxrss(void) > > int main(int argc, char **argv) > { > - int ret; > + int ret, maskFlag; We do not do camelCase. > BenchmarkTimeStamps ti; > - > + char **argv2; > init_dynload(); > > register_exit(ffmpeg_cleanup); > @@ -3877,15 +3877,25 @@ int main(int argc, char **argv) > av_log_set_flags(AV_LOG_SKIP_REPEATED); > parse_loglevel(argc, argv, options); > > + maskFlag = 0; > + if (argc > 1 && !strcmp(argv[1], "-mask_url")) { > + argv[1] = argv[0]; > + maskFlag = 1; > + argc--; > + argv++; > + } This option is not special nor important enough to warrant a special treatment like that. > #if CONFIG_AVDEVICE > avdevice_register_all(); > #endif > avformat_network_init(); > > show_banner(argc, argv, options); > + argv2 = copy_argv(argc, argv); > + if (maskFlag) > + param_masking(argc, argv); This is duplicated in all three files and unnecessary: have a single function do the copy and the masking. > > /* parse options and open all input/output files */ > - ret = ffmpeg_parse_options(argc, argv); > + ret = ffmpeg_parse_options(argc, argv2); > if (ret < 0) > exit_program(1); > > diff --git a/fftools/ffplay.c b/fftools/ffplay.c > index fc7e1c2..5d282f1 100644 > --- a/fftools/ffplay.c > +++ b/fftools/ffplay.c > @@ -3663,10 +3663,18 @@ void show_help_default(const char *opt, const char *arg) > /* Called from the main */ > int main(int argc, char **argv) > { > - int flags; > + int flags, maskFlag; > + char **argv2; > VideoState *is; > > init_dynload(); > + maskFlag = 0; > + if (argc > 1 && !strcmp(argv[1], "-mask_url")) { > + argv[1] = argv[0]; > + maskFlag = 1; > + argc--; > + argv++; > + } > > av_log_set_flags(AV_LOG_SKIP_REPEATED); > parse_loglevel(argc, argv, options); > @@ -3682,7 +3690,10 @@ int main(int argc, char **argv) > > show_banner(argc, argv, options); > > - parse_options(NULL, argc, argv, options, opt_input_file); > + argv2 = copy_argv(argc, argv); > + parse_options(NULL, argc, argv2, options, opt_input_file); > + if (maskFlag) > + param_masking(argc, argv); > > if (!input_filename) { > show_usage(); > diff --git a/fftools/ffprobe.c b/fftools/ffprobe.c > index d2f126d..e69f49f 100644 > --- a/fftools/ffprobe.c > +++ b/fftools/ffprobe.c > @@ -4035,9 +4035,16 @@ int main(int argc, char **argv) > WriterContext *wctx; > char *buf; > char *w_name = NULL, *w_args = NULL; > - int ret, input_ret, i; > - > + int ret, input_ret, i, maskFlag; > + char **argv2; > init_dynload(); > + maskFlag = 0; > + if (argc > 1 && !strcmp(argv[1], "-mask_url")) { > + argv[1] = argv[0]; > + maskFlag = 1; > + argc--; > + argv++; > + } > > #if HAVE_THREADS > ret = pthread_mutex_init(&log_mutex, NULL); > @@ -4056,8 +4063,10 @@ int main(int argc, char **argv) > #endif > > show_banner(argc, argv, options); > - parse_options(NULL, argc, argv, options, opt_input_file); > - > + argv2 = copy_argv(argc, argv); > + parse_options(NULL, argc, argv2, options, opt_input_file); > + if (maskFlag) > + param_masking(argc, argv); > if (do_show_log) > av_log_set_callback(log_callback); > > @@ -4173,6 +4182,7 @@ end: > av_freep(&print_format); > av_freep(&read_intervals); > av_hash_freep(&hash); > + free_pp(argc, argv2); > > uninit_opts(); > for (i = 0; i < FF_ARRAY_ELEMS(sections); i++) Regards, -- Nicolas George _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".