diff mbox series

[FFmpeg-devel] fftools/ffmpeg_ffplay_ffprobe_cmdutils:add -mask_url to replace the protocol address in the command with the asterisk (*)

Message ID 2f52afe919284d9a872738dd68ab886f@huawei.com
State New
Headers show
Series [FFmpeg-devel] fftools/ffmpeg_ffplay_ffprobe_cmdutils:add -mask_url to replace the protocol address in the command with the asterisk (*) | expand

Checks

Context Check Description
yinshiyou/commit_msg_loongarch64 warning The first line of the commit message must start with a context terminated by a colon and a space, for example "lavu/opt: " or "doc: ".
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 success Make fate finished

Commit Message

Wujian(Chin) Dec. 21, 2022, 10:10 a.m. UTC
I have modified the issues again. Please review it again. Thank you.

If the protocol address contains the user name and password, the ps -ef
command exposes plaintext. 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.

Signed-off-by: wujian_nanjing <wujian2@huawei.com>
---
 doc/fftools-common-opts.texi | 11 +++++++
 fftools/cmdutils.c           | 75 ++++++++++++++++++++++++++++++++++++++++++--
 fftools/cmdutils.h           | 25 +++++++++++++++
 fftools/ffmpeg.c             | 10 +++---
 fftools/ffplay.c             |  9 ++++--
 fftools/ffprobe.c            | 10 +++---
 6 files changed, 126 insertions(+), 14 deletions(-)

Comments

Nicolas George Dec. 22, 2022, 7:28 p.m. UTC | #1
Wujian(Chin) (12022-12-21):
> I have modified the issues again. Please review it again. Thank you.
> 
> If the protocol address contains the user name and password, the ps -ef
> command exposes plaintext. 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.
> 
> Signed-off-by: wujian_nanjing <wujian2@huawei.com>
> ---
>  doc/fftools-common-opts.texi | 11 +++++++
>  fftools/cmdutils.c           | 75 ++++++++++++++++++++++++++++++++++++++++++--
>  fftools/cmdutils.h           | 25 +++++++++++++++
>  fftools/ffmpeg.c             | 10 +++---
>  fftools/ffplay.c             |  9 ++++--
>  fftools/ffprobe.c            | 10 +++---
>  6 files changed, 126 insertions(+), 14 deletions(-)

This new patch still has issues not addressed since the first review.

Regards,
Wujian(Chin) Dec. 23, 2022, 7:14 a.m. UTC | #2
>Wujian(Chin) (12022-12-21):
>> I have modified the issues again. Please review it again. Thank you.
>> 
>> If the protocol address contains the user name and password, the ps 
>> -ef command exposes plaintext. 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.
>> 
>> Signed-off-by: wujian_nanjing <wujian2@huawei.com>
>> ---
>>  doc/fftools-common-opts.texi | 11 +++++++
>>  fftools/cmdutils.c           | 75 ++++++++++++++++++++++++++++++++++++++++++--
>>  fftools/cmdutils.h           | 25 +++++++++++++++
>>  fftools/ffmpeg.c             | 10 +++---
>>  fftools/ffplay.c             |  9 ++++--
>>  fftools/ffprobe.c            | 10 +++---
>>  6 files changed, 126 insertions(+), 14 deletions(-)

>This new patch still has issues not addressed since the first review.

>Regards,

--
>  Nicolas George

I've modified most of the issues, and I've explained some of the issues that don't.
If you don't accept my explanation, do you have any other better suggestions and methods?

Thank you.
Nicolas George Dec. 23, 2022, 9:13 a.m. UTC | #3
Wujian(Chin) (12022-12-23):
> I've modified most of the issues, and I've explained some of the issues that don't.
> If you don't accept my explanation, do you have any other better suggestions and methods?

I have already made a more detailed comment in the first thread.
Wujian(Chin) Dec. 23, 2022, 11:04 a.m. UTC | #4
Other issues have been modified. The following three issues have not been modified. I have explained them:

>> @@ -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.
   
  I think that it's more concise to use code this way.


>> +    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.

 I think that it would be better to replace the entire url, so that the code implementation is simple.


>> +    char **argv2;

>> +    argv2 = av_mallocz(argc * sizeof(char *));

>sizeof(*argv2)
>
>> +    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.

 This option needs to replace the URL. It is more appropriate to judge mask_url and copy argv in this place. 


If you think my idea is wrong, please give your specific advice, 

thank you, Nicolas George.

-----邮件原件-----
发件人: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] 代表 Nicolas George
发送时间: 2022年12月23日 17:13
收件人: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
抄送: wangqinghua (I) <wangqinghua9@huawei.com>
主题: Re: [FFmpeg-devel] 答复: [PATCH] fftools/ffmpeg_ffplay_ffprobe_cmdutils:add -mask_url to replace the protocol address in the command with the asterisk (*)

Wujian(Chin) (12022-12-23):
> I've modified most of the issues, and I've explained some of the issues that don't.
> If you don't accept my explanation, do you have any other better suggestions and methods?

I have already made a more detailed comment in the first thread.

-- 
  Nicolas George

_______________________________________________
ffmpeg-user mailing list
ffmpeg-user@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-user

To unsubscribe, visit link above, or email ffmpeg-user-request@ffmpeg.org with subject "unsubscribe".
Nicolas George Dec. 23, 2022, 11:06 a.m. UTC | #5
Wujian(Chin) (12022-12-23):
> If you think my idea is wrong, please give your specific advice, 

I already have.
diff mbox series

Patch

diff --git a/doc/fftools-common-opts.texi b/doc/fftools-common-opts.texi
index d914570..77b4e4a 100644
--- a/doc/fftools-common-opts.texi
+++ b/doc/fftools-common-opts.texi
@@ -363,6 +363,17 @@  for testing. Do not use it unless you know what you're doing.
 ffmpeg -cpucount 2
 @end example
 
+@item -mask_url -i @var{url} (@emph{output})
+If the protocol address contains the user name and password, the ps -ef
+command exposes plaintext. 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.
+@example
+ffmpeg -mask_url -i rtsp://username:password-ip:port/stream/test
+@end example
+
 @item -max_alloc @var{bytes}
 Set the maximum size limit for allocating a block on the heap by ffmpeg's
 family of malloc functions. Exercise @strong{extreme caution} when using
diff --git a/fftools/cmdutils.c b/fftools/cmdutils.c
index a1de621..08b6c28 100644
--- a/fftools/cmdutils.c
+++ b/fftools/cmdutils.c
@@ -61,6 +61,69 @@  AVDictionary *format_opts, *codec_opts;
 
 int hide_banner = 0;
 
+void mask_param(int argc, char **argv)
+{
+    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] = '*';
+            }
+        }
+    }
+}
+
+char **copy_argv(int argc, char **argv)
+{
+    char **argv_copy;
+    argv_copy = av_mallocz(argc * sizeof(char *));
+    if (!argv_copy) {
+        av_log(NULL, AV_LOG_FATAL, "argv_copy malloc failed\n");
+        exit_program(1);
+    }
+
+    for (int i = 0; i < argc; i++) {
+        int length = strlen(argv[i]) + 1;
+        argv_copy[i] = av_mallocz(length * sizeof(char *));
+        if (!argv_copy[i]) {
+            av_log(NULL, AV_LOG_FATAL, "argv_copy[%d] malloc failed\n", i);
+            exit_program(1);
+        }
+        memcpy(argv_copy[i], argv[i], length);
+    }
+    return argv_copy;
+}
+
+char **handle_arg_param(int argc, int mask_flag, char **argv)
+{
+    char **argv_copy;
+    argv_copy = copy_argv(argc, argv);
+    if (mask_flag)
+        mask_param(argc, argv);
+    return argv_copy;
+}
+
+int get_mask_flag(int *argc, char ***argv)
+{
+    if (*argc > 1 && !strcmp((*argv)[1], "-mask_url")) {
+        (*argv)[1] = (*argv)[0];
+        (*argc)--;
+        (*argv)++;
+        return 1;
+    }
+    
+    return 0;
+}
+
+void free_argv_copy(int argc, char **argv)
+{
+    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 +278,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;
     }
 
     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 +295,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++) {
@@ -246,6 +309,12 @@  static void prepare_app_arguments(int *argc_ptr, char ***argv_ptr)
 
     *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..08c4da7 100644
--- a/fftools/cmdutils.h
+++ b/fftools/cmdutils.h
@@ -50,6 +50,31 @@  extern AVDictionary *format_opts, *codec_opts;
 extern int hide_banner;
 
 /**
+ * Using to mask sensitive info.
+ */
+void mask_param(int argc, char **argv);
+
+/**
+ * Using to copy ori argv.
+ */
+char **copy_argv(int argc, char **argv);
+
+/**
+ * Handle argv and argv_copy.
+ */
+char **handle_arg_param(int argc, int mask_flag, char **argv);
+
+/**
+ * Get mask flag.
+ */
+int get_mask_flag(int *argc, char ***argv);
+
+/**
+ * Free argv.
+ */
+void free_argv_copy(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..d16eb36 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, mask_flag;
     BenchmarkTimeStamps ti;
-
+    char **argv_copy;
     init_dynload();
 
     register_exit(ffmpeg_cleanup);
@@ -3877,15 +3877,16 @@  int main(int argc, char **argv)
     av_log_set_flags(AV_LOG_SKIP_REPEATED);
     parse_loglevel(argc, argv, options);
 
+    mask_flag = get_mask_flag(&argc, &argv);
 #if CONFIG_AVDEVICE
     avdevice_register_all();
 #endif
     avformat_network_init();
 
     show_banner(argc, argv, options);
-
+    argv_copy = handle_arg_param(argc, mask_flag, argv);
     /* parse options and open all input/output files */
-    ret = ffmpeg_parse_options(argc, argv);
+    ret = ffmpeg_parse_options(argc, argv_copy);
     if (ret < 0)
         exit_program(1);
 
@@ -3920,5 +3921,6 @@  int main(int argc, char **argv)
         exit_program(69);
 
     exit_program(received_nb_signals ? 255 : main_return_code);
+    free_argv_copy(argc, argv_copy);
     return main_return_code;
 }
diff --git a/fftools/ffplay.c b/fftools/ffplay.c
index fc7e1c2..559e417 100644
--- a/fftools/ffplay.c
+++ b/fftools/ffplay.c
@@ -3663,10 +3663,12 @@  void show_help_default(const char *opt, const char *arg)
 /* Called from the main */
 int main(int argc, char **argv)
 {
-    int flags;
+    int flags, mask_flag;
+    char **argv_copy;
     VideoState *is;
 
     init_dynload();
+    mask_flag = get_mask_flag(&argc, &argv);
 
     av_log_set_flags(AV_LOG_SKIP_REPEATED);
     parse_loglevel(argc, argv, options);
@@ -3682,7 +3684,8 @@  int main(int argc, char **argv)
 
     show_banner(argc, argv, options);
 
-    parse_options(NULL, argc, argv, options, opt_input_file);
+    argv_copy = handle_arg_param(argc, mask_flag, argv);
+    parse_options(NULL, argc, argv_copy, options, opt_input_file);
 
     if (!input_filename) {
         show_usage();
@@ -3759,6 +3762,6 @@  int main(int argc, char **argv)
     event_loop(is);
 
     /* never returns */
-
+    free_argv_copy(argc, argv_copy);
     return 0;
 }
diff --git a/fftools/ffprobe.c b/fftools/ffprobe.c
index d2f126d..49375bd 100644
--- a/fftools/ffprobe.c
+++ b/fftools/ffprobe.c
@@ -4035,9 +4035,10 @@  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, mask_flag;
+    char **argv_copy;
     init_dynload();
+    mask_flag = get_mask_flag(&argc, &argv);
 
 #if HAVE_THREADS
     ret = pthread_mutex_init(&log_mutex, NULL);
@@ -4056,8 +4057,8 @@  int main(int argc, char **argv)
 #endif
 
     show_banner(argc, argv, options);
-    parse_options(NULL, argc, argv, options, opt_input_file);
-
+    argv_copy = handle_arg_param(argc, mask_flag, argv);
+    parse_options(NULL, argc, argv_copy, options, opt_input_file);
     if (do_show_log)
         av_log_set_callback(log_callback);
 
@@ -4173,6 +4174,7 @@  end:
     av_freep(&print_format);
     av_freep(&read_intervals);
     av_hash_freep(&hash);
+    free_argv_copy(argc, argv_copy);
 
     uninit_opts();
     for (i = 0; i < FF_ARRAY_ELEMS(sections); i++)