[Qemu-devel] [PATCH v3 0/3] restrict bridge interface name to IFNAMSIZ

[Qemu-devel] [PATCH v3 0/3] restrict bridge interface name to IFNAMSIZ

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

Hello,

Linux net_deivce defines network interface name to be of IFNAMSIZE(=16)
bytes, including the terminating null('\0') byte.

Qemu tap deivce, while invoking 'qemu-bridge-helper' tool to set up the
network bridge interface, supplies bridge name of 16 characters, thus
allowing to create an ACL bypass scenario.

This patch series attempts to fix it. It also refactors bridge helper
invocation routine 'net_bridge_run_helper' to directly invoke the helper
command.

Thank you.
---
Prasad J Pandit (3):
  qemu-bridge-helper: restrict interface name to IFNAMSIZ
  qemu-bridge-helper: move repeating code in parse_acl_file
  net: tap: refactor net_bridge_run_helper routine

 net/tap.c            | 43 +++++++++----------------------------------
 qemu-bridge-helper.c | 24 +++++++++++++++++-------
 2 files changed, 26 insertions(+), 41 deletions(-)

-- 
2.21.0

[Qemu-devel] [PATCH v3 1/3] qemu-bridge-helper: restrict interface name to IFNAMSIZ

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

The network interface name in Linux is defined to be of size
IFNAMSIZ(=16), including the terminating null('\0') byte.
The same is applied to interface names read from 'bridge.conf'
file to form ACL rules. If user supplied '--br=bridge' name
is not restricted to the same length, it could lead to ACL bypass
issue. Restrict interface name to IFNAMSIZ, including null byte.

Reported-by: Riccardo Schirone <rschiron@redhat.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 qemu-bridge-helper.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

Update v3: use g_str_equal() and %zu for strlen()
  -> https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg00072.html

diff --git a/qemu-bridge-helper.c b/qemu-bridge-helper.c
index f9940deefd..e90c22f07d 100644
--- a/qemu-bridge-helper.c
+++ b/qemu-bridge-helper.c
@@ -109,6 +109,13 @@ static int parse_acl_file(const char *filename, ACLList *acl_list)
         }
         *argend = 0;
 
+        if (!g_str_equal(cmd, "include") && strlen(arg) >= IFNAMSIZ) {
+            fprintf(stderr, "name `%s' too long: %zu\n", arg, strlen(arg));
+            fclose(f);
+            errno = EINVAL;
+            return -1;
+        }
+
         if (strcmp(cmd, "deny") == 0) {
             acl_rule = g_malloc(sizeof(*acl_rule));
             if (strcmp(arg, "all") == 0) {
@@ -259,6 +266,10 @@ int main(int argc, char **argv)
         usage();
         return EXIT_FAILURE;
     }
+    if (strlen(bridge) >= IFNAMSIZ) {
+        fprintf(stderr, "name `%s' too long: %zu\n", bridge, strlen(bridge));
+        return EXIT_FAILURE;
+    }
 
     /* parse default acl file */
     QSIMPLEQ_INIT(&acl_list);
-- 
2.21.0

[Qemu-devel] [PATCH v3 2/3] qemu-bridge-helper: move repeating code in parse_acl_file

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

Move repeating error handling sequence in parse_acl_file routine
to an 'err' label.

Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 qemu-bridge-helper.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/qemu-bridge-helper.c b/qemu-bridge-helper.c
index e90c22f07d..91a02f9611 100644
--- a/qemu-bridge-helper.c
+++ b/qemu-bridge-helper.c
@@ -92,9 +92,7 @@ static int parse_acl_file(const char *filename, ACLList *acl_list)
 
         if (arg == NULL) {
             fprintf(stderr, "Invalid config line:\n  %s\n", line);
-            fclose(f);
-            errno = EINVAL;
-            return -1;
+            goto err;
         }
 
         *arg = 0;
@@ -111,9 +109,7 @@ static int parse_acl_file(const char *filename, ACLList *acl_list)
 
         if (!g_str_equal(cmd, "include") && strlen(arg) >= IFNAMSIZ) {
             fprintf(stderr, "name `%s' too long: %zu\n", arg, strlen(arg));
-            fclose(f);
-            errno = EINVAL;
-            return -1;
+            goto err;
         }
 
         if (strcmp(cmd, "deny") == 0) {
@@ -139,15 +135,18 @@ static int parse_acl_file(const char *filename, ACLList *acl_list)
             parse_acl_file(arg, acl_list);
         } else {
             fprintf(stderr, "Unknown command `%s'\n", cmd);
-            fclose(f);
-            errno = EINVAL;
-            return -1;
+            goto err;
         }
     }
 
     fclose(f);
-
     return 0;
+
+err:
+    fclose(f);
+    errno = EINVAL;
+    return -1;
+
 }
 
 static bool has_vnet_hdr(int fd)
-- 
2.21.0

[Qemu-devel] [PATCH v3 3/3] net: tap: refactor net_bridge_run_helper routine

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

Refactor 'net_bridge_run_helper' routine to avoid buffer
formatting to prepare 'helper_cmd' and using shell to invoke
helper command. Instead directly execute helper program with
due arguments.

Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 net/tap.c | 43 +++++++++----------------------------------
 1 file changed, 9 insertions(+), 34 deletions(-)

Update v3: remove buffer formatting and use of shell to invoke helper
  -> https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg00071.html

diff --git a/net/tap.c b/net/tap.c
index e8aadd8d4b..bc9b3407a6 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -478,7 +478,6 @@ static int net_bridge_run_helper(const char *helper, const char *bridge,
     sigset_t oldmask, mask;
     int pid, status;
     char *args[5];
-    char **parg;
     int sv[2];
 
     sigemptyset(&mask);
@@ -498,9 +497,6 @@ static int net_bridge_run_helper(const char *helper, const char *bridge,
     }
     if (pid == 0) {
         int open_max = sysconf(_SC_OPEN_MAX), i;
-        char fd_buf[6+10];
-        char br_buf[6+IFNAMSIZ] = {0};
-        char helper_cmd[PATH_MAX + sizeof(fd_buf) + sizeof(br_buf) + 15];
 
         for (i = 3; i < open_max; i++) {
             if (i != sv[1]) {
@@ -508,39 +504,18 @@ static int net_bridge_run_helper(const char *helper, const char *bridge,
             }
         }
 
-        snprintf(fd_buf, sizeof(fd_buf), "%s%d", "--fd=", sv[1]);
+        args[0] = (char *)helper;
+        args[1] = (char *)"--use-vnet";
+        args[2] = g_strdup_printf("%s%d", "--fd=", sv[1]);
+        args[3] = g_strdup_printf("%s%s", "--br=", bridge);
+        args[4] = NULL;
 
-        if (strrchr(helper, ' ') || strrchr(helper, '\t')) {
-            /* assume helper is a command */
+        execv(helper, args);
 
-            if (strstr(helper, "--br=") == NULL) {
-                snprintf(br_buf, sizeof(br_buf), "%s%s", "--br=", bridge);
-            }
+        g_free(args[2]);
+        g_free(args[3]);
+        fprintf(stderr, "failed to execute helper: %s\n", helper);
 
-            snprintf(helper_cmd, sizeof(helper_cmd), "%s %s %s %s",
-                     helper, "--use-vnet", fd_buf, br_buf);
-
-            parg = args;
-            *parg++ = (char *)"sh";
-            *parg++ = (char *)"-c";
-            *parg++ = helper_cmd;
-            *parg++ = NULL;
-
-            execv("/bin/sh", args);
-        } else {
-            /* assume helper is just the executable path name */
-
-            snprintf(br_buf, sizeof(br_buf), "%s%s", "--br=", bridge);
-
-            parg = args;
-            *parg++ = (char *)helper;
-            *parg++ = (char *)"--use-vnet";
-            *parg++ = fd_buf;
-            *parg++ = br_buf;
-            *parg++ = NULL;
-
-            execv(helper, args);
-        }
         _exit(1);
 
     } else {
-- 
2.21.0

Re: [Qemu-devel] [PATCH v3 1/3] qemu-bridge-helper: restrict interface name to IFNAMSIZ

[Li Qiang]

P J P <ppandit@redhat.com> 于2019年7月1日周一 下午8:38写道：

> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> The network interface name in Linux is defined to be of size
> IFNAMSIZ(=16), including the terminating null('\0') byte.
> The same is applied to interface names read from 'bridge.conf'
> file to form ACL rules. If user supplied '--br=bridge' name
> is not restricted to the same length, it could lead to ACL bypass
> issue. Restrict interface name to IFNAMSIZ, including null byte.
>
> Reported-by: Riccardo Schirone <rschiron@redhat.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  qemu-bridge-helper.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> Update v3: use g_str_equal() and %zu for strlen()
>   -> https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg00072.html
>
> diff --git a/qemu-bridge-helper.c b/qemu-bridge-helper.c
> index f9940deefd..e90c22f07d 100644
> --- a/qemu-bridge-helper.c
> +++ b/qemu-bridge-helper.c
> @@ -109,6 +109,13 @@ static int parse_acl_file(const char *filename,
> ACLList *acl_list)
>          }
>          *argend = 0;
>
> +        if (!g_str_equal(cmd, "include") && strlen(arg) >= IFNAMSIZ) {
> +            fprintf(stderr, "name `%s' too long: %zu\n", arg,
> strlen(arg));
> +            fclose(f);
> +            errno = EINVAL;
> +            return -1;
> +        }
> +
>

g_str_equal is not consistent style with the other 'strcmp' in this file.
With g_str_equal or strcmp:

Reviewed-by: Li Qiang <liq3ea@gmail.com>




>          if (strcmp(cmd, "deny") == 0) {
>              acl_rule = g_malloc(sizeof(*acl_rule));
>              if (strcmp(arg, "all") == 0) {
> @@ -259,6 +266,10 @@ int main(int argc, char **argv)
>          usage();
>          return EXIT_FAILURE;
>      }
> +    if (strlen(bridge) >= IFNAMSIZ) {
> +        fprintf(stderr, "name `%s' too long: %zu\n", bridge,
> strlen(bridge));
> +        return EXIT_FAILURE;
> +    }
>
>      /* parse default acl file */
>      QSIMPLEQ_INIT(&acl_list);
> --
> 2.21.0
>
>

Re: [Qemu-devel] [PATCH v3 2/3] qemu-bridge-helper: move repeating code in parse_acl_file

[Li Qiang]

P J P <ppandit@redhat.com> 于2019年7月1日周一 下午8:38写道：

> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> Move repeating error handling sequence in parse_acl_file routine
> to an 'err' label.
>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
>

Reviewed-by: Li Qiang <liq3ea@gmail.com>




> ---
>  qemu-bridge-helper.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/qemu-bridge-helper.c b/qemu-bridge-helper.c
> index e90c22f07d..91a02f9611 100644
> --- a/qemu-bridge-helper.c
> +++ b/qemu-bridge-helper.c
> @@ -92,9 +92,7 @@ static int parse_acl_file(const char *filename, ACLList
> *acl_list)
>
>          if (arg == NULL) {
>              fprintf(stderr, "Invalid config line:\n  %s\n", line);
> -            fclose(f);
> -            errno = EINVAL;
> -            return -1;
> +            goto err;
>          }
>
>          *arg = 0;
> @@ -111,9 +109,7 @@ static int parse_acl_file(const char *filename,
> ACLList *acl_list)
>
>          if (!g_str_equal(cmd, "include") && strlen(arg) >= IFNAMSIZ) {
>              fprintf(stderr, "name `%s' too long: %zu\n", arg,
> strlen(arg));
> -            fclose(f);
> -            errno = EINVAL;
> -            return -1;
> +            goto err;
>          }
>
>          if (strcmp(cmd, "deny") == 0) {
> @@ -139,15 +135,18 @@ static int parse_acl_file(const char *filename,
> ACLList *acl_list)
>              parse_acl_file(arg, acl_list);
>          } else {
>              fprintf(stderr, "Unknown command `%s'\n", cmd);
> -            fclose(f);
> -            errno = EINVAL;
> -            return -1;
> +            goto err;
>          }
>      }
>
>      fclose(f);
> -
>      return 0;
> +
> +err:
> +    fclose(f);
> +    errno = EINVAL;
> +    return -1;
> +
>  }
>
>  static bool has_vnet_hdr(int fd)
> --
> 2.21.0
>
>

Re: [Qemu-devel] [PATCH v3 3/3] net: tap: refactor net_bridge_run_helper routine

[Li Qiang]

P J P <ppandit@redhat.com> 于2019年7月1日周一 下午8:38写道：

> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> Refactor 'net_bridge_run_helper' routine to avoid buffer
> formatting to prepare 'helper_cmd' and using shell to invoke
> helper command. Instead directly execute helper program with
> due arguments.
>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
>


My two cents:
You do two things here(avoid buffer formatting and get rid of calling
shell),
I would suggest you split these into split patch.

Thanks,
Li Qiang


---
>  net/tap.c | 43 +++++++++----------------------------------
>  1 file changed, 9 insertions(+), 34 deletions(-)
>
> Update v3: remove buffer formatting and use of shell to invoke helper
>   -> https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg00071.html
>
> diff --git a/net/tap.c b/net/tap.c
> index e8aadd8d4b..bc9b3407a6 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -478,7 +478,6 @@ static int net_bridge_run_helper(const char *helper,
> const char *bridge,
>      sigset_t oldmask, mask;
>      int pid, status;
>      char *args[5];
> -    char **parg;
>      int sv[2];
>
>      sigemptyset(&mask);
> @@ -498,9 +497,6 @@ static int net_bridge_run_helper(const char *helper,
> const char *bridge,
>      }
>      if (pid == 0) {
>          int open_max = sysconf(_SC_OPEN_MAX), i;
> -        char fd_buf[6+10];
> -        char br_buf[6+IFNAMSIZ] = {0};
> -        char helper_cmd[PATH_MAX + sizeof(fd_buf) + sizeof(br_buf) + 15];
>
>          for (i = 3; i < open_max; i++) {
>              if (i != sv[1]) {
> @@ -508,39 +504,18 @@ static int net_bridge_run_helper(const char *helper,
> const char *bridge,
>              }
>          }
>
> -        snprintf(fd_buf, sizeof(fd_buf), "%s%d", "--fd=", sv[1]);
> +        args[0] = (char *)helper;
> +        args[1] = (char *)"--use-vnet";
> +        args[2] = g_strdup_printf("%s%d", "--fd=", sv[1]);
> +        args[3] = g_strdup_printf("%s%s", "--br=", bridge);
> +        args[4] = NULL;
>
> -        if (strrchr(helper, ' ') || strrchr(helper, '\t')) {
> -            /* assume helper is a command */
> +        execv(helper, args);
>
> -            if (strstr(helper, "--br=") == NULL) {
> -                snprintf(br_buf, sizeof(br_buf), "%s%s", "--br=", bridge);
> -            }
> +        g_free(args[2]);
> +        g_free(args[3]);
> +        fprintf(stderr, "failed to execute helper: %s\n", helper);
>
> -            snprintf(helper_cmd, sizeof(helper_cmd), "%s %s %s %s",
> -                     helper, "--use-vnet", fd_buf, br_buf);
> -
> -            parg = args;
> -            *parg++ = (char *)"sh";
> -            *parg++ = (char *)"-c";
> -            *parg++ = helper_cmd;
> -            *parg++ = NULL;
> -
> -            execv("/bin/sh", args);
> -        } else {
> -            /* assume helper is just the executable path name */
> -
> -            snprintf(br_buf, sizeof(br_buf), "%s%s", "--br=", bridge);
> -
> -            parg = args;
> -            *parg++ = (char *)helper;
> -            *parg++ = (char *)"--use-vnet";
> -            *parg++ = fd_buf;
> -            *parg++ = br_buf;
> -            *parg++ = NULL;
> -
> -            execv(helper, args);
> -        }
>          _exit(1);
>
>      } else {
> --
> 2.21.0
>
>

Re: [Qemu-devel] [PATCH v3 3/3] net: tap: refactor net_bridge_run_helper routine

[P J P]

  Hello Li,

+-- On Mon, 1 Jul 2019, Li Qiang wrote --+
| You do two things here(avoid buffer formatting and get rid of calling 
| shell), I would suggest you split these into split patch.

Both are related, 'helper_cmd' formatting was used with the shell invocation 
as:

  helper_cmd = "qemu-bridge-helper --use-vnet --fd=sv[1] --br=bridge"
  execv("/bin/sh", "sh", "-c", helper_cmd, NULL);

The 'else' part wherein 'helper' is a /path/to/qemu-bridge-helper binary, it 
is invoked without shell "sh" and 'helper_cmd' formatting.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] [PATCH v3 3/3] net: tap: refactor net_bridge_run_helper routine

[Daniel P. Berrangé]

On Mon, Jul 01, 2019 at 06:05:58PM +0530, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
> 
> Refactor 'net_bridge_run_helper' routine to avoid buffer
> formatting to prepare 'helper_cmd' and using shell to invoke
> helper command. Instead directly execute helper program with
> due arguments.
> 
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  net/tap.c | 43 +++++++++----------------------------------
>  1 file changed, 9 insertions(+), 34 deletions(-)
> 
> Update v3: remove buffer formatting and use of shell to invoke helper
>   -> https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg00071.html
> 
> diff --git a/net/tap.c b/net/tap.c
> index e8aadd8d4b..bc9b3407a6 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -478,7 +478,6 @@ static int net_bridge_run_helper(const char *helper, const char *bridge,
>      sigset_t oldmask, mask;
>      int pid, status;
>      char *args[5];
> -    char **parg;
>      int sv[2];
>  
>      sigemptyset(&mask);
> @@ -498,9 +497,6 @@ static int net_bridge_run_helper(const char *helper, const char *bridge,
>      }
>      if (pid == 0) {
>          int open_max = sysconf(_SC_OPEN_MAX), i;
> -        char fd_buf[6+10];
> -        char br_buf[6+IFNAMSIZ] = {0};
> -        char helper_cmd[PATH_MAX + sizeof(fd_buf) + sizeof(br_buf) + 15];
>  
>          for (i = 3; i < open_max; i++) {
>              if (i != sv[1]) {
> @@ -508,39 +504,18 @@ static int net_bridge_run_helper(const char *helper, const char *bridge,
>              }
>          }
>  
> -        snprintf(fd_buf, sizeof(fd_buf), "%s%d", "--fd=", sv[1]);
> +        args[0] = (char *)helper;
> +        args[1] = (char *)"--use-vnet";
> +        args[2] = g_strdup_printf("%s%d", "--fd=", sv[1]);
> +        args[3] = g_strdup_printf("%s%s", "--br=", bridge);
> +        args[4] = NULL;
>  
> -        if (strrchr(helper, ' ') || strrchr(helper, '\t')) {
> -            /* assume helper is a command */
> +        execv(helper, args);
>  
> -            if (strstr(helper, "--br=") == NULL) {
> -                snprintf(br_buf, sizeof(br_buf), "%s%s", "--br=", bridge);
> -            }
> +        g_free(args[2]);
> +        g_free(args[3]);
> +        fprintf(stderr, "failed to execute helper: %s\n", helper);
>  
> -            snprintf(helper_cmd, sizeof(helper_cmd), "%s %s %s %s",
> -                     helper, "--use-vnet", fd_buf, br_buf);
> -
> -            parg = args;
> -            *parg++ = (char *)"sh";
> -            *parg++ = (char *)"-c";
> -            *parg++ = helper_cmd;
> -            *parg++ = NULL;
> -
> -            execv("/bin/sh", args);
> -        } else {
> -            /* assume helper is just the executable path name */
> -
> -            snprintf(br_buf, sizeof(br_buf), "%s%s", "--br=", bridge);
> -
> -            parg = args;
> -            *parg++ = (char *)helper;
> -            *parg++ = (char *)"--use-vnet";
> -            *parg++ = fd_buf;
> -            *parg++ = br_buf;
> -            *parg++ = NULL;
> -
> -            execv(helper, args);
> -        }

Hmm, it seems I was probaly a bit too optimistic in my suggestion to
drop use of shell entirely.

The original code was passing through to the shell to handle the case
where the user requested

   -netdev bridge,helper="/path/to/helper myarg otherarg"

In theory any parts could contain shell meta characters, but even if
they don't we'll have slightly broken compat with this change.

The QEMU man page has never documented that you can pass a command
and args, which get sent via the shell though. It only ever documented
the helper arg as being a plain qualified binary path.

So the question is how strictly we need to consider compatiblity.

The "if it isn't documented it never existed" option is to use your
patch here.

The moderately aggressive option is to just use g_shell_parse_argv()
to split the "helper" into a set of argv we can exec directly, and
declare that we don't support shell meta characters in helper.

The safest option is to put in a place a deprecation saying we'll
drop use of shell in future, only implementing the agressive
option in a later release.

Perhaps from your POV, the easy thing is to avoid this entire
question - just leave the code calling shell, but switch to
g_strdup_printf instead of snprintf.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v3 3/3] net: tap: refactor net_bridge_run_helper routine

[P J P]

  Hello Dan,

+-- On Tue, 2 Jul 2019, Daniel P. Berrangé wrote --+
| The original code was passing through to the shell to handle the case
| where the user requested
| 
|    -netdev bridge,helper="/path/to/helper myarg otherarg"
| 
| In theory any parts could contain shell meta characters, but even if
| they don't we'll have slightly broken compat with this change.

I wonder if anybody uses it like that. Because of the 3 arguments that 
qemu-bridge-helper takes

  --use-vnet --fd=sv[1] --br=bridge

only bridge name is supplied by user; Which is anyway comming without 'helper' 
having to include '--br=bridge' argument, as is looked for before shell 
invocation

  if (strstr(helper, "--br=") == NULL) {
      snprintf(br_buf, sizeof(br_buf), "%s%s", "--br=", bridge);
  }

'--br=bridge' has limited scope to use shell meta characters, ie. other than 
space(' ') and tab('\t').


| The QEMU man page has never documented that you can pass a command
| and args, which get sent via the shell though. It only ever documented
| the helper arg as being a plain qualified binary path.
| 
| So the question is how strictly we need to consider compatibility.
| 
| The "if it isn't documented it never existed" option is to use your
| patch here.

We don't know if "/path/to/helper arg1 arg2" usage exists in practice. And 
considering user would still be able to supply 'bridge' argument, I wonder if 
we are breaking compatibility.

| The safest option is to put in a place a deprecation saying we'll
| drop use of shell in future, only implementing the aggressive
| option in a later release.

ie. for Qemu > v4.0.0? How do we do this?

| Perhaps from your POV, the easy thing is to avoid this entire
| question - just leave the code calling shell, but switch to
| g_strdup_printf instead of snprintf.

Okay, this will be for Qemu <= v4.0.0?


Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] [PATCH v3 3/3] net: tap: refactor net_bridge_run_helper routine

[P J P]

+-- On Tue, 2 Jul 2019, P J P wrote --+
| |    -netdev bridge,helper="/path/to/helper myarg otherarg"
| | 
| | In theory any parts could contain shell meta characters, but even if
| | they don't we'll have slightly broken compat with this change.
| 
| I wonder if anybody uses it like that. Because of the 3 arguments that 
| qemu-bridge-helper takes
| 
|   --use-vnet --fd=sv[1] --br=bridge
| 
| only bridge name is supplied by user; Which is anyway comming without 'helper' 
| having to include '--br=bridge' argument, as is looked for before shell 
| invocation
| 
|   if (strstr(helper, "--br=") == NULL) {
|       snprintf(br_buf, sizeof(br_buf), "%s%s", "--br=", bridge);
|   }
| 
| '--br=bridge' has limited scope to use shell meta characters, ie. other than 
| space(' ') and tab('\t').
| 
| 
| | The QEMU man page has never documented that you can pass a command
| | and args, which get sent via the shell though. It only ever documented
| | the helper arg as being a plain qualified binary path.
| | 
| | So the question is how strictly we need to consider compatibility.
| | 
| | The "if it isn't documented it never existed" option is to use your
| | patch here.
| 
| We don't know if "/path/to/helper arg1 arg2" usage exists in practice. And 
| considering user would still be able to supply 'bridge' argument, I wonder if 
| we are breaking compatibility.
| 
| | The safest option is to put in a place a deprecation saying we'll
| | drop use of shell in future, only implementing the aggressive
| | option in a later release.
| 
| ie. for Qemu > v4.0.0? How do we do this?
| 
| | Perhaps from your POV, the easy thing is to avoid this entire
| | question - just leave the code calling shell, but switch to
| | g_strdup_printf instead of snprintf.
| 
| Okay, this will be for Qemu <= v4.0.0?
| 

@Dan:...ping!?
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F