bug#11100: Racy code in copy.c

[Jim Meyering]

On Thu, Nov 18, 2021 at 8:38 AM Paul Eggert  wrote:
> 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).

Nice! Thanks for finding and fixing my old bug.

bug#11100: Racy code in copy.c

[Paul Eggert]

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 
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 

bug#11100: Racy code in copy.c

[Philipp Thomas]

* Jim Meyering (j...@meyering.net) [20120504 17:30]:

 If there's a bugzilla reference for this, let me know
 and I'll add it to the commit log.

There is, but as it's a SLES bug it's only open for SUSE employees and
customers and thus useless for a coreutils commit log. I'll instead
reference the commit from said bug report.

Philipp

bug#11100: Racy code in copy.c

[Jim Meyering]

Philipp Thomas wrote:
 * Jim Meyering (j...@meyering.net) [20120504 17:30]:

 If there's a bugzilla reference for this, let me know
 and I'll add it to the commit log.

 There is, but as it's a SLES bug it's only open for SUSE employees and
 customers and thus useless for a coreutils commit log. I'll instead
 reference the commit from said bug report.

Ok.  I've pushed that change.

bug#11100: Racy code in copy.c

[Jim Meyering]

Jim Meyering wrote:
 Philipp Thomas wrote:
 * Jim Meyering (j...@meyering.net) [20120328 18:09]:

 At first glance, that might be reasonable: the additional open
 is incurred only after a failed stat.
 I'll look more closely in a week or two if no one else investigates.

 Ping, it's been more than two weeks and I'm being bugged for a possible
 solution and will refrain from making changes that aren't accepted upstream.

 Thanks for the reminder.
 I did investigate it back then, but didn't make any progress.
 Today I looked again and saw the light ;-)

 Here's a partial patch.
 Still to come: a NEWS addition and a test case.

Here's that same patch, along with a glibc-specific LD_PRELOAD-based
test and a NEWS entry.

From d8a631cb1a08ee73bde061d36b068585358ff6fe Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Fri, 4 May 2012 16:42:31 +0200
Subject: [PATCH 1/2] cp: handle a race condition more sensibly

* src/copy.c (copy_reg): In a narrow race (stat sees dest, yet
open-without-O_CREAT fails with ENOENT), retry the open with O_CREAT.
Reported by Philipp Thomas and Neil F. Brown in
http://bugs.gnu.org/11100
---
 THANKS.in  |  1 +
 src/copy.c | 19 +++
 2 files changed, 20 insertions(+)

diff --git a/THANKS.in b/THANKS.in
index a7403fd..d8f7a4b 100644
--- a/THANKS.in
+++ b/THANKS.in
@@ -489,6 +489,7 @@ Phil Richards   
phil.richa...@vf.vodafone.co.uk
 Philippe De Muyter  p...@macqel.be
 Philippe Schnoebelenphilippe.schnoebe...@imag.fr
 Phillip Jones   mo...@datastacks.com
+Philipp Thomas  p...@suse.de
 Piergiorgio Sartor  sar...@sony.de
 Pieter Bowman   bow...@math.utah.edu
 Piotr Gackiewiczga...@intertele.pl
diff --git a/src/copy.c b/src/copy.c
index 844ebcd..2558fea 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -892,6 +892,8 @@ copy_reg (char const *src_name, char const *dst_name,

   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);
@@ -942,6 +944,23 @@ 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),
  quote (dst_name));
   return_val = false;
--
1.7.10.1.457.g8275905


From 59bee5a6bfac96bba6684e740fdf35063b4461a8 Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Sat, 5 May 2012 21:58:13 +0200
Subject: [PATCH 2/2] tests: test for just-fixed cp change

* tests/cp/nfs-removal-race: New file.
* tests/Makefile.am (TESTS): Add it.
* NEWS (Bug fixes): Mention it.
---
 NEWS  |  6 
 tests/Makefile.am |  1 +
 tests/cp/nfs-removal-race | 70 +++
 3 files changed, 77 insertions(+)
 create mode 100755 tests/cp/nfs-removal-race

diff --git a/NEWS b/NEWS
index 1c00d96..5e32f79 100644
--- a/NEWS
+++ b/NEWS
@@ -11,6 +11,12 @@ GNU coreutils NEWS-*- 
outline -*-
   changed, the new group ID would be listed, even though it is not
   yet effective.

+  cp S D is no longer subject to a race: if D were removed between the
+  initial stat and subsequent open-without-O_CREATE, cp would fail with
+  a confusing diagnostic saying that the destination, D, was not found.
+  Now, in this unusual case, it retries the open (but with O_CREATE),
+  and hence usually succeeds.
+
 ** New features

   fmt now accepts the --goal=WIDTH (-g) option.
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 72717e3..ca19051 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -349,6 +349,7 @@ TESTS = \
   cp/link-no-deref \
   cp/link-preserve \
   cp/link-symlink  \
+  cp/nfs-removal-race

bug#11100: Racy code in copy.c

[Jim Meyering]

Pádraig Brady wrote:
 I can't think of any issue with this.
 Code looks good.
 Test triggers the new condition.

Thanks for the review.
I've squashed the test-adding commit onto the fix, added this sentence
to NEWS:

 With NFS attribute caching, the condition
 was particularly easy to trigger, since there, the removal of D
 could precede the initial stat.

I was about to push, then wondered...
I'd been assuming that the Neil Brown ne...@cse.unsw.edu.au
already listed THANKS.in is the same as the Neil F. Brown at SuSE.
We know that assuming is bad, so I checked linux's git log, which
appears to confirm.  So I have updated Neil's name (to add the F.)
and email, too.

bug#11100: Racy code in copy.c

[Philipp Thomas]

* Jim Meyering (j...@meyering.net) [20120328 18:09]:

 At first glance, that might be reasonable: the additional open
 is incurred only after a failed stat.
 I'll look more closely in a week or two if no one else investigates.

Ping, it's been more than two weeks and I'm being bugged for a possible
solution and will refrain from making changes that aren't accepted upstream.

Philipp

bug#11100: Racy code in copy.c

[Jim Meyering]

Philipp Thomas wrote:
 * Jim Meyering (j...@meyering.net) [20120328 18:09]:

 At first glance, that might be reasonable: the additional open
 is incurred only after a failed stat.
 I'll look more closely in a week or two if no one else investigates.

 Ping, it's been more than two weeks and I'm being bugged for a possible
 solution and will refrain from making changes that aren't accepted upstream.

Thanks for the reminder.
I did investigate it back then, but didn't make any progress.
Today I looked again and saw the light ;-)

Here's a partial patch.
Still to come: a NEWS addition and a test case.

From b85446f612846ec9c99fe3e13dd78e861428ddde Mon Sep 17 00:00:00 2001
From: Jim Meyering meyer...@redhat.com
Date: Fri, 4 May 2012 16:42:31 +0200
Subject: [PATCH] cp: handle a race condition more sensibly

* src/copy.c (copy_reg): In a narrow race (stat sees dest, yet
open-without-O_CREAT fails with ENOENT), retry the open with O_CREAT.
Reported by Philipp Thomas and Neil F. Brown in
http://debbugs.gnu.org/11100
---
 src/copy.c | 19 +++
 1 file changed, 19 insertions(+)

diff --git a/src/copy.c b/src/copy.c
index 844ebcd..2558fea 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -892,6 +892,8 @@ copy_reg (char const *src_name, char const *dst_name,

   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);
@@ -942,6 +944,23 @@ 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),
  quote (dst_name));
   return_val = false;
--
1.7.10.1.456.g16798d0

bug#11100: Racy code in copy.c

[Eric Blake]

On 05/04/2012 08:47 AM, Jim Meyering wrote:
 
 * src/copy.c (copy_reg): In a narrow race (stat sees dest, yet
 open-without-O_CREAT fails with ENOENT), retry the open with O_CREAT.
 Reported by Philipp Thomas and Neil F. Brown in
 http://debbugs.gnu.org/11100

Question - when we retry with adding O_CREAT, should we also add O_EXCL?
 That is, we already lost the race once, but the O_EXCL will ensure that
we don't lose the race a second time and that we really are creating a file.

-- 
Eric Blake   ebl...@redhat.com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

bug#11100: Racy code in copy.c

[Jim Meyering]

Eric Blake wrote:

 On 05/04/2012 08:47 AM, Jim Meyering wrote:

 * src/copy.c (copy_reg): In a narrow race (stat sees dest, yet
 open-without-O_CREAT fails with ENOENT), retry the open with O_CREAT.
 Reported by Philipp Thomas and Neil F. Brown in
 http://debbugs.gnu.org/11100

 Question - when we retry with adding O_CREAT, should we also add O_EXCL?
  That is, we already lost the race once, but the O_EXCL will ensure that
 we don't lose the race a second time and that we really are creating a file.

I was concerned about that, too, but noted
it's already always done in the O_CREAT/*new_dst path:

  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);

bug#11100: Racy code in copy.c

[Jim Meyering]

Eric Blake wrote:
 On 05/04/2012 08:47 AM, Jim Meyering wrote:

 * src/copy.c (copy_reg): In a narrow race (stat sees dest, yet
 open-without-O_CREAT fails with ENOENT), retry the open with O_CREAT.
 Reported by Philipp Thomas and Neil F. Brown in
 http://debbugs.gnu.org/11100

 Question - when we retry with adding O_CREAT, should we also add O_EXCL?
  That is, we already lost the race once, but the O_EXCL will ensure that
 we don't lose the race a second time and that we really are creating a file.

And thanks for looking.

bug#11100: Racy code in copy.c

[Jim Meyering]

NeilBrown wrote:
 On Tue, 27 Mar 2012 15:40:25 +0200 Jim Meyering j...@meyering.net wrote:

 Philipp Thomas wrote:
  I'd like to pass on observations from my collegue Neil Brown:
 
  in src/copy.c, copy_reg() is passed bool *new_dst.
 
  This is 'false' if the file already exists, in which case it attempts to
  open the file with O_WRONLY | O_TRUNC | O_BINARY.
  If it is 'true', only then does it use O_CREAT (and others).
 
  Somewhere up the call chain - I'm not sure where - new_dst is set if 'stat'
  on the file succeeds.  The above mentioned code assumes that the file still
  exists.  This is racy - particularly for NFS where deletions from other
  clients can take a while to appear.

 Thanks for the report.
 However, much of what cp does is mandated by the POSIX spec, including
 that inevitable-looking (POSIX-mandated) TOCTOU race.  Here's part of that
 spec, from http://pubs.opengroup.org/onlinepubs/9699919799/utilities/cp.html:
...
 If you can find a way to make cp work sensibly in your specific case,
 yet without impacting any other use case, please let us know.

 The above doesn't specify how you determine if the dest file exists.
 You could do this with stat plus open(O_WRONLY).
 Only try the open on REG files.

It seems that any change here would be working around the non-POSIX
nature of NFS: note that POSIX seems clear[*] that stat must tell us
whether a file exists (barring additional or alternate file access
control mechanisms which aren't an issue here).

I.e., this is an RFE: avoid NFS races due to inherent
POSIX-non-compliance of NFS, rather than a bug.

if ((use_stat
 -   ? stat (dst_name, dst_sb)
 +   ? (stat (dst_name, dst_sb)  0 ? -1 :
 +   (fd = open (dst_name, O_WRONLY))  0 ? -1 : 0)
 : lstat (dst_name, dst_sb))
!= 0)

At first glance, that might be reasonable: the additional open
is incurred only after a failed stat.
I'll look more closely in a week or two if no one else investigates.

One thing that would help a lot is a test case (even using gdb, strace,
stap, inotify, fuse, etc.) that consistently triggers the failure without
the need for an NFS set-up.

 If the stat fails, or the open fails with ENOENT, then the file doesn't
 exist, otherwise assume it does.
 Keep the file descriptor around (if there is one) and then the actions
 equivalent to open() ... would be
if (fd  0)
  fd = open(pathname, O_WRONLY | O_TRUNC);
else
  ftruncate(fd, 0);


 I started making a patch - but it got messy quickly.  Too many conditional
 calls to close().
 And at over 1000 lines, copy_internal was too big for me to work with :-(

[*] excerpt from a possibly dated spec:

The stat() function shall obtain information about the named file and
write it to the area pointed to by the buf argument. The path argument
points to a pathname naming a file. Read, write, or execute permission
of the named file is not required. An implementation that provides
additional or alternate file access control mechanisms may, under
implementation-defined conditions, cause stat() to fail. In particular,
the system may deny the existence of the file specified by path.

If the named file is a symbolic link, the stat() function shall
continue pathname resolution using the contents of the symbolic link,
and shall return information pertaining to the resulting file if the file
exists. The buf argument is a pointer to a stat structure, as defined
in the sys/stat.h header, into which information is placed concerning
the file. The stat() function shall update any time-related fields
(as described in XBD Section 4.8, on page 109), before writing into the
stat structure.

If the named file is a shared memory object, ...

If the named file is a typed memory object, ...

bug#11100: Racy code in copy.c

[NeilBrown]

On Wed, 28 Mar 2012 18:07:51 +0200 Jim Meyering j...@meyering.net wrote:

 NeilBrown wrote:
  On Tue, 27 Mar 2012 15:40:25 +0200 Jim Meyering j...@meyering.net wrote:
 
  Philipp Thomas wrote:
   I'd like to pass on observations from my collegue Neil Brown:
  
   in src/copy.c, copy_reg() is passed bool *new_dst.
  
   This is 'false' if the file already exists, in which case it attempts to
   open the file with O_WRONLY | O_TRUNC | O_BINARY.
   If it is 'true', only then does it use O_CREAT (and others).
  
   Somewhere up the call chain - I'm not sure where - new_dst is set if 
   'stat'
   on the file succeeds.  The above mentioned code assumes that the file 
   still
   exists.  This is racy - particularly for NFS where deletions from other
   clients can take a while to appear.
 
  Thanks for the report.
  However, much of what cp does is mandated by the POSIX spec, including
  that inevitable-looking (POSIX-mandated) TOCTOU race.  Here's part of that
  spec, from 
  http://pubs.opengroup.org/onlinepubs/9699919799/utilities/cp.html:
 ...
  If you can find a way to make cp work sensibly in your specific case,
  yet without impacting any other use case, please let us know.
 
  The above doesn't specify how you determine if the dest file exists.
  You could do this with stat plus open(O_WRONLY).
  Only try the open on REG files.
 
 It seems that any change here would be working around the non-POSIX
 nature of NFS: note that POSIX seems clear[*] that stat must tell us
 whether a file exists (barring additional or alternate file access
 control mechanisms which aren't an issue here).

stat() cannot tell whether a file exists - only whether it existed very
recently.  open() can ensure that the file still exists - though there is no
guarantee that the name still exists...

 
 I.e., this is an RFE: avoid NFS races due to inherent
 POSIX-non-compliance of NFS, rather than a bug.

Maybe .

Suppose a 'cp' and an 'rm' are racing.  i.e you cp to 'foo' at much the same
time that someone else does 'rm foo'.
What results are valid?
Certain it is valid for both to succeed and 'foo' not to exist if 'rm foo'
happened last.
Similarly it is valid for both to succeed and foo to be a newly created file
if 'cp' happened last.
But is it valid for 'rm' to succeed and 'cp' to report that it couldn't
create the target because no such file or directory ?

I would suggest not, though I doubt the spec is at all clear on this.
However that is what the current code allows.

NFS doesn't exactly introduce new behaviour, it just makes a particular race
condition much easier to hit.

So I'll not press that it is a bug in cp, but I would suggest that it is an
opportunity to improve behaviour in a corner-case.


 
 if ((use_stat
  -   ? stat (dst_name, dst_sb)
  +   ? (stat (dst_name, dst_sb)  0 ? -1 :
  + (fd = open (dst_name, O_WRONLY))  0 ? -1 : 0)
  : lstat (dst_name, dst_sb))
 != 0)
 
 At first glance, that might be reasonable: the additional open
 is incurred only after a failed stat.

This isn't what the proposed code does.  The open is incurred after a
*successful* stat (though it should only be tried if stat reported S_IFREG).
And it is not intended as an 'additional' open.  Rather the open that we
would expect anyway is being performed earlier to avoid a race condition.

 I'll look more closely in a week or two if no one else investigates.
 
 One thing that would help a lot is a test case (even using gdb, strace,
 stap, inotify, fuse, etc.) that consistently triggers the failure without
 the need for an NFS set-up.

gdb cp
run cp foo # this succeeds ofcourse - 'foo' is a copy of 'cp'
break copy.c:1679  # this is 'have_dst_lstat = !use_stat;'
shell rm foo
c

Result:

/home/git/coreutils/src/cp: cannot create regular file 'foo': No such file or 
directory

With NFS, the 'rm' happens on a different machine and due to the caching
protocol of NFS the unlink is not visible to the 'stat', but it is to the
subsequent 'open' - it is as though it happened between those two calls.

NeilBrown



signature.asc
Description: PGP signature

bug#11100: Racy code in copy.c

[Paul Eggert]

On 03/28/2012 09:07 AM, Jim Meyering wrote:
if ((use_stat
  -   ? stat (dst_name, dst_sb)
  +   ? (stat (dst_name, dst_sb)  0 ? -1 :
  +(fd = open (dst_name, O_WRONLY))  0 ? -1 : 0)
  : lstat (dst_name, dst_sb))
 != 0)
 At first glance, that might be reasonable: the additional open
 is incurred only after a failed stat.
 I'll look more closely in a week or two if no one else investigates.

Come to think of it, wouldn't it be more efficient to
do an open (dst_name, O_WRONLY | O_BINARY), and then
fstat the resulting fd, falling back on 'stat' only if the
open fails with errno == EACCES?   That should be
more efficient in the usual case, since it'd resolve
the file name fewer times in the usual case.

bug#11100: Racy code in copy.c

[Philipp Thomas]

I'd like to pass on observations from my collegue Neil Brown:

in src/copy.c, copy_reg() is passed bool *new_dst.

This is 'false' if the file already exists, in which case it attempts to
open the file with O_WRONLY | O_TRUNC | O_BINARY.
If it is 'true', only then does it use O_CREAT (and others).

Somewhere up the call chain - I'm not sure where - new_dst is set if 'stat'
on the file succeeds.  The above mentioned code assumes that the file still
exists.  This is racy - particularly for NFS where deletions from other
clients can take a while to appear.

Philipp

bug#11100: Racy code in copy.c

[Jim Meyering]

Philipp Thomas wrote:
 I'd like to pass on observations from my collegue Neil Brown:

 in src/copy.c, copy_reg() is passed bool *new_dst.

 This is 'false' if the file already exists, in which case it attempts to
 open the file with O_WRONLY | O_TRUNC | O_BINARY.
 If it is 'true', only then does it use O_CREAT (and others).

 Somewhere up the call chain - I'm not sure where - new_dst is set if 'stat'
 on the file succeeds.  The above mentioned code assumes that the file still
 exists.  This is racy - particularly for NFS where deletions from other
 clients can take a while to appear.

Thanks for the report.
However, much of what cp does is mandated by the POSIX spec, including
that inevitable-looking (POSIX-mandated) TOCTOU race.  Here's part of that
spec, from http://pubs.opengroup.org/onlinepubs/9699919799/utilities/cp.html:

For each source_file, the following steps shall be taken:

...
If source_file is of type directory, the following steps shall be taken:

...

If source_file is of type regular file, the following steps
shall be taken:

The behavior is unspecified if dest_file exists and was
written by a previous step. Otherwise, if dest_file exists,
the following steps shall be taken:

If the -i option is in effect, the cp utility shall
write a prompt to the standard error and read a line from
the standard input. If the response is not affirmative,
cp shall do nothing more with source_file and go on to
any remaining files.

A file descriptor for dest_file shall be obtained by
performing actions equivalent to the open() function
defined in the System Interfaces volume of POSIX.1-2008
called using dest_file as the path argument, and the
bitwise-inclusive OR of O_WRONLY and O_TRUNC as the
oflag argument.

If the attempt to obtain a file descriptor fails and the
-f option is in effect, cp shall attempt to remove the
file by performing actions equivalent to the unlink()
function defined in the System Interfaces volume
of POSIX.1-2008 called using dest_file as the path
argument. If this attempt succeeds, cp shall continue
with step 3b.

If dest_file does not exist, a file descriptor shall be
obtained by performing actions equivalent to the open()
function defined in the System Interfaces volume of
POSIX.1-2008 called using dest_file as the path argument,
and the bitwise-inclusive OR of O_WRONLY and O_CREAT as
the oflag argument. The file permission bits of source_file
shall be the mode argument.

If the attempt to obtain a file descriptor fails, cp shall
write a diagnostic message to standard error, do nothing
more with source_file, and go on to any remaining files.

The contents of source_file shall be written to the file
descriptor. Any write errors shall cause cp to write a
diagnostic message to standard error and continue to step 3e.

The file descriptor shall be closed.

The cp utility shall do nothing more with source_file. If
a write error occurred in step 3d, it is unspecified if
cp continues with any remaining files. If no write error
occurred in step 3d, cp shall go on to any remaining files.

If you can find a way to make cp work sensibly in your specific case,
yet without impacting any other use case, please let us know.

bug#11100: Racy code in copy.c

[Philipp Thomas]

Hi Jim,

* Jim Meyering (j...@meyering.net) [20120327 15:40]:

 If you can find a way to make cp work sensibly in your specific case,
 yet without impacting any other use case, please let us know.

Thanks for the clarification! In that light though I doubt there is a way :(

Philipp

bug#11100: Racy code in copy.c

[Pádraig Brady]

On 03/27/2012 01:58 PM, Philipp Thomas wrote:
 I'd like to pass on observations from my collegue Neil Brown:
 
 in src/copy.c, copy_reg() is passed bool *new_dst.
 
 This is 'false' if the file already exists, in which case it attempts to
 open the file with O_WRONLY | O_TRUNC | O_BINARY.
 If it is 'true', only then does it use O_CREAT (and others).
 
 Somewhere up the call chain - I'm not sure where - new_dst is set if 'stat'
 on the file succeeds.  The above mentioned code assumes that the file still
 exists.  This is racy - particularly for NFS where deletions from other
 clients can take a while to appear.

True. That would result in:
  cannot create regular file ...: ENOENT
or if -f was specified a more confusing:
  cannot remove ...: ENOENT
You could patch to avoid that edge case,
but handling dangling symlinks would complicate things.
It's borderline whether this is warranted.

Note the opposite case where a file is
created between the stat() and the creat()
would result in:
  cannot create regular file ...: EEXIST
even if -f was specified.
Note on NFS  3 O_EXCL is not supported
so this condition won't be flagged at all.
Note also handling of this case would
need to honor the --no-clobber option(s).
Again awkward for an unusual edge case.

I suppose some comments would be appropriate at least.

cheers,
Pádraig.

bug#11100: Racy code in copy.c

[Paul Eggert]

On 03/27/2012 05:58 AM, Philipp Thomas wrote:
 The above mentioned code assumes that the file still
 exists.  This is racy - particularly for NFS where deletions from other
 clients can take a while to appear.

*NEW_DST is a bit more complicated than that.  At least for part
of the code it means the file was known not to exist, which is not
the same thing as being true if the file does not exist and false
otherwise.  (Sometimes I think I need to take a course on epistemology
to understand this code, which is not a good sign)

In general 'cp' is not and cannot be free from races due to other
active processes.  A fix could well be needed here, to work around NFS
bugs, but it'd need to be thought through in the light of other
use cases.

bug#11100: Racy code in copy.c

[NeilBrown]

On Tue, 27 Mar 2012 15:40:25 +0200 Jim Meyering j...@meyering.net wrote:

 Philipp Thomas wrote:
  I'd like to pass on observations from my collegue Neil Brown:
 
  in src/copy.c, copy_reg() is passed bool *new_dst.
 
  This is 'false' if the file already exists, in which case it attempts to
  open the file with O_WRONLY | O_TRUNC | O_BINARY.
  If it is 'true', only then does it use O_CREAT (and others).
 
  Somewhere up the call chain - I'm not sure where - new_dst is set if 'stat'
  on the file succeeds.  The above mentioned code assumes that the file still
  exists.  This is racy - particularly for NFS where deletions from other
  clients can take a while to appear.
 
 Thanks for the report.
 However, much of what cp does is mandated by the POSIX spec, including
 that inevitable-looking (POSIX-mandated) TOCTOU race.  Here's part of that
 spec, from http://pubs.opengroup.org/onlinepubs/9699919799/utilities/cp.html:
 
 For each source_file, the following steps shall be taken:
 
 ...
 If source_file is of type directory, the following steps shall be 
 taken:
 
 ...
 
 If source_file is of type regular file, the following steps
 shall be taken:
 
 The behavior is unspecified if dest_file exists and was
 written by a previous step. Otherwise, if dest_file exists,
 the following steps shall be taken:
 
 If the -i option is in effect, the cp utility shall
 write a prompt to the standard error and read a line from
 the standard input. If the response is not affirmative,
 cp shall do nothing more with source_file and go on to
 any remaining files.
 
 A file descriptor for dest_file shall be obtained by
 performing actions equivalent to the open() function
 defined in the System Interfaces volume of POSIX.1-2008
 called using dest_file as the path argument, and the
 bitwise-inclusive OR of O_WRONLY and O_TRUNC as the
 oflag argument.
 
 If the attempt to obtain a file descriptor fails and the
 -f option is in effect, cp shall attempt to remove the
 file by performing actions equivalent to the unlink()
 function defined in the System Interfaces volume
 of POSIX.1-2008 called using dest_file as the path
 argument. If this attempt succeeds, cp shall continue
 with step 3b.
 
 If dest_file does not exist, a file descriptor shall be
 obtained by performing actions equivalent to the open()
 function defined in the System Interfaces volume of
 POSIX.1-2008 called using dest_file as the path argument,
 and the bitwise-inclusive OR of O_WRONLY and O_CREAT as
 the oflag argument. The file permission bits of source_file
 shall be the mode argument.
 
 If the attempt to obtain a file descriptor fails, cp shall
 write a diagnostic message to standard error, do nothing
 more with source_file, and go on to any remaining files.
 
 The contents of source_file shall be written to the file
 descriptor. Any write errors shall cause cp to write a
 diagnostic message to standard error and continue to step 3e.
 
 The file descriptor shall be closed.
 
 The cp utility shall do nothing more with source_file. If
 a write error occurred in step 3d, it is unspecified if
 cp continues with any remaining files. If no write error
 occurred in step 3d, cp shall go on to any remaining files.
 
 If you can find a way to make cp work sensibly in your specific case,
 yet without impacting any other use case, please let us know.

The above doesn't specify how you determine if the dest file exists.
You could do this with stat plus open(O_WRONLY).
Only try the open on REG files.

   if ((use_stat
-   ? stat (dst_name, dst_sb)
+   ? (stat (dst_name, dst_sb)  0 ? -1 :
+ (fd = open (dst_name, O_WRONLY))  0 ? -1 : 0)
: lstat (dst_name, dst_sb))
   != 0)


If the stat fails, or the open fails with ENOENT, then the file doesn't
exist, otherwise assume it does.
Keep the file descriptor around (if there is one) and then the actions
equivalent to open() ... would be
   if (fd  0)
 fd = open(pathname, O_WRONLY | O_TRUNC);
   else
 ftruncate(fd, 0);


I started making a patch - but it got messy quickly.  Too many conditional
calls to close().
And at over 1000 lines, copy_internal was too big for me to work with :-(

NeilBrown


signature.asc
Description: PGP signature