Re: [PATCH] cp, mv: do preserve extended attributes even for read-only source files

[Ondřej Vašík]

Pádraig Brady wrote:
 Yes I agree that the change is required.
 I've tweaked it so that the geteuid() syscall is only called
 if readonly files. Also I removed the error message on chmod failure
 as the user will still get an error message _if_ the copy_xattr fails.
 Also I ran it through indent and did s/write access rights/write access/.
 I'll push it soon if there are no objections.

Thanks for word tweaks and other patch-amending. I spotted one error -
initial value of access_changed was not changed to false when you
changed the name and logic from access_unchanged (fixed by attached
patch).

Greetings,
 Ondřej

From a4aee231c48e1cb80e63762fd65b6d09f4936bb2 Mon Sep 17 00:00:00 2001
From: =?utf-8?q?Ond=C5=99ej=20Va=C5=A1=C3=ADk?= ova...@redhat.com
Date: Tue, 15 Sep 2009 08:57:43 +0200
Subject: [PATCH] cp: fix initial value of access_changed variable

* src/copy.c (copy_reg): fix initial value of access_changed variable
---
 src/copy.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/src/copy.c b/src/copy.c
index ad2060b..b7d113f 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -839,7 +839,7 @@ copy_reg (char const *src_name, char const *dst_name,
  by xattr_permission() in fs/xattr.c of the GNU/Linux kernel tree.  */
   if (x-preserve_xattr)
 {
-  bool access_changed = true;
+  bool access_changed = false;
 
   if (!(sb.st_mode  S_IWUSR)  geteuid() != 0)
 access_changed = fchmod_or_lchmod (dest_desc, dst_name, 0600) == 0;
-- 
1.5.6.1.156.ge903b



signature.asc
Description: Toto je digitálně	 podepsaná část	 zprávy

Re: [PATCH] cp, mv: do preserve extended attributes even for read-only source files

[Jim Meyering]

Ondřej Vašík wrote:
 Pádraig Brady wrote:
 Yes I agree that the change is required.
 I've tweaked it so that the geteuid() syscall is only called
 if readonly files. Also I removed the error message on chmod failure
 as the user will still get an error message _if_ the copy_xattr fails.
 Also I ran it through indent and did s/write access rights/write access/.
 I'll push it soon if there are no objections.

 Thanks for word tweaks and other patch-amending. I spotted one error -
 initial value of access_changed was not changed to false when you
 changed the name and logic from access_unchanged (fixed by attached
 patch).

Thanks, Ondřej!
Can you contrive a test that would expose that?

Re: [PATCH] cp, mv: do preserve extended attributes even for read-only source files

[Ondřej Vašík]

Jim Meyering wrote:
 Ondřej Vašík wrote:
  Pádraig Brady wrote:
  Yes I agree that the change is required.
  I've tweaked it so that the geteuid() syscall is only called
  if readonly files. Also I removed the error message on chmod failure
  as the user will still get an error message _if_ the copy_xattr fails.
  Also I ran it through indent and did s/write access rights/write access/.
  I'll push it soon if there are no objections.
 
  Thanks for word tweaks and other patch-amending. I spotted one error -
  initial value of access_changed was not changed to false when you
  changed the name and logic from access_unchanged (fixed by attached
  patch).
 
 Thanks, Ondřej!
 Can you contrive a test that would expose that?

I'm not sure if this is useful - as this boolean is just saving chmod
call for the case, that the chmod was not necessary. So even the
Pádraig's patch works, but for files with write access there is one
unnecessary chmod call. Adding test would probably mean strace usage, so
if such test is really required, I would prefer to have that test
separate.

greetings,
 Ondřej


signature.asc
Description: Toto je digitálně	 podepsaná část	 zprávy

Re: [PATCH] cp, mv: do preserve extended attributes even for read-only source files

[Jim Meyering]

Ondřej Vašík wrote:

 Jim Meyering wrote:
 Ondřej Vašík wrote:
  Pádraig Brady wrote:
  Yes I agree that the change is required.
  I've tweaked it so that the geteuid() syscall is only called
  if readonly files. Also I removed the error message on chmod failure
  as the user will still get an error message _if_ the copy_xattr fails.
  Also I ran it through indent and did s/write access rights/write access/.
  I'll push it soon if there are no objections.
 
  Thanks for word tweaks and other patch-amending. I spotted one error -
  initial value of access_changed was not changed to false when you
  changed the name and logic from access_unchanged (fixed by attached
  patch).

 Thanks, Ondřej!
 Can you contrive a test that would expose that?

 I'm not sure if this is useful - as this boolean is just saving chmod
 call for the case, that the chmod was not necessary. So even the
 Pádraig's patch works, but for files with write access there is one
 unnecessary chmod call. Adding test would probably mean strace usage, so
 if such test is really required, I would prefer to have that test
 separate.

Indeed.  Testing for an excess chmod call would not be worthwhile.
Thanks.

Re: [PATCH] cp, mv: do preserve extended attributes even for read-only source files

[Pádraig Brady]

Ondřej Vašík wrote:
 Thanks for word tweaks and other patch-amending. I spotted one error -
 initial value of access_changed was not changed to false when you
 changed the name and logic from access_unchanged

Blush

thanks Ondřej

Re: [PATCH] cp, mv: do preserve extended attributes even for read-only source files

[Ondřej Vašík]

Pádraig Brady wrote:
 Since we're only doing u+rw, and we've already stat'd it's
 probably better to just (sb.mode  S_IWUSR) rather than access(...).
 Also a couple of the  if statements are indented too far.

Hopefully ok with the attached patch.

 This should now be safer but as Jim says it
 only effects file systems mounted user_xattr.
 Perhaps we should wait until coreutils-7.7 and
 also feedback from libattr devs so as we can put
 an accurate comment in the code.

As libattr feedback is already here at
http://lists.gnu.org/archive/html/bug-coreutils/2009-09/msg00166.html
and it seems it is not a bug in libattr (just strange requirement by
kernel), I modified the comment about workaround - as the culprit is
probably in kernel. 

Greetings,
 Ondřej Vašík

From 500acf63e78cc6bdb4fdd8b84c1f5e2305b96fb1 Mon Sep 17 00:00:00 2001
From: =?utf-8?q?Ond=C5=99ej=20Va=C5=A1=C3=ADk?= ova...@redhat.com
Date: Thu, 3 Sep 2009 16:10:21 +0200
Subject: [PATCH] cp,mv: do preserve extended attributes even for read-only source files

* src/copy.c (copy_reg): Set mode on file descriptor to 0600 for copying
extended attributes to prevent failures when source file doesn't have
write access rights. Reported by Ernest N. Mamikonyan.
* tests/misc/xattr: Test that change.
* NEWS (Bug fixes): Mention it.
---
 NEWS |6 ++
 src/copy.c   |   33 -
 tests/misc/xattr |   42 --
 3 files changed, 62 insertions(+), 19 deletions(-)

diff --git a/NEWS b/NEWS
index 980fb54..0ba0d50 100644
--- a/NEWS
+++ b/NEWS
@@ -2,6 +2,12 @@ GNU coreutils NEWS-*- outline -*-
 
 * Noteworthy changes in release ?.? (-??-??) [?]
 
+** Bug fixes
+
+  cp --preserve=xattr and --archive now preserves extended attributes even
+  when the source file doesn't have write access rights
+  [bug introduced in coreutils-7.1]
+
 ** Changes in behavior
 
   id no longer prints SELinux  context=... when the POSIXLY_CORRECT
diff --git a/src/copy.c b/src/copy.c
index f3ff5a2..0cb6094 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -834,6 +834,34 @@ copy_reg (char const *src_name, char const *dst_name,
 }
 }
 
+  /* To allow copying extended attributes on read-only files, change
+ destination file descriptor access rights to 0600 for a while
+ if required.
+ This workaround is required as full permission check is done
+ by xattr_permission in fs/xattr.c of the kernel tree. */
+  if (x-preserve_xattr)
+   {
+ bool access_unchanged = true;
+
+ if (geteuid () != 0  !(sb.st_mode  S_IWUSR) 
+ (access_unchanged = fchmod_or_lchmod (dest_desc, dst_name, 0600)))
+   {
+ if (!x-reduce_diagnostics || x-require_preserve_xattr)
+   error (0, errno,
+ _(failed to set writable mode for %s, copying xattrs may fail),
+ quote (dst_name));
+   }
+
+ if (! copy_attr_by_fd (src_name, source_desc, dst_name, dest_desc, x)
+ x-require_preserve_xattr)
+   return_val = false;
+
+ if (!access_unchanged)
+   fchmod_or_lchmod (dest_desc, dst_name,
+ dst_mode  ~omitted_permissions);
+   }
+
+
   if (x-preserve_ownership  ! SAME_OWNER_AND_GROUP (*src_sb, sb))
 {
   switch (set_owner (x, dst_name, dest_desc, src_sb, *new_dst, sb))
@@ -850,11 +878,6 @@ copy_reg (char const *src_name, char const *dst_name,
 
   set_author (dst_name, dest_desc, src_sb);
 
-  if (x-preserve_xattr  ! copy_attr_by_fd (src_name, source_desc,
-  dst_name, dest_desc, x)
-   x-require_preserve_xattr)
-return_val = false;
-
   if (x-preserve_mode || x-move_mode)
 {
   if (copy_acl (src_name, source_desc, dst_name, dest_desc, src_mode) != 0
diff --git a/tests/misc/xattr b/tests/misc/xattr
index a27e1f6..404085f 100755
--- a/tests/misc/xattr
+++ b/tests/misc/xattr
@@ -29,7 +29,7 @@ fi
 
 # Skip this test if cp was built without xattr support:
 touch src dest || framework_failure
-cp --preserve=xattr -n src dest 2/dev/null \
+cp --preserve=xattr -n src dest \
   || skip_test_ coreutils built without xattr support
 
 # this code was taken from test mv/backup-is-src
@@ -46,13 +46,13 @@ xattr_pair=$xattr_name=\$xattr_value\
 # create new file and check its xattrs
 touch a || framework_failure
 getfattr -d a out_a || skip_test_ failed to get xattr of file
-grep -F $xattr_pair out_a /dev/null  framework_failure
+grep -F $xattr_pair out_a  framework_failure
 
 # try to set user xattr on file
 setfattr -n $xattr_name -v $xattr_value a out_a \
   || skip_test_ failed to set xattr of file
 getfattr -d a out_a || skip_test_ failed to get xattr of file
-grep -F $xattr_pair out_a /dev/null \
+grep -F $xattr_pair out_a \
   || skip_test_ failed to set xattr of file
 
 fail=0
@@ -60,36 +60,50 @@ fail=0
 # cp should not preserve xattr by default
 cp a b || fail=1
 getfattr -d b out_b || skip_test_ failed to get xattr of file

Re: [PATCH] cp, mv: do preserve extended attributes even for read-only source files

[Jim Meyering]

Pádraig Brady wrote:
 Ondřej Vašík wrote:
 Pádraig Brady wrote:
 Since we're only doing u+rw, and we've already stat'd it's
 probably better to just (sb.mode  S_IWUSR) rather than access(...).
 Also a couple of the  if statements are indented too far.

 Hopefully ok with the attached patch.

 This should now be safer but as Jim says it
 only effects file systems mounted user_xattr.
 Perhaps we should wait until coreutils-7.7 and
 also feedback from libattr devs so as we can put
 an accurate comment in the code.

 As libattr feedback is already here at
 http://lists.gnu.org/archive/html/bug-coreutils/2009-09/msg00166.html
 and it seems it is not a bug in libattr (just strange requirement by
 kernel), I modified the comment about workaround - as the culprit is
 probably in kernel.

 Yes I agree that the change is required.
 I've tweaked it so that the geteuid() syscall is only called
 if readonly files. Also I removed the error message on chmod failure
 as the user will still get an error message _if_ the copy_xattr fails.
 Also I ran it through indent and did s/write access rights/write access/.
 The crux of the patch is now:

   if (x-preserve_xattr)
 {
   bool access_changed = true;

   if (!(sb.st_mode  S_IWUSR)  geteuid() != 0)
 access_changed = fchmod_or_lchmod (dest_desc, dst_name, 0600) == 0;

   if (!copy_attr_by_fd (src_name, source_desc, dst_name, dest_desc, x)
x-require_preserve_xattr)
 return_val = false;

   if (access_changed)
 fchmod_or_lchmod (dest_desc, dst_name, dst_mode  
 ~omitted_permissions);
 }

 I'll push it soon if there are no objections.

Sounds good.  Thanks.

Re: [PATCH] cp, mv: do preserve extended attributes even for read-only source files

[Ondřej Vašík]

Hi,

Jim Meyering wrote:
 Pádraig Brady wrote:
  Ondřej Vašík wrote:
  As reported in
  http://lists.gnu.org/archive/html/bug-coreutils/2009-08/msg00342.html by
  Ernest N. Mamikonyan, cp/mv fails to preserve extended attributes for
  read-only source files.
  Following patch fixes the issue for me, although maybe it's not perfect
  solution. But I don't know about better one at the moment.
  Test included...
 
  Thanks for that, and especially the test.
  It looks to me that the cause of this is actually a bug
  in fsetxattr() (called by attr_copy_fd) as it's being
  passed a writable fd, but still giving permission denied?
 
 Hi Ondřej,
 
 Thanks for working on that.
 Note that your patch relaxes permissions
 on the destination and does not restore them.
 
 If you continue to work on this, please use the adjusted patch below.
 It makes the test script detect that failure, and also removes most
 of the redirections to /dev/null.  Now that nearly all test-related
 output is directed to a log file, there's no point in redirecting small
 outputs like that, and seeing them in the log can even make it easier
 to diagnose problems.
 
 Since this problem affects only users of file systems
 mounted with user_xattr, I may defer the fix until coreutils-7.7.

Ah, I knew I forgot to do something :). Thanks for spotting this.

Restoring to dest_mode  ~omitted_permissions done in attached patch,
dropped redirections from the test as well. Additionally - I modified
the copy.c patch a bit - failure of mode change now doesn't mean that I
don't try to preserve extended attributes (as it still could pass). 
Pádraig is right that it looks like some kind of bug in libattr and
fsetxattr() function, as the descriptor should be writable, anyway this
should workaround it - at least until they'll fix/change it or other way
of solution will be found.
Ok with passing to 7.7, although with such small impact and relatively
low danger, it could maybe included to 7.6 (if more snapshots will be
before real release).

Greetings,
 Ondřej

From 5fca1fbaf2e7594496c854f4c3eef60bd3013697 Mon Sep 17 00:00:00 2001
From: =?utf-8?q?Ond=C5=99ej=20Va=C5=A1=C3=ADk?= ova...@redhat.com
Date: Thu, 3 Sep 2009 16:10:21 +0200
Subject: [PATCH] cp,mv: do preserve extended attributes even for read-only source files

* src/copy.c (copy_reg): Set mode on file descriptor to 0600 for copying
extended attributes to prevent failures when source file doesn't have
write access rights. Reported by Ernest N. Mamikonyan.
* tests/misc/xattr: Test that change.
* NEWS (Bug fixes): Mention it.
---
 NEWS |4 
 src/copy.c   |   21 +
 tests/misc/xattr |   42 --
 3 files changed, 49 insertions(+), 18 deletions(-)

diff --git a/NEWS b/NEWS
index 59270eb..8c511c6 100644
--- a/NEWS
+++ b/NEWS
@@ -15,6 +15,10 @@ GNU coreutils NEWS-*- outline -*-
   cp --preserve=xattr no longer leaks resources on each preservation failure.
   [bug introduced in coreutils-7.1]
 
+  cp --preserve=xattr and --archive now preserves extended attributes even
+  when the source file doesn't have write access rights
+  [bug introduced in coreutils-7.1]
+
   dd now returns non-zero status if it encountered a write error while
   printing a summary to stderr.
   [bug introduced in coreutils-6.11]
diff --git a/src/copy.c b/src/copy.c
index e604ec5..9506fb4 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -850,10 +850,23 @@ copy_reg (char const *src_name, char const *dst_name,
 
   set_author (dst_name, dest_desc, src_sb);
 
-  if (x-preserve_xattr  ! copy_attr_by_fd (src_name, source_desc,
-  dst_name, dest_desc, x)
-   x-require_preserve_xattr)
-return_val = false;
+  /* to allow copying extended attributes on read-only files, change
+ destination file descriptor access rights to 0600 for a while. */
+  if (x-preserve_xattr)
+   {
+ if (! fchmod_or_lchmod (dest_desc, dst_name, 0600))
+   {
+ if (!x-reduce_diagnostics || x-require_preserve_xattr)
+error (0, errno,
+  _(failed to set writable mode for %s, copying xattrs may fail),
+  quote (dst_name));
+   }
+ if (! copy_attr_by_fd (src_name, source_desc, dst_name, dest_desc, x)
+ x-require_preserve_xattr)
+ return_val = false;
+ fchmod_or_lchmod (dest_desc, dst_name, dst_mode  ~omitted_permissions);
+   }
+
 
   if (x-preserve_mode || x-move_mode)
 {
diff --git a/tests/misc/xattr b/tests/misc/xattr
index a27e1f6..404085f 100755
--- a/tests/misc/xattr
+++ b/tests/misc/xattr
@@ -29,7 +29,7 @@ fi
 
 # Skip this test if cp was built without xattr support:
 touch src dest || framework_failure
-cp --preserve=xattr -n src dest 2/dev/null \
+cp --preserve=xattr -n src dest \
   || skip_test_ coreutils built without xattr support
 
 # this code was taken from test mv/backup-is-src
@@ -46,13 +46,13 @@ 

Re: [PATCH] cp, mv: do preserve extended attributes even for read-only source files

[Pádraig Brady]

Ondřej Vašík wrote:
 Ah, I knew I forgot to do something :). Thanks for spotting this.
 
 Restoring to dest_mode  ~omitted_permissions done in attached patch,
 dropped redirections from the test as well. Additionally - I modified
 the copy.c patch a bit - failure of mode change now doesn't mean that I
 don't try to preserve extended attributes (as it still could pass). 


 Pádraig is right that it looks like some kind of bug in libattr and
 fsetxattr() function, as the descriptor should be writable, anyway this
 should workaround it - at least until they'll fix/change it or other way
 of solution will be found.

What's the best place to report that?
It would be good to add a comment in the code that this is a workaround
rather than expected behaviour (after confirming the bug of course).

 Ok with passing to 7.7, although with such small impact and relatively
 low danger, it could maybe included to 7.6 (if more snapshots will be
 before real release).

To minimize side affects perhaps we should only do the chmod(600)
if (geteuid () != 0  !access (src_name, W_OK)) ?

cheers,
Pádraig.

Re: [PATCH] cp, mv: do preserve extended attributes even for read-only source files

[Jim Meyering]

Ondřej Vašík wrote:
 Ah, I knew I forgot to do something :). Thanks for spotting this.

 Restoring to dest_mode  ~omitted_permissions done in attached patch,
 dropped redirections from the test as well. Additionally - I modified
 the copy.c patch a bit - failure of mode change now doesn't mean that I
 don't try to preserve extended attributes (as it still could pass).
 Pádraig is right that it looks like some kind of bug in libattr and
 fsetxattr() function, as the descriptor should be writable, anyway this
 should workaround it - at least until they'll fix/change it or other way
 of solution will be found.
 Ok with passing to 7.7, although with such small impact and relatively
 low danger, it could maybe included to 7.6 (if more snapshots will be
 before real release).

Thanks for the update.
However, I'd rather avoid that permission-relaxing code completely.
Not only does it appear to constitute a security problem when run by
root, but it may also fail, when copying, as non-priveleged, to a file
that is writable but owned by someone else.

Re: [PATCH] cp, mv: do preserve extended attributes even for read-only source files

[Ondřej Vašík]

Pádraig Brady wrote:
 Ondřej Vašík wrote:
  Ah, I knew I forgot to do something :). Thanks for spotting this.
  
  Restoring to dest_mode  ~omitted_permissions done in attached patch,
  dropped redirections from the test as well. Additionally - I modified
  the copy.c patch a bit - failure of mode change now doesn't mean that I
  don't try to preserve extended attributes (as it still could pass). 
 
 
  Pádraig is right that it looks like some kind of bug in libattr and
  fsetxattr() function, as the descriptor should be writable, anyway this
  should workaround it - at least until they'll fix/change it or other way
  of solution will be found.
 
 What's the best place to report that?
 It would be good to add a comment in the code that this is a workaround
 rather than expected behaviour (after confirming the bug of course).

libattr upstream has mailing list xfs at oss.sgi.com , so maybe the best
place is there. 

  Ok with passing to 7.7, although with such small impact and relatively
  low danger, it could maybe included to 7.6 (if more snapshots will be
  before real release).
 
 To minimize side affects perhaps we should only do the chmod(600)
 if (geteuid () != 0  !access (src_name, W_OK)) ?

Good idea, it would reduce possibility of security leak, playing with
access rights is always a bit dangerous (although here we play with
rights on destination descriptor, which is imho much more safe).

Additionally - Jim is correct that for different owner 0600 rights are
not sufficient for different owner of the file - and 0666 is too much
devil-like ;) . Any idea?

Greetings,
 Ondřej


signature.asc
Description: Toto je digitálně	 podepsaná část	 zprávy

Re: [PATCH] cp, mv: do preserve extended attributes even for read-only source files

[Pádraig Brady]

Ondřej Vašík wrote:
 Pádraig Brady wrote:
 To minimize side affects perhaps we should only do the chmod(600)
 if (geteuid () != 0  !access (src_name, W_OK)) ?
 
 Good idea, it would reduce possibility of security leak, playing with
 access rights is always a bit dangerous (although here we play with
 rights on destination descriptor, which is imho much more safe).
 
 Additionally - Jim is correct that for different owner 0600 rights are
 not sufficient for different owner of the file - and 0666 is too much
 devil-like ;) . Any idea?

preserve_xattr before preserve_ownership ?

cheers,
Pádraig.

Re: [PATCH] cp, mv: do preserve extended attributes even for read-only source files

[Ondřej Vašík]

Pádraig Brady wrote:
 Ondřej Vašík wrote:
  Pádraig Brady wrote:
  To minimize side affects perhaps we should only do the chmod(600)
  if (geteuid () != 0  !access (src_name, W_OK)) ?
  
  Good idea, it would reduce possibility of security leak, playing with
  access rights is always a bit dangerous (although here we play with
  rights on destination descriptor, which is imho much more safe).
  
  Additionally - Jim is correct that for different owner 0600 rights are
  not sufficient for different owner of the file - and 0666 is too much
  devil-like ;) . Any idea?
 
 preserve_xattr before preserve_ownership ?

Good idea, moved there and used that (geteuid () != 0  access
(src_name, W_OK)) construction - additionally I tried to reduce those
chmod calls (call for returning permissions only when the write_access 
granting call was used) - so it should be safer now.

Anyway, added comment that real problem is in libattr and this is just
workaround and added FIXME. Better now?

Greetings,
 Ondřej


From 3aae1734c715cb8246a4961f5bea5e6fa58c1d61 Mon Sep 17 00:00:00 2001
From: =?utf-8?q?Ond=C5=99ej=20Va=C5=A1=C3=ADk?= ova...@redhat.com
Date: Thu, 3 Sep 2009 16:10:21 +0200
Subject: [PATCH] cp,mv: do preserve extended attributes even for read-only source files

* src/copy.c (copy_reg): Set mode on file descriptor to 0600 for copying
extended attributes to prevent failures when source file doesn't have
write access rights. Reported by Ernest N. Mamikonyan.
* tests/misc/xattr: Test that change.
* NEWS (Bug fixes): Mention it.
---
 NEWS |4 
 src/copy.c   |   34 +-
 tests/misc/xattr |   42 --
 3 files changed, 61 insertions(+), 19 deletions(-)

diff --git a/NEWS b/NEWS
index 59270eb..8c511c6 100644
--- a/NEWS
+++ b/NEWS
@@ -15,6 +15,10 @@ GNU coreutils NEWS-*- outline -*-
   cp --preserve=xattr no longer leaks resources on each preservation failure.
   [bug introduced in coreutils-7.1]
 
+  cp --preserve=xattr and --archive now preserves extended attributes even
+  when the source file doesn't have write access rights
+  [bug introduced in coreutils-7.1]
+
   dd now returns non-zero status if it encountered a write error while
   printing a summary to stderr.
   [bug introduced in coreutils-6.11]
diff --git a/src/copy.c b/src/copy.c
index e604ec5..c8f0a45 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -834,6 +834,35 @@ copy_reg (char const *src_name, char const *dst_name,
 }
 }
 
+  /* To allow copying extended attributes on read-only files, change
+ destination file descriptor access rights to 0600 for a while
+ if required.
+ Note: it's just workaround - fsetxattr from libattr needs write
+ access rights even with file descriptor opened with O_WRONLY
+ FIXME: remove that ugly access rights change. */
+  if (x-preserve_xattr)
+   {
+ bool access_unchanged = true;
+
+ if (geteuid () != 0  !access (src_name, W_OK) 
+ (access_unchanged = fchmod_or_lchmod (dest_desc, dst_name, 0600)))
+   {
+ if (!x-reduce_diagnostics || x-require_preserve_xattr)
+error (0, errno,
+  _(failed to set writable mode for %s, copying xattrs may fail),
+  quote (dst_name));
+   }
+
+ if (! copy_attr_by_fd (src_name, source_desc, dst_name, dest_desc, x)
+ x-require_preserve_xattr)
+ return_val = false;
+
+ if (!access_unchanged)
+ fchmod_or_lchmod (dest_desc, dst_name,
+   dst_mode  ~omitted_permissions);
+   }
+
+
   if (x-preserve_ownership  ! SAME_OWNER_AND_GROUP (*src_sb, sb))
 {
   switch (set_owner (x, dst_name, dest_desc, src_sb, *new_dst, sb))
@@ -850,11 +879,6 @@ copy_reg (char const *src_name, char const *dst_name,
 
   set_author (dst_name, dest_desc, src_sb);
 
-  if (x-preserve_xattr  ! copy_attr_by_fd (src_name, source_desc,
-  dst_name, dest_desc, x)
-   x-require_preserve_xattr)
-return_val = false;
-
   if (x-preserve_mode || x-move_mode)
 {
   if (copy_acl (src_name, source_desc, dst_name, dest_desc, src_mode) != 0
diff --git a/tests/misc/xattr b/tests/misc/xattr
index a27e1f6..404085f 100755
--- a/tests/misc/xattr
+++ b/tests/misc/xattr
@@ -29,7 +29,7 @@ fi
 
 # Skip this test if cp was built without xattr support:
 touch src dest || framework_failure
-cp --preserve=xattr -n src dest 2/dev/null \
+cp --preserve=xattr -n src dest \
   || skip_test_ coreutils built without xattr support
 
 # this code was taken from test mv/backup-is-src
@@ -46,13 +46,13 @@ xattr_pair=$xattr_name=\$xattr_value\
 # create new file and check its xattrs
 touch a || framework_failure
 getfattr -d a out_a || skip_test_ failed to get xattr of file
-grep -F $xattr_pair out_a /dev/null  framework_failure
+grep -F $xattr_pair out_a  framework_failure
 
 # try to set user xattr on file
 setfattr -n $xattr_name 

Re: [PATCH] cp, mv: do preserve extended attributes even for read-only source files

[Pádraig Brady]

Ondřej Vašík wrote:
 Pádraig Brady wrote:
 Ondřej Vašík wrote:
 Pádraig Brady wrote:
 To minimize side affects perhaps we should only do the chmod(600)
 if (geteuid () != 0  !access (src_name, W_OK)) ?
 Good idea, it would reduce possibility of security leak, playing with
 access rights is always a bit dangerous (although here we play with
 rights on destination descriptor, which is imho much more safe).

 Additionally - Jim is correct that for different owner 0600 rights are
 not sufficient for different owner of the file - and 0666 is too much
 devil-like ;) . Any idea?
 preserve_xattr before preserve_ownership ?
 
 Good idea, moved there and used that (geteuid () != 0  access
 (src_name, W_OK)) construction - additionally I tried to reduce those
 chmod calls (call for returning permissions only when the write_access 
 granting call was used) - so it should be safer now.
 
 Anyway, added comment that real problem is in libattr and this is just
 workaround and added FIXME. Better now?

That looks much better, thanks.
Since we're only doing u+rw, and we've already stat'd it's
probably better to just (sb.mode  S_IWUSR) rather than access(...).
Also a couple of the  if statements are indented too far.

This should now be safer but as Jim says it
only effects file systems mounted user_xattr.
Perhaps we should wait until coreutils-7.7 and
also feedback from libattr devs so as we can put
an accurate comment in the code.

cheers,
Pádraig.

Re: [PATCH] cp, mv: do preserve extended attributes even for read-only source files

[Jim Meyering]

Pádraig Brady wrote:
 Ondřej Vašík wrote:
 As reported in
 http://lists.gnu.org/archive/html/bug-coreutils/2009-08/msg00342.html by
 Ernest N. Mamikonyan, cp/mv fails to preserve extended attributes for
 read-only source files.
 Following patch fixes the issue for me, although maybe it's not perfect
 solution. But I don't know about better one at the moment.
 Test included...

 Thanks for that, and especially the test.
 It looks to me that the cause of this is actually a bug
 in fsetxattr() (called by attr_copy_fd) as it's being
 passed a writable fd, but still giving permission denied?

 rsync does copy the xattrs as it uses a different approach,
 which you can see comparing the strace output of cp and
 rsync below:

Hi Ondřej,

Thanks for working on that.
Note that your patch relaxes permissions
on the destination and does not restore them.

If you continue to work on this, please use the adjusted patch below.
It makes the test script detect that failure, and also removes most
of the redirections to /dev/null.  Now that nearly all test-related
output is directed to a log file, there's no point in redirecting small
outputs like that, and seeing them in the log can even make it easier
to diagnose problems.

Since this problem affects only users of file systems
mounted with user_xattr, I may defer the fix until coreutils-7.7.


From 158ca2f14ff61d8984b1b13bbf76ae0ce12b83c1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ond=C5=99ej=20Va=C5=A1=C3=ADk?= ova...@redhat.com
Date: Thu, 3 Sep 2009 16:10:21 +0200
Subject: [PATCH] cp,mv: preserve extended attributes even for read-only source 
files

* src/copy.c (copy_reg): Set mode on file descriptor to 0600 for copying
extended attributes to prevent failures when source file doesn't have
write access rights.  Reported by Ernest N. Mamikonyan.
* tests/misc/xattr: Test that change.
* NEWS (Bug fixes): Mention it.
---
 NEWS |4 
 src/copy.c   |   24 
 tests/misc/xattr |   42 --
 3 files changed, 52 insertions(+), 18 deletions(-)

diff --git a/NEWS b/NEWS
index cb01227..843f76b 100644
--- a/NEWS
+++ b/NEWS
@@ -15,6 +15,10 @@ GNU coreutils NEWS-*- 
outline -*-
   cp --preserve=xattr no longer leaks resources on each preservation failure.
   [bug introduced in coreutils-7.1]

+  cp --preserve=xattr and --archive now preserves extended attributes even
+  when the source file doesn't have write access rights
+  [bug introduced in coreutils-7.1]
+
   dd now returns non-zero status if it encountered a write error while
   printing a summary to stderr.
   [bug introduced in coreutils-6.11]
diff --git a/src/copy.c b/src/copy.c
index 178a640..b299313 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -850,10 +850,26 @@ copy_reg (char const *src_name, char const *dst_name,

   set_author (dst_name, dest_desc, src_sb);

-  if (x-preserve_xattr  ! copy_attr_by_fd (src_name, source_desc,
-  dst_name, dest_desc, x)
-   x-require_preserve_xattr)
-return_val = false;
+  /* to allow copying extended attributes on read-only files, change
+ destination file descriptor access rights to 0600 for a while. */
+  if (x-preserve_xattr)
+   {
+ if (! fchmod_or_lchmod (dest_desc, dst_name, 0600))
+   {
+ if (! copy_attr_by_fd (src_name, source_desc, dst_name, dest_desc, x)
+ x-require_preserve_xattr)
+   return_val = false;
+   }
+ else
+   {
+  if (!x-reduce_diagnostics || x-require_preserve_xattr)
+error (0, errno, _(can't write extended attributes to %s),
+   quote (dst_name));
+  if (x-require_preserve_xattr)
+return_val = false;
+   }
+}
+

   if (x-preserve_mode || x-move_mode)
 {
diff --git a/tests/misc/xattr b/tests/misc/xattr
index a27e1f6..5da8bf4 100755
--- a/tests/misc/xattr
+++ b/tests/misc/xattr
@@ -29,7 +29,7 @@ fi

 # Skip this test if cp was built without xattr support:
 touch src dest || framework_failure
-cp --preserve=xattr -n src dest 2/dev/null \
+cp --preserve=xattr -n src dest \
   || skip_test_ coreutils built without xattr support

 # this code was taken from test mv/backup-is-src
@@ -46,13 +46,13 @@ xattr_pair=$xattr_name=\$xattr_value\
 # create new file and check its xattrs
 touch a || framework_failure
 getfattr -d a out_a || skip_test_ failed to get xattr of file
-grep -F $xattr_pair out_a /dev/null  framework_failure
+grep -F $xattr_pair out_a  framework_failure

 # try to set user xattr on file
 setfattr -n $xattr_name -v $xattr_value a out_a \
   || skip_test_ failed to set xattr of file
 getfattr -d a out_a || skip_test_ failed to get xattr of file
-grep -F $xattr_pair out_a /dev/null \
+grep -F $xattr_pair out_a \
   || skip_test_ failed to set xattr of file

 fail=0
@@ -60,36 +60,50 @@ fail=0
 # cp should not preserve xattr by default
 cp a b || fail=1
 getfattr -d b out_b || 

Re: [PATCH] cp, mv: do preserve extended attributes even for read-only source files

[Pádraig Brady]

Ondřej Vašík wrote:
 As reported in
 http://lists.gnu.org/archive/html/bug-coreutils/2009-08/msg00342.html by
 Ernest N. Mamikonyan, cp/mv fails to preserve extended attributes for
 read-only source files.
 Following patch fixes the issue for me, although maybe it's not perfect
 solution. But I don't know about better one at the moment.
 Test included...

Thanks for that, and especially the test.
It looks to me that the cause of this is actually a bug
in fsetxattr() (called by attr_copy_fd) as it's being
passed a writable fd, but still giving permission denied?

rsync does copy the xattrs as it uses a different approach,
which you can see comparing the strace output of cp and
rsync below:

strace -f rsync -X xattr xattr-copy
  open(.xattr-copy.w0p0Cr, O_RDWR|O_CREAT|O_EXCL|O_LARGEFILE, 0600) = 1
  fchmod(1, 0600) = 0
  mmap2(NULL, 266240, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 
0xb7dd5000
  write(1, t\n..., 2)   = 2
  close(1)= 0
  lstat64(.xattr-copy.w0p0Cr, {st_mode=S_IFREG|0600, st_size=2, ...}) = 0
  llistxattr(.xattr-copy.w0p0Cr, 0x938, 1024) = 0
  lsetxattr(.xattr-copy.w0p0Cr, user.test, test, 4, 0) = 0
  chmod(.xattr-copy.w0p0Cr, 0444) = 0
  rename(.xattr-copy.w0p0Cr, xattr-copy1) = 0

strace cp --preserve=xattr xattr xattr-copy
  open(xattr-copy, O_WRONLY|O_CREAT|O_EXCL|O_LARGEFILE, 0444) = 4
  fstat64(4, {st_mode=S_IFREG|0444, st_size=0, ...}) = 0
  read(3, t\n..., 32768)= 2
  write(4, t\n..., 2)   = 2
  read(3, ..., 32768)   = 0
  flistxattr(3, (nil), 0) = 10
  flistxattr(3, 0xbff59780, 10)   = 10
  open(/etc/xattr.conf, O_RDONLY|O_LARGEFILE) = -1 ENOENT (No such file or 
directory)
  fgetxattr(3, user.test, 0x0, 0)   = 4
  fgetxattr(3, user.test, test, 4)= 4
  fsetxattr(4, user.test, test, 4, 0) = -1 EACCES (Permission denied

BTW, I think you forgot to restore the permissions in your patch.

cheers,
Pádraig.