All of lore.kernel.org
 help / color / mirror / Atom feed
From: Andy Lutomirski <luto@kernel.org>
To: Kees Cook <keescook@chromium.org>,
	Andrew Morton <akpm@linux-foundation.org>
Cc: Ariadne Conill <ariadne@dereferenced.org>,
	Michael Kerrisk <mtk.manpages@gmail.com>,
	Matthew Wilcox <willy@infradead.org>,
	Christian Brauner <brauner@kernel.org>,
	Rich Felker <dalias@libc.org>,
	Eric Biederman <ebiederm@xmission.com>,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	linux-fsdevel@vger.kernel.org, stable@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-hardening@vger.kernel.org,
	Linux API <linux-api@vger.kernel.org>
Subject: Re: [PATCH] exec: Force single empty string when argv is empty
Date: Mon, 31 Jan 2022 18:00:01 -0800	[thread overview]
Message-ID: <234f8cb0-8f9c-0caf-c169-cf9355b33075@kernel.org> (raw)
In-Reply-To: <20220201000947.2453721-1-keescook@chromium.org>

On 1/31/22 16:09, Kees Cook wrote:
> Quoting[1] Ariadne Conill:
> 
> "In several other operating systems, it is a hard requirement that the
> second argument to execve(2) be the name of a program, thus prohibiting
> a scenario where argc < 1. POSIX 2017 also recommends this behaviour,
> but it is not an explicit requirement[2]:
> 
>      The argument arg0 should point to a filename string that is
>      associated with the process being started by one of the exec
>      functions.
> ...
> Interestingly, Michael Kerrisk opened an issue about this in 2008[3],
> but there was no consensus to support fixing this issue then.
> Hopefully now that CVE-2021-4034 shows practical exploitative use[4]
> of this bug in a shellcode, we can reconsider.
> 
> This issue is being tracked in the KSPP issue tracker[5]."
> 
> While the initial code searches[6][7] turned up what appeared to be
> mostly corner case tests, trying to that just reject argv == NULL
> (or an immediately terminated pointer list) quickly started tripping[8]
> existing userspace programs.
> 
> The next best approach is forcing a single empty string into argv and
> adjusting argc to match. The number of programs depending on argc == 0
> seems a smaller set than those calling execve with a NULL argv.
> 
> Account for the additional stack space in bprm_stack_limits(). Inject an
> empty string when argc == 0 (and set argc = 1). Warn about the case so
> userspace has some notice about the change:
> 
>      process './argc0' launched './argc0' with NULL argv: empty string added
> 
> Additionally WARN() and reject NULL argv usage for kernel threads.
> 
> [1] https://lore.kernel.org/lkml/20220127000724.15106-1-ariadne@dereferenced.org/
> [2] https://pubs.opengroup.org/onlinepubs/9699919799/functions/exec.html
> [3] https://bugzilla.kernel.org/show_bug.cgi?id=8408
> [4] https://www.qualys.com/2022/01/25/cve-2021-4034/pwnkit.txt
> [5] https://github.com/KSPP/linux/issues/176
> [6] https://codesearch.debian.net/search?q=execve%5C+*%5C%28%5B%5E%2C%5D%2B%2C+*NULL&literal=0
> [7] https://codesearch.debian.net/search?q=execlp%3F%5Cs*%5C%28%5B%5E%2C%5D%2B%2C%5Cs*NULL&literal=0
> [8] https://lore.kernel.org/lkml/20220131144352.GE16385@xsang-OptiPlex-9020/

Acked-by: Andy Lutomirski <luto@kernel.org>

and cc-ing linux-api.

I agree that this should be done regardless of any security context change.

> 
> Reported-by: Ariadne Conill <ariadne@dereferenced.org>
> Reported-by: Michael Kerrisk <mtk.manpages@gmail.com>
> Cc: Matthew Wilcox <willy@infradead.org>
> Cc: Christian Brauner <brauner@kernel.org>
> Cc: Rich Felker <dalias@libc.org>
> Cc: Eric Biederman <ebiederm@xmission.com>
> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> Cc: linux-fsdevel@vger.kernel.org
> Cc: stable@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>   fs/exec.c | 26 +++++++++++++++++++++++++-
>   1 file changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/exec.c b/fs/exec.c
> index 79f2c9483302..bbf3aadf7ce1 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -495,8 +495,14 @@ static int bprm_stack_limits(struct linux_binprm *bprm)
>   	 * the stack. They aren't stored until much later when we can't
>   	 * signal to the parent that the child has run out of stack space.
>   	 * Instead, calculate it here so it's possible to fail gracefully.
> +	 *
> +	 * In the case of argc = 0, make sure there is space for adding a
> +	 * empty string (which will bump argc to 1), to ensure confused
> +	 * userspace programs don't start processing from argv[1], thinking
> +	 * argc can never be 0, to keep them from walking envp by accident.
> +	 * See do_execveat_common().
>   	 */
> -	ptr_size = (bprm->argc + bprm->envc) * sizeof(void *);
> +	ptr_size = (min(bprm->argc, 1) + bprm->envc) * sizeof(void *);
>   	if (limit <= ptr_size)
>   		return -E2BIG;
>   	limit -= ptr_size;
> @@ -1897,6 +1903,9 @@ static int do_execveat_common(int fd, struct filename *filename,
>   	}
>   
>   	retval = count(argv, MAX_ARG_STRINGS);
> +	if (retval == 0)
> +		pr_warn_once("process '%s' launched '%s' with NULL argv: empty string added\n",
> +			     current->comm, bprm->filename);
>   	if (retval < 0)
>   		goto out_free;
>   	bprm->argc = retval;
> @@ -1923,6 +1932,19 @@ static int do_execveat_common(int fd, struct filename *filename,
>   	if (retval < 0)
>   		goto out_free;
>   
> +	/*
> +	 * When argv is empty, add an empty string ("") as argv[0] to
> +	 * ensure confused userspace programs that start processing
> +	 * from argv[1] won't end up walking envp. See also
> +	 * bprm_stack_limits().
> +	 */
> +	if (bprm->argc == 0) {
> +		retval = copy_string_kernel("", bprm);
> +		if (retval < 0)
> +			goto out_free;
> +		bprm->argc = 1;
> +	}
> +
>   	retval = bprm_execve(bprm, fd, filename, flags);
>   out_free:
>   	free_bprm(bprm);
> @@ -1951,6 +1973,8 @@ int kernel_execve(const char *kernel_filename,
>   	}
>   
>   	retval = count_strings_kernel(argv);
> +	if (WARN_ON_ONCE(retval == 0))
> +		retval = -EINVAL;
>   	if (retval < 0)
>   		goto out_free;
>   	bprm->argc = retval;


  parent reply	other threads:[~2022-02-01  2:01 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-02-01  0:09 [PATCH] exec: Force single empty string when argv is empty Kees Cook
2022-02-01  1:00 ` Ariadne Conill
2022-02-01  2:00 ` Andy Lutomirski [this message]
2022-02-01  9:17 ` David Laight
2022-02-02 20:31   ` Kees Cook
2022-02-01 13:22 ` Christian Brauner
2022-02-01 14:53 ` Rich Felker
2022-02-02 15:50   ` Kees Cook
2022-02-02 17:12     ` Rich Felker
2022-02-02 19:03       ` Kees Cook

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=234f8cb0-8f9c-0caf-c169-cf9355b33075@kernel.org \
    --to=luto@kernel.org \
    --cc=akpm@linux-foundation.org \
    --cc=ariadne@dereferenced.org \
    --cc=brauner@kernel.org \
    --cc=dalias@libc.org \
    --cc=ebiederm@xmission.com \
    --cc=keescook@chromium.org \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-hardening@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mtk.manpages@gmail.com \
    --cc=stable@vger.kernel.org \
    --cc=viro@zeniv.linux.org.uk \
    --cc=willy@infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.