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

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 <[email protected]>

---
 linux-user/syscall.c | 47 ++++++++++++++++++++++++++++-------------------
 1 file changed, 28 insertions(+), 19 deletions(-)

linux-user/syscall.c +28 -20

index 9bf901fa11..6c1f28a1f7 100644
@@ -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

Friendly ping :)

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

*Zach Riggle*

Quoted 3 lines · Zach Riggle

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"

Quoted 77 lines

    $ 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 <[email protected]>

---
 linux-user/syscall.c | 47 ++++++++++++++++++++++++++++-------------------
 1 file changed, 28 insertions(+), 19 deletions(-)

linux-user/syscall.c +28 -20

index 9bf901fa11..6c1f28a1f7 100644
@@ -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

Quoted 3 lines · Zach Riggle

Friendly ping :)

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

I'll have a look at it soon.

Quoted 4 lines

*Zach Riggle*

On Tue, Oct 24, 2017 at 10:34 PM, Zach Riggle <[email protected]>

Quoted 2 lines · wrote:

Quoted 2 lines

Previously, it was possible to get a handle to the "real"

/proc/self/mem

Quoted 2 lines

Quoted 2 lines

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.

Quoted 9 lines

Quoted 9 lines

    $ 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
    ...

Quoted 3 lines

Quoted 3 lines

Signed-off-by Zach Riggle <[email protected]>

---
 linux-user/syscall.c | 47 ++++++++++++++++++++++++++++--

-----------------

Quoted 7 lines

linux-user/syscall.c static int open_self_auxv(void *cpu_env, int

Quoted 7 lines

 1 file changed, 28 insertions(+), 19 deletions(-)

linux-user/syscall.c +0 -0

index 9bf901fa11..6c1f28a1f7 100644
@@ -7496,26 +7496,35 @@ static int open_self_auxv(void *cpu_env, int

fd)

Quoted 58 lines

Quoted 58 lines

linux-user/syscall.c +28 -20

 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

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*

Quoted 101 lines · Riku Voipio

Quoted 3 lines · Zach Riggle

Friendly ping :)

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

I'll have a look at it soon.

Quoted 4 lines

*Zach Riggle*

On Tue, Oct 24, 2017 at 10:34 PM, Zach Riggle <[email protected]>

Quoted 2 lines · wrote:

Quoted 2 lines

Previously, it was possible to get a handle to the "real"

/proc/self/mem

Quoted 2 lines

Quoted 2 lines

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.

Quoted 9 lines

Quoted 9 lines

    $ 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
    ...

Quoted 3 lines

Quoted 3 lines

Signed-off-by Zach Riggle <[email protected]>

---
 linux-user/syscall.c | 47 ++++++++++++++++++++++++++++--

-----------------

Quoted 7 lines

linux-user/syscall.c static int open_self_auxv(void *cpu_env, int

Quoted 7 lines

 1 file changed, 28 insertions(+), 19 deletions(-)

linux-user/syscall.c +0 -0

index 9bf901fa11..6c1f28a1f7 100644
@@ -7496,26 +7496,35 @@ static int open_self_auxv(void *cpu_env, int

fd)

Quoted 58 lines

Quoted 58 lines

linux-user/syscall.c +28 -20

 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

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.

Quoted 2 lines · Zach Riggle

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.

Quoted 10 lines

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

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

Quoted 2 lines · Eric Blake

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

Ping! What needs to be done to move this forward? My current implementation is compatible with musl.

Quoted 9 lines · Peter Maydell

Quoted 2 lines · Eric Blake

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

Hi,

Quoted 1 line · Zach Riggle

Ping! What needs to be done to move this forward? My current

implementation

Quoted 1 line

is compatible with musl.

I'll have a look at it soon.

Riku

Quoted 12 lines

Quoted 9 lines · Peter Maydell

Quoted 2 lines · Eric Blake

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

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 <[email protected]>

---
 linux-user/syscall.c | 47 ++++++++++++++++++++++++++++-------------------
 1 file changed, 28 insertions(+), 19 deletions(-)

linux-user/syscall.c +13 -19

index 9bf901fa11..6c1f28a1f7 100644
@@ -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 */

Quoted 7 lines · Zach Riggle

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

Quoted 1 line

    604bc000-6050f000 rw-p 002bc000 fc:01 270375

/usr/bin/qemu-arm-static

Quoted 5 lines

    ...

Signed-off-by Zach Riggle <[email protected]>

---
 linux-user/syscall.c | 47 ++++++++++++++++++++++++++++--

-----------------

Quoted 43 lines

 1 file changed, 28 insertions(+), 19 deletions(-)

linux-user/syscall.c +13 -19

index 9bf901fa11..6c1f28a1f7 100644
@@ -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 */

linux-user/syscall.c static int open_self_auxv(void *cpu_env, int fd) · line 7522

Quoted 10 lines

linux-user/syscall.c +10 -0

+        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

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*

Quoted 80 lines · Laurent Vivier

Quoted 7 lines · Zach Riggle

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

Quoted 1 line

    604bc000-6050f000 rw-p 002bc000 fc:01 270375

/usr/bin/qemu-arm-static

Quoted 5 lines

    ...

Signed-off-by Zach Riggle <[email protected]>

---
 linux-user/syscall.c | 47 ++++++++++++++++++++++++++++--

-----------------

Quoted 43 lines

 1 file changed, 28 insertions(+), 19 deletions(-)

linux-user/syscall.c +13 -19

index 9bf901fa11..6c1f28a1f7 100644
@@ -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 */

linux-user/syscall.c static int open_self_auxv(void *cpu_env, int fd) · line 7522

Quoted 10 lines

linux-user/syscall.c +10 -0

+        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

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/");
}