| Message ID | 20220511184225.218062-3-cgzones@googlemail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 7e979b56fd2c |
| Headers | show |
| Series | [RFC] libselinux: emulate O_PATH support in fgetfilecon/fsetfilecon | expand |
On Wed, May 11, 2022 at 3:32 PM Christian Göttsche <cgzones@googlemail.com> 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@googlemail.com> Acked-by: James Carter <jwcart2@gmail.com> > --- > 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); > + 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 >
On Thu, May 12, 2022 at 1:39 PM James Carter <jwcart2@gmail.com> wrote: > > On Wed, May 11, 2022 at 3:32 PM Christian Göttsche > <cgzones@googlemail.com> 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@googlemail.com> > > Acked-by: James Carter <jwcart2@gmail.com> > 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); > > + 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 > >
On Mon, 16 May 2022 at 19:13, James Carter <jwcart2@gmail.com> wrote: > > On Thu, May 12, 2022 at 1:39 PM James Carter <jwcart2@gmail.com> wrote: > > > > On Wed, May 11, 2022 at 3:32 PM Christian Göttsche > > <cgzones@googlemail.com> 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@googlemail.com> > > > > Acked-by: James Carter <jwcart2@gmail.com> > > > > 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 > > > + 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 > > >
On Mon, May 16, 2022 at 3:01 PM Christian Göttsche <cgzones@googlemail.com> wrote: > > On Mon, 16 May 2022 at 19:13, James Carter <jwcart2@gmail.com> wrote: > > > > On Thu, May 12, 2022 at 1:39 PM James Carter <jwcart2@gmail.com> wrote: > > > > > > On Wed, May 11, 2022 at 3:32 PM Christian Göttsche > > > <cgzones@googlemail.com> 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@googlemail.com> > > > > > > Acked-by: James Carter <jwcart2@gmail.com> > > > > > > > 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 > > > >
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); + 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; }
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@googlemail.com> --- libselinux/src/selinux_restorecon.c | 43 +++++++++++++++++------------ 1 file changed, 25 insertions(+), 18 deletions(-)