Re: [PATCH v7] seunshare: fix the frail tmpdir cleanup

[Stephen Smalley <stephen.smalley.work@gmail.com>]

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
>