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