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

Limit the number of arguments passed to execve(2) call from
a user program, as large number of them could lead to a bad
guest address error.

Reported-by: Jann Horn <jannh@google.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 linux-user/syscall.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 9be8e95..c545c12 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -7766,6 +7766,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
 #endif
     case TARGET_NR_execve:
         {
+#define ARG_MAX 65535
             char **argp, **envp;
             int argc, envc;
             abi_ulong gp;
@@ -7794,6 +7795,12 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
                 envc++;
             }
 
+            if (argc > ARG_MAX || envc > ARG_MAX) {
+                fprintf(stderr,
+                        "argc(%d), envc(%d) exceed %d\n", argc, envc, ARG_MAX);
+                ret = -TARGET_EFAULT;
+                break;
+            }
             argp = alloca((argc + 1) * sizeof(void *));
             envp = alloca((envc + 1) * sizeof(void *));
 
-- 
2.9.3

On 03/03/2017 05:25 AM, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
> 
> Limit the number of arguments passed to execve(2) call from
> a user program, as large number of them could lead to a bad
> guest address error.

Depending on how the kernel was compiled, you may have a limited maximum
size for the combined storage used by argc, environ, and all pointers.
Older kernels had a hard ceiling on storage used by argc and environ as
small as 128k (not just the bytes pointed to, but also the storage
occupied by the pointers) - it was possible to trigger E2BIG failures
with a large number of arguments in the array, but also with a single
argument that was too long.  Modern kernels have increased the limits
somewhat (now it is as large as 1/4 available stack size), but still has
maximum sizes for a single argument (larger than before, but still
finite).  Any one of these E2BIG failures is something we need to handle
gracefully, especially if the guest is expecting to be able to use a
larger limit than the host is supporting.  So I'm not sure that limiting
the number of arguments is sufficient - you may also have to limit the
size of the arguments.

I typed the above without looking at your patch.  Now that I've done
that, I see that you are tackling yet another type of corruption...

> 
> Reported-by: Jann Horn <jannh@google.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  linux-user/syscall.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 9be8e95..c545c12 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -7766,6 +7766,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>  #endif
>      case TARGET_NR_execve:
>          {
> +#define ARG_MAX 65535
>              char **argp, **envp;
>              int argc, envc;
>              abi_ulong gp;
> @@ -7794,6 +7795,12 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>                  envc++;
>              }
>  
> +            if (argc > ARG_MAX || envc > ARG_MAX) {
> +                fprintf(stderr,
> +                        "argc(%d), envc(%d) exceed %d\n", argc, envc, ARG_MAX);

Isn't there something better than fprintf() for reporting errors?

> +                ret = -TARGET_EFAULT;
> +                break;
> +            }
>              argp = alloca((argc + 1) * sizeof(void *));
>              envp = alloca((envc + 1) * sizeof(void *));

...Uggh. You're using alloca() but allowing an allocation of way more
than 4k.  That means a guest can cause corruption of the stack (or, with
large enough arguments, even escape out of the stack) before you even
get to the execve() call to even worry about E2BIG issues.  Even though
your new limit of 64k argc/envc limits the damage compared to what it
used to be, you are still prone to an allocation that walks beyond the
guard page and causes bad behavior.  Either you need the limits to be
much smaller, or you should consider using the heap instead of the stack
(alloca should never be used for more than about 4k).  And there's still
the possibility that even with your cap, that you are not handling E2BIG
correctly.  You'll need to respin this patch.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

On 3 March 2017 at 14:54, Eric Blake <eblake@redhat.com> wrote:
>> +                ret = -TARGET_EFAULT;
>> +                break;
>> +            }
>>              argp = alloca((argc + 1) * sizeof(void *));
>>              envp = alloca((envc + 1) * sizeof(void *));
>
> ...Uggh. You're using alloca() but allowing an allocation of way more
> than 4k.  That means a guest can cause corruption of the stack (or, with
> large enough arguments, even escape out of the stack) before you even
> get to the execve() call to even worry about E2BIG issues.

Yeah, linux-user is shot through with that kind of alloca() usage.

(It's not great, but it's not a security hole because we already
give the guest binary complete control to do anything it likes.
Worth fixing bugs if we run into them, though.)

thanks
-- PMM

On Fri, Mar 3, 2017 at 4:56 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 3 March 2017 at 14:54, Eric Blake <eblake@redhat.com> wrote:
>>> +                ret = -TARGET_EFAULT;
>>> +                break;
>>> +            }
>>>              argp = alloca((argc + 1) * sizeof(void *));
>>>              envp = alloca((envc + 1) * sizeof(void *));
>>
>> ...Uggh. You're using alloca() but allowing an allocation of way more
>> than 4k.  That means a guest can cause corruption of the stack (or, with
>> large enough arguments, even escape out of the stack) before you even
>> get to the execve() call to even worry about E2BIG issues.
>
> Yeah, linux-user is shot through with that kind of alloca() usage.
>
> (It's not great, but it's not a security hole because we already
> give the guest binary complete control to do anything it likes.
> Worth fixing bugs if we run into them, though.)

It could be a security hole if a benign guest userspace process decides to
allow a remote client to specify a bunch of environment variables or so.
E.g. HTTP servers with CGI support pass HTTP headers on as
environment variables whose names start with "HTTP_"; however, since
HTTP servers usually limit the request size, this isn't actually exploitable
in that case. But there could theoretically be similar scenarios.

  Hello Eric,

+-- On Fri, 3 Mar 2017, Eric Blake wrote --+
| much smaller, or you should consider using the heap instead of the stack
| (alloca should never be used for more than about 4k).  And there's still
| the possibility that even with your cap, that you are not handling E2BIG
| correctly.  You'll need to respin this patch.

  Yes, sent a revised patch set v2.

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

On 3 March 2017 at 11:25, P J P <ppandit@redhat.com> wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> Limit the number of arguments passed to execve(2) call from
> a user program, as large number of them could lead to a bad
> guest address error.
>
> Reported-by: Jann Horn <jannh@google.com>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  linux-user/syscall.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 9be8e95..c545c12 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -7766,6 +7766,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>  #endif
>      case TARGET_NR_execve:
>          {
> +#define ARG_MAX 65535
>              char **argp, **envp;
>              int argc, envc;
>              abi_ulong gp;
> @@ -7794,6 +7795,12 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>                  envc++;
>              }
>
> +            if (argc > ARG_MAX || envc > ARG_MAX) {
> +                fprintf(stderr,
> +                        "argc(%d), envc(%d) exceed %d\n", argc, envc, ARG_MAX);
> +                ret = -TARGET_EFAULT;
> +                break;
> +            }
>              argp = alloca((argc + 1) * sizeof(void *));
>              envp = alloca((envc + 1) * sizeof(void *));

This code is already supposed to handle "argument string too big",
see commit a6f79cc9a5e.

What's the actual bug case we're trying to handle here?

EFAULT looks like a decidedly odd error to return here, too.

thanks
-- PMM

On Fri, Mar 3, 2017 at 4:55 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 3 March 2017 at 11:25, P J P <ppandit@redhat.com> wrote:
>> From: Prasad J Pandit <pjp@fedoraproject.org>
>>
>> Limit the number of arguments passed to execve(2) call from
>> a user program, as large number of them could lead to a bad
>> guest address error.
>>
>> Reported-by: Jann Horn <jannh@google.com>
>> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
>> ---
>>  linux-user/syscall.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>> index 9be8e95..c545c12 100644
>> --- a/linux-user/syscall.c
>> +++ b/linux-user/syscall.c
>> @@ -7766,6 +7766,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>>  #endif
>>      case TARGET_NR_execve:
>>          {
>> +#define ARG_MAX 65535
>>              char **argp, **envp;
>>              int argc, envc;
>>              abi_ulong gp;
>> @@ -7794,6 +7795,12 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>>                  envc++;
>>              }
>>
>> +            if (argc > ARG_MAX || envc > ARG_MAX) {
>> +                fprintf(stderr,
>> +                        "argc(%d), envc(%d) exceed %d\n", argc, envc, ARG_MAX);
>> +                ret = -TARGET_EFAULT;
>> +                break;
>> +            }
>>              argp = alloca((argc + 1) * sizeof(void *));
>>              envp = alloca((envc + 1) * sizeof(void *));
>
> This code is already supposed to handle "argument string too big",
> see commit a6f79cc9a5e.
>
> What's the actual bug case we're trying to handle here?

commit a6f79cc9a5e doesn't help here, it only bails out after the two
alloca() calls

On 3 March 2017 at 15:57, Jann Horn <jannh@google.com> wrote:
> On Fri, Mar 3, 2017 at 4:55 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> This code is already supposed to handle "argument string too big",
>> see commit a6f79cc9a5e.
>>
>> What's the actual bug case we're trying to handle here?
>
> commit a6f79cc9a5e doesn't help here, it only bails out after the two
> alloca() calls

That's why I asked "what's the actual bug case we're trying to handle?"...

thanks
-- PMM

+-- On Fri, 3 Mar 2017, Jann Horn wrote --+
| On Fri, Mar 3, 2017 at 4:55 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
| >> +            if (argc > ARG_MAX || envc > ARG_MAX) {
| >> +                fprintf(stderr,
| >> +                        "argc(%d), envc(%d) exceed %d\n", argc, envc, ARG_MAX);
| >> +                ret = -TARGET_EFAULT;
| >> +                break;
| >> +            }
| >>              argp = alloca((argc + 1) * sizeof(void *));
| >>              envp = alloca((envc + 1) * sizeof(void *));
| >
| > This code is already supposed to handle "argument string too big",
| > see commit a6f79cc9a5e.
| >
| > What's the actual bug case we're trying to handle here?

Not argument string, but argument count too big leads to a bad address in 
get_user_ual(...), resulting in segfault.

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