Patchwork qga: check length of command-line & environment variables

login
register
mail settings
Submitter P J P
Date Jan. 11, 2019, 9:52 a.m.
Message ID <nycvar.YSQ.7.76.1901111453590.12783@xnncv>
Download mbox | patch
Permalink /patch/697519/
State New
Headers show

Comments

P J P - Jan. 11, 2019, 9:52 a.m.
+-- On Mon, 7 Jan 2019, P J P wrote --+
| 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.
| 
| -    size_t str_size = 1;
| +    size_t str_size = 1, args_max;
|  
| +    args_max = sysconf(_SC_ARG_MAX);

Looks like sysconf()/_SC_ARG_MAX declarations aren't available. Is it okay to 
include header <unistd.h> ?
Daniel P. Berrange - Jan. 11, 2019, 9:56 a.m.
On Fri, Jan 11, 2019 at 03:22:51PM +0530, P J P wrote:
> +-- On Mon, 7 Jan 2019, P J P wrote --+
> | 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.
> | 
> | -    size_t str_size = 1;
> | +    size_t str_size = 1, args_max;
> |  
> | +    args_max = sysconf(_SC_ARG_MAX);
> 
> Looks like sysconf()/_SC_ARG_MAX declarations aren't available. Is it okay to 
> include header <unistd.h> ?

qga/commands.c already includes qemu/osdep.h which includs unistd.h.

The build problem patchew reported was from *mingw* builds where
sysconf does not exist.

Regards,
Daniel
P J P - Jan. 13, 2019, 5:28 p.m.
+-- On Fri, 11 Jan 2019, Daniel P. Berrangé wrote --+
| qga/commands.c already includes qemu/osdep.h which includs unistd.h.
|
| The build problem patchew reported was from *mingw* builds where
| sysconf does not exist.

I see; Not sure how to fix it. Maybe with conditional declaration?

#ifdef __MINGW[32|64]__
extern long int sysconf (int __name);
#endif

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
niuguoxiang - Jan. 22, 2019, 3:49 a.m.
Hi,

Is this issue fixed?


Br,
Guoxiang Niu

华为技术有限公司 Huawei Technologies Co., Ltd.



本邮件及其附件含有华为公司的保密信息,仅限于发送给上面地址中列出的个人或群组。禁
止任何其他人以任何形式使用(包括但不限于全部或部分地泄露、复制、或散发)本邮件中
的信息。如果您错收了本邮件,请您立即电话或邮件通知发件人并删除本邮件!
This e-mail and its attachments contain confidential information from HUAWEI, which 
is intended only for the person or entity whose address is listed above. Any use of the 
information contained herein in any way (including, but not limited to, total or partial 
disclosure, reproduction, or dissemination) by persons other than the intended 
recipient(s) is prohibited. If you receive this e-mail in error, please notify the sender by 
phone or email immediately and delete it!

-----邮件原件-----
发件人: P J P [mailto:ppandit@redhat.com] 
发送时间: 2019年1月14日 1:28
收件人: Daniel P. Berrangé <berrange@redhat.com>
抄送: Michael Roth <mdroth@linux.vnet.ibm.com>; QEMU Developers <qemu-devel@nongnu.org>; niuguoxiang <niuguoxiang@huawei.com>
主题: Re: [Qemu-devel] [PATCH] qga: check length of command-line & environment variables

+-- On Fri, 11 Jan 2019, Daniel P. Berrangé wrote --+
| qga/commands.c already includes qemu/osdep.h which includs unistd.h.
|
| The build problem patchew reported was from *mingw* builds where 
| sysconf does not exist.

I see; Not sure how to fix it. Maybe with conditional declaration?

#ifdef __MINGW[32|64]__
extern long int sysconf (int __name);
#endif

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
Michael Roth - Jan. 24, 2019, 5:38 p.m.
Quoting P J P (2019-01-13 11:28:03)
> +-- On Fri, 11 Jan 2019, Daniel P. Berrangé wrote --+
> | qga/commands.c already includes qemu/osdep.h which includs unistd.h.
> |
> | The build problem patchew reported was from *mingw* builds where
> | sysconf does not exist.
> 
> I see; Not sure how to fix it. Maybe with conditional declaration?
> 
> #ifdef __MINGW[32|64]__
> extern long int sysconf (int __name);
> #endif

I would call a helper function like get_args_max() or whatever and have
the posix implementation in qga/commands-posix.c and a stub'd version
in qga/commands-win32.c. There's an article here that might be useful
for figuring out how we would implement get_args_max() it for win32:

  https://blogs.msdn.microsoft.com/oldnewthing/20031210-00/?p=41553

> 
> Thank you.
> --
> Prasad J Pandit / Red Hat Product Security Team
> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
P J P - Jan. 24, 2019, 5:53 p.m.
+-- On Thu, 24 Jan 2019, Michael Roth wrote --+
| I would call a helper function like get_args_max() or whatever and have
| the posix implementation in qga/commands-posix.c and a stub'd version
| in qga/commands-win32.c. There's an article here that might be useful
| for figuring out how we would implement get_args_max() it for win32:
| 
|   https://blogs.msdn.microsoft.com/oldnewthing/20031210-00/?p=41553

Interesting, I'll follow-up on it.

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

Patch

===
diff --git a/qga/commands.c b/qga/commands.c
--- a/qga/commands.c
+++ b/qga/commands.c
@@ -18,6 +18,7 @@ 
 #include "qemu/atomic.h"
+#include <unistd.h>
===

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