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" -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"
-grep -F "$xattr_pair" out_b >/dev/null && fail=1
+grep -F "$xattr_pair" out_b && fail=1
# test if --preserve=xattr option works
cp --preserve=xattr a b || fail=1
getfattr -d b >out_b || skip_test_ "failed to get xattr of file"
-grep -F "$xattr_pair" out_b >/dev/null || fail=1
+grep -F "$xattr_pair" out_b || fail=1
#test if --preserve=all option works
cp --preserve=all a c || fail=1
getfattr -d c >out_c || skip_test_ "failed to get xattr of file"
-grep -F "$xattr_pair" out_c >/dev/null || fail=1
+grep -F "$xattr_pair" out_c || fail=1
#test if -a option works without any diagnostics
cp -a a d 2>err && test -s err && fail=1
getfattr -d d >out_d || skip_test_ "failed to get xattr of file"
-grep -F "$xattr_pair" out_d >/dev/null || fail=1
+grep -F "$xattr_pair" out_d || fail=1
+
+#test if --preserve=xattr works even for files without write rights
+chmod a-w a || framework_failure
+rm -f e
+cp --preserve=xattr a e || fail=1
+getfattr -d e >out_e || skip_test_ "failed to get xattr of file"
+grep -F "$xattr_pair" out_e || fail=1
+
+#Ensure that permission bits are preserved, too.
+src_perm=$(stat --format=%a a)
+dst_perm=$(stat --format=%a e)
+test "$dst_perm" = "$src_perm" || fail=1
+
+chmod u+w a || framework_failure
rm b || framework_failure
# install should never preserve xattr
ginstall a b || fail=1
getfattr -d b >out_b || skip_test_ "failed to get xattr of file"
-grep -F "$xattr_pair" out_b >/dev/null && fail=1
+grep -F "$xattr_pair" out_b && fail=1
# mv should preserve xattr when renaming within a file system.
# This is implicitly done by rename () and doesn't need explicit
# xattr support in mv.
mv a b || fail=1
getfattr -d b >out_b || skip_test_ "failed to get xattr of file"
-grep -F "$xattr_pair" out_b >/dev/null || cat >&2 <<EOF
+grep -F "$xattr_pair" out_b || cat >&2 <<EOF
=================================================================
$0: WARNING!!!
rename () does not preserve extended attributes
@@ -99,18 +113,18 @@ EOF
# try to set user xattr on file on other partition
test_mv=1
touch "$b_other" || framework_failure
-setfattr -n "$xattr_name" -v "$xattr_value" "$b_other" >out_a 2>/dev/null \
+setfattr -n "$xattr_name" -v "$xattr_value" "$b_other" >out_a \
|| test_mv=0
-getfattr -d "$b_other" >out_b 2>/dev/null || test_mv=0
-grep -F "$xattr_pair" out_b >/dev/null || test_mv=0
+getfattr -d "$b_other" >out_b || test_mv=0
+grep -F "$xattr_pair" out_b || test_mv=0
rm -f "$b_other" || framework_failure
if test $test_mv -eq 1; then
# mv should preserve xattr when copying content from one partition to another
mv b "$b_other" || fail=1
- getfattr -d "$b_other" >out_b 2>/dev/null ||
+ getfattr -d "$b_other" >out_b ||
skip_test_ "failed to get xattr of file"
- grep -F "$xattr_pair" out_b >/dev/null || fail=1
+ grep -F "$xattr_pair" out_b || fail=1
else
cat >&2 <<EOF
=================================================================
--
1.5.6.1.156.ge903b
signature.asc
Description: Toto je digitálně podepsaná část zprávy
