On 01/11/2026 01:44, Collin Funk wrote:
Hi Pádraig,
Pádraig Brady <[email protected]> writes:
On 31/10/2026 03:55, Collin Funk wrote:
if (specified_context == NULL)
{
- int status = (affect_symlink_referent
- ? getfileconat (fd, file, &file_context)
- : lgetfileconat (fd, file, &file_context));
-
- if (status < 0 && errno != ENODATA)
+ /* libselinux versions before 3.4 did not support O_PATH.
+ The O_PATH support works by opening the file descriptor under
+ /proc/self/fd. We could check if fsetfilecon returns EBADF and
+ do the same, but there is likely not much benefit over just
+ using O_RDONLY. */
+ fd = openat (dirfd, file,
+ (O_RDONLY | O_NOCTTY | O_NONBLOCK
+ | (affect_symlink_referent ? 0 : O_NOFOLLOW)));
I dislike the real open for a metadata update.
There may be side effects on devices etc.
or requiring read permissions where previously we did not,
and also symlinks could cause issues.
E.g. the following now fails with ELOOP:
d='testd'
mkdir -p $d
touch $d/file
ln -s file $d/link
se_type=$(stat -c %C $d/link | cut -d: -f3)
src/chcon -h -t $se_type $d/link
This is a tricky one due to the O_PATH limitation you commented on.
Though libselinux 3.4 was 2022, and missing /proc would be very
unusual on selinux systems, so it's probably fine to fall back
to the existing implementation in that case. Note I would not
fall back on any openat() failure as an attacker could be replacing
a file so you'd want to bail out to avoid that. I.e. only fall back
when fgetfilecon() returns EBADF.
Note we could fail rather than fall back if any partial option is used,
but that would be a bit too draconian for this I think.
Thanks for the input. I think I am entirely in agreement with you. I
just want to share a few things I noticed while investigating this, so
you are aware and can take them into account.
First, I think our implementation would currently fail with /proc
missing. See the current strace output before this patch:
$ mkdir -p a/b; strace chcon -R -u root a; rm -rf a
[...]
openat(AT_FDCWD, "a",
O_RDONLY|O_NOCTTY|O_NONBLOCK|O_NOFOLLOW|O_CLOEXEC|O_DIRECTORY) = 3
fstat(3, {st_mode=S_IFDIR|0755, st_size=2, ...}) = 0
fcntl(3, F_GETFL) = 0x38800 (flags
O_RDONLY|O_NONBLOCK|O_LARGEFILE|O_NOFOLLOW|O_DIRECTORY)
fcntl(3, F_SETFD, FD_CLOEXEC) = 0
fcntl(3, F_DUPFD_CLOEXEC, 3) = 4
[...]
lgetxattr("/proc/self/fd/4/b", "security.selinux",
"unconfined_u:object_r:gconf_home"..., 255) = 38
access("/var/run/setrans/.setrans-unix", F_OK) = -1 ENOENT (No such file
or directory)
futex(0x7f865971a5a0, FUTEX_WAKE_PRIVATE, 2147483647) = 0
lsetxattr("/proc/self/fd/4/b", "security.selinux",
"root:object_r:gconf_home_t:s0", 30, 0) = 0
[...]
I.e., the file descriptor functions in selinux-at.h use /proc.
If we use my patch, but with s/O_RDONLY/O_PATHSEARCH/g, then here is
what we see:
$ mkdir -p a/b; strace ./src/chcon -R -u root a; rm -rf a
[...]
openat(AT_FDCWD, "a",
O_RDONLY|O_NOCTTY|O_NONBLOCK|O_NOFOLLOW|O_CLOEXEC|O_DIRECTORY) = 3
fstat(3, {st_mode=S_IFDIR|0755, st_size=2, ...}) = 0
fcntl(3, F_GETFL) = 0x38800 (flags
O_RDONLY|O_NONBLOCK|O_LARGEFILE|O_NOFOLLOW|O_DIRECTORY)
fcntl(3, F_SETFD, FD_CLOEXEC) = 0
fcntl(3, F_DUPFD_CLOEXEC, 3) = 4
[...]
openat(4, "b", O_RDONLY|O_NOCTTY|O_NONBLOCK|O_NOFOLLOW|O_PATH) = 3
fgetxattr(3, "security.selinux", 0x3f7ca9c0, 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",
"unconfined_u:object_r:gconf_home"..., 255) = 38
access("/var/run/setrans/.setrans-unix", F_OK) = -1 ENOENT (No such file
or directory)
futex(0x7f332549b5a0, FUTEX_WAKE_PRIVATE, 2147483647) = 0
fsetxattr(3, "security.selinux", "root:object_r:gconf_home_t:s0", 30, 0) =
-1 EBADF (Bad file descriptor)
fcntl(3, F_GETFL) = 0x220000 (flags
O_RDONLY|O_NOFOLLOW|O_PATH)
setxattr("/proc/self/fd/3", "security.selinux",
"root:object_r:gconf_home_t:s0", 30, 0) = 0
I.e., when we use O_PATH, libselinux-3.4 and later will get EBADF from
fgetxattr or fsetxattr, and call getxattr or setxattr respectively on
the /proc/self/fd path associated with the file descriptor. My
understanding is that the versions before libselinux-3.4 would just
return EBADF instead of that fallback.
This also means that a portable implementation could just open a file
descriptor with O_PATH, and call getfilecon and setfilecon on the
/proc/self/fd path directly, instead of letting libselinux handle the
fallback for us. WDYT?
Yes good observations.
However this would couple the implementation to /proc unnecessarily I think.
Also isn't the libselinux /proc usage observation only the fast path.
So it seems better to use the more abstract interfaces IMHO.
I've attached code for what I was thinking.
cheers,
Padraig
diff --git a/src/chcon.c b/src/chcon.c
index 4b0e71ed3..1f41dd9bf 100644
--- a/src/chcon.c
+++ b/src/chcon.c
@@ -22,6 +22,7 @@
#include "system.h"
#include "dev-ino.h"
+#include "fcntl--.h"
#include "fts-missing.h"
#include "ignore-value.h"
#include "quote.h"
@@ -145,22 +146,52 @@ change_file_context (char const *file_full_name, int fd, char const *file,
context_t context IF_LINT (= NULL);
char const * context_string;
int errors = 0;
+ int file_fd = -1;
if (specified_context == NULL)
{
- int status = (affect_symlink_referent
- ? getfileconat (fd, file, &file_context)
- : lgetfileconat (fd, file, &file_context));
-
- if (status < 0 && errno != ENODATA)
+ int status;
+#ifdef O_PATH
+ /* Pin the file while deriving and applying a partial context.
+ O_PATH also works for symlinks and unreadable files. */
+ file_fd = openat (fd, file,
+ (O_PATH | O_CLOEXEC
+ | (affect_symlink_referent ? 0 : O_NOFOLLOW)));
+ if (file_fd < 0)
{
if (ignore_missing && ignorable_traversal_errno (errno))
return 0;
- error (0, errno, _("failed to get security context of %s"),
- quoteaf (file_full_name));
+ error (0, errno, _("failed to open %s"), quoteaf (file_full_name));
return 1;
}
+ status = fgetfilecon (file_fd, &file_context);
+ if (status < 0 && errno == EBADF)
+ {
+ /* libselinux before 3.4, or an unavailable /proc/self/fd,
+ cannot handle O_PATH descriptors, so fall back to
+ potentially racy behavior with partial contexts. */
+ close (file_fd);
+ file_fd = -1;
+ }
+#endif
+ if (file_fd < 0)
+ status = (affect_symlink_referent
+ ? getfileconat (fd, file, &file_context)
+ : lgetfileconat (fd, file, &file_context));
+
+ if (status < 0 && errno != ENODATA)
+ {
+ if (! (file_fd < 0 && ignore_missing
+ && ignorable_traversal_errno (errno)))
+ {
+ error (0, errno, _("failed to get security context of %s"),
+ quoteaf (file_full_name));
+ errors = 1;
+ }
+ goto done;
+ }
+
/* If the file doesn't have a context, and we're not setting all of
the context components, there isn't really an obvious default.
Thus, we just give up. */
@@ -168,13 +199,15 @@ change_file_context (char const *file_full_name, int fd, char const *file,
{
error (0, 0, _("can't apply partial context to unlabeled file %s"),
quoteaf (file_full_name));
- return 1;
+ errors = 1;
+ goto done;
}
if (compute_context_from_mask (file_context, &context))
{
freecon (file_context);
- return 1;
+ errors = 1;
+ goto done;
}
context_string = context_str (context);
@@ -186,13 +219,16 @@ change_file_context (char const *file_full_name, int fd, char const *file,
if (file_context == NULL || ! streq (context_string, file_context))
{
- int fail = (affect_symlink_referent
+ int fail = (0 <= file_fd
+ ? fsetfilecon (file_fd, context_string)
+ : affect_symlink_referent
? setfileconat (fd, file, context_string)
: lsetfileconat (fd, file, context_string));
if (fail)
{
- if (! (ignore_missing && ignorable_traversal_errno (errno)))
+ if (! (file_fd < 0 && ignore_missing
+ && ignorable_traversal_errno (errno)))
{
errors = 1;
error (0, errno, _("failed to change context of %s to %s"),
@@ -208,6 +244,9 @@ change_file_context (char const *file_full_name, int fd, char const *file,
freecon (file_context);
}
+ done:
+ if (0 <= file_fd)
+ close (file_fd);
return errors;
}
