diff mbox series

Revert "libselinux: restorecon: pin file to avoid TOCTOU issues"

Message ID 20220531125750.278211-1-plautrba@redhat.com (mailing list archive)
State Accepted
Commit de285252a180
Headers show
Series Revert "libselinux: restorecon: pin file to avoid TOCTOU issues" | expand

Commit Message

Petr Lautrbach May 31, 2022, 12:57 p.m. UTC
This reverts commit 7e979b56fd2cee28f647376a7233d2ac2d12ca50.

The reverted commit broke `setfiles` when it's run from a chroot
without /proc mounted, e.g.

    # chroot /mnt/sysimage

    chroot# setfiles -e /proc -e /sys /sys /etc/selinux/targeted/contexts/files/file_contexts /
    [strace]
    openat(AT_FDCWD, "/", O_RDONLY|O_EXCL|O_NOFOLLOW|O_PATH) = 3
    newfstatat(3, "", {st_mode=S_IFDIR|0555, st_size=4096, ...}, AT_EMPTY_PATH) = 0
    mmap(NULL, 2101248, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f1697c91000
    fgetxattr(3, "security.selinux", 0x55be8881d3f0, 255) = -1 EBADF (Bad file descriptor)
    fcntl(3, F_GETFL)                       = 0x220000 (flags O_RDONLY|O_NOFOLLOW|O_PATH)
    getxattr("/proc/self/fd/3", "security.selinux", 0x55be8881d3f0, 255) = -1 ENOENT (No such file or directory)
    [/strace]
    setfiles: Could not set context for /:  No such file or directory

Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
---
 libselinux/src/selinux_restorecon.c | 43 ++++++++++++-----------------
 1 file changed, 18 insertions(+), 25 deletions(-)

Comments

James Carter June 1, 2022, 6:14 p.m. UTC | #1
On Wed, Jun 1, 2022 at 12:39 PM Petr Lautrbach <plautrba@redhat.com> wrote:
>
> This reverts commit 7e979b56fd2cee28f647376a7233d2ac2d12ca50.
>
> The reverted commit broke `setfiles` when it's run from a chroot
> without /proc mounted, e.g.
>
>     # chroot /mnt/sysimage
>
>     chroot# setfiles -e /proc -e /sys /sys /etc/selinux/targeted/contexts/files/file_contexts /
>     [strace]
>     openat(AT_FDCWD, "/", O_RDONLY|O_EXCL|O_NOFOLLOW|O_PATH) = 3
>     newfstatat(3, "", {st_mode=S_IFDIR|0555, st_size=4096, ...}, AT_EMPTY_PATH) = 0
>     mmap(NULL, 2101248, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f1697c91000
>     fgetxattr(3, "security.selinux", 0x55be8881d3f0, 255) = -1 EBADF (Bad file descriptor)
>     fcntl(3, F_GETFL)                       = 0x220000 (flags O_RDONLY|O_NOFOLLOW|O_PATH)
>     getxattr("/proc/self/fd/3", "security.selinux", 0x55be8881d3f0, 255) = -1 ENOENT (No such file or directory)
>     [/strace]
>     setfiles: Could not set context for /:  No such file or directory
>
> Signed-off-by: Petr Lautrbach <plautrba@redhat.com>

Acked-by: James Carter <jwcart2@gmail.com>

> ---
>  libselinux/src/selinux_restorecon.c | 43 ++++++++++++-----------------
>  1 file changed, 18 insertions(+), 25 deletions(-)
>
> diff --git a/libselinux/src/selinux_restorecon.c b/libselinux/src/selinux_restorecon.c
> index 9dd6be817832..9f5b326c19ec 100644
> --- a/libselinux/src/selinux_restorecon.c
> +++ b/libselinux/src/selinux_restorecon.c
> @@ -623,13 +623,13 @@ out:
>         return rc;
>  }
>
> -static int restorecon_sb(const char *pathname, struct rest_flags *flags, bool first)
> +static int restorecon_sb(const char *pathname, const struct stat *sb,
> +                           struct rest_flags *flags, bool first)
>  {
>         char *newcon = NULL;
>         char *curcon = NULL;
>         char *newtypecon = NULL;
> -       int fd = -1, rc;
> -       struct stat stat_buf;
> +       int rc;
>         bool updated = false;
>         const char *lookup_path = pathname;
>         float pc;
> @@ -644,21 +644,13 @@ static int restorecon_sb(const char *pathname, struct rest_flags *flags, bool fi
>                 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, "/",
> -                                                   stat_buf.st_mode);
> +                                                   sb->st_mode);
>         else
>                 rc = selabel_lookup_raw(fc_sehandle, &newcon, lookup_path,
> -                                                   stat_buf.st_mode);
> +                                                   sb->st_mode);
>
>         if (rc < 0) {
>                 if (errno == ENOENT) {
> @@ -667,10 +659,10 @@ static int restorecon_sb(const char *pathname, struct rest_flags *flags, bool fi
>                                             "Warning no default label for %s\n",
>                                             lookup_path);
>
> -                       goto out; /* no match, but not an error */
> +                       return 0; /* no match, but not an error */
>                 }
>
> -               goto err;
> +               return -1;
>         }
>
>         if (flags->progress) {
> @@ -690,17 +682,19 @@ static int restorecon_sb(const char *pathname, struct rest_flags *flags, bool fi
>         }
>
>         if (flags->add_assoc) {
> -               rc = filespec_add(stat_buf.st_ino, newcon, pathname, flags);
> +               rc = filespec_add(sb->st_ino, newcon, pathname, flags);
>
>                 if (rc < 0) {
>                         selinux_log(SELINUX_ERROR,
>                                     "filespec_add error: %s\n", pathname);
> -                       goto out1;
> +                       freecon(newcon);
> +                       return -1;
>                 }
>
>                 if (rc > 0) {
>                         /* Already an association and it took precedence. */
> -                       goto out;
> +                       freecon(newcon);
> +                       return 0;
>                 }
>         }
>
> @@ -708,7 +702,7 @@ static int restorecon_sb(const char *pathname, struct rest_flags *flags, bool fi
>                 selinux_log(SELINUX_INFO, "%s matched by %s\n",
>                             pathname, newcon);
>
> -       if (fgetfilecon_raw(fd, &curcon) < 0) {
> +       if (lgetfilecon_raw(pathname, &curcon) < 0) {
>                 if (errno != ENODATA)
>                         goto err;
>
> @@ -741,7 +735,7 @@ static int restorecon_sb(const char *pathname, struct rest_flags *flags, bool fi
>                 }
>
>                 if (!flags->nochange) {
> -                       if (fsetfilecon(fd, newcon) < 0)
> +                       if (lsetfilecon(pathname, newcon) < 0)
>                                 goto err;
>                         updated = true;
>                 }
> @@ -766,8 +760,6 @@ static int restorecon_sb(const char *pathname, struct rest_flags *flags, bool fi
>  out:
>         rc = 0;
>  out1:
> -       if (fd >= 0)
> -               close(fd);
>         freecon(curcon);
>         freecon(newcon);
>         return rc;
> @@ -865,6 +857,7 @@ 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)
> @@ -962,11 +955,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, &state->flags,
> +                       error = restorecon_sb(ent_path, &ent_st, &state->flags,
>                                               first);
>
>                         if (state->parallel) {
> @@ -1162,7 +1155,7 @@ static int selinux_restorecon_common(const char *pathname_orig,
>                         goto cleanup;
>                 }
>
> -               error = restorecon_sb(pathname, &state.flags, true);
> +               error = restorecon_sb(pathname, &sb, &state.flags, true);
>                 goto cleanup;
>         }
>
> --
> 2.36.1
>
James Carter June 2, 2022, 1:04 p.m. UTC | #2
On Wed, Jun 1, 2022 at 2:14 PM James Carter <jwcart2@gmail.com> wrote:
>
> On Wed, Jun 1, 2022 at 12:39 PM Petr Lautrbach <plautrba@redhat.com> wrote:
> >
> > This reverts commit 7e979b56fd2cee28f647376a7233d2ac2d12ca50.
> >
> > The reverted commit broke `setfiles` when it's run from a chroot
> > without /proc mounted, e.g.
> >
> >     # chroot /mnt/sysimage
> >
> >     chroot# setfiles -e /proc -e /sys /sys /etc/selinux/targeted/contexts/files/file_contexts /
> >     [strace]
> >     openat(AT_FDCWD, "/", O_RDONLY|O_EXCL|O_NOFOLLOW|O_PATH) = 3
> >     newfstatat(3, "", {st_mode=S_IFDIR|0555, st_size=4096, ...}, AT_EMPTY_PATH) = 0
> >     mmap(NULL, 2101248, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f1697c91000
> >     fgetxattr(3, "security.selinux", 0x55be8881d3f0, 255) = -1 EBADF (Bad file descriptor)
> >     fcntl(3, F_GETFL)                       = 0x220000 (flags O_RDONLY|O_NOFOLLOW|O_PATH)
> >     getxattr("/proc/self/fd/3", "security.selinux", 0x55be8881d3f0, 255) = -1 ENOENT (No such file or directory)
> >     [/strace]
> >     setfiles: Could not set context for /:  No such file or directory
> >
> > Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
>
> Acked-by: James Carter <jwcart2@gmail.com>
>

Merged.
Thanks,
Jim

> > ---
> >  libselinux/src/selinux_restorecon.c | 43 ++++++++++++-----------------
> >  1 file changed, 18 insertions(+), 25 deletions(-)
> >
> > diff --git a/libselinux/src/selinux_restorecon.c b/libselinux/src/selinux_restorecon.c
> > index 9dd6be817832..9f5b326c19ec 100644
> > --- a/libselinux/src/selinux_restorecon.c
> > +++ b/libselinux/src/selinux_restorecon.c
> > @@ -623,13 +623,13 @@ out:
> >         return rc;
> >  }
> >
> > -static int restorecon_sb(const char *pathname, struct rest_flags *flags, bool first)
> > +static int restorecon_sb(const char *pathname, const struct stat *sb,
> > +                           struct rest_flags *flags, bool first)
> >  {
> >         char *newcon = NULL;
> >         char *curcon = NULL;
> >         char *newtypecon = NULL;
> > -       int fd = -1, rc;
> > -       struct stat stat_buf;
> > +       int rc;
> >         bool updated = false;
> >         const char *lookup_path = pathname;
> >         float pc;
> > @@ -644,21 +644,13 @@ static int restorecon_sb(const char *pathname, struct rest_flags *flags, bool fi
> >                 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, "/",
> > -                                                   stat_buf.st_mode);
> > +                                                   sb->st_mode);
> >         else
> >                 rc = selabel_lookup_raw(fc_sehandle, &newcon, lookup_path,
> > -                                                   stat_buf.st_mode);
> > +                                                   sb->st_mode);
> >
> >         if (rc < 0) {
> >                 if (errno == ENOENT) {
> > @@ -667,10 +659,10 @@ static int restorecon_sb(const char *pathname, struct rest_flags *flags, bool fi
> >                                             "Warning no default label for %s\n",
> >                                             lookup_path);
> >
> > -                       goto out; /* no match, but not an error */
> > +                       return 0; /* no match, but not an error */
> >                 }
> >
> > -               goto err;
> > +               return -1;
> >         }
> >
> >         if (flags->progress) {
> > @@ -690,17 +682,19 @@ static int restorecon_sb(const char *pathname, struct rest_flags *flags, bool fi
> >         }
> >
> >         if (flags->add_assoc) {
> > -               rc = filespec_add(stat_buf.st_ino, newcon, pathname, flags);
> > +               rc = filespec_add(sb->st_ino, newcon, pathname, flags);
> >
> >                 if (rc < 0) {
> >                         selinux_log(SELINUX_ERROR,
> >                                     "filespec_add error: %s\n", pathname);
> > -                       goto out1;
> > +                       freecon(newcon);
> > +                       return -1;
> >                 }
> >
> >                 if (rc > 0) {
> >                         /* Already an association and it took precedence. */
> > -                       goto out;
> > +                       freecon(newcon);
> > +                       return 0;
> >                 }
> >         }
> >
> > @@ -708,7 +702,7 @@ static int restorecon_sb(const char *pathname, struct rest_flags *flags, bool fi
> >                 selinux_log(SELINUX_INFO, "%s matched by %s\n",
> >                             pathname, newcon);
> >
> > -       if (fgetfilecon_raw(fd, &curcon) < 0) {
> > +       if (lgetfilecon_raw(pathname, &curcon) < 0) {
> >                 if (errno != ENODATA)
> >                         goto err;
> >
> > @@ -741,7 +735,7 @@ static int restorecon_sb(const char *pathname, struct rest_flags *flags, bool fi
> >                 }
> >
> >                 if (!flags->nochange) {
> > -                       if (fsetfilecon(fd, newcon) < 0)
> > +                       if (lsetfilecon(pathname, newcon) < 0)
> >                                 goto err;
> >                         updated = true;
> >                 }
> > @@ -766,8 +760,6 @@ static int restorecon_sb(const char *pathname, struct rest_flags *flags, bool fi
> >  out:
> >         rc = 0;
> >  out1:
> > -       if (fd >= 0)
> > -               close(fd);
> >         freecon(curcon);
> >         freecon(newcon);
> >         return rc;
> > @@ -865,6 +857,7 @@ 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)
> > @@ -962,11 +955,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, &state->flags,
> > +                       error = restorecon_sb(ent_path, &ent_st, &state->flags,
> >                                               first);
> >
> >                         if (state->parallel) {
> > @@ -1162,7 +1155,7 @@ static int selinux_restorecon_common(const char *pathname_orig,
> >                         goto cleanup;
> >                 }
> >
> > -               error = restorecon_sb(pathname, &state.flags, true);
> > +               error = restorecon_sb(pathname, &sb, &state.flags, true);
> >                 goto cleanup;
> >         }
> >
> > --
> > 2.36.1
> >
diff mbox series

Patch

diff --git a/libselinux/src/selinux_restorecon.c b/libselinux/src/selinux_restorecon.c
index 9dd6be817832..9f5b326c19ec 100644
--- a/libselinux/src/selinux_restorecon.c
+++ b/libselinux/src/selinux_restorecon.c
@@ -623,13 +623,13 @@  out:
 	return rc;
 }
 
-static int restorecon_sb(const char *pathname, struct rest_flags *flags, bool first)
+static int restorecon_sb(const char *pathname, const struct stat *sb,
+			    struct rest_flags *flags, bool first)
 {
 	char *newcon = NULL;
 	char *curcon = NULL;
 	char *newtypecon = NULL;
-	int fd = -1, rc;
-	struct stat stat_buf;
+	int rc;
 	bool updated = false;
 	const char *lookup_path = pathname;
 	float pc;
@@ -644,21 +644,13 @@  static int restorecon_sb(const char *pathname, struct rest_flags *flags, bool fi
 		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, "/",
-						    stat_buf.st_mode);
+						    sb->st_mode);
 	else
 		rc = selabel_lookup_raw(fc_sehandle, &newcon, lookup_path,
-						    stat_buf.st_mode);
+						    sb->st_mode);
 
 	if (rc < 0) {
 		if (errno == ENOENT) {
@@ -667,10 +659,10 @@  static int restorecon_sb(const char *pathname, struct rest_flags *flags, bool fi
 					    "Warning no default label for %s\n",
 					    lookup_path);
 
-			goto out; /* no match, but not an error */
+			return 0; /* no match, but not an error */
 		}
 
-		goto err;
+		return -1;
 	}
 
 	if (flags->progress) {
@@ -690,17 +682,19 @@  static int restorecon_sb(const char *pathname, struct rest_flags *flags, bool fi
 	}
 
 	if (flags->add_assoc) {
-		rc = filespec_add(stat_buf.st_ino, newcon, pathname, flags);
+		rc = filespec_add(sb->st_ino, newcon, pathname, flags);
 
 		if (rc < 0) {
 			selinux_log(SELINUX_ERROR,
 				    "filespec_add error: %s\n", pathname);
-			goto out1;
+			freecon(newcon);
+			return -1;
 		}
 
 		if (rc > 0) {
 			/* Already an association and it took precedence. */
-			goto out;
+			freecon(newcon);
+			return 0;
 		}
 	}
 
@@ -708,7 +702,7 @@  static int restorecon_sb(const char *pathname, struct rest_flags *flags, bool fi
 		selinux_log(SELINUX_INFO, "%s matched by %s\n",
 			    pathname, newcon);
 
-	if (fgetfilecon_raw(fd, &curcon) < 0) {
+	if (lgetfilecon_raw(pathname, &curcon) < 0) {
 		if (errno != ENODATA)
 			goto err;
 
@@ -741,7 +735,7 @@  static int restorecon_sb(const char *pathname, struct rest_flags *flags, bool fi
 		}
 
 		if (!flags->nochange) {
-			if (fsetfilecon(fd, newcon) < 0)
+			if (lsetfilecon(pathname, newcon) < 0)
 				goto err;
 			updated = true;
 		}
@@ -766,8 +760,6 @@  static int restorecon_sb(const char *pathname, struct rest_flags *flags, bool fi
 out:
 	rc = 0;
 out1:
-	if (fd >= 0)
-		close(fd);
 	freecon(curcon);
 	freecon(newcon);
 	return rc;
@@ -865,6 +857,7 @@  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)
@@ -962,11 +955,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, &state->flags,
+			error = restorecon_sb(ent_path, &ent_st, &state->flags,
 					      first);
 
 			if (state->parallel) {
@@ -1162,7 +1155,7 @@  static int selinux_restorecon_common(const char *pathname_orig,
 			goto cleanup;
 		}
 
-		error = restorecon_sb(pathname, &state.flags, true);
+		error = restorecon_sb(pathname, &sb, &state.flags, true);
 		goto cleanup;
 	}