I spotted a SELinux security-context race introduced by the circa-2012
fix for Bug#11100, and installed the attached patch into coreutils
master. This also gets rid of a label and goto (which is what led me to
find the issue).From d0f035fc64fb348cb092fbb6ae7e8ce76b4d82db Mon Sep 17 00:00:00 2001
From: Paul Eggert <egg...@cs.ucla.edu>
Date: Wed, 17 Nov 2021 13:22:06 -0800
Subject: [PATCH] cp: fix security context race
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
This fixes an issue introduced in the fix for Bug#11100.
* NEWS: Mention this.
* src/copy.c (copy_reg): Fix obscure bug where open-without-CREAT
failed with ENOENT and we forget to call set_process_security_ctx
before calling open-with-CREAT. Also, don’t bother to unlink
DST_NAME if open failed with ENOENT; and if unlink fails with
ENOENT, don’t consider that to be an error (someone else could
have removed the file for us, and that’s OK). Also, don’t worry
about move mode, since we use O_EXCL|O_CREAT and so won’t open
an existing file.
---
NEWS | 5 +++++
src/copy.c | 50 +++++++++++++++++++++-----------------------------
2 files changed, 26 insertions(+), 29 deletions(-)
diff --git a/NEWS b/NEWS
index 33865eb8c..6350abc3c 100644
--- a/NEWS
+++ b/NEWS
@@ -8,6 +8,11 @@ GNU coreutils NEWS -*- outline -*-
All files would be processed correctly, but the exit status was incorrect.
[bug introduced in coreutils-9.0]
+ If 'cp -Z A B' checks B's status and some other process then removes B,
+ cp no longer creates B with a too-generous SELinux security context
+ before adjusting it to the correct value.
+ [bug introduced in coreutils-8.17]
+
On macOS, 'cp A B' no longer miscopies when A is in an APFS file system
and B is in some other file system.
[bug introduced in coreutils-9.0]
diff --git a/src/copy.c b/src/copy.c
index 52e0aefb7..196c78104 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -1120,7 +1120,9 @@ infer_scantype (int fd, struct stat const *sb,
OMITTED_PERMISSIONS after copying as needed.
X provides many option settings.
Return true if successful.
- *NEW_DST is as in copy_internal.
+ *NEW_DST is initially as in copy_internal.
+ If successful, set *NEW_DST to true if the destination file was created and
+ to false otherwise; if unsuccessful, perhaps set *NEW_DST to some value.
SRC_SB is the result of calling follow_fstatat on SRC_NAME. */
static bool
@@ -1185,8 +1187,8 @@ copy_reg (char const *src_name, char const *dst_name,
the existing context according to the system default for the dest.
Note we set the context here, _after_ the file is opened, lest the
new context disallow that. */
- if ((x->set_security_context || x->preserve_security_context)
- && 0 <= dest_desc)
+ if (0 <= dest_desc
+ && (x->set_security_context || x->preserve_security_context))
{
if (! set_file_security_ctx (dst_name, false, x))
{
@@ -1198,38 +1200,45 @@ copy_reg (char const *src_name, char const *dst_name,
}
}
- if (dest_desc < 0 && x->unlink_dest_after_failed_open)
+ if (dest_desc < 0 && dest_errno != ENOENT
+ && x->unlink_dest_after_failed_open)
{
- if (unlink (dst_name) != 0)
+ if (unlink (dst_name) == 0)
+ {
+ if (x->verbose)
+ printf (_("removed %s\n"), quoteaf (dst_name));
+ }
+ else if (errno != ENOENT)
{
error (0, errno, _("cannot remove %s"), quoteaf (dst_name));
return_val = false;
goto close_src_desc;
}
- if (x->verbose)
- printf (_("removed %s\n"), quoteaf (dst_name));
- /* Tell caller that the destination file was unlinked. */
- *new_dst = true;
+ dest_errno = ENOENT;
+ }
+ if (dest_desc < 0 && dest_errno == ENOENT)
+ {
/* Ensure there is no race where a file may be left without
an appropriate security context. */
if (x->set_security_context)
{
if (! set_process_security_ctx (src_name, dst_name, dst_mode,
- *new_dst, x))
+ true, x))
{
return_val = false;
goto close_src_desc;
}
}
+
+ /* Tell caller that the destination file is created. */
+ *new_dst = true;
}
}
if (*new_dst)
{
- open_with_O_CREAT:;
-
int open_flags = O_WRONLY | O_CREAT | O_BINARY;
dest_desc = open (dst_name, open_flags | O_EXCL,
dst_mode & ~omitted_permissions);
@@ -1280,23 +1289,6 @@ copy_reg (char const *src_name, char const *dst_name,
if (dest_desc < 0)
{
- /* If we've just failed due to ENOENT for an ostensibly preexisting
- destination (*new_dst was 0), that's a bit of a contradiction/race:
- the prior stat/lstat said the file existed (*new_dst was 0), yet
- the subsequent open-existing-file failed with ENOENT. With NFS,
- the race window is wider still, since its meta-data caching tends
- to make the stat succeed for a just-removed remote file, while the
- more-definitive initial open call will fail with ENOENT. When this
- situation arises, we attempt to open again, but this time with
- O_CREAT. Do this only when not in move-mode, since when handling
- a cross-device move, we must never open an existing destination. */
- if (dest_errno == ENOENT && ! *new_dst && ! x->move_mode)
- {
- *new_dst = 1;
- goto open_with_O_CREAT;
- }
-
- /* Otherwise, it's an error. */
error (0, dest_errno, _("cannot create regular file %s"),
quoteaf (dst_name));
return_val = false;
--
2.32.0
