cp: fix security context race

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.

diff --git a/NEWS b/NEWS
index 33865eb8ce48d2c8a87c13b4eebc24bb9897d73e..6350abc3cdb9ec8f3a3623e45d9293aa5b084d11 100644 (file)
--- 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 52e0aefb7ada4bfd41429fa7ebed4219d013aeff..196c7810406d7fe39bda7a5fcbe071616622bef5 100644 (file)
--- 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;