Re: [RFC PATCH 4/4] libselinux: restorecon: pin file to avoid TOCTOU issues

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, May 16, 2022 at 3:01 PM Christian Göttsche
<cgzones@xxxxxxxxxxxxxx> wrote:
>
> On Mon, 16 May 2022 at 19:13, James Carter <jwcart2@xxxxxxxxx> wrote:
> >
> > On Thu, May 12, 2022 at 1:39 PM James Carter <jwcart2@xxxxxxxxx> wrote:
> > >
> > > On Wed, May 11, 2022 at 3:32 PM Christian Göttsche
> > > <cgzones@xxxxxxxxxxxxxx> wrote:
> > > >
> > > > Pin the file to operate on in restorecon_sb() to prevent symlink attacks
> > > > in between the label database lookup, the current context query and the
> > > > final context write.  Also don't use the file information from
> > > > fts_read(3), which might also be out of sync.
> > > >
> > > > Due to querying file information twice, one in fts_read(3) needed for
> > > > the cross device check and one on the pinned file descriptor for the
> > > > database lookup, there is a slight slowdown:
> > > >
> > > >     [current]
> > > >     Time (mean ± σ):     14.456 s ±  0.306 s    [User: 45.863 s, System: 4.463 s]
> > > >     Range (min … max):   14.275 s … 15.294 s    10 runs
> > > >
> > > >     [changed]
> > > >     Time (mean ± σ):     15.843 s ±  0.045 s    [User: 46.274 s, System: 9.495 s]
> > > >     Range (min … max):   15.787 s … 15.916 s    10 runs
> > > >
> > > > Signed-off-by: Christian Göttsche <cgzones@xxxxxxxxxxxxxx>
> > >
> > > Acked-by: James Carter <jwcart2@xxxxxxxxx>
> > >
> >
> > Merged.
> > Thanks,
> > Jim
> >
> > > > ---
> > > >  libselinux/src/selinux_restorecon.c | 43 +++++++++++++++++------------
> > > >  1 file changed, 25 insertions(+), 18 deletions(-)
> > > >
> > > > diff --git a/libselinux/src/selinux_restorecon.c b/libselinux/src/selinux_restorecon.c
> > > > index 42ef30cb..12b85101 100644
> > > > --- a/libselinux/src/selinux_restorecon.c
> > > > +++ b/libselinux/src/selinux_restorecon.c
> > > > @@ -621,13 +621,13 @@ out:
> > > >         return rc;
> > > >  }
> > > >
> > > > -static int restorecon_sb(const char *pathname, const struct stat *sb,
> > > > -                           struct rest_flags *flags, bool first)
> > > > +static int restorecon_sb(const char *pathname, struct rest_flags *flags, bool first)
> > > >  {
> > > >         char *newcon = NULL;
> > > >         char *curcon = NULL;
> > > >         char *newtypecon = NULL;
> > > > -       int rc;
> > > > +       int fd = -1, rc;
> > > > +       struct stat stat_buf;
> > > >         bool updated = false;
> > > >         const char *lookup_path = pathname;
> > > >         float pc;
> > > > @@ -642,13 +642,21 @@ static int restorecon_sb(const char *pathname, const struct stat *sb,
> > > >                 lookup_path += rootpathlen;
> > > >         }
> > > >
> > > > +       fd = open(pathname, O_PATH | O_NOFOLLOW | O_EXCL);
> > > > +       if (fd < 0)
> > > > +               goto err;
> > > > +
> > > > +       rc = fstat(fd, &stat_buf);
>
> According to man:open(2) fstat(2) on a file descriptor obtained via
> O_PATH is only available since Linux 3.6.
> What versions should be supported by libselinux?
> Or should there exist a fallback, checked similarly at [1].
>
>
> [1]: https://github.com/SELinuxProject/selinux/blob/9e096e6ef0a87b05c11ce57077ecb20a1b2a6995/libselinux/src/selinux_restorecon.c#L250
>

I didn't really think about that. Linux 3.6 was 10 years ago, so it
doesn't seem like it should be a problem, but it is probably better to
provide a fallback similar to the one you referenced.
Thanks,
Jim


> > > > +       if (rc < 0)
> > > > +               goto err;
> > > > +
> > > >         if (rootpath != NULL && lookup_path[0] == '\0')
> > > >                 /* this is actually the root dir of the alt root. */
> > > >                 rc = selabel_lookup_raw(fc_sehandle, &newcon, "/",
> > > > -                                                   sb->st_mode);
> > > > +                                                   stat_buf.st_mode);
> > > >         else
> > > >                 rc = selabel_lookup_raw(fc_sehandle, &newcon, lookup_path,
> > > > -                                                   sb->st_mode);
> > > > +                                                   stat_buf.st_mode);
> > > >
> > > >         if (rc < 0) {
> > > >                 if (errno == ENOENT) {
> > > > @@ -657,10 +665,10 @@ static int restorecon_sb(const char *pathname, const struct stat *sb,
> > > >                                             "Warning no default label for %s\n",
> > > >                                             lookup_path);
> > > >
> > > > -                       return 0; /* no match, but not an error */
> > > > +                       goto out; /* no match, but not an error */
> > > >                 }
> > > >
> > > > -               return -1;
> > > > +               goto err;
> > > >         }
> > > >
> > > >         if (flags->progress) {
> > > > @@ -680,19 +688,17 @@ static int restorecon_sb(const char *pathname, const struct stat *sb,
> > > >         }
> > > >
> > > >         if (flags->add_assoc) {
> > > > -               rc = filespec_add(sb->st_ino, newcon, pathname, flags);
> > > > +               rc = filespec_add(stat_buf.st_ino, newcon, pathname, flags);
> > > >
> > > >                 if (rc < 0) {
> > > >                         selinux_log(SELINUX_ERROR,
> > > >                                     "filespec_add error: %s\n", pathname);
> > > > -                       freecon(newcon);
> > > > -                       return -1;
> > > > +                       goto out1;
> > > >                 }
> > > >
> > > >                 if (rc > 0) {
> > > >                         /* Already an association and it took precedence. */
> > > > -                       freecon(newcon);
> > > > -                       return 0;
> > > > +                       goto out;
> > > >                 }
> > > >         }
> > > >
> > > > @@ -700,7 +706,7 @@ static int restorecon_sb(const char *pathname, const struct stat *sb,
> > > >                 selinux_log(SELINUX_INFO, "%s matched by %s\n",
> > > >                             pathname, newcon);
> > > >
> > > > -       if (lgetfilecon_raw(pathname, &curcon) < 0) {
> > > > +       if (fgetfilecon_raw(fd, &curcon) < 0) {
> > > >                 if (errno != ENODATA)
> > > >                         goto err;
> > > >
> > > > @@ -733,7 +739,7 @@ static int restorecon_sb(const char *pathname, const struct stat *sb,
> > > >                 }
> > > >
> > > >                 if (!flags->nochange) {
> > > > -                       if (lsetfilecon(pathname, newcon) < 0)
> > > > +                       if (fsetfilecon(fd, newcon) < 0)
> > > >                                 goto err;
> > > >                         updated = true;
> > > >                 }
> > > > @@ -758,6 +764,8 @@ static int restorecon_sb(const char *pathname, const struct stat *sb,
> > > >  out:
> > > >         rc = 0;
> > > >  out1:
> > > > +       if (fd >= 0)
> > > > +               close(fd);
> > > >         freecon(curcon);
> > > >         freecon(newcon);
> > > >         return rc;
> > > > @@ -855,7 +863,6 @@ static void *selinux_restorecon_thread(void *arg)
> > > >         FTSENT *ftsent;
> > > >         int error;
> > > >         char ent_path[PATH_MAX];
> > > > -       struct stat ent_st;
> > > >         bool first = false;
> > > >
> > > >         if (state->parallel)
> > > > @@ -953,11 +960,11 @@ loop_body:
> > > >                         /* fall through */
> > > >                 default:
> > > >                         strcpy(ent_path, ftsent->fts_path);
> > > > -                       ent_st = *ftsent->fts_statp;
> > > > +
> > > >                         if (state->parallel)
> > > >                                 pthread_mutex_unlock(&state->mutex);
> > > >
> > > > -                       error = restorecon_sb(ent_path, &ent_st, &state->flags,
> > > > +                       error = restorecon_sb(ent_path, &state->flags,
> > > >                                               first);
> > > >
> > > >                         if (state->parallel) {
> > > > @@ -1153,7 +1160,7 @@ static int selinux_restorecon_common(const char *pathname_orig,
> > > >                         goto cleanup;
> > > >                 }
> > > >
> > > > -               error = restorecon_sb(pathname, &sb, &state.flags, true);
> > > > +               error = restorecon_sb(pathname, &state.flags, true);
> > > >                 goto cleanup;
> > > >         }
> > > >
> > > > --
> > > > 2.36.1
> > > >




[Index of Archives]     [Selinux Refpolicy]     [Linux SGX]     [Fedora Users]     [Fedora Desktop]     [Yosemite Photos]     [Yosemite Camping]     [Yosemite Campsites]     [KDE Users]     [Gnome Users]

  Powered by Linux