Re: binfmts.h MAX_ARG_STRINGS excessive value allows heap spraying

[Carlos O'Donell <carlos-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

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.