* Re: [PATCH v5] seunshare: fix the frail tmpdir cleanup [not found] <CAEjxPJ4wxMi0kXc7wDi qwboAcz1Y0UvzDoZMZrpUgcNH_cNRg@mail.gmail.com> @ 2025-10-07 17:02 ` Rahul Sandhu 2025-10-07 17:06 ` [PATCH v6] " Rahul Sandhu 0 siblings, 1 reply; 7+ messages in thread From: Rahul Sandhu @ 2025-10-07 17:02 UTC (permalink / raw) To: stephen.smalley.work; +Cc: jwcart2, nvraxn, selinux > With this patch applied, I see the following output when I use sandbox > -M and create anything in the tmpdir. > > $ sandbox -M bash > bash: cannot set terminal process group (-1): Inappropriate ioctl for device > bash: no job control in this shell > bash-5.2$ mkdir /tmp/foobar > bash-5.2$ exit > Failed to recursively remove directory /tmp/.sandbox-sdsmall-chjRXi Some idiot just spent a few hours trying to reproduce this and failing, because they changed the function signature to an int to a bool, and although they commited that, turns out that this was unstaged: - if (rm_rf(dirfd(dir), entry->d_name) != 0) { + if (!rm_rf(dirfd(dir), entry->d_name)) { ... I'm really sorry, new patch coming soon. Regards, Rahul ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v6] seunshare: fix the frail tmpdir cleanup 2025-10-07 17:02 ` [PATCH v5] seunshare: fix the frail tmpdir cleanup Rahul Sandhu @ 2025-10-07 17:06 ` Rahul Sandhu 2025-10-07 17:42 ` Stephen Smalley 0 siblings, 1 reply; 7+ messages in thread From: Rahul Sandhu @ 2025-10-07 17:06 UTC (permalink / raw) To: nvraxn; +Cc: jwcart2, selinux, stephen.smalley.work For some reason, rm is invoked via system (3) to cleanup the runtime temp directory. This really isn't all that robust, *especially* given that seunshare is supposed to be a security boundary. Instead do this using libc, the API designed to be used within C programs. Also make sure that we don't follow symbolic links; the input being deleted is untrusted, and hence a malicious symbolic link may be placed outside of the sandbox. Signed-off-by: Rahul Sandhu <nvraxn@gmail.com> --- sandbox/seunshare.c | 77 ++++++++++++++++++++++++++++++++++++++------- 1 file changed, 65 insertions(+), 12 deletions(-) v2: don't use else after return v3: don't follow symlinks in rm_rf (). This is pretty important as we we are operating on an untrusted directory, which may have symlinks pointed to privileged content. However, as we only really need to operate on the contents of the tmpdir, we can ignore symlinks. v4: fix spelling in commit message v5: fix spelling in comment v6: fix the error checking for the rm_rf () function. diff --git a/sandbox/seunshare.c b/sandbox/seunshare.c index 106f625f..337a9205 100644 --- a/sandbox/seunshare.c +++ b/sandbox/seunshare.c @@ -403,6 +403,66 @@ err: return rc; } +/* + * Recursively delete a directory. + * SAFETY: This function will NOT follow symbolic links (AT_SYMLINK_NOFOLLOW). + * As a result, this function can be run safely on a directory owned by + * a non-root user: symbolic links to root paths (such as /root) will + * not be followed. + */ +static bool rm_rf(int targetfd, const char *path) { + struct stat statbuf; + + if (fstatat(targetfd, path, &statbuf, AT_SYMLINK_NOFOLLOW) < 0) { + if (errno == ENOENT) { + return true; + } + perror("fstatat"); + return false; + } + + if (S_ISDIR(statbuf.st_mode)) { + const int newfd = openat(targetfd, path, O_RDONLY | O_DIRECTORY | O_CLOEXEC); + if (newfd < 0) { + perror("openat"); + return false; + } + + DIR *dir = fdopendir(newfd); + if (!dir) { + perror("fdopendir"); + close(newfd); + return false; + } + + struct dirent *entry; + int rc = true; + while ((entry = readdir(dir)) != NULL) { + if (strcmp(entry->d_name, ".") == 0 || strcmp(entry->d_name, "..") == 0) { + continue; + } + + if (!rm_rf(dirfd(dir), entry->d_name)) { + rc = false; + } + } + + closedir(dir); + + if (unlinkat(targetfd, path, AT_REMOVEDIR) < 0) { + perror("unlinkat"); + rc = false; + } + + return rc; + } + if (unlinkat(targetfd, path, 0) < 0) { + perror("unlinkat"); + return false; + } + return true; +} + /** * Clean up runtime temporary directory. Returns 0 if no problem was detected, * >0 if some error was detected, but errors here are treated as non-fatal and @@ -428,24 +488,17 @@ static int cleanup_tmpdir(const char *tmpdir, const char *src, free(cmdbuf); cmdbuf = NULL; } - /* remove files from the runtime temporary directory */ - if (asprintf(&cmdbuf, "/bin/rm -r '%s/' 2>/dev/null", tmpdir) == -1) { - fprintf(stderr, _("Out of memory\n")); - cmdbuf = NULL; + if ((uid_t)setfsuid(0) != 0) { + /* setfsuid does not return error, but this check makes code checkers happy */ rc++; } - /* this may fail if there's root-owned file left in the runtime tmpdir */ - if (cmdbuf && spawn_command(cmdbuf, pwd->pw_uid) != 0) rc++; - free(cmdbuf); cmdbuf = NULL; - /* remove runtime temporary directory */ - if ((uid_t)setfsuid(0) != 0) { - /* setfsuid does not return error, but this check makes code checkers happy */ + /* Recursively remove the runtime temp directory. */ + if (!rm_rf(AT_FDCWD, tmpdir)) { + fprintf(stderr, _("Failed to recursively remove directory %s\n"), tmpdir); rc++; } - if (pwd->pw_uid != 0 && rmdir(tmpdir) == -1) - fprintf(stderr, _("Failed to remove directory %s: %s\n"), tmpdir, strerror(errno)); if ((uid_t)setfsuid(pwd->pw_uid) != 0) { fprintf(stderr, _("unable to switch back to user after clearing tmp dir\n")); rc++; -- 2.50.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v6] seunshare: fix the frail tmpdir cleanup 2025-10-07 17:06 ` [PATCH v6] " Rahul Sandhu @ 2025-10-07 17:42 ` Stephen Smalley 2025-10-07 18:01 ` Stephen Smalley 0 siblings, 1 reply; 7+ messages in thread From: Stephen Smalley @ 2025-10-07 17:42 UTC (permalink / raw) To: Rahul Sandhu, Petr Lautrbach; +Cc: jwcart2, selinux On Tue, Oct 7, 2025 at 1:06 PM Rahul Sandhu <nvraxn@gmail.com> wrote: > > For some reason, rm is invoked via system (3) to cleanup the runtime > temp directory. This really isn't all that robust, *especially* given > that seunshare is supposed to be a security boundary. Instead do this > using libc, the API designed to be used within C programs. > > Also make sure that we don't follow symbolic links; the input being > deleted is untrusted, and hence a malicious symbolic link may be placed > outside of the sandbox. > > Signed-off-by: Rahul Sandhu <nvraxn@gmail.com> Curiously the /tmp/.sandbox-$USER-$random directory remains after exiting the sandbox, but this also seems to be true with the current seunshare. Consequently, I think this is fine as is although maybe we ought to look into removing that too. Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com> > --- > sandbox/seunshare.c | 77 ++++++++++++++++++++++++++++++++++++++------- > 1 file changed, 65 insertions(+), 12 deletions(-) > > v2: don't use else after return > v3: don't follow symlinks in rm_rf (). This is pretty important as we > we are operating on an untrusted directory, which may have symlinks > pointed to privileged content. However, as we only really need to > operate on the contents of the tmpdir, we can ignore symlinks. > v4: fix spelling in commit message > v5: fix spelling in comment > v6: fix the error checking for the rm_rf () function. > > diff --git a/sandbox/seunshare.c b/sandbox/seunshare.c > index 106f625f..337a9205 100644 > --- a/sandbox/seunshare.c > +++ b/sandbox/seunshare.c > @@ -403,6 +403,66 @@ err: > return rc; > } > > +/* > + * Recursively delete a directory. > + * SAFETY: This function will NOT follow symbolic links (AT_SYMLINK_NOFOLLOW). > + * As a result, this function can be run safely on a directory owned by > + * a non-root user: symbolic links to root paths (such as /root) will > + * not be followed. > + */ > +static bool rm_rf(int targetfd, const char *path) { > + struct stat statbuf; > + > + if (fstatat(targetfd, path, &statbuf, AT_SYMLINK_NOFOLLOW) < 0) { > + if (errno == ENOENT) { > + return true; > + } > + perror("fstatat"); > + return false; > + } > + > + if (S_ISDIR(statbuf.st_mode)) { > + const int newfd = openat(targetfd, path, O_RDONLY | O_DIRECTORY | O_CLOEXEC); > + if (newfd < 0) { > + perror("openat"); > + return false; > + } > + > + DIR *dir = fdopendir(newfd); > + if (!dir) { > + perror("fdopendir"); > + close(newfd); > + return false; > + } > + > + struct dirent *entry; > + int rc = true; > + while ((entry = readdir(dir)) != NULL) { > + if (strcmp(entry->d_name, ".") == 0 || strcmp(entry->d_name, "..") == 0) { > + continue; > + } > + > + if (!rm_rf(dirfd(dir), entry->d_name)) { > + rc = false; > + } > + } > + > + closedir(dir); > + > + if (unlinkat(targetfd, path, AT_REMOVEDIR) < 0) { > + perror("unlinkat"); > + rc = false; > + } > + > + return rc; > + } > + if (unlinkat(targetfd, path, 0) < 0) { > + perror("unlinkat"); > + return false; > + } > + return true; > +} > + > /** > * Clean up runtime temporary directory. Returns 0 if no problem was detected, > * >0 if some error was detected, but errors here are treated as non-fatal and > @@ -428,24 +488,17 @@ static int cleanup_tmpdir(const char *tmpdir, const char *src, > free(cmdbuf); cmdbuf = NULL; > } > > - /* remove files from the runtime temporary directory */ > - if (asprintf(&cmdbuf, "/bin/rm -r '%s/' 2>/dev/null", tmpdir) == -1) { > - fprintf(stderr, _("Out of memory\n")); > - cmdbuf = NULL; > + if ((uid_t)setfsuid(0) != 0) { > + /* setfsuid does not return error, but this check makes code checkers happy */ > rc++; > } > - /* this may fail if there's root-owned file left in the runtime tmpdir */ > - if (cmdbuf && spawn_command(cmdbuf, pwd->pw_uid) != 0) rc++; > - free(cmdbuf); cmdbuf = NULL; > > - /* remove runtime temporary directory */ > - if ((uid_t)setfsuid(0) != 0) { > - /* setfsuid does not return error, but this check makes code checkers happy */ > + /* Recursively remove the runtime temp directory. */ > + if (!rm_rf(AT_FDCWD, tmpdir)) { > + fprintf(stderr, _("Failed to recursively remove directory %s\n"), tmpdir); > rc++; > } > > - if (pwd->pw_uid != 0 && rmdir(tmpdir) == -1) > - fprintf(stderr, _("Failed to remove directory %s: %s\n"), tmpdir, strerror(errno)); > if ((uid_t)setfsuid(pwd->pw_uid) != 0) { > fprintf(stderr, _("unable to switch back to user after clearing tmp dir\n")); > rc++; > -- > 2.50.1 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v6] seunshare: fix the frail tmpdir cleanup 2025-10-07 17:42 ` Stephen Smalley @ 2025-10-07 18:01 ` Stephen Smalley 2025-10-07 18:09 ` [PATCH v7] " Rahul Sandhu 0 siblings, 1 reply; 7+ messages in thread From: Stephen Smalley @ 2025-10-07 18:01 UTC (permalink / raw) To: Rahul Sandhu, Petr Lautrbach; +Cc: jwcart2, selinux On Tue, Oct 7, 2025 at 1:42 PM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > > On Tue, Oct 7, 2025 at 1:06 PM Rahul Sandhu <nvraxn@gmail.com> wrote: > > > > For some reason, rm is invoked via system (3) to cleanup the runtime > > temp directory. This really isn't all that robust, *especially* given > > that seunshare is supposed to be a security boundary. Instead do this > > using libc, the API designed to be used within C programs. > > > > Also make sure that we don't follow symbolic links; the input being > > deleted is untrusted, and hence a malicious symbolic link may be placed > > outside of the sandbox. > > > > Signed-off-by: Rahul Sandhu <nvraxn@gmail.com> > > Curiously the /tmp/.sandbox-$USER-$random directory remains after > exiting the sandbox, but this also seems to be true with the current > seunshare. > Consequently, I think this is fine as is although maybe we ought to > look into removing that too. > > Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com> Oops, missing #include <stdbool.h>, broke CI, https://github.com/SELinuxProject/selinux/actions/runs/18321378627/job/52175025361?pr=496#logs > > > --- > > sandbox/seunshare.c | 77 ++++++++++++++++++++++++++++++++++++++------- > > 1 file changed, 65 insertions(+), 12 deletions(-) > > > > v2: don't use else after return > > v3: don't follow symlinks in rm_rf (). This is pretty important as we > > we are operating on an untrusted directory, which may have symlinks > > pointed to privileged content. However, as we only really need to > > operate on the contents of the tmpdir, we can ignore symlinks. > > v4: fix spelling in commit message > > v5: fix spelling in comment > > v6: fix the error checking for the rm_rf () function. > > > > diff --git a/sandbox/seunshare.c b/sandbox/seunshare.c > > index 106f625f..337a9205 100644 > > --- a/sandbox/seunshare.c > > +++ b/sandbox/seunshare.c > > @@ -403,6 +403,66 @@ err: > > return rc; > > } > > > > +/* > > + * Recursively delete a directory. > > + * SAFETY: This function will NOT follow symbolic links (AT_SYMLINK_NOFOLLOW). > > + * As a result, this function can be run safely on a directory owned by > > + * a non-root user: symbolic links to root paths (such as /root) will > > + * not be followed. > > + */ > > +static bool rm_rf(int targetfd, const char *path) { > > + struct stat statbuf; > > + > > + if (fstatat(targetfd, path, &statbuf, AT_SYMLINK_NOFOLLOW) < 0) { > > + if (errno == ENOENT) { > > + return true; > > + } > > + perror("fstatat"); > > + return false; > > + } > > + > > + if (S_ISDIR(statbuf.st_mode)) { > > + const int newfd = openat(targetfd, path, O_RDONLY | O_DIRECTORY | O_CLOEXEC); > > + if (newfd < 0) { > > + perror("openat"); > > + return false; > > + } > > + > > + DIR *dir = fdopendir(newfd); > > + if (!dir) { > > + perror("fdopendir"); > > + close(newfd); > > + return false; > > + } > > + > > + struct dirent *entry; > > + int rc = true; > > + while ((entry = readdir(dir)) != NULL) { > > + if (strcmp(entry->d_name, ".") == 0 || strcmp(entry->d_name, "..") == 0) { > > + continue; > > + } > > + > > + if (!rm_rf(dirfd(dir), entry->d_name)) { > > + rc = false; > > + } > > + } > > + > > + closedir(dir); > > + > > + if (unlinkat(targetfd, path, AT_REMOVEDIR) < 0) { > > + perror("unlinkat"); > > + rc = false; > > + } > > + > > + return rc; > > + } > > + if (unlinkat(targetfd, path, 0) < 0) { > > + perror("unlinkat"); > > + return false; > > + } > > + return true; > > +} > > + > > /** > > * Clean up runtime temporary directory. Returns 0 if no problem was detected, > > * >0 if some error was detected, but errors here are treated as non-fatal and > > @@ -428,24 +488,17 @@ static int cleanup_tmpdir(const char *tmpdir, const char *src, > > free(cmdbuf); cmdbuf = NULL; > > } > > > > - /* remove files from the runtime temporary directory */ > > - if (asprintf(&cmdbuf, "/bin/rm -r '%s/' 2>/dev/null", tmpdir) == -1) { > > - fprintf(stderr, _("Out of memory\n")); > > - cmdbuf = NULL; > > + if ((uid_t)setfsuid(0) != 0) { > > + /* setfsuid does not return error, but this check makes code checkers happy */ > > rc++; > > } > > - /* this may fail if there's root-owned file left in the runtime tmpdir */ > > - if (cmdbuf && spawn_command(cmdbuf, pwd->pw_uid) != 0) rc++; > > - free(cmdbuf); cmdbuf = NULL; > > > > - /* remove runtime temporary directory */ > > - if ((uid_t)setfsuid(0) != 0) { > > - /* setfsuid does not return error, but this check makes code checkers happy */ > > + /* Recursively remove the runtime temp directory. */ > > + if (!rm_rf(AT_FDCWD, tmpdir)) { > > + fprintf(stderr, _("Failed to recursively remove directory %s\n"), tmpdir); > > rc++; > > } > > > > - if (pwd->pw_uid != 0 && rmdir(tmpdir) == -1) > > - fprintf(stderr, _("Failed to remove directory %s: %s\n"), tmpdir, strerror(errno)); > > if ((uid_t)setfsuid(pwd->pw_uid) != 0) { > > fprintf(stderr, _("unable to switch back to user after clearing tmp dir\n")); > > rc++; > > -- > > 2.50.1 > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v7] seunshare: fix the frail tmpdir cleanup 2025-10-07 18:01 ` Stephen Smalley @ 2025-10-07 18:09 ` Rahul Sandhu 2025-10-07 18:20 ` Stephen Smalley 0 siblings, 1 reply; 7+ messages in thread From: Rahul Sandhu @ 2025-10-07 18:09 UTC (permalink / raw) To: stephen.smalley.work; +Cc: jwcart2, lautrbach, nvraxn, selinux For some reason, rm is invoked via system (3) to cleanup the runtime temp directory. This really isn't all that robust, *especially* given that seunshare is supposed to be a security boundary. Instead do this using libc, the API designed to be used within C programs. Also make sure that we don't follow symbolic links; the input being deleted is untrusted, and hence a malicious symbolic link may be placed outside of the sandbox. Signed-off-by: Rahul Sandhu <nvraxn@gmail.com> --- sandbox/seunshare.c | 78 ++++++++++++++++++++++++++++++++++++++------- 1 file changed, 66 insertions(+), 12 deletions(-) v2: don't use else after return v3: don't follow symlinks in rm_rf (). This is pretty important as we we are operating on an untrusted directory, which may have symlinks pointed to privileged content. However, as we only really need to operate on the contents of the tmpdir, we can ignore symlinks. v4: fix spelling in commit message v5: fix spelling in comment v6: fix the error checking for the rm_rf () function. v7: include stdbool.h diff --git a/sandbox/seunshare.c b/sandbox/seunshare.c index 106f625f..01ed9d8e 100644 --- a/sandbox/seunshare.c +++ b/sandbox/seunshare.c @@ -4,6 +4,7 @@ */ #define _GNU_SOURCE +#include <stdbool.h> #include <signal.h> #include <sys/fsuid.h> #include <sys/stat.h> @@ -403,6 +404,66 @@ err: return rc; } +/* + * Recursively delete a directory. + * SAFETY: This function will NOT follow symbolic links (AT_SYMLINK_NOFOLLOW). + * As a result, this function can be run safely on a directory owned by + * a non-root user: symbolic links to root paths (such as /root) will + * not be followed. + */ +static bool rm_rf(int targetfd, const char *path) { + struct stat statbuf; + + if (fstatat(targetfd, path, &statbuf, AT_SYMLINK_NOFOLLOW) < 0) { + if (errno == ENOENT) { + return true; + } + perror("fstatat"); + return false; + } + + if (S_ISDIR(statbuf.st_mode)) { + const int newfd = openat(targetfd, path, O_RDONLY | O_DIRECTORY | O_CLOEXEC); + if (newfd < 0) { + perror("openat"); + return false; + } + + DIR *dir = fdopendir(newfd); + if (!dir) { + perror("fdopendir"); + close(newfd); + return false; + } + + struct dirent *entry; + int rc = true; + while ((entry = readdir(dir)) != NULL) { + if (strcmp(entry->d_name, ".") == 0 || strcmp(entry->d_name, "..") == 0) { + continue; + } + + if (!rm_rf(dirfd(dir), entry->d_name)) { + rc = false; + } + } + + closedir(dir); + + if (unlinkat(targetfd, path, AT_REMOVEDIR) < 0) { + perror("unlinkat"); + rc = false; + } + + return rc; + } + if (unlinkat(targetfd, path, 0) < 0) { + perror("unlinkat"); + return false; + } + return true; +} + /** * Clean up runtime temporary directory. Returns 0 if no problem was detected, * >0 if some error was detected, but errors here are treated as non-fatal and @@ -428,24 +489,17 @@ static int cleanup_tmpdir(const char *tmpdir, const char *src, free(cmdbuf); cmdbuf = NULL; } - /* remove files from the runtime temporary directory */ - if (asprintf(&cmdbuf, "/bin/rm -r '%s/' 2>/dev/null", tmpdir) == -1) { - fprintf(stderr, _("Out of memory\n")); - cmdbuf = NULL; + if ((uid_t)setfsuid(0) != 0) { + /* setfsuid does not return error, but this check makes code checkers happy */ rc++; } - /* this may fail if there's root-owned file left in the runtime tmpdir */ - if (cmdbuf && spawn_command(cmdbuf, pwd->pw_uid) != 0) rc++; - free(cmdbuf); cmdbuf = NULL; - /* remove runtime temporary directory */ - if ((uid_t)setfsuid(0) != 0) { - /* setfsuid does not return error, but this check makes code checkers happy */ + /* Recursively remove the runtime temp directory. */ + if (!rm_rf(AT_FDCWD, tmpdir)) { + fprintf(stderr, _("Failed to recursively remove directory %s\n"), tmpdir); rc++; } - if (pwd->pw_uid != 0 && rmdir(tmpdir) == -1) - fprintf(stderr, _("Failed to remove directory %s: %s\n"), tmpdir, strerror(errno)); if ((uid_t)setfsuid(pwd->pw_uid) != 0) { fprintf(stderr, _("unable to switch back to user after clearing tmp dir\n")); rc++; -- 2.50.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v7] seunshare: fix the frail tmpdir cleanup 2025-10-07 18:09 ` [PATCH v7] " Rahul Sandhu @ 2025-10-07 18:20 ` Stephen Smalley 2025-10-08 13:16 ` Stephen Smalley 0 siblings, 1 reply; 7+ messages in thread From: Stephen Smalley @ 2025-10-07 18:20 UTC (permalink / raw) To: Rahul Sandhu; +Cc: jwcart2, lautrbach, selinux On Tue, Oct 7, 2025 at 2:09 PM Rahul Sandhu <nvraxn@gmail.com> wrote: > > For some reason, rm is invoked via system (3) to cleanup the runtime > temp directory. This really isn't all that robust, *especially* given > that seunshare is supposed to be a security boundary. Instead do this > using libc, the API designed to be used within C programs. > > Also make sure that we don't follow symbolic links; the input being > deleted is untrusted, and hence a malicious symbolic link may be placed > outside of the sandbox. > > Signed-off-by: Rahul Sandhu <nvraxn@gmail.com> 7th time's the charm ;) Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com> > --- > sandbox/seunshare.c | 78 ++++++++++++++++++++++++++++++++++++++------- > 1 file changed, 66 insertions(+), 12 deletions(-) > > v2: don't use else after return > v3: don't follow symlinks in rm_rf (). This is pretty important as we > we are operating on an untrusted directory, which may have symlinks > pointed to privileged content. However, as we only really need to > operate on the contents of the tmpdir, we can ignore symlinks. > v4: fix spelling in commit message > v5: fix spelling in comment > v6: fix the error checking for the rm_rf () function. > v7: include stdbool.h > > diff --git a/sandbox/seunshare.c b/sandbox/seunshare.c > index 106f625f..01ed9d8e 100644 > --- a/sandbox/seunshare.c > +++ b/sandbox/seunshare.c > @@ -4,6 +4,7 @@ > */ > > #define _GNU_SOURCE > +#include <stdbool.h> > #include <signal.h> > #include <sys/fsuid.h> > #include <sys/stat.h> > @@ -403,6 +404,66 @@ err: > return rc; > } > > +/* > + * Recursively delete a directory. > + * SAFETY: This function will NOT follow symbolic links (AT_SYMLINK_NOFOLLOW). > + * As a result, this function can be run safely on a directory owned by > + * a non-root user: symbolic links to root paths (such as /root) will > + * not be followed. > + */ > +static bool rm_rf(int targetfd, const char *path) { > + struct stat statbuf; > + > + if (fstatat(targetfd, path, &statbuf, AT_SYMLINK_NOFOLLOW) < 0) { > + if (errno == ENOENT) { > + return true; > + } > + perror("fstatat"); > + return false; > + } > + > + if (S_ISDIR(statbuf.st_mode)) { > + const int newfd = openat(targetfd, path, O_RDONLY | O_DIRECTORY | O_CLOEXEC); > + if (newfd < 0) { > + perror("openat"); > + return false; > + } > + > + DIR *dir = fdopendir(newfd); > + if (!dir) { > + perror("fdopendir"); > + close(newfd); > + return false; > + } > + > + struct dirent *entry; > + int rc = true; > + while ((entry = readdir(dir)) != NULL) { > + if (strcmp(entry->d_name, ".") == 0 || strcmp(entry->d_name, "..") == 0) { > + continue; > + } > + > + if (!rm_rf(dirfd(dir), entry->d_name)) { > + rc = false; > + } > + } > + > + closedir(dir); > + > + if (unlinkat(targetfd, path, AT_REMOVEDIR) < 0) { > + perror("unlinkat"); > + rc = false; > + } > + > + return rc; > + } > + if (unlinkat(targetfd, path, 0) < 0) { > + perror("unlinkat"); > + return false; > + } > + return true; > +} > + > /** > * Clean up runtime temporary directory. Returns 0 if no problem was detected, > * >0 if some error was detected, but errors here are treated as non-fatal and > @@ -428,24 +489,17 @@ static int cleanup_tmpdir(const char *tmpdir, const char *src, > free(cmdbuf); cmdbuf = NULL; > } > > - /* remove files from the runtime temporary directory */ > - if (asprintf(&cmdbuf, "/bin/rm -r '%s/' 2>/dev/null", tmpdir) == -1) { > - fprintf(stderr, _("Out of memory\n")); > - cmdbuf = NULL; > + if ((uid_t)setfsuid(0) != 0) { > + /* setfsuid does not return error, but this check makes code checkers happy */ > rc++; > } > - /* this may fail if there's root-owned file left in the runtime tmpdir */ > - if (cmdbuf && spawn_command(cmdbuf, pwd->pw_uid) != 0) rc++; > - free(cmdbuf); cmdbuf = NULL; > > - /* remove runtime temporary directory */ > - if ((uid_t)setfsuid(0) != 0) { > - /* setfsuid does not return error, but this check makes code checkers happy */ > + /* Recursively remove the runtime temp directory. */ > + if (!rm_rf(AT_FDCWD, tmpdir)) { > + fprintf(stderr, _("Failed to recursively remove directory %s\n"), tmpdir); > rc++; > } > > - if (pwd->pw_uid != 0 && rmdir(tmpdir) == -1) > - fprintf(stderr, _("Failed to remove directory %s: %s\n"), tmpdir, strerror(errno)); > if ((uid_t)setfsuid(pwd->pw_uid) != 0) { > fprintf(stderr, _("unable to switch back to user after clearing tmp dir\n")); > rc++; > -- > 2.50.1 > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v7] seunshare: fix the frail tmpdir cleanup 2025-10-07 18:20 ` Stephen Smalley @ 2025-10-08 13:16 ` Stephen Smalley 0 siblings, 0 replies; 7+ messages in thread From: Stephen Smalley @ 2025-10-08 13:16 UTC (permalink / raw) To: Rahul Sandhu; +Cc: jwcart2, lautrbach, selinux On Tue, Oct 7, 2025 at 2:20 PM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > > On Tue, Oct 7, 2025 at 2:09 PM Rahul Sandhu <nvraxn@gmail.com> wrote: > > > > For some reason, rm is invoked via system (3) to cleanup the runtime > > temp directory. This really isn't all that robust, *especially* given > > that seunshare is supposed to be a security boundary. Instead do this > > using libc, the API designed to be used within C programs. > > > > Also make sure that we don't follow symbolic links; the input being > > deleted is untrusted, and hence a malicious symbolic link may be placed > > outside of the sandbox. > > > > Signed-off-by: Rahul Sandhu <nvraxn@gmail.com> > > 7th time's the charm ;) > > Acked-by: Stephen Smalley <stephen.smalley.work@gmail.com> Thanks, merged. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2025-10-08 13:16 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <CAEjxPJ4wxMi0kXc7wDi qwboAcz1Y0UvzDoZMZrpUgcNH_cNRg@mail.gmail.com>
2025-10-07 17:02 ` [PATCH v5] seunshare: fix the frail tmpdir cleanup Rahul Sandhu
2025-10-07 17:06 ` [PATCH v6] " Rahul Sandhu
2025-10-07 17:42 ` Stephen Smalley
2025-10-07 18:01 ` Stephen Smalley
2025-10-07 18:09 ` [PATCH v7] " Rahul Sandhu
2025-10-07 18:20 ` Stephen Smalley
2025-10-08 13:16 ` Stephen Smalley
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).