| Message ID | 20190701123558.30512-4-ppandit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | restrict bridge interface name to IFNAMSIZ | expand |
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 > >
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
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
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
+-- 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
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 {