diff mbox series

[v2] linux-user: fix is_proc_myself to check the paths via realpath

Message ID 20171025033442.44872-1-zachriggle@gmail.com
State New
Headers show
Series [v2] linux-user: fix is_proc_myself to check the paths via realpath | expand

Commit Message

Zach Riggle Oct. 25, 2017, 3:34 a.m. UTC 
Previously, it was possible to get a handle to the "real" /proc/self/mem
by creating a symlink to it and opening the symlink, or opening e.g.
"./mem" after chdir'ing to "/proc/self".

    $ ln -s /proc/self self
    $ cat self/maps
    60000000-602bc000 r-xp 00000000 fc:01 270375                             /usr/bin/qemu-arm-static
    604bc000-6050f000 rw-p 002bc000 fc:01 270375                             /usr/bin/qemu-arm-static
    ...

Signed-off-by: Zach Riggle <zachriggle@gmail.com>
---
 linux-user/syscall.c | 47 ++++++++++++++++++++++++++++-------------------
 1 file changed, 28 insertions(+), 19 deletions(-)

--
2.14.3

Comments

Zach Riggle Oct. 26, 2017, 9:06 p.m. UTC | #1 
Friendly ping :)

I've updated the patch with v2 which addresses the style issue


*Zach Riggle*

On Tue, Oct 24, 2017 at 10:34 PM, Zach Riggle <zachriggle@gmail.com> wrote:

> Previously, it was possible to get a handle to the "real" /proc/self/mem
> by creating a symlink to it and opening the symlink, or opening e.g.
> "./mem" after chdir'ing to "/proc/self".
>
>     $ ln -s /proc/self self
>     $ cat self/maps
>     60000000-602bc000 r-xp 00000000 fc:01 270375
>    /usr/bin/qemu-arm-static
>     604bc000-6050f000 rw-p 002bc000 fc:01 270375
>    /usr/bin/qemu-arm-static
>     ...
>
> Signed-off-by: Zach Riggle <zachriggle@gmail.com>
> ---
>  linux-user/syscall.c | 47 ++++++++++++++++++++++++++++-------------------
>  1 file changed, 28 insertions(+), 19 deletions(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 9bf901fa11..6c1f28a1f7 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -7496,26 +7496,35 @@ static int open_self_auxv(void *cpu_env, int fd)
>
>  static int is_proc_myself(const char *filename, const char *entry)
>  {
> -    if (!strncmp(filename, "/proc/", strlen("/proc/"))) {
> -        filename += strlen("/proc/");
> -        if (!strncmp(filename, "self/", strlen("self/"))) {
> -            filename += strlen("self/");
> -        } else if (*filename >= '1' && *filename <= '9') {
> -            char myself[80];
> -            snprintf(myself, sizeof(myself), "%d/", getpid());
> -            if (!strncmp(filename, myself, strlen(myself))) {
> -                filename += strlen(myself);
> -            } else {
> -                return 0;
> -            }
> -        } else {
> -            return 0;
> -        }
> -        if (!strcmp(filename, entry)) {
> -            return 1;
> -        }
> +    char proc_self_entry[PATH_MAX + 1];
> +    char proc_self_entry_realpath[PATH_MAX + 1];
> +    char filename_realpath[PATH_MAX + 1];
> +
> +    if (PATH_MAX < snprintf(proc_self_entry,
> +                            sizeof(proc_self_entry),
> +                            "/proc/self/%s",
> +                            entry)) {
> +        /* Full path to "entry" is too long to fit in the buffer */
> +        return 0;
>      }
> -    return 0;
> +
> +    if (!realpath(filename, filename_realpath)) {
> +        /* File does not exist, or can't be canonicalized */
> +        return 0;
> +    }
> +
> +    if (!realpath(proc_self_entry, proc_self_entry_realpath)) {
> +        /* Procfs entry does not exist */
> +        return 0;
> +    }
> +
> +    if (strcmp(filename_realpath, proc_self_entry_realpath) != 0) {
> +        /* Paths are different */
> +        return 0;
> +    }
> +
> +    /* filename refers to /proc/self/<entry> */
> +    return 1;
>  }
>
>  #if defined(HOST_WORDS_BIGENDIAN) != defined(TARGET_WORDS_BIGENDIAN)
> --
> 2.14.3
>
>
Riku Voipio Oct. 27, 2017, 9:36 a.m. UTC | #2 
On Thu, Oct 26, 2017 at 04:06:22PM -0500, Zach Riggle wrote:
> Friendly ping :)
> 
> I've updated the patch with v2 which addresses the style issue

I'll have a look at it soon.
 
> 
> *Zach Riggle*
> 
> On Tue, Oct 24, 2017 at 10:34 PM, Zach Riggle <zachriggle@gmail.com> wrote:
> 
> > Previously, it was possible to get a handle to the "real" /proc/self/mem
> > by creating a symlink to it and opening the symlink, or opening e.g.
> > "./mem" after chdir'ing to "/proc/self"

When is this a problem? Symlinking to /proc/self seems to be a quite weird usecase.

> >
> >     $ ln -s /proc/self self
> >     $ cat self/maps
> >     60000000-602bc000 r-xp 00000000 fc:01 270375
> >    /usr/bin/qemu-arm-static
> >     604bc000-6050f000 rw-p 002bc000 fc:01 270375
> >    /usr/bin/qemu-arm-static
> >     ...
> >
> > Signed-off-by: Zach Riggle <zachriggle@gmail.com>
> > ---
> >  linux-user/syscall.c | 47 ++++++++++++++++++++++++++++-------------------
> >  1 file changed, 28 insertions(+), 19 deletions(-)
> >
> > diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> > index 9bf901fa11..6c1f28a1f7 100644
> > --- a/linux-user/syscall.c
> > +++ b/linux-user/syscall.c
> > @@ -7496,26 +7496,35 @@ static int open_self_auxv(void *cpu_env, int fd)
> >
> >  static int is_proc_myself(const char *filename, const char *entry)
> >  {
> > -    if (!strncmp(filename, "/proc/", strlen("/proc/"))) {
> > -        filename += strlen("/proc/");
> > -        if (!strncmp(filename, "self/", strlen("self/"))) {
> > -            filename += strlen("self/");
> > -        } else if (*filename >= '1' && *filename <= '9') {
> > -            char myself[80];
> > -            snprintf(myself, sizeof(myself), "%d/", getpid());
> > -            if (!strncmp(filename, myself, strlen(myself))) {
> > -                filename += strlen(myself);
> > -            } else {
> > -                return 0;
> > -            }
> > -        } else {
> > -            return 0;
> > -        }
> > -        if (!strcmp(filename, entry)) {
> > -            return 1;
> > -        }
> > +    char proc_self_entry[PATH_MAX + 1];
> > +    char proc_self_entry_realpath[PATH_MAX + 1];
> > +    char filename_realpath[PATH_MAX + 1];
> > +
> > +    if (PATH_MAX < snprintf(proc_self_entry,
> > +                            sizeof(proc_self_entry),
> > +                            "/proc/self/%s",
> > +                            entry)) {
> > +        /* Full path to "entry" is too long to fit in the buffer */
> > +        return 0;
> >      }
> > -    return 0;
> > +
> > +    if (!realpath(filename, filename_realpath)) {
> > +        /* File does not exist, or can't be canonicalized */
> > +        return 0;
> > +    }
> > +
> > +    if (!realpath(proc_self_entry, proc_self_entry_realpath)) {
> > +        /* Procfs entry does not exist */
> > +        return 0;
> > +    }
> > +
> > +    if (strcmp(filename_realpath, proc_self_entry_realpath) != 0) {
> > +        /* Paths are different */
> > +        return 0;
> > +    }
> > +
> > +    /* filename refers to /proc/self/<entry> */
> > +    return 1;
> >  }
> >
> >  #if defined(HOST_WORDS_BIGENDIAN) != defined(TARGET_WORDS_BIGENDIAN)
> > --
> > 2.14.3
> >
> >
Zach Riggle Oct. 27, 2017, 5:05 p.m. UTC | #3 
The symlink was just an easy test case.  Doing cd proc && cat ./self/maps
will achieve the same thing.

One instance where this matters is for testing IoT device firmware
exploits, where one might do a GET request against
../../../../../../proc/self/maps in order to bypass ASLR.  Currently, this
will return the memory layout of QEMU, not the emulated memory layout of
the software under test.


*Zach Riggle*

On Fri, Oct 27, 2017 at 4:36 AM, Riku Voipio <riku.voipio@iki.fi> wrote:

> On Thu, Oct 26, 2017 at 04:06:22PM -0500, Zach Riggle wrote:
> > Friendly ping :)
> >
> > I've updated the patch with v2 which addresses the style issue
>
> I'll have a look at it soon.
>
> >
> > *Zach Riggle*
> >
> > On Tue, Oct 24, 2017 at 10:34 PM, Zach Riggle <zachriggle@gmail.com>
> wrote:
> >
> > > Previously, it was possible to get a handle to the "real"
> /proc/self/mem
> > > by creating a symlink to it and opening the symlink, or opening e.g.
> > > "./mem" after chdir'ing to "/proc/self"
>
> When is this a problem? Symlinking to /proc/self seems to be a quite weird
> usecase.
>
> > >
> > >     $ ln -s /proc/self self
> > >     $ cat self/maps
> > >     60000000-602bc000 r-xp 00000000 fc:01 270375
> > >    /usr/bin/qemu-arm-static
> > >     604bc000-6050f000 rw-p 002bc000 fc:01 270375
> > >    /usr/bin/qemu-arm-static
> > >     ...
> > >
> > > Signed-off-by: Zach Riggle <zachriggle@gmail.com>
> > > ---
> > >  linux-user/syscall.c | 47 ++++++++++++++++++++++++++++--
> -----------------
> > >  1 file changed, 28 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> > > index 9bf901fa11..6c1f28a1f7 100644
> > > --- a/linux-user/syscall.c
> > > +++ b/linux-user/syscall.c
> > > @@ -7496,26 +7496,35 @@ static int open_self_auxv(void *cpu_env, int
> fd)
> > >
> > >  static int is_proc_myself(const char *filename, const char *entry)
> > >  {
> > > -    if (!strncmp(filename, "/proc/", strlen("/proc/"))) {
> > > -        filename += strlen("/proc/");
> > > -        if (!strncmp(filename, "self/", strlen("self/"))) {
> > > -            filename += strlen("self/");
> > > -        } else if (*filename >= '1' && *filename <= '9') {
> > > -            char myself[80];
> > > -            snprintf(myself, sizeof(myself), "%d/", getpid());
> > > -            if (!strncmp(filename, myself, strlen(myself))) {
> > > -                filename += strlen(myself);
> > > -            } else {
> > > -                return 0;
> > > -            }
> > > -        } else {
> > > -            return 0;
> > > -        }
> > > -        if (!strcmp(filename, entry)) {
> > > -            return 1;
> > > -        }
> > > +    char proc_self_entry[PATH_MAX + 1];
> > > +    char proc_self_entry_realpath[PATH_MAX + 1];
> > > +    char filename_realpath[PATH_MAX + 1];
> > > +
> > > +    if (PATH_MAX < snprintf(proc_self_entry,
> > > +                            sizeof(proc_self_entry),
> > > +                            "/proc/self/%s",
> > > +                            entry)) {
> > > +        /* Full path to "entry" is too long to fit in the buffer */
> > > +        return 0;
> > >      }
> > > -    return 0;
> > > +
> > > +    if (!realpath(filename, filename_realpath)) {
> > > +        /* File does not exist, or can't be canonicalized */
> > > +        return 0;
> > > +    }
> > > +
> > > +    if (!realpath(proc_self_entry, proc_self_entry_realpath)) {
> > > +        /* Procfs entry does not exist */
> > > +        return 0;
> > > +    }
> > > +
> > > +    if (strcmp(filename_realpath, proc_self_entry_realpath) != 0) {
> > > +        /* Paths are different */
> > > +        return 0;
> > > +    }
> > > +
> > > +    /* filename refers to /proc/self/<entry> */
> > > +    return 1;
> > >  }
> > >
> > >  #if defined(HOST_WORDS_BIGENDIAN) != defined(TARGET_WORDS_BIGENDIAN)
> > > --
> > > 2.14.3
> > >
> > >
>
Zach Riggle Oct. 27, 2017, 7:07 p.m. UTC | #4 
Another case that may be more relevant for general QEMU use, is that the
current code fails if the software under test has poor path-handling code.
For example, any of

- //proc/self/maps
- /proc//self/maps
- /proc/self//maps

Will all return the non-emulated results.  Those examples are just path
canonicalization issues and could be resolved with e.g.
canonicalize_file_name, but I'm not sure if QEMU allows GNU extensions --
and realpath() is portable.




*Zach Riggle*

On Fri, Oct 27, 2017 at 12:05 PM, Zach Riggle <zachriggle@gmail.com> wrote:

> The symlink was just an easy test case.  Doing cd proc && cat ./self/maps
> will achieve the same thing.
>
> One instance where this matters is for testing IoT device firmware
> exploits, where one might do a GET request against
> ../../../../../../proc/self/maps in order to bypass ASLR.  Currently,
> this will return the memory layout of QEMU, not the emulated memory layout
> of the software under test.
>
>
> *Zach Riggle*
>
> On Fri, Oct 27, 2017 at 4:36 AM, Riku Voipio <riku.voipio@iki.fi> wrote:
>
>> On Thu, Oct 26, 2017 at 04:06:22PM -0500, Zach Riggle wrote:
>> > Friendly ping :)
>> >
>> > I've updated the patch with v2 which addresses the style issue
>>
>> I'll have a look at it soon.
>>
>> >
>> > *Zach Riggle*
>> >
>> > On Tue, Oct 24, 2017 at 10:34 PM, Zach Riggle <zachriggle@gmail.com>
>> wrote:
>> >
>> > > Previously, it was possible to get a handle to the "real"
>> /proc/self/mem
>> > > by creating a symlink to it and opening the symlink, or opening e.g.
>> > > "./mem" after chdir'ing to "/proc/self"
>>
>> When is this a problem? Symlinking to /proc/self seems to be a quite
>> weird usecase.
>>
>> > >
>> > >     $ ln -s /proc/self self
>> > >     $ cat self/maps
>> > >     60000000-602bc000 r-xp 00000000 fc:01 270375
>> > >    /usr/bin/qemu-arm-static
>> > >     604bc000-6050f000 rw-p 002bc000 fc:01 270375
>> > >    /usr/bin/qemu-arm-static
>> > >     ...
>> > >
>> > > Signed-off-by: Zach Riggle <zachriggle@gmail.com>
>> > > ---
>> > >  linux-user/syscall.c | 47 ++++++++++++++++++++++++++++--
>> -----------------
>> > >  1 file changed, 28 insertions(+), 19 deletions(-)
>> > >
>> > > diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>> > > index 9bf901fa11..6c1f28a1f7 100644
>> > > --- a/linux-user/syscall.c
>> > > +++ b/linux-user/syscall.c
>> > > @@ -7496,26 +7496,35 @@ static int open_self_auxv(void *cpu_env, int
>> fd)
>> > >
>> > >  static int is_proc_myself(const char *filename, const char *entry)
>> > >  {
>> > > -    if (!strncmp(filename, "/proc/", strlen("/proc/"))) {
>> > > -        filename += strlen("/proc/");
>> > > -        if (!strncmp(filename, "self/", strlen("self/"))) {
>> > > -            filename += strlen("self/");
>> > > -        } else if (*filename >= '1' && *filename <= '9') {
>> > > -            char myself[80];
>> > > -            snprintf(myself, sizeof(myself), "%d/", getpid());
>> > > -            if (!strncmp(filename, myself, strlen(myself))) {
>> > > -                filename += strlen(myself);
>> > > -            } else {
>> > > -                return 0;
>> > > -            }
>> > > -        } else {
>> > > -            return 0;
>> > > -        }
>> > > -        if (!strcmp(filename, entry)) {
>> > > -            return 1;
>> > > -        }
>> > > +    char proc_self_entry[PATH_MAX + 1];
>> > > +    char proc_self_entry_realpath[PATH_MAX + 1];
>> > > +    char filename_realpath[PATH_MAX + 1];
>> > > +
>> > > +    if (PATH_MAX < snprintf(proc_self_entry,
>> > > +                            sizeof(proc_self_entry),
>> > > +                            "/proc/self/%s",
>> > > +                            entry)) {
>> > > +        /* Full path to "entry" is too long to fit in the buffer */
>> > > +        return 0;
>> > >      }
>> > > -    return 0;
>> > > +
>> > > +    if (!realpath(filename, filename_realpath)) {
>> > > +        /* File does not exist, or can't be canonicalized */
>> > > +        return 0;
>> > > +    }
>> > > +
>> > > +    if (!realpath(proc_self_entry, proc_self_entry_realpath)) {
>> > > +        /* Procfs entry does not exist */
>> > > +        return 0;
>> > > +    }
>> > > +
>> > > +    if (strcmp(filename_realpath, proc_self_entry_realpath) != 0) {
>> > > +        /* Paths are different */
>> > > +        return 0;
>> > > +    }
>> > > +
>> > > +    /* filename refers to /proc/self/<entry> */
>> > > +    return 1;
>> > >  }
>> > >
>> > >  #if defined(HOST_WORDS_BIGENDIAN) != defined(TARGET_WORDS_BIGENDIAN)
>> > > --
>> > > 2.14.3
>> > >
>> > >
>>
>
>
Eric Blake Oct. 28, 2017, 5:14 a.m. UTC | #5 
On 10/27/2017 09:07 PM, Zach Riggle wrote:
> Another case that may be more relevant for general QEMU use, is that the
> current code fails if the software under test has poor path-handling code.
> For example, any of
> 
> - //proc/self/maps
> - /proc//self/maps
> - /proc/self//maps
> 
> Will all return the non-emulated results.  Those examples are just path
> canonicalization issues and could be resolved with e.g.
> canonicalize_file_name, but I'm not sure if QEMU allows GNU extensions --
> and realpath() is portable.

By definition, in linux-user, we ARE using glibc; therefore, you are
free to use all GNU extensions.

And you'd be surprised at how many non-glibc implementations of
realpath() are not POSIX-compliant, even though that is not relevant here.
Zach Riggle Nov. 2, 2017, 6:26 p.m. UTC | #6 
Ping. What changes do I need to make to land this?
On Sat, Oct 28, 2017 at 12:49 AM Eric Blake <eblake@redhat.com> wrote:

> On 10/27/2017 09:07 PM, Zach Riggle wrote:
> > Another case that may be more relevant for general QEMU use, is that the
> > current code fails if the software under test has poor path-handling
> code.
> > For example, any of
> >
> > - //proc/self/maps
> > - /proc//self/maps
> > - /proc/self//maps
> >
> > Will all return the non-emulated results.  Those examples are just path
> > canonicalization issues and could be resolved with e.g.
> > canonicalize_file_name, but I'm not sure if QEMU allows GNU extensions --
> > and realpath() is portable.
>
> By definition, in linux-user, we ARE using glibc; therefore, you are
> free to use all GNU extensions.
>
> And you'd be surprised at how many non-glibc implementations of
> realpath() are not POSIX-compliant, even though that is not relevant here.
>
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org
>
>
Peter Maydell Nov. 2, 2017, 7:35 p.m. UTC | #7 
On 28 October 2017 at 06:14, Eric Blake <eblake@redhat.com> wrote:
> By definition, in linux-user, we ARE using glibc; therefore, you are
> free to use all GNU extensions.

Don't we also support musl libc? I forget...

thanks
-- PMM
Zach Riggle Nov. 6, 2017, 8:17 p.m. UTC | #8 
Ping! What needs to be done to move this forward? My current implementation
is compatible with musl.
On Thu, Nov 2, 2017 at 12:36 PM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On 28 October 2017 at 06:14, Eric Blake <eblake@redhat.com> wrote:
> > By definition, in linux-user, we ARE using glibc; therefore, you are
> > free to use all GNU extensions.
>
> Don't we also support musl libc? I forget...
>
> thanks
> -- PMM
>
Riku Voipio Nov. 7, 2017, 8:06 p.m. UTC | #9 
Hi,

On Mon, Nov 06, 2017 at 08:17:44PM +0000, Zach Riggle wrote:
> Ping! What needs to be done to move this forward? My current implementation
> is compatible with musl.

I'll have a look at it soon.

Riku

> On Thu, Nov 2, 2017 at 12:36 PM Peter Maydell <peter.maydell@linaro.org>
> wrote:
> 
> > On 28 October 2017 at 06:14, Eric Blake <eblake@redhat.com> wrote:
> > > By definition, in linux-user, we ARE using glibc; therefore, you are
> > > free to use all GNU extensions.
> >
> > Don't we also support musl libc? I forget...
> >
> > thanks
> > -- PMM
> >
Zach Riggle Nov. 10, 2017, 9:44 p.m. UTC | #10 
Day 17 Ping :)



*Zach Riggle*

On Tue, Nov 7, 2017 at 2:06 PM, Riku Voipio <riku.voipio@iki.fi> wrote:

> Hi,
>
> On Mon, Nov 06, 2017 at 08:17:44PM +0000, Zach Riggle wrote:
> > Ping! What needs to be done to move this forward? My current
> implementation
> > is compatible with musl.
>
> I'll have a look at it soon.
>
> Riku
>
> > On Thu, Nov 2, 2017 at 12:36 PM Peter Maydell <peter.maydell@linaro.org>
> > wrote:
> >
> > > On 28 October 2017 at 06:14, Eric Blake <eblake@redhat.com> wrote:
> > > > By definition, in linux-user, we ARE using glibc; therefore, you are
> > > > free to use all GNU extensions.
> > >
> > > Don't we also support musl libc? I forget...
> > >
> > > thanks
> > > -- PMM
> > >
>
Laurent Vivier Nov. 10, 2017, 11:44 p.m. UTC | #11 
Le 25/10/2017 à 05:34, Zach Riggle a écrit :
> Previously, it was possible to get a handle to the "real" /proc/self/mem
> by creating a symlink to it and opening the symlink, or opening e.g.
> "./mem" after chdir'ing to "/proc/self".
> 
>     $ ln -s /proc/self self
>     $ cat self/maps
>     60000000-602bc000 r-xp 00000000 fc:01 270375                             /usr/bin/qemu-arm-static
>     604bc000-6050f000 rw-p 002bc000 fc:01 270375                             /usr/bin/qemu-arm-static
>     ...
> 
> Signed-off-by: Zach Riggle <zachriggle@gmail.com>
> ---
>  linux-user/syscall.c | 47 ++++++++++++++++++++++++++++-------------------
>  1 file changed, 28 insertions(+), 19 deletions(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 9bf901fa11..6c1f28a1f7 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -7496,26 +7496,35 @@ static int open_self_auxv(void *cpu_env, int fd)
> 
>  static int is_proc_myself(const char *filename, const char *entry)
>  {
> -    if (!strncmp(filename, "/proc/", strlen("/proc/"))) {
> -        filename += strlen("/proc/");
> -        if (!strncmp(filename, "self/", strlen("self/"))) {
> -            filename += strlen("self/");
> -        } else if (*filename >= '1' && *filename <= '9') {
> -            char myself[80];
> -            snprintf(myself, sizeof(myself), "%d/", getpid());
> -            if (!strncmp(filename, myself, strlen(myself))) {
> -                filename += strlen(myself);
> -            } else {
> -                return 0;
> -            }
> -        } else {
> -            return 0;
> -        }
> -        if (!strcmp(filename, entry)) {
> -            return 1;
> -        }
> +    char proc_self_entry[PATH_MAX + 1];
> +    char proc_self_entry_realpath[PATH_MAX + 1];
> +    char filename_realpath[PATH_MAX + 1];
> +
> +    if (PATH_MAX < snprintf(proc_self_entry,
> +                            sizeof(proc_self_entry),
> +                            "/proc/self/%s",
> +                            entry)) {
> +        /* Full path to "entry" is too long to fit in the buffer */
> +        return 0;
>      }
> -    return 0;
> +
> +    if (!realpath(filename, filename_realpath)) {
> +        /* File does not exist, or can't be canonicalized */
> +        return 0;
> +    }
> +
> +    if (!realpath(proc_self_entry, proc_self_entry_realpath)) {
> +        /* Procfs entry does not exist */
> +        return 0;
> +    }
> +
> +    if (strcmp(filename_realpath, proc_self_entry_realpath) != 0) {
> +        /* Paths are different */

I think it doesn't work with /proc/map/exe (or other soft link in
/proc/self) as realpath will give the path of the executable and not the
path inside /proc so it will be true for any process with the same
executable (which in our case is qemu for all).

Thanks,
Laurent
Zach Riggle Nov. 11, 2017, 1:47 a.m. UTC | #12 
Good catch.  Relying on realpath() for *exe* does cause issues.

A better general solution (which handles the "exe" case) is to use open(2)
with O_PATH | O_NOFOLLOW for the candidate path (e.g. /proc/self/exe) and
to do the same for the path we're testing along with readlink().

If, in the process of link resolution via the readlink() loop, we end up
with the same path as our candidate, we can return true.  This avoids the
need to rely on any libc implementation of realpath(), since we're just
relying on the host kernel.


*Zach Riggle*

On Fri, Nov 10, 2017 at 5:44 PM, Laurent Vivier <laurent@vivier.eu> wrote:

> Le 25/10/2017 à 05:34, Zach Riggle a écrit :
> > Previously, it was possible to get a handle to the "real" /proc/self/mem
> > by creating a symlink to it and opening the symlink, or opening e.g.
> > "./mem" after chdir'ing to "/proc/self".
> >
> >     $ ln -s /proc/self self
> >     $ cat self/maps
> >     60000000-602bc000 r-xp 00000000 fc:01 270375
>      /usr/bin/qemu-arm-static
> >     604bc000-6050f000 rw-p 002bc000 fc:01 270375
>      /usr/bin/qemu-arm-static
> >     ...
> >
> > Signed-off-by: Zach Riggle <zachriggle@gmail.com>
> > ---
> >  linux-user/syscall.c | 47 ++++++++++++++++++++++++++++--
> -----------------
> >  1 file changed, 28 insertions(+), 19 deletions(-)
> >
> > diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> > index 9bf901fa11..6c1f28a1f7 100644
> > --- a/linux-user/syscall.c
> > +++ b/linux-user/syscall.c
> > @@ -7496,26 +7496,35 @@ static int open_self_auxv(void *cpu_env, int fd)
> >
> >  static int is_proc_myself(const char *filename, const char *entry)
> >  {
> > -    if (!strncmp(filename, "/proc/", strlen("/proc/"))) {
> > -        filename += strlen("/proc/");
> > -        if (!strncmp(filename, "self/", strlen("self/"))) {
> > -            filename += strlen("self/");
> > -        } else if (*filename >= '1' && *filename <= '9') {
> > -            char myself[80];
> > -            snprintf(myself, sizeof(myself), "%d/", getpid());
> > -            if (!strncmp(filename, myself, strlen(myself))) {
> > -                filename += strlen(myself);
> > -            } else {
> > -                return 0;
> > -            }
> > -        } else {
> > -            return 0;
> > -        }
> > -        if (!strcmp(filename, entry)) {
> > -            return 1;
> > -        }
> > +    char proc_self_entry[PATH_MAX + 1];
> > +    char proc_self_entry_realpath[PATH_MAX + 1];
> > +    char filename_realpath[PATH_MAX + 1];
> > +
> > +    if (PATH_MAX < snprintf(proc_self_entry,
> > +                            sizeof(proc_self_entry),
> > +                            "/proc/self/%s",
> > +                            entry)) {
> > +        /* Full path to "entry" is too long to fit in the buffer */
> > +        return 0;
> >      }
> > -    return 0;
> > +
> > +    if (!realpath(filename, filename_realpath)) {
> > +        /* File does not exist, or can't be canonicalized */
> > +        return 0;
> > +    }
> > +
> > +    if (!realpath(proc_self_entry, proc_self_entry_realpath)) {
> > +        /* Procfs entry does not exist */
> > +        return 0;
> > +    }
> > +
> > +    if (strcmp(filename_realpath, proc_self_entry_realpath) != 0) {
> > +        /* Paths are different */
>
> I think it doesn't work with /proc/map/exe (or other soft link in
> /proc/self) as realpath will give the path of the executable and not the
> path inside /proc so it will be true for any process with the same
> executable (which in our case is qemu for all).
>
> Thanks,
> Laurent
>
>
Zach Riggle Nov. 11, 2017, 1:48 a.m. UTC | #13 
I wrote up a quick example to show that this should work specifically for
/proc/self/exe:

#define _GNU_SOURCE
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <unistd.h>
int main(int argc, char** argv) {
    int fd = open("/proc/self/exe", O_NOFOLLOW | O_PATH);
    system("ls -la /proc/$PPID/fd/");
}


*Zach Riggle*

On Fri, Nov 10, 2017 at 7:47 PM, Zach Riggle <zachriggle@gmail.com> wrote:

> Good catch.  Relying on realpath() for *exe* does cause issues.
>
> A better general solution (which handles the "exe" case) is to use open(2)
> with O_PATH | O_NOFOLLOW for the candidate path (e.g. /proc/self/exe) and
> to do the same for the path we're testing along with readlink().
>
> If, in the process of link resolution via the readlink() loop, we end up
> with the same path as our candidate, we can return true.  This avoids the
> need to rely on any libc implementation of realpath(), since we're just
> relying on the host kernel.
>
>
> *Zach Riggle*
>
> On Fri, Nov 10, 2017 at 5:44 PM, Laurent Vivier <laurent@vivier.eu> wrote:
>
>> Le 25/10/2017 à 05:34, Zach Riggle a écrit :
>> > Previously, it was possible to get a handle to the "real" /proc/self/mem
>> > by creating a symlink to it and opening the symlink, or opening e.g.
>> > "./mem" after chdir'ing to "/proc/self".
>> >
>> >     $ ln -s /proc/self self
>> >     $ cat self/maps
>> >     60000000-602bc000 r-xp 00000000 fc:01 270375
>>      /usr/bin/qemu-arm-static
>> >     604bc000-6050f000 rw-p 002bc000 fc:01 270375
>>      /usr/bin/qemu-arm-static
>> >     ...
>> >
>> > Signed-off-by: Zach Riggle <zachriggle@gmail.com>
>> > ---
>> >  linux-user/syscall.c | 47 ++++++++++++++++++++++++++++--
>> -----------------
>> >  1 file changed, 28 insertions(+), 19 deletions(-)
>> >
>> > diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>> > index 9bf901fa11..6c1f28a1f7 100644
>> > --- a/linux-user/syscall.c
>> > +++ b/linux-user/syscall.c
>> > @@ -7496,26 +7496,35 @@ static int open_self_auxv(void *cpu_env, int fd)
>> >
>> >  static int is_proc_myself(const char *filename, const char *entry)
>> >  {
>> > -    if (!strncmp(filename, "/proc/", strlen("/proc/"))) {
>> > -        filename += strlen("/proc/");
>> > -        if (!strncmp(filename, "self/", strlen("self/"))) {
>> > -            filename += strlen("self/");
>> > -        } else if (*filename >= '1' && *filename <= '9') {
>> > -            char myself[80];
>> > -            snprintf(myself, sizeof(myself), "%d/", getpid());
>> > -            if (!strncmp(filename, myself, strlen(myself))) {
>> > -                filename += strlen(myself);
>> > -            } else {
>> > -                return 0;
>> > -            }
>> > -        } else {
>> > -            return 0;
>> > -        }
>> > -        if (!strcmp(filename, entry)) {
>> > -            return 1;
>> > -        }
>> > +    char proc_self_entry[PATH_MAX + 1];
>> > +    char proc_self_entry_realpath[PATH_MAX + 1];
>> > +    char filename_realpath[PATH_MAX + 1];
>> > +
>> > +    if (PATH_MAX < snprintf(proc_self_entry,
>> > +                            sizeof(proc_self_entry),
>> > +                            "/proc/self/%s",
>> > +                            entry)) {
>> > +        /* Full path to "entry" is too long to fit in the buffer */
>> > +        return 0;
>> >      }
>> > -    return 0;
>> > +
>> > +    if (!realpath(filename, filename_realpath)) {
>> > +        /* File does not exist, or can't be canonicalized */
>> > +        return 0;
>> > +    }
>> > +
>> > +    if (!realpath(proc_self_entry, proc_self_entry_realpath)) {
>> > +        /* Procfs entry does not exist */
>> > +        return 0;
>> > +    }
>> > +
>> > +    if (strcmp(filename_realpath, proc_self_entry_realpath) != 0) {
>> > +        /* Paths are different */
>>
>> I think it doesn't work with /proc/map/exe (or other soft link in
>> /proc/self) as realpath will give the path of the executable and not the
>> path inside /proc so it will be true for any process with the same
>> executable (which in our case is qemu for all).
>>
>> Thanks,
>> Laurent
>>
>>
>
Laurent Vivier Nov. 14, 2017, 7:44 p.m. UTC | #14 
Le 11/11/2017 à 02:48, Zach Riggle a écrit :
> I wrote up a quick example to show that this should work specifically for
> /proc/self/exe:
> 
> #define _GNU_SOURCE
> #include <sys/types.h>
> #include <sys/stat.h>
> #include <fcntl.h>
> #include <unistd.h>
> int main(int argc, char** argv) {
>     int fd = open("/proc/self/exe", O_NOFOLLOW | O_PATH);
>     system("ls -la /proc/$PPID/fd/");
> }
> 

And what about a readlink() in a loop until we cross "/proc/<pid>" (or not)?

Thanks,
Laurent
diff mbox series

Patch

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 9bf901fa11..6c1f28a1f7 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -7496,26 +7496,35 @@  static int open_self_auxv(void *cpu_env, int fd)

 static int is_proc_myself(const char *filename, const char *entry)
 {
-    if (!strncmp(filename, "/proc/", strlen("/proc/"))) {
-        filename += strlen("/proc/");
-        if (!strncmp(filename, "self/", strlen("self/"))) {
-            filename += strlen("self/");
-        } else if (*filename >= '1' && *filename <= '9') {
-            char myself[80];
-            snprintf(myself, sizeof(myself), "%d/", getpid());
-            if (!strncmp(filename, myself, strlen(myself))) {
-                filename += strlen(myself);
-            } else {
-                return 0;
-            }
-        } else {
-            return 0;
-        }
-        if (!strcmp(filename, entry)) {
-            return 1;
-        }
+    char proc_self_entry[PATH_MAX + 1];
+    char proc_self_entry_realpath[PATH_MAX + 1];
+    char filename_realpath[PATH_MAX + 1];
+
+    if (PATH_MAX < snprintf(proc_self_entry,
+                            sizeof(proc_self_entry),
+                            "/proc/self/%s",
+                            entry)) {
+        /* Full path to "entry" is too long to fit in the buffer */
+        return 0;
     }
-    return 0;
+
+    if (!realpath(filename, filename_realpath)) {
+        /* File does not exist, or can't be canonicalized */
+        return 0;
+    }
+
+    if (!realpath(proc_self_entry, proc_self_entry_realpath)) {
+        /* Procfs entry does not exist */
+        return 0;
+    }
+
+    if (strcmp(filename_realpath, proc_self_entry_realpath) != 0) {
+        /* Paths are different */
+        return 0;
+    }
+
+    /* filename refers to /proc/self/<entry> */
+    return 1;
 }

 #if defined(HOST_WORDS_BIGENDIAN) != defined(TARGET_WORDS_BIGENDIAN)