* [PATCH] Revert "libselinux: restorecon: pin file to avoid TOCTOU issues"
@ 2022-05-31 12:57 Petr Lautrbach
2022-06-01 18:14 ` James Carter
0 siblings, 1 reply; 3+ messages in thread
From: Petr Lautrbach @ 2022-05-31 12:57 UTC (permalink / raw)
To: selinux; +Cc: Petr Lautrbach, Christian Göttsche
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(-)
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] Revert "libselinux: restorecon: pin file to avoid TOCTOU issues"
2022-05-31 12:57 [PATCH] Revert "libselinux: restorecon: pin file to avoid TOCTOU issues" Petr Lautrbach
@ 2022-06-01 18:14 ` James Carter
2022-06-02 13:04 ` James Carter
0 siblings, 1 reply; 3+ messages in thread
From: James Carter @ 2022-06-01 18:14 UTC (permalink / raw)
To: Petr Lautrbach; +Cc: SElinux list, Christian Göttsche
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
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] Revert "libselinux: restorecon: pin file to avoid TOCTOU issues"
2022-06-01 18:14 ` James Carter
@ 2022-06-02 13:04 ` James Carter
0 siblings, 0 replies; 3+ messages in thread
From: James Carter @ 2022-06-02 13:04 UTC (permalink / raw)
To: Petr Lautrbach; +Cc: SElinux list, Christian Göttsche
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
> >
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-06-02 13:05 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-31 12:57 [PATCH] Revert "libselinux: restorecon: pin file to avoid TOCTOU issues" Petr Lautrbach
2022-06-01 18:14 ` James Carter
2022-06-02 13:04 ` James Carter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.