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

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

On Mon, Oct 6, 2025 at 11:57 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Thu, Sep 25, 2025 at 1:38 AM 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, don't bother trying to delete the contents of the tmpdir and then
> > trying to delete the parent directory with rmdir later - for some...
> > undocumented reason currently we attempt to delete the contents of the
> > dir with dropped perms, *then* elevate perms, *then* delete the tmpdir.
>
> I think this was for security/safety reasons, to prevent unlinking any
> files linked from the tmpdir that weren't actually owned by the user.
> However, in theory, your rewrite makes that obsolete.
>
> >
> > This doesn't really make all that much sense as far as I can tell.  We
> > should be the only ones using the tmpdir, so we may as well just delete
> > the entire thing using the rm_rf () function with elevated permissions.
> >
> > 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
> >
> > diff --git a/sandbox/seunshare.c b/sandbox/seunshare.c
> > index 106f625f..c46db90f 100644
> > --- a/sandbox/seunshare.c
> > +++ b/sandbox/seunshare.c
> > @@ -403,6 +403,66 @@ err:
> >         return rc;
> >  }
> >
> > +/*
> > + * Recursively delete a directory.
> > + * SAFTEY: This function will NOT follow symbolic links (AT_SYMLINK_NOFOLLOW).
>
> Spelling here too: SAFETY
>
> > + *         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) != 0) {
> > +                               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 */

I'm a little confused by this - further down there is a
setfsuid(pwd->pw_uid) call which seems to suggest that we are already
running as uid-0 at this point? If so, why do we need this setfsuid(0)
call?

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