From a4f7b723f0723ec17b48ae25f0a218adcab2ff54 Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Wed, 6 Dec 2006 20:44:08 +0100 Subject: * NEWS: Document the cp --preserve=ownership fix. * m4/jm-macros.m4 (coreutils_MACROS): Check for fchmod. * src/copy.c (fchmod_or_lchmod): New function. (copy_reg): New arg OMITTED_PERMISSIONS. All uses changed. Omit confusing and unused ", dst_mode" arg to 'open' without O_CREAT. When creating a file, use O_EXCL, so we're more likely to detect funny business by other processes. At the end, if permissions were omitted, chmod them back in. (copy_internal): If the ownership might change, omit some permissions at first, then restore them after chowning the file. * src/cp.c (make_dir_parents_private): Likewise. * src/copy.c (cached_umask): New function. * src/copy.h (cached_umask): New decl. --- ChangeLog | 16 ++++++++ NEWS | 9 +++++ m4/jm-macros.m4 | 1 + src/copy.c | 112 +++++++++++++++++++++++++++++++++++++++++++++++++------- src/copy.h | 1 + src/cp.c | 45 +++++++++++++---------- 6 files changed, 151 insertions(+), 33 deletions(-) diff --git a/ChangeLog b/ChangeLog index ec4e3ecde..e2ee3ca09 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,19 @@ +2006-12-06 Paul Eggert + + * NEWS: Document the cp --preserve=ownership fix. + * m4/jm-macros.m4 (coreutils_MACROS): Check for fchmod. + * src/copy.c (fchmod_or_lchmod): New function. + (copy_reg): New arg OMITTED_PERMISSIONS. All uses changed. + Omit confusing and unused ", dst_mode" arg to 'open' without O_CREAT. + When creating a file, use O_EXCL, so we're more likely to detect + funny business by other processes. At the end, if permissions + were omitted, chmod them back in. + (copy_internal): If the ownership might change, omit some permissions + at first, then restore them after chowning the file. + * src/cp.c (make_dir_parents_private): Likewise. + * src/copy.c (cached_umask): New function. + * src/copy.h (cached_umask): New decl. + 2006-12-06 Jim Meyering Make the output of "make check" more reproducible. diff --git a/NEWS b/NEWS index 50eb623b4..fad4e1c63 100644 --- a/NEWS +++ b/NEWS @@ -4,6 +4,15 @@ GNU coreutils NEWS -*- outline -*- ** Bug fixes + cp --preserve=ownership would create output files that temporarily + had too-generous permissions in some cases. For example, when + copying a file with group A and mode 644 into a group-B sticky + directory, the output file was briefly readable by group B. + Fix similar problems with cp options like -p that imply + --preserve=ownership, with install -d when combined with either -o + or -g, and with mv when copying across file system boundaries. + This bug affects coreutils 6.0 through 6.6. + du --one-file-system (-x) would skip subdirectories of any directory listed as second or subsequent command line argument. This bug affects coreutils-6.4, 6.5 and 6.6. diff --git a/m4/jm-macros.m4 b/m4/jm-macros.m4 index d65f1086f..5bf46a9e3 100644 --- a/m4/jm-macros.m4 +++ b/m4/jm-macros.m4 @@ -62,6 +62,7 @@ AC_DEFUN([coreutils_MACROS], endgrent \ endpwent \ fchown \ + fchmod \ ftruncate \ iswspace \ mkfifo \ diff --git a/src/copy.c b/src/copy.c index 5b66b281d..d49f9b4e6 100644 --- a/src/copy.c +++ b/src/copy.c @@ -230,11 +230,26 @@ set_author (const char *dst_name, int dest_desc, const struct stat *src_sb) #endif } +/* Change the file mode bits of the file identified by DESC or NAME to MODE. + Use DESC if DESC is valid and fchmod is available, NAME otherwise. */ + +static int +fchmod_or_lchmod (int desc, char const *name, mode_t mode) +{ +#if HAVE_FCHMOD + if (0 <= desc) + return fchmod (desc, mode); +#endif + return lchmod (name, mode); +} + /* Copy a regular file from SRC_NAME to DST_NAME. If the source file contains holes, copies holes and blocks of zeros in the source file as holes in the destination file. (Holes are read as zeroes by the `read' system call.) - Use DST_MODE as the 3rd argument in the call to open. + When creating the destination, use DST_MODE & ~OMITTED_PERMISSIONS + as the third argument in the call to open, adding + OMITTED_PERMISSIONS after copying as needed. X provides many option settings. Return true if successful. *NEW_DST is as in copy_internal. @@ -242,7 +257,8 @@ set_author (const char *dst_name, int dest_desc, const struct stat *src_sb) static bool copy_reg (char const *src_name, char const *dst_name, - const struct cp_options *x, mode_t dst_mode, bool *new_dst, + const struct cp_options *x, + mode_t dst_mode, mode_t omitted_permissions, bool *new_dst, struct stat const *src_sb) { char *buf; @@ -282,7 +298,7 @@ copy_reg (char const *src_name, char const *dst_name, The if-block will be taken in move_mode. */ if (! *new_dst) { - dest_desc = open (dst_name, O_WRONLY | O_TRUNC | O_BINARY, dst_mode); + dest_desc = open (dst_name, O_WRONLY | O_TRUNC | O_BINARY); if (dest_desc < 0 && x->unlink_dest_after_failed_open) { @@ -301,7 +317,10 @@ copy_reg (char const *src_name, char const *dst_name, } if (*new_dst) - dest_desc = open (dst_name, O_WRONLY | O_CREAT | O_BINARY, dst_mode); + dest_desc = open (dst_name, O_WRONLY | O_CREAT | O_EXCL | O_BINARY, + dst_mode & ~omitted_permissions); + else + omitted_permissions = 0; if (dest_desc < 0) { @@ -520,6 +539,18 @@ copy_reg (char const *src_name, char const *dst_name, if (set_acl (dst_name, dest_desc, x->mode) != 0) return_val = false; } + else if (omitted_permissions) + { + omitted_permissions &= ~ cached_umask (); + if (omitted_permissions + && fchmod_or_lchmod (dest_desc, dst_name, dst_mode) != 0) + { + error (0, errno, _("preserving permissions for %s"), + quote (dst_name)); + if (x->require_preserve) + return_val = false; + } + } close_src_and_dst_desc: if (close (dest_desc) < 0) @@ -967,6 +998,7 @@ copy_internal (char const *src_name, char const *dst_name, struct stat dst_sb; mode_t src_mode; mode_t dst_mode IF_LINT (= 0); + mode_t omitted_permissions; bool restore_dst_mode = false; char *earlier_file = NULL; char *dst_backup = NULL; @@ -1465,6 +1497,14 @@ copy_internal (char const *src_name, char const *dst_name, new_dst = true; } + /* If the ownership might change, omit some permissions at first, so + unauthorized users cannot nip in before the file has the right + ownership. */ + omitted_permissions = + (x->preserve_ownership + ? (x->set_mode ? x->mode : src_mode) & (S_IRWXG | S_IRWXO) + : 0); + delayed_ok = true; /* In certain modes (cp's --symbolic-link), and for certain file types @@ -1502,7 +1542,10 @@ copy_internal (char const *src_name, char const *dst_name, (src_mode & ~S_IRWXUGO) != 0. However, common practice is to ask mkdir to copy all the CHMOD_MODE_BITS, letting mkdir decide what to do with S_ISUID | S_ISGID | S_ISVTX. */ - if (mkdir (dst_name, src_mode & CHMOD_MODE_BITS) != 0) + mode_t mkdir_mode = + ((x->set_mode ? x->mode : src_mode) + & CHMOD_MODE_BITS & ~omitted_permissions); + if (mkdir (dst_name, mkdir_mode) != 0) { error (0, errno, _("cannot create directory %s"), quote (dst_name)); @@ -1629,7 +1672,7 @@ copy_internal (char const *src_name, char const *dst_name, practice passed all the source mode bits to 'open', but the extra bits were ignored, so it should be the same either way. */ if (! copy_reg (src_name, dst_name, x, src_mode & S_IRWXUGO, - &new_dst, &src_sb)) + omitted_permissions, &new_dst, &src_sb)) goto un_backup; } else if (S_ISFIFO (src_mode)) @@ -1637,7 +1680,7 @@ copy_internal (char const *src_name, char const *dst_name, /* Use mknod, rather than mkfifo, because the former preserves the special mode bits of a fifo on Solaris 10, while mkfifo does not. */ - if (mknod (dst_name, src_mode, 0) != 0) + if (mknod (dst_name, src_mode & ~omitted_permissions, 0) != 0) { error (0, errno, _("cannot create fifo %s"), quote (dst_name)); goto un_backup; @@ -1645,7 +1688,8 @@ copy_internal (char const *src_name, char const *dst_name, } else if (S_ISBLK (src_mode) || S_ISCHR (src_mode) || S_ISSOCK (src_mode)) { - if (mknod (dst_name, src_mode, src_sb.st_rdev) != 0) + if (mknod (dst_name, src_mode & ~omitted_permissions, src_sb.st_rdev) + != 0) { error (0, errno, _("cannot create special file %s"), quote (dst_name)); @@ -1774,14 +1818,40 @@ copy_internal (char const *src_name, char const *dst_name, if (set_acl (dst_name, -1, x->mode) != 0) return false; } - else if (restore_dst_mode) + else { - if (lchmod (dst_name, dst_mode) != 0) + if (omitted_permissions) { - error (0, errno, _("preserving permissions for %s"), - quote (dst_name)); - if (x->require_preserve) - return false; + omitted_permissions &= ~ cached_umask (); + + if (omitted_permissions && !restore_dst_mode) + { + /* Permissions were deliberately omitted when the file + was created due to security concerns. See whether + they need to be re-added now. It'd be faster to omit + the lstat, but deducing the current destination mode + is tricky in the presence of implementation-defined + rules for special mode bits. */ + if (new_dst && lstat (dst_name, &dst_sb) != 0) + { + error (0, errno, _("cannot stat %s"), quote (dst_name)); + return false; + } + dst_mode = dst_sb.st_mode; + if (omitted_permissions & ~dst_mode) + restore_dst_mode = true; + } + } + + if (restore_dst_mode) + { + if (lchmod (dst_name, dst_mode | omitted_permissions) != 0) + { + error (0, errno, _("preserving permissions for %s"), + quote (dst_name)); + if (x->require_preserve) + return false; + } } } @@ -1885,3 +1955,17 @@ chown_failure_ok (struct cp_options const *x) return ((errno == EPERM || errno == EINVAL) && !x->chown_privileges); } + +/* Return the user's umask, caching the result. */ + +extern mode_t +cached_umask (void) +{ + static mode_t mask = -1; + if (mask == (mode_t) -1) + { + mask = umask (0); + umask (mask); + } + return mask; +} diff --git a/src/copy.h b/src/copy.h index f08b62520..c815baf64 100644 --- a/src/copy.h +++ b/src/copy.h @@ -213,5 +213,6 @@ void src_info_init (struct cp_options *); bool chown_privileges (void); bool chown_failure_ok (struct cp_options const *); +mode_t cached_umask (void); #endif diff --git a/src/cp.c b/src/cp.c index 9ba334255..8fe11a1e7 100644 --- a/src/cp.c +++ b/src/cp.c @@ -413,6 +413,8 @@ make_dir_parents_private (char const *const_dir, size_t src_offset, if (XSTAT (x, dir, &stats)) { mode_t src_mode; + mode_t omitted_permissions; + mode_t mkdir_mode; /* This component does not exist. We must set *new_dst and new->mode inside this loop because, @@ -427,12 +429,15 @@ make_dir_parents_private (char const *const_dir, size_t src_offset, return false; } src_mode = stats.st_mode; + omitted_permissions = + x->preserve_ownership ? src_mode & (S_IRWXG | S_IRWXO) : 0; /* POSIX says mkdir's behavior is implementation-defined when (src_mode & ~S_IRWXUGO) != 0. However, common practice is to ask mkdir to copy all the CHMOD_MODE_BITS, letting mkdir decide what to do with S_ISUID | S_ISGID | S_ISVTX. */ - if (mkdir (dir, src_mode & CHMOD_MODE_BITS) != 0) + mkdir_mode = src_mode & CHMOD_MODE_BITS & ~omitted_permissions; + if (mkdir (dir, mkdir_mode) != 0) { error (0, errno, _("cannot make directory %s"), quote (dir)); @@ -454,28 +459,30 @@ make_dir_parents_private (char const *const_dir, size_t src_offset, quote (dir)); return false; } - else - { - if (x->preserve_mode) - { - new->mode = src_mode; - new->restore_mode = (src_mode != stats.st_mode); - } - if ((stats.st_mode & S_IRWXU) != S_IRWXU) - { - /* Make the new directory searchable and writable. The - original permissions will be restored later. */ - new->mode = stats.st_mode; + if (! x->preserve_mode) + { + if (omitted_permissions & ~stats.st_mode) + omitted_permissions &= ~ cached_umask (); + if (omitted_permissions & ~stats.st_mode + || (stats.st_mode & S_IRWXU) != S_IRWXU) + { + new->mode = stats.st_mode | omitted_permissions; new->restore_mode = true; + } + } - if (lchmod (dir, stats.st_mode | S_IRWXU) != 0) - { - error (0, errno, _("setting permissions for %s"), - quote (dir)); - return false; - } + if ((stats.st_mode & S_IRWXU) != S_IRWXU) + { + /* Make the new directory searchable and writable. + The original permissions will be restored later. */ + + if (lchmod (dir, stats.st_mode | S_IRWXU) != 0) + { + error (0, errno, _("setting permissions for %s"), + quote (dir)); + return false; } } } -- cgit v1.2.3-70-g09d2