binfmts.h MAX_ARG_STRINGS excessive value allows heap spraying

binfmts.h MAX_ARG_STRINGS excessive value allows heap spraying

[Leonard den Ottolander]

In the article 
https://googleprojectzero.blogspot.nl/2014/08/the-poisoned-nul-byte-2014-edition.html
the author describes launching an attack on an off by one NUL byte bug
in glibc.

He explains that both the memory leak in the option parsing of PolKits
pkexec.c and the excessive value of MAX_ARG_STRINGS in the kernels
include/uapi/linux/binfmts.h (#define MAX_ARG_STRINGS 0x7FFFFFFF)
enabled his attack.

The rationale for that excessive value is a bit odd:

"MAX_ARG_STRINGS is chosen to fit in a signed 32-bit integer."

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/include/uapi/linux/binfmts.h?id=607ca46e97a1b6594b29647d98a32d545c24bdff

The fact that the integer is so large makes that on a 32 bit platform
the entire heap can be initialized with values the attacker provides,
given the memory leak in the option parsing of pkexec.c, an approach the
author calls "heap spraying".

(Note that two similar memory leaks exist in the option parsing of
pkexecs sibling pkcheck.c, so pkcheck will also allow an attacker to
heap spray its entire memory on a 32 bit system.)

If you compare http://www.linuxjournal.com/article/6060 you will see
that the TOTAL amount of memory reserved for command line arguments used
to be 32 pages, i.e. 128KiB (#define MAX_ARG_PAGES 32). The amount of
memory reserved for command line arguments in more current kernels is
the multiplication of MAX_ARG_STRINGS and MAX_ARG_STRLEN.

Both the memory leaks in pkexec.c and pkcheck.c seem very severe, but
their impact would be much less if MAX_ARG_STRINGS would hold a sensible
value.

After some experimentation with
$ rpmbuild -ba kernel.spec
on CentOS-7 I've come up with values that allow this kernel compilation.
As this kind of build is probably one of the most demanding actions in
relation to MAX_ARG_STRINGS and MAX_ARG_STRLEN I believe below values to
be sensible and much safer defaults:

#define MAX_ARG_STRLEN 65536
#define MAX_ARG_STRINGS 4096

allow me to build a kernel on a system using those values.

Since the above value of MAX_ARG_STRLEN is half of the current value we
might leave it alone and only alter MAX_ARG_STRINGS, resulting in

--- a/include/uapi/linux/binfmts.h	2016-11-23 21:02:31.000000000 +0100
+++ b/include/uapi/linux/binfmts.h	2017-02-07 15:40:13.548403615 +0100
@@ -12,7 +12,7 @@ struct pt_regs;
  * MAX_ARG_STRINGS is chosen to fit in a signed 32-bit integer.
  */
 #define MAX_ARG_STRLEN (PAGE_SIZE * 32)
-#define MAX_ARG_STRINGS 0x7FFFFFFF
+#define MAX_ARG_STRINGS 4096
 
 /* sizeof(linux_binprm->buf) */
 #define BINPRM_BUF_SIZE 128

Filed a bug at https://bugzilla.kernel.org/show_bug.cgi?id=194809 .

Regards,
Leonard.

-- 
mount -t life -o ro /dev/dna /genetic/research

Re: binfmts.h MAX_ARG_STRINGS excessive value allows heap spraying

[Carlos O'Donell]

On 03/07/2017 09:44 AM, Leonard den Ottolander wrote:
> After some experimentation with
> $ rpmbuild -ba kernel.spec
> on CentOS-7 I've come up with values that allow this kernel compilation.
> As this kind of build is probably one of the most demanding actions in
> relation to MAX_ARG_STRINGS and MAX_ARG_STRLEN I believe below values to
> be sensible and much safer defaults:

The most demanding application I've ever seen when playing with this is the
compiler because it has to pass some very large strings from the driver
to a subprocess. So much so that we have `@file` to use intermediate file
storage to workaround kernel limits on various operating systems.
 
> #define MAX_ARG_STRLEN 65536
> #define MAX_ARG_STRINGS 4096
> 
> allow me to build a kernel on a system using those values.
> 
> Since the above value of MAX_ARG_STRLEN is half of the current value we
> might leave it alone and only alter MAX_ARG_STRINGS, resulting in
> 
> --- a/include/uapi/linux/binfmts.h	2016-11-23 21:02:31.000000000 +0100
> +++ b/include/uapi/linux/binfmts.h	2017-02-07 15:40:13.548403615 +0100
> @@ -12,7 +12,7 @@ struct pt_regs;
>   * MAX_ARG_STRINGS is chosen to fit in a signed 32-bit integer.
>   */
>  #define MAX_ARG_STRLEN (PAGE_SIZE * 32)
> -#define MAX_ARG_STRINGS 0x7FFFFFFF
> +#define MAX_ARG_STRINGS 4096

In glibc we limit setuid applications, for example sanitizing their
environment where it would cause problems or alter behaviour in 
unintended ways.

Can we avoid imposing a limit on all applications?

-- 
Cheers,
Carlos.

Re: binfmts.h MAX_ARG_STRINGS excessive value allows heap spraying

[Leonard den Ottolander]

On Wed, 2017-03-08 at 12:54 -0500, Carlos O'Donell wrote:
> The most demanding application I've ever seen when playing with this is the
> compiler because it has to pass some very large strings from the driver
> to a subprocess. So much so that we have `@file` to use intermediate file
> storage to workaround kernel limits on various operating systems.

So a value that would satisfy glibc would suffice. Any problem with that
limit of 4096 arguments? The size of each individual argument is still
the old limit of 128KiB.

By the way, my kernel build example seems to already catch your
scenario. Kernel build uses a compiler.

> In glibc we limit setuid applications, for example sanitizing their
> environment where it would cause problems or alter behaviour in 
> unintended ways.
> 
> Can we avoid imposing a limit on all applications?

Not imposing a limit - btw, 0x7FFFFFFF *is* a limit albeit a ridiculous
and dangerously large limit - is a bad idea, because it allows the
aforementioned "heap-spraying" which is a serious attack vector.

Note that 128KiB * 4096 arguments still adds up to 512MiB!!!

If you don't feel the limit of 4096 arguments is sufficient please
provide us with an example where that limit is insufficient.

Regards,
Leonard.

-- 
mount -t life -o ro /dev/dna /genetic/research

Re: binfmts.h MAX_ARG_STRINGS excessive value allows heap spraying

[Leonard den Ottolander]

On Wed, 2017-03-08 at 19:18 +0100, Leonard den Ottolander wrote:
> So a value that would satisfy glibc would suffice.

That of course should be "gcc" not "glibc".

-- 
mount -t life -o ro /dev/dna /genetic/research

Re: binfmts.h MAX_ARG_STRINGS excessive value allows heap spraying

[Leonard den Ottolander]

On Wed, 2017-03-08 at 12:54 -0500, Carlos O'Donell wrote:
> In glibc we limit setuid applications, for example sanitizing their
> environment where it would cause problems or alter behaviour in 
> unintended ways.

Please explain what these limitations are, and when they were imposed,
as in the article
https://googleprojectzero.blogspot.nl/2014/08/the-poisoned-nul-byte-2014-edition.html
the author is actually using a setuid binary (pkexec) and clearly not
running into any limitations with that particular exploit.

Also note that heap spraying can happen in any binary that has memory
leaks in its option parsing. pkexec.c and pkcheck.c are known to suffer
such issues, but other binaries could be affected. Setting
MAX_ARG_STRINGS to a sensible value significantly reduces the impact of
such heap spraying.

Regards,
Leonard.

-- 
mount -t life -o ro /dev/dna /genetic/research

Re: binfmts.h MAX_ARG_STRINGS excessive value allows heap spraying

[Joseph Myers]

On Wed, 8 Mar 2017, Leonard den Ottolander wrote:

> If you don't feel the limit of 4096 arguments is sufficient please
> provide us with an example where that limit is insufficient.

mv * /some/where/

(in a directory with many files).

Linking thousands of .o files into a large program or putting them in a .a 
file in a single command.

Constructing a make command line with thousands of -o options to mark 
files as considered to be old (a real case where I ran into the old 128k 
limit when it was there).

There are of course workarounds for these - but "the number of files in 
the source + build trees of a very large project" seems a better way of 
judging limits than 4096.

-- 
Joseph S. Myers
joseph-qD8j1LwMmJjtCj0u4l0SBw@public.gmane.org

Re: binfmts.h MAX_ARG_STRINGS excessive value allows heap spraying

[Carlos O'Donell]

On 03/08/2017 01:18 PM, Leonard den Ottolander wrote:
> On Wed, 2017-03-08 at 12:54 -0500, Carlos O'Donell wrote:
>> In glibc we limit setuid applications, for example sanitizing their
>> environment where it would cause problems or alter behaviour in 
>> unintended ways.
>>
>> Can we avoid imposing a limit on all applications?
> 
> Not imposing a limit - btw, 0x7FFFFFFF *is* a limit albeit a ridiculous
> and dangerously large limit - is a bad idea, because it allows the
> aforementioned "heap-spraying" which is a serious attack vector.
> 
> Note that 128KiB * 4096 arguments still adds up to 512MiB!!!
> 
> If you don't feel the limit of 4096 arguments is sufficient please
> provide us with an example where that limit is insufficient.

(a) Justification.

A good design should not unduly limit what users can do without a good
justification.

It is my opinion that it is not enough data to say that a kernel build
is sufficient to justify a limit that will affect all applications.

Minimally I'd expect you to run with such a kernel patch through an entire
distro build cycle to see if anything else breaks. For example reaching
out to the Fedora or SUSE teams to test the patch in Rawhide or
Tumbleweed.

What if 4096 is too much? Why not lower? You could try using systemtap
or dtrace and running a distro start to finish and auditing the mean
values you see (see (c) for a discussion on ensuring the userspace tooling
remains correct after this change).

(b) Attack vector.

The privilege escalation in your example is caused by a fault in a
setuid application.

Can we impose stricter limits on setuid binaries instead of on all
binaries?

In glibc for example we do not allow certain environment variables to
impact the behaviour of setuid binaries e.g. MALLOC_PERTURB_ env var
is ignored in secure mode for setuid binaries (which might be used
for similar heap-spraying).

Can the kernel similarly enforce a different MAX_ARG_STRINGS for setuid?

(c) What limit is appropriate?

No limit is appropriate. Users should be able to use all of their system
resources to accomplish whatever task they need to get done.

Yes, portable applications have limits e.g. ARG_MAX, sysconf(_SC_ARG_MAX),
xargs --show-limits, getconf ARG_MAX etc. We should make sure that any
limit we set does not conflict with current implementations and standards
conformance.

Yes, limits should be balanced against security issues, and for setuid
binaries I agree, but you propose a blanket limit, which is why I'm asking
the question again: Can we impose stricter limits on setuid binaries
instead of on all binaries?

-- 
Cheers,
Carlos.

Re: binfmts.h MAX_ARG_STRINGS excessive value allows heap spraying

[Leonard den Ottolander]

On Wed, 2017-03-08 at 16:05 -0500, Carlos O'Donell wrote: 
> On 03/08/2017 01:18 PM, Leonard den Ottolander wrote:
> > Note that 128KiB * 4096 arguments still adds up to 512MiB!!!
> > 
> > If you don't feel the limit of 4096 arguments is sufficient please
> > provide us with an example where that limit is insufficient.
> 
> (a) Justification.
> 
> A good design should not unduly limit what users can do without a good
> justification.

The justification is that the excessive value of the MAX_ARG_STRINGS
allows heap spraying on 32 bit processes. Or more precisely, the fact
that the multiplication of MAX_ARG_STRINGS and MAX_ARG_STRLEN being
equal to or larger than the 2^32 allows heap spraying.

> It is my opinion that it is not enough data to say that a kernel build
> is sufficient to justify a limit that will affect all applications.
> 
> Minimally I'd expect you to run with such a kernel patch through an entire
> distro build cycle to see if anything else breaks.

I don't think that's really necessary. We can argue whether that value
should be a bit bigger, but a limit is necessary to disable the very
serious attack vector that heap spraying is. In any binary, not just
setuid ones. It's a jumping board to launch any attack via the heap
sprayed binary.

> What if 4096 is too much? Why not lower?

Well, building that C7 kernel didn't work with 2048, it choked on a make
file with something like a little over 3000 -o options. I chose 4096 as
it was sufficient for what I considered to be one of the heaviest use
cases.

The problem is that the multiplication of MAX_ARG_STRLEN and
MAX_ARG_STRINGS should stay below 4GiB to disable the heap spraying.

This means that if f.e. both MAX_ARG_STRINGS and MAX_ARG_STRLEN are
65536 the multiplication is 4GiB and heap spraying is possible.

Of course an extra value limiting this multiplier could be added
(MAX_ARG_STRSLEN along the lines of MAX_ARG_PAGES) and used in exec.c to
limit the total amount of reserved memory. This would allow for the
multiplication of MAX_ARG_STRINGS and MAX_ARG_STRLEN to go beyond 2^32.

And it would save a lot of memory because even with my conservative
values the amount of memory reserved for options is still 65536 x 4096 =
256MiB.

> You could try using systemtap
> or dtrace and running a distro start to finish and auditing the mean
> values you see (see (c) for a discussion on ensuring the userspace tooling
> remains correct after this change).

I have not been dtracing for, but I've not encountered any issue with
kernels using MAX_ARG_STRLEN 65536 and MAX_ARG_STRINGS 4096 so far
(running a.o. on this desktop).

> (b) Attack vector.
> 
> The privilege escalation in your example is caused by a fault in a
> setuid application.

The orthogonality of attack vectors seems to be lost on many I speak on
the issue. It means that every vector has to be judged individually.

The fact that the binary in the example is setuid is
orthogonal/irrelevant to the fact that the high value of MAX_ARG_STRINGS
combined with memory leaks in executables option parsing allow heap
spraying. The heap spraying in itself is a very serious vector that
allows launching any attack via the sprayed heap whether or not the
involved binary is setuid. 

> Can we impose stricter limits on setuid binaries instead of on all
> binaries?

Because an zero day can also be launched via a heap sprayed non setuid
binary like pkcheck. 

> In glibc for example we do not allow certain environment variables to
> impact the behaviour of setuid binaries e.g. MALLOC_PERTURB_ env var
> is ignored in secure mode for setuid binaries (which might be used
> for similar heap-spraying).

This is all very nice but it did not block the attack described in the
article I linked. Your solution sounds rather convoluted. With a
sensible value for MAX_ARG_STRINGS all this working around the real
issue is unnecessary: The heap will no longer collide into the stack of
any binary that does not sanitize its argument list. 

> (c) What limit is appropriate?
> 
> No limit is appropriate. Users should be able to use all of their system
> resources to accomplish whatever task they need to get done.

Oh the poor user. So you rather leave the user exposed to attacks that
leverage heap spraying than imposing a limit?

The point is that this value (the multiplication of both really) allows
an adversary to launch any attack in his inventory using a heap sprayed
binary. That is not a situation you want to prolong.

Regards,
Leonard.

-- 
mount -t life -o ro /dev/dna /genetic/research

Re: binfmts.h MAX_ARG_STRINGS excessive value allows heap spraying

[Leonard den Ottolander]

--- a/include/uapi/linux/binfmts.h	2016-11-23 21:02:31.000000000 +0100
+++ b/include/uapi/linux/binfmts.h	2017-03-09 01:52:14.716319950 +0100
@@ -9,10 +9,15 @@ struct pt_regs;
  * These are the maximum length and maximum number of strings passed to the
  * execve() system call.  MAX_ARG_STRLEN is essentially random but serves to
  * prevent the kernel from being unduly impacted by misaddressed pointers.
- * MAX_ARG_STRINGS is chosen to fit in a signed 32-bit integer.
+ * MAX_ARG_STRINGS * MAX_ARG_STRLEN should be smaller than the 4GiB
+ *  address space on 32 bit to avoid heap spraying.
+ * MAX_ARG_STRSLEN to the rescue - the MAX_ARG_PAGES concept was there
+ *  for a reason.
+ * We can now safely increase STRINGS * STRLEN beyond 4GiB if need be.
  */
-#define MAX_ARG_STRLEN (PAGE_SIZE * 32)
-#define MAX_ARG_STRINGS 0x7FFFFFFF
+#define MAX_ARG_STRSLEN 262144
+#define MAX_ARG_STRLEN 65536
+#define MAX_ARG_STRINGS 4096
 
 /* sizeof(linux_binprm->buf) */
 #define BINPRM_BUF_SIZE 128
--- a/fs/exec.c	2017-02-20 08:04:43.000000000 +0100
+++ b/fs/exec.c	2017-03-09 01:54:25.931476370 +0100
@@ -459,6 +459,7 @@ static int copy_strings(int argc, struct
 	char *kaddr = NULL;
 	unsigned long kpos = 0;
 	int ret;
+	int total_bytes = 0;
 
 	while (argc-- > 0) {
 		const char __user *str;
@@ -478,6 +479,11 @@ static int copy_strings(int argc, struct
 		if (!valid_arg_len(bprm, len))
 			goto out;
 
+		/* -E2BIG is fine for now */
+		total_bytes += len;
+		if (total_bytes > MAX_ARG_STRSLEN)
+			goto out;
+
 		/* We're going to work our way backwords. */
 		pos = bprm->p;
 		str += len;

I've successfully built a kernel on a system with a kernel using above
values.

As we have now introduced a safeguard (MAX_ARG_STRSLEN capping the total
amount of memory reserved) the values of MAX_ARG_STRLEN and
MAX_ARG_STRINGS can be increased beyond where their multiplication
reaches 2^32.

So if we really want to support lets say users having directories of
128k files we can now safely set MAX_ARG_STRINGS to 131072 and assuming
an average file name length of 32 set MAX_ARG_STRSLEN to 4194304.

Seems a little excessive to me for a default, but it is now safe.

Regards,
Leonard.

Re: binfmts.h MAX_ARG_STRINGS excessive value allows heap spraying

[Carlos O'Donell]

On 03/09/2017 09:04 AM, Leonard den Ottolander wrote:
> On Wed, 2017-03-08 at 16:05 -0500, Carlos O'Donell wrote: 
>> On 03/08/2017 01:18 PM, Leonard den Ottolander wrote:
>>> Note that 128KiB * 4096 arguments still adds up to 512MiB!!!
>>>
>>> If you don't feel the limit of 4096 arguments is sufficient please
>>> provide us with an example where that limit is insufficient.
>>
>> (a) Justification.
>>
>> A good design should not unduly limit what users can do without a good
>> justification.
> 
> The justification is that the excessive value of the MAX_ARG_STRINGS
> allows heap spraying on 32 bit processes. Or more precisely, the fact
> that the multiplication of MAX_ARG_STRINGS and MAX_ARG_STRLEN being
> equal to or larger than the 2^32 allows heap spraying.
> 
>> It is my opinion that it is not enough data to say that a kernel build
>> is sufficient to justify a limit that will affect all applications.
>>
>> Minimally I'd expect you to run with such a kernel patch through an entire
>> distro build cycle to see if anything else breaks.
> 
> I don't think that's really necessary. We can argue whether that value
> should be a bit bigger, but a limit is necessary to disable the very
> serious attack vector that heap spraying is. In any binary, not just
> setuid ones. It's a jumping board to launch any attack via the heap
> sprayed binary.

It is my opinion that your justification is insufficient to limit all
of userspace to the newer more restrictive limit without any remedy
should existing build systems start to fail. One could argue the use of
@file support in the compiler, static linker, and assembler might help,
but that doesn't fix all issues and could be costly to re-architect such
build systems.

We should pursue other avenues to limit the attack vector first.

> Of course an extra value limiting this multiplier could be added
> (MAX_ARG_STRSLEN along the lines of MAX_ARG_PAGES) and used in exec.c to
> limit the total amount of reserved memory. This would allow for the
> multiplication of MAX_ARG_STRINGS and MAX_ARG_STRLEN to go beyond 2^32.

This sounds like a much better option, particularly if as a remedy the
system administrator can control the max pages allowed for this to a
configurable limit.

> I have not been dtracing for, but I've not encountered any issue with
> kernels using MAX_ARG_STRLEN 65536 and MAX_ARG_STRINGS 4096 so far
> (running a.o. on this desktop).

This is insufficient research into the problem to justify lowering
the limit.

>> (c) What limit is appropriate?
>>
>> No limit is appropriate. Users should be able to use all of their system
>> resources to accomplish whatever task they need to get done.
> 
> Oh the poor user. So you rather leave the user exposed to attacks that
> leverage heap spraying than imposing a limit?

There is a balance between keeping large build systems functional and
limiting arguments.

My position is that I would rather harden malloc metadata than limit
argument lengths. Florian Weimer on my team (Red Hat glibc team) is
already looking at this:

https://sourceware.org/ml/libc-alpha/2016-10/msg00531.html

This would force attackers to have to find entirely new ways to attack
the heap metadata.
 
> The point is that this value (the multiplication of both really) allows
> an adversary to launch any attack in his inventory using a heap sprayed
> binary. That is not a situation you want to prolong.

This is hyperbole. You have to take a common sense approach to closing
these attack vectors and balance them against system usability.

-- 
Cheers,
Carlos.

Re: binfmts.h MAX_ARG_STRINGS excessive value allows heap spraying

[Carlos O'Donell]

On 03/09/2017 09:14 AM, Leonard den Ottolander wrote:
> --- a/include/uapi/linux/binfmts.h	2016-11-23 21:02:31.000000000 +0100
> +++ b/include/uapi/linux/binfmts.h	2017-03-09 01:52:14.716319950 +0100
> @@ -9,10 +9,15 @@ struct pt_regs;
>   * These are the maximum length and maximum number of strings passed to the
>   * execve() system call.  MAX_ARG_STRLEN is essentially random but serves to
>   * prevent the kernel from being unduly impacted by misaddressed pointers.
> - * MAX_ARG_STRINGS is chosen to fit in a signed 32-bit integer.
> + * MAX_ARG_STRINGS * MAX_ARG_STRLEN should be smaller than the 4GiB
> + *  address space on 32 bit to avoid heap spraying.

OK

> + * MAX_ARG_STRSLEN to the rescue - the MAX_ARG_PAGES concept was there
> + *  for a reason.

Why not just use MAX_ARG_PAGES?

Define it in include/linux/binfmts.h for !CONFIG_MMU case.

> + * We can now safely increase STRINGS * STRLEN beyond 4GiB if need be.


>   */
> -#define MAX_ARG_STRLEN (PAGE_SIZE * 32)
> -#define MAX_ARG_STRINGS 0x7FFFFFFF
> +#define MAX_ARG_STRSLEN 262144

You don't need this if you define MAX_ARG_PAGES.

> +#define MAX_ARG_STRLEN 65536
> +#define MAX_ARG_STRINGS 4096

These should be left untouched at their original values.

>  /* sizeof(linux_binprm->buf) */
>  #define BINPRM_BUF_SIZE 128
> --- a/fs/exec.c	2017-02-20 08:04:43.000000000 +0100
> +++ b/fs/exec.c	2017-03-09 01:54:25.931476370 +0100
> @@ -459,6 +459,7 @@ static int copy_strings(int argc, struct
>  	char *kaddr = NULL;
>  	unsigned long kpos = 0;
>  	int ret;
> +	int total_bytes = 0;
>  
>  	while (argc-- > 0) {
>  		const char __user *str;
> @@ -478,6 +479,11 @@ static int copy_strings(int argc, struct
>  		if (!valid_arg_len(bprm, len))
>  			goto out;
>  
> +		/* -E2BIG is fine for now */
> +		total_bytes += len;
> +		if (total_bytes > MAX_ARG_STRSLEN)

Should be PAGE_SIZE * MAX_ARG_PAGES, similar to the !CONFIG_MMU case.

> +			goto out;
> +
>  		/* We're going to work our way backwords. */
>  		pos = bprm->p;
>  		str += len;
> 
> I've successfully built a kernel on a system with a kernel using above
> values.
> 
> As we have now introduced a safeguard (MAX_ARG_STRSLEN capping the total
> amount of memory reserved) the values of MAX_ARG_STRLEN and
> MAX_ARG_STRINGS can be increased beyond where their multiplication
> reaches 2^32.
> 
> So if we really want to support lets say users having directories of
> 128k files we can now safely set MAX_ARG_STRINGS to 131072 and assuming
> an average file name length of 32 set MAX_ARG_STRSLEN to 4194304.
> 
> Seems a little excessive to me for a default, but it is now safe.

We're getting closer to a solution.

There is still no justification for the value of MAX_ARG_PAGES though.

My objection that build systems will break still stands.

The point of memory is to be used.

A reasonable limit might be 1/4 of the virtual address space used for
argument pages though.

-- 
Cheers,
Carlos.

Re: binfmts.h MAX_ARG_STRINGS excessive value allows heap spraying

[Joseph Myers]

On Thu, 9 Mar 2017, Leonard den Ottolander wrote:

> So if we really want to support lets say users having directories of
> 128k files we can now safely set MAX_ARG_STRINGS to 131072 and assuming
> an average file name length of 32 set MAX_ARG_STRSLEN to 4194304.

I think 128k files is too small.  E.g. I have nearly 4 million archived 
emails here, and I'm sure many people have many more than that, and it's 
common to store emails one per file in a large directory (although I'm not 
storing them like that), and I think normal command-line tools ought to be 
able to take all the files in such a directory on the command line at 
once.

I think it's best not to limit the number of arguments beyond the limit 
implied by argc being an int, and no individual object (e.g. the argv 
array) taking up half or more of the address space, along possibly with a 
limit on the proportion of the whole address space taken up by 
(command-line arguments + environment variables + all the pointers to 
those).

-- 
Joseph S. Myers
joseph-qD8j1LwMmJjtCj0u4l0SBw@public.gmane.org

Re: binfmts.h MAX_ARG_STRINGS excessive value allows heap spraying

[Carlos O'Donell]

On 03/09/2017 06:10 PM, Joseph Myers wrote:
> On Thu, 9 Mar 2017, Leonard den Ottolander wrote:
> 
>> So if we really want to support lets say users having directories of
>> 128k files we can now safely set MAX_ARG_STRINGS to 131072 and assuming
>> an average file name length of 32 set MAX_ARG_STRSLEN to 4194304.
> 
> I think 128k files is too small.  E.g. I have nearly 4 million archived 
> emails here, and I'm sure many people have many more than that, and it's 
> common to store emails one per file in a large directory (although I'm not 
> storing them like that), and I think normal command-line tools ought to be 
> able to take all the files in such a directory on the command line at 
> once.
> 
> I think it's best not to limit the number of arguments beyond the limit 
> implied by argc being an int, and no individual object (e.g. the argv 
> array) taking up half or more of the address space, along possibly with a 
> limit on the proportion of the whole address space taken up by 
> (command-line arguments + environment variables + all the pointers to 
> those).
 
Agreed.

-- 
Cheers,
Carlos.

Re: binfmts.h MAX_ARG_STRINGS excessive value allows heap spraying

[Leonard den Ottolander]

On Thu, 2017-03-09 at 15:34 -0500, Carlos O'Donell wrote:
> Why not just use MAX_ARG_PAGES?

Well it is really just a name, but MAX_ARG_PAGES is in use for non MMU.
So to avoid name collisions a different name seems appropriate.

> On 03/09/2017 09:14 AM, Leonard den Ottolander wrote:
> > +#define MAX_ARG_STRLEN 65536
> > +#define MAX_ARG_STRINGS 4096
> 
> These should be left untouched at their original values.

They could actually be removed unless something external relies on them.
But not to disturb too much they can stay around without causing issues.

> > +		if (total_bytes > MAX_ARG_STRSLEN)
> 
> Should be PAGE_SIZE * MAX_ARG_PAGES, similar to the !CONFIG_MMU case.

The use of PAGE_SIZE is arbitrary in this case. There is no reason why
the total amount of memory used for arguments should be a multitude of
PAGE_SIZE. Something like MAX_ARG_BYTES is just fine.

As I mentioned above, reusing MAX_ARG_PAGES might not be a good idea as
it might cause collisions with the non MMU case.

> > I've successfully built a kernel on a system with a kernel using above
> > values.
> > 
> > As we have now introduced a safeguard (MAX_ARG_STRSLEN capping the total
> > amount of memory reserved) the values of MAX_ARG_STRLEN and
> > MAX_ARG_STRINGS can be increased beyond where their multiplication
> > reaches 2^32.
> > 
> > So if we really want to support lets say users having directories of
> > 128k files we can now safely set MAX_ARG_STRINGS to 131072 and assuming
> > an average file name length of 32 set MAX_ARG_STRSLEN to 4194304.
> > 
> > Seems a little excessive to me for a default, but it is now safe.

> There is still no justification for the value of MAX_ARG_PAGES though.

The value for MAX_ARG_PAGES was chosen to allow users to specify large
command lines, for example when building a kernel or other large
project, or handling many files in a directory. 131072 bytes (32 pages)
did the trick in most cases 15 years ago. Only in special circumstances
did that value cause trouble, as you can see in the Linux Journal
article I linked.

A value of 262144 bytes (64 pages) causes no obvious problems on my
systems today.

> My objection that build systems will break still stands.

Well, you can keep repeating that argument without showing a use case
where the values I chose cause trouble. It's hard for me to argue
against conjecture.

As I have pointed out I can build a kernel on CentOS-7 with a kernel
using 262144 for MAX_ARG_STRSLEN.

If you have a heavier use case that you want to support then describe
that use case and tell us how much memory it needs for command line
options. I have come up with my values after quite some experimentation.

> The point of memory is to be used.

Not if it is dangerous for the user. Limits are there not to annoy the
user but to protect the user from the system misbehaving.

I would call heap spraying, an attack vector that allows launching any
exploit via the affected (memory leaking) binary as very serious
misbehaviour.

> A reasonable limit might be 1/4 of the virtual address space used for
> argument pages though.

A whopping gigabyte on 32 bit systems for arguments passed to an
executable. That sounds excessive. A use case of "ls *"-ing 128k files
with 32 byte names adds up to 4 MiB. That is 16 times what you need to
build a kernel. That sounds sufficient don't you think? And again, if
not, show us a use case.

Regards,
Leonard.

-- 
mount -t life -o ro /dev/dna /genetic/research

Re: binfmts.h MAX_ARG_STRINGS excessive value allows heap spraying

[Carlos O'Donell]

On 03/10/2017 07:10 AM, Leonard den Ottolander wrote:
> A whopping gigabyte on 32 bit systems for arguments passed to an
> executable. That sounds excessive. A use case of "ls *"-ing 128k files
> with 32 byte names adds up to 4 MiB. That is 16 times what you need to
> build a kernel. That sounds sufficient don't you think? And again, if
> not, show us a use case.

You have already been given use cases.

* Mail spool directory with millions of files.

* Large and complex build system with potentially long paths.

Artificial limits are always going to cause problems, particularly when
going from a higher limit to a lower limit.

Your present justification is simply that nobody could have use for
lots of arguments, which is not true, and that heap spraying needs to be
stopped, which may be right but may not be the place to start.

We should investigate other ways in which we can tackle the manner in
which the zero day was carried out (like malloc metadata hardening for
the default glibc allocator).

Fundamentally our disagreement is about the risk versus value of the
code in the kernel. Because we disagree about risk and value we will
disagree about the action that needs to be taken.

You have now been given the expert opinion of Joseph Myers and myself,
so take that as you will.

Deployed existing code stands, and the burden of proof is on _you_ to
justify changing the code, not on me to justify all the use cases that
users might have for the current implementation.

Please invite more experts, security or otherwise, to come discuss on
linux-api, the relative merits of _where_ we should be fixing attacks
of this kind.

-- 
Cheers,
Carlos.

Re: binfmts.h MAX_ARG_STRINGS excessive value allows heap spraying

[Leonard den Ottolander]

On Mon, 2017-03-13 at 20:51 -0400, Carlos O'Donell wrote:
> On 03/10/2017 07:10 AM, Leonard den Ottolander wrote:
> > A whopping gigabyte on 32 bit systems for arguments passed to an
> > executable. That sounds excessive. A use case of "ls *"-ing 128k files
> > with 32 byte names adds up to 4 MiB. That is 16 times what you need to
> > build a kernel. That sounds sufficient don't you think? And again, if
> > not, show us a use case.
> 
> You have already been given use cases.
> 
> * Mail spool directory with millions of files.

That is quite an excessive use case to bring up to counter my effort to
get a serious attack vector, heap spraying, fixed. Also not that ls on
such a directory will work fine, it's only when you start to use
wildcards that you run into limits. Easily fixed by someone with only a
basic knowledge of scripting.

> * Large and complex build system with potentially long paths.

I have given you a concrete example: You can build a kernel on CentOS-7
with a kernel built with these values. You still have not shown a
counter example. Perhaps the build of glibc fails with such values?
Firefox? MySQL/MariaDB?

You keep repeating conjectured arguments to counter my concrete example.
Please provide a real case where a total of 256KiB for command line
arguments breaks anything in real life.

> Artificial limits are always going to cause problems, particularly when
> going from a higher limit to a lower limit.

15 years ago a total of 128KiB arguments was fine. I can imagine we need
a multitude of that by now, but for the use case I present 256KiB seems
plenty.

As I have pointed out all systems use artificial limits everywhere, not
to annoy the user, but to protect the users system against (dangerously)
misbehaving.

> Your present justification is simply that nobody could have use for
> lots of arguments, which is not true, and that heap spraying needs to be
> stopped, which may be right but may not be the place to start.

No, I have presented you a concrete use case that I believe to be at the
top end of what one would need for the total amount of argument data.
You have still not presented a serious counter example.

> You have now been given the expert opinion of Joseph Myers and myself,
> so take that as you will.

Who exactly is the judge of that qualification? Are you trying to
disqualify me by using such terms and applying them to yourselves?

> Deployed existing code stands, and the burden of proof is on _you_ to
> justify changing the code, not on me to justify all the use cases that
> users might have for the current implementation.

The example I have given you
https://googleprojectzero.blogspot.nl/2014/08/the-poisoned-nul-byte-2014-edition.html
clearly illustrates the viability of attacking a system with heap
spraying and its potency as an attack vector.

> Please invite more experts, security or otherwise, to come discuss on
> linux-api, the relative merits of _where_ we should be fixing attacks
> of this kind.

Apparently the amount of people chipping in is more important to you
than the value of the arguments. You being an employee of a large
company will always be able to bring out more people to argue your side
of the argument. That in itself does not make your arguments more valid
though.

The change to split that single value MAX_ARG_PAGES into two
(MAX_ARG_STRINGS & MAX_ARG_STRLEN) when MMU was implemented has not been
justified. It now turns out to have been a dangerous decision as the
multiplication of the two quickly brings us in the danger zone where the
entire heap can be initialized by an attacker (malevolent local user, or
someone cracking f.e. apache running on a 32 bit ABI).

Instead of trying to disqualify me and my arguments with cheap rhetoric
you might run some tests with the value(s) I provided so we can
establish a sensible limit for MAX_ARG_STRINGS * MAX_ARG_STRLEN or its
(to be restored) cap "MAX_ARG_BYTES" and fix the dangerous attack venue
that heap spraying is.

Regards,
Leonard.

-- 
mount -t life -o ro /dev/dna /genetic/research

binfmts.h MAX_ARG_STRINGS excessive value allows heap spraying

[Leonard den Ottolander]

In the article 
https://googleprojectzero.blogspot.nl/2014/08/the-poisoned-nul-byte-2014-edition.html
the author describes launching an attack on an off by one NUL byte bug
in glibc.

He explains that both the memory leak in the option parsing of PolKits
pkexec.c and the excessive value of MAX_ARG_STRINGS in the kernels
include/uapi/linux/binfmts.h (#define MAX_ARG_STRINGS 0x7FFFFFFF)
enabled his attack.

The rationale for that excessive value is a bit odd:

"MAX_ARG_STRINGS is chosen to fit in a signed 32-bit integer."

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/include/uapi/linux/binfmts.h?id=607ca46e97a1b6594b29647d98a32d545c24bdff

The fact that the integer is so large makes that on a 32 bit platform
the entire heap can be initialized with values the attacker provides,
given the memory leak in the option parsing of pkexec.c, an approach the
author calls "heap spraying".

(Note that two similar memory leaks exist in the option parsing of
pkexecs sibling pkcheck.c, so pkcheck will also allow an attacker to
heap spray its entire memory on a 32 bit system.)

If you compare http://www.linuxjournal.com/article/6060 you will see
that the TOTAL amount of memory reserved for command line arguments used
to be 32 pages, i.e. 128KiB (#define MAX_ARG_PAGES 32). The amount of
memory reserved for command line arguments in more current kernels is
the multiplication of MAX_ARG_STRINGS and MAX_ARG_STRLEN.

Both the memory leaks in pkexec.c and pkcheck.c seem very severe, but
their impact would be much less if MAX_ARG_STRINGS would hold a sensible
value.

After some experimentation with
$ rpmbuild -ba kernel.spec
on CentOS-7 I've come up with values that allow this kernel compilation.
As this kind of build is probably one of the most demanding actions in
relation to MAX_ARG_STRINGS and MAX_ARG_STRLEN I believe below values to
be sensible and much safer defaults:

#define MAX_ARG_STRLEN 65536
#define MAX_ARG_STRINGS 4096

Please let me know if this needs to be filed as a bug somewhere.

Regards,
Leonard.

-- 
mount -t life -o ro /dev/dna /genetic/research