Dear Debian maintainers
I stumbled over a use-after-free bug in pmount. It's in its realpath implementation when dealing with stacked symlinks, i.e. symlinks pointing to symlinks. (Ironically, pmount "switched to a [self-made] implementation of realpath, for security reasons", so that's that).
The bug is in realpath.c lines 144 to 149:
```
// while (symlink) {
// [...]
if (buf)
free(buf); // (1)
buf = xmalloc(m + n + 1);
memcpy(buf, link_path, n);
memcpy(buf + n, path, m + 1); // (2)
path = buf; // (3)
// [...]
// }
```
This snippet is iterated in a while loop over the stacked symlinks, e.g. twice for a symlink pointing to a symlink pointing to a file. In this case `buf` is freed to early (1) as the memory region is still pointed to by `path` (3) and used afterwards (2).
A simple (but properly bad) fix is to delay the freeing as in the follow up message. I don't fully understand all the pointer tricker going on in that function, so there might be better solutions.
Upstream of this package seams dead a long time ago and Fedora uses Debian as upstream, so a fix in Debian would at least hit two major Linux distributions and their derivative ecosystems and maybe even others.
Lastly, how to trigger this bug. Run the test suite `make check` of pmount. Though you have to initialise the test data first (first follow up commit to this). The test_policy the fails. On my system the `resolved_path` variable contained garbage at the end (probably copied from the invalid pointer reference) and `readlink()` failed with an error as such a file did not exist.
This is derived (and slightly expanded) from the old-upstream revision 144.--- tests/check_fstab/a | 1 + tests/check_fstab/b | 1 + tests/check_fstab/c | 1 + tests/check_fstab/d | 1 + tests/check_fstab/e | 1 + 5 files changed, 5 insertions(+) create mode 100644 tests/check_fstab/a create mode 120000 tests/check_fstab/b create mode 120000 tests/check_fstab/c create mode 100644 tests/check_fstab/d create mode 120000 tests/check_fstab/e diff --git a/tests/check_fstab/a b/tests/check_fstab/a new file mode 100644 index 0000000..df3f5a7 --- /dev/null +++ b/tests/check_fstab/a @@ -0,0 +1 @@ +dummy file a diff --git a/tests/check_fstab/b b/tests/check_fstab/b new file mode 120000 index 0000000..2e65efe --- /dev/null +++ b/tests/check_fstab/b @@ -0,0 +1 @@ +a \ No newline at end of file diff --git a/tests/check_fstab/c b/tests/check_fstab/c new file mode 120000 index 0000000..c59d9b6 --- /dev/null +++ b/tests/check_fstab/c @@ -0,0 +1 @@ +d \ No newline at end of file diff --git a/tests/check_fstab/d b/tests/check_fstab/d new file mode 100644 index 0000000..54519ff --- /dev/null +++ b/tests/check_fstab/d @@ -0,0 +1 @@ +dummy file d diff --git a/tests/check_fstab/e b/tests/check_fstab/e new file mode 120000 index 0000000..3410062 --- /dev/null +++ b/tests/check_fstab/e @@ -0,0 +1 @@ +c \ No newline at end of file -- 2.31.1
The memory provided by `buf` is still reference by `path` and used after
the free call. Delay the freeing until after using it.
---
src/realpath.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/src/realpath.c b/src/realpath.c
index 1cf7eaf..9133605 100644
--- a/src/realpath.c
+++ b/src/realpath.c
@@ -64,6 +64,7 @@ private_realpath(const char *path, char *resolved_path, int maxreslth) {
char link_path[PATH_MAX+1];
int n;
char *buf = NULL;
+ char *oldbuf = NULL;
npath = resolved_path;
@@ -141,12 +142,19 @@ private_realpath(const char *path, char *resolved_path, int maxreslth) {
/* Insert symlink contents into path. */
m = strlen(path);
- if (buf)
- free(buf);
+ if (buf) {
+ /* Delay freeing of 'buf', as 'path' might
+ * still be pointing to it. */
+ oldbuf = buf;
+ }
buf = xmalloc(m + n + 1);
memcpy(buf, link_path, n);
memcpy(buf + n, path, m + 1);
path = buf;
+ if (oldbuf) {
+ free(oldbuf);
+ oldbuf = NULL;
+ }
#endif
}
*npath++ = '/';
--
2.31.1
Hi Madie, Last year I wrote a lot of patches for pmount, amongst which two remove the bundled implementation of realpath and switch to the "modern" interface char *realpath(const char *restrict path, NULL); which has been supported by the libc for quite some time. The original program (mount(8) from util-linux) from which the current implementation was taken even dropped it in 2013. Instead of the patch you send, why not drop it completely like I did? https://github.com/MisterDA/pmount/commit/3f1c9229d828698c348e0f933d1438bbd32a9ed7 https://github.com/MisterDA/pmount/commit/391a9752df966a65afe35308c81e5db975f85a6d I wasn't ready to release my updated pmount as the current head commit is broken, and I haven't had time to fix it. I also need to convince myself that the commit history looks good and that I haven't introduced more bugs than I've fixed. If you have some time to spare, please take a look! I'm also afraid that the Debian package is unmaintained. Best regards,
Hi Madie, Last year I wrote a lot of patches for pmount, amongst which two remove the bundled implementation of realpath and switch to the "modern" interface char *realpath(const char *restrict path, NULL); which has been supported by the libc for quite some time. The original program (mount(8) from util-linux) from which the current implementation was taken even dropped it in 2013. Instead of the patch you send, why not drop it completely like I did? https://github.com/MisterDA/pmount/commit/3f1c9229d828698c348e0f933d1438bbd32a9ed7 https://github.com/MisterDA/pmount/commit/391a9752df966a65afe35308c81e5db975f85a6d I wasn't ready to release my updated pmount as the current head commit is broken, and I haven't had time to fix it. I also need to convince myself that the commit history looks good and that I haven't introduced more bugs than I've fixed. If you have some time to spare, please take a look! I'm also afraid that the Debian package is unmaintained. Best regards,
Dear Antonin Thanks for your replay. Definitely the better option in any regard! I feared the packages is not well-maintained, so I opted for a patch which doesn't touch much hoping to reduce maintenance burden and get this fixed easily. (And also because I don't program C and don't know the (often subtle) differences in those implementations). Sad news. So I guess the best is to avoid pmount packages for now. Nice to see that someone gave some love to pmount in the last years! I tested your HEAD but as you mentioned it's doesn't work. As I said, I don't program C, so probably can't help you with that problem. I hope you're fork will become the source eventually. Maybe linking to your repo instead of a dead alioth-archive page would rise its visibility, getting one step closer to that goal. Regards Madie