[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2] qga: check length of command-line & environm
From: |
Marc-André Lureau |
Subject: |
Re: [Qemu-devel] [PATCH v2] qga: check length of command-line & environment variables |
Date: |
Wed, 22 May 2019 15:34:30 +0200 |
Hi
On Sun, May 19, 2019 at 10:55 AM P J P <address@hidden> wrote:
>
> From: Prasad J Pandit <address@hidden>
>
> Qemu guest agent while executing user commands does not seem to
> check length of argument list and/or environment variables passed.
> It may lead to integer overflow or infinite loop issues. Add check
> to avoid it.
Are you intentionally not telling where these overflow or loop happen?
Isn't the kernel already giving an error if given too much
environment/arguments on exec?
>
> Reported-by: Niu Guoxiang <address@hidden>
> Signed-off-by: Prasad J Pandit <address@hidden>
> ---
> qga/commands-posix.c | 18 ++++++++++++++++++
> qga/commands-win32.c | 13 +++++++++++++
> qga/commands.c | 8 ++++++--
> qga/guest-agent-core.h | 2 ++
> 4 files changed, 39 insertions(+), 2 deletions(-)
>
> Update v2: add helper function ga_get_arg_max()
> -> https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg06360.html
>
> diff --git a/qga/commands-posix.c b/qga/commands-posix.c
> index 7ee6a33cce..e0455722e0 100644
> --- a/qga/commands-posix.c
> +++ b/qga/commands-posix.c
> @@ -60,6 +60,24 @@ extern char **environ;
> #endif
> #endif
>
> +size_t ga_get_arg_max(void)
> +{
> + /* Since kernel 2.6.23, most architectures support argument size limit
> + * derived from the soft RLIMIT_STACK resource limit (see getrlimit(2)).
> + * For these architectures, the total size is limited to 1/4 of the
> + * allowed stack size. (see execve(2))
> + *
> + * struct rlimit r;
> + *
> + * getrlimit(RLIMIT_STACK, &r);
> + * ARG_MAX = r.rlim_cur / 4;
> + *
> + * ARG_MAX is _NOT_ the maximum number of arguments. It is size of the
> + * memory used to hold command line arguments and environment variables.
> + */
> + return sysconf(_SC_ARG_MAX);
> +}
> +
> static void ga_wait_child(pid_t pid, int *status, Error **errp)
> {
> pid_t rpid;
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index 6b67f16faf..47bbddd74a 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -92,6 +92,19 @@ static OpenFlags guest_file_open_modes[] = {
> g_free(suffix); \
> } while (0)
>
> +size_t ga_get_arg_max(void)
> +{
> + /* Win32 environment has different values for the ARG_MAX constant.
> + * We'll go with the maximum here.
> + *
> + * https://devblogs.microsoft.com/oldnewthing/?p=41553
> + *
> + * ARG_MAX is _NOT_ the maximum number of arguments. It is size of the
> + * memory used to hold command line arguments and environment variables.
> + */
> + return 32767;
> +}
> +
> static OpenFlags *find_open_flag(const char *mode_str)
> {
> int mode;
> diff --git a/qga/commands.c b/qga/commands.c
> index 0c7d1385c2..425a4c405f 100644
> --- a/qga/commands.c
> +++ b/qga/commands.c
> @@ -231,17 +231,21 @@ static char **guest_exec_get_args(const strList *entry,
> bool log)
> int count = 1, i = 0; /* reserve for NULL terminator */
> char **args;
> char *str; /* for logging array of arguments */
> - size_t str_size = 1;
> + size_t str_size = 1, arg_max;
>
> + arg_max = ga_get_arg_max();
> for (it = entry; it != NULL; it = it->next) {
> count++;
> str_size += 1 + strlen(it->value);
> + if (str_size >= arg_max || count >= arg_max / 2) {
> + break;
This seems to silently drop remaining arguments, which is probably not
what you want.
> + }
> }
>
> str = g_malloc(str_size);
> *str = 0;
> args = g_malloc(count * sizeof(char *));
> - for (it = entry; it != NULL; it = it->next) {
> + for (it = entry; it != NULL && i < count; it = it->next) {
> args[i++] = it->value;
> pstrcat(str, str_size, it->value);
> if (it->next) {
> diff --git a/qga/guest-agent-core.h b/qga/guest-agent-core.h
> index 60eae16f27..300ff7e482 100644
> --- a/qga/guest-agent-core.h
> +++ b/qga/guest-agent-core.h
> @@ -46,6 +46,8 @@ const char *ga_fsfreeze_hook(GAState *s);
> int64_t ga_get_fd_handle(GAState *s, Error **errp);
> int ga_parse_whence(GuestFileWhence *whence, Error **errp);
>
> +size_t ga_get_arg_max(void);
> +
> #ifndef _WIN32
> void reopen_fd_to_null(int fd);
> #endif
> --
> 2.20.1
>
>
--
Marc-André Lureau
- [Qemu-devel] [PATCH v2] qga: check length of command-line & environment variables, P J P, 2019/05/19
- Re: [Qemu-devel] [PATCH v2] qga: check length of command-line & environment variables, Daniel Henrique Barboza, 2019/05/20
- Re: [Qemu-devel] [PATCH v2] qga: check length of command-line & environment variables,
Marc-André Lureau <=
- Re: [Qemu-devel] [PATCH v2] qga: check length of command-line & environment variables, P J P, 2019/05/23
- Re: [Qemu-devel] [PATCH v2] qga: check length of command-line & environment variables, Marc-André Lureau, 2019/05/23
- Re: [Qemu-devel] [PATCH v2] qga: check length of command-line & environment variables, P J P, 2019/05/29
- Re: [Qemu-devel] [PATCH v2] qga: check length of command-line & environment variables, Marc-André Lureau, 2019/05/29
- Re: [Qemu-devel] [PATCH v2] qga: check length of command-line & environment variables, P J P, 2019/05/29
- Re: [Qemu-devel] [PATCH v2] qga: check length of command-line & environment variables, Marc-André Lureau, 2019/05/29
- Re: [Qemu-devel] [PATCH v2] qga: check length of command-line & environment variables, P J P, 2019/05/29