From b28a8851ed22dbf0cd123974a0c97ae0b82bec2b Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Sat, 3 Feb 2007 18:45:46 +0100 Subject: * NEWS: Document fix for cp --preserve=mode. * src/copy.c (copy_internal): Omit the group- or other-writeable permissions when creating a directory, to avoid a race condition if the special mode bits aren't right just after the directory is created. * src/cp.c (make_dir_parents_private): Likewise. * tests/cp/parent-perm-race: Test for the "cp --preserve=mode" race fix in copy.c. --- ChangeLog | 9 ++++++ NEWS | 7 +++++ src/copy.c | 23 +++++++------- src/cp.c | 12 ++++++-- tests/cp/parent-perm-race | 76 +++++++++++++++++++++++++++-------------------- 5 files changed, 81 insertions(+), 46 deletions(-) diff --git a/ChangeLog b/ChangeLog index c2ad0b5e2..dbd305992 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,14 @@ 2007-02-02 Paul Eggert + * NEWS: Document fix for cp --preserve=mode. + * src/copy.c (copy_internal): Omit the group- or other-writeable + permissions when creating a directory, to avoid a race condition + if the special mode bits aren't right just after the directory is + created. + * src/cp.c (make_dir_parents_private): Likewise. + * tests/cp/parent-perm-race: Test for the "cp --preserve=mode" + race fix in copy.c. + * NEWS: Document fix for cp --parents. * src/cp.c (make_dir_parents_private): Report the error sooner with "cp --parents DIR/FILE DEST" when DIR is a non-directory, thus not diff --git a/NEWS b/NEWS index c90a1b3c5..bd307c14e 100644 --- a/NEWS +++ b/NEWS @@ -13,6 +13,13 @@ GNU coreutils NEWS -*- outline -*- "cp --parents F/G D" no longer creates a directory D/F when F is not a directory (and F/G is therefore invalid). + "cp --preserve=mode" would create directories that briefly had + too-generous permissions in some cases. For example, when copying a + directory with permissions 777 the destination directory might + temporarily be setgid on some file systems, which would allow other + users to create subfiles with the same group as the directory. Fix + similar problems with 'install' and 'mv'. + cut no longer dumps core for usage like "cut -f2- f1 f2" with two or more file arguments. This was due to a double-free bug, introduced in coreutils-5.3.0. diff --git a/src/copy.c b/src/copy.c index d9ad254a9..5ec5a921c 100644 --- a/src/copy.c +++ b/src/copy.c @@ -1,5 +1,5 @@ /* copy.c -- core functions for copying files and directories - Copyright (C) 89, 90, 91, 1995-2006 Free Software Foundation. + Copyright (C) 89, 90, 91, 1995-2007 Free Software Foundation. This program is free software; you can redistribute it and/or modify it under the terms of the GNU General Public License as published by @@ -1005,6 +1005,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 dst_mode_bits; mode_t omitted_permissions; bool restore_dst_mode = false; char *earlier_file = NULL; @@ -1504,13 +1505,16 @@ 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. */ + /* If the ownership might change, or if it is a directory (whose + special mode bits may change after the directory is created), + omit some permissions at first, so unauthorized users cannot nip + in before the file is ready. */ + dst_mode_bits = (x->set_mode ? x->mode : src_mode) & CHMOD_MODE_BITS; omitted_permissions = - (x->preserve_ownership - ? (x->set_mode ? x->mode : src_mode) & (S_IRWXG | S_IRWXO) - : 0); + (dst_mode_bits + & (x->preserve_ownership ? S_IRWXG | S_IRWXO + : S_ISDIR (src_mode) ? S_IWGRP | S_IWOTH + : 0)); delayed_ok = true; @@ -1549,10 +1553,7 @@ 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. */ - mode_t mkdir_mode = - ((x->set_mode ? x->mode : src_mode) - & CHMOD_MODE_BITS & ~omitted_permissions); - if (mkdir (dst_name, mkdir_mode) != 0) + if (mkdir (dst_name, dst_mode_bits & ~omitted_permissions) != 0) { error (0, errno, _("cannot create directory %s"), quote (dst_name)); diff --git a/src/cp.c b/src/cp.c index aea63dce7..5759e0d07 100644 --- a/src/cp.c +++ b/src/cp.c @@ -435,8 +435,16 @@ 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; + + /* If the ownership or special mode bits might change, + omit some permissions at first, so unauthorized users + cannot nip in before the file is ready. */ + omitted_permissions = (src_mode + & (x->preserve_ownership + ? S_IRWXG | S_IRWXO + : x->preserve_mode + ? S_IWGRP | S_IWOTH + : 0)); /* POSIX says mkdir's behavior is implementation-defined when (src_mode & ~S_IRWXUGO) != 0. However, common practice is diff --git a/tests/cp/parent-perm-race b/tests/cp/parent-perm-race index 80c95a69b..d2870bcea 100755 --- a/tests/cp/parent-perm-race +++ b/tests/cp/parent-perm-race @@ -1,7 +1,7 @@ #!/bin/sh # Make sure cp -pR --parents isn't too generous with parent permissions. -# Copyright (C) 2006 Free Software Foundation, Inc. +# Copyright (C) 2006, 2007 Free Software Foundation, Inc. # This program is free software; you can redistribute it and/or modify # it under the terms of the GNU General Public License as published by @@ -32,37 +32,47 @@ framework_failure=0 mkdir -p $tmp || framework_failure=1 cd $tmp || framework_failure=1 -umask 022 -mkdir -p a d || framework_failure=1 -mkfifo a/fifo || { - echo "$0: fifos not supported; skipping this test." 1>&2 - (exit 77); exit 77 -} - -# Copy a fifo's contents. That way, we can examine d/a's -# state while cp is running. -cp -p -R --copy-contents --parents a d & -cp_pid=$! - -( - # Now 'cp' is reading the fifo. - # Check the permissions of the temporary destination - # directory that 'cp' has made. - ls -ld d/a >d/ls.out - - # Close the fifo so that "cp" can continue. But output first, - # before exiting, otherwise some shells would optimize away the file - # descriptor that holds the fifo open. - echo foo -) >a/fifo - -case `cat d/ls.out` in -d???--[-S]--[-S]*) - fail=0;; -*) - fail=1;; -esac - -wait $cp_pid || fail=1 +umask 002 +mkdir mode ownership d || framework_failure=1 +chmod g+s d 2>/dev/null # The cp test is valid either way. + +fail=0 + +for attr in mode ownership +do + mkfifo $attr/fifo || { + echo "$0: fifos not supported; skipping this test." 1>&2 + (exit 77); exit 77 + } + + # Copy a fifo's contents. That way, we can examine d/$attr's + # state while cp is running. + cp --preserve=$attr -R --copy-contents --parents $attr d & + cp_pid=$! + + ( + # Now 'cp' is reading the fifo. + # Check the permissions of the temporary destination + # directory that 'cp' has made. + ls -ld d/$attr >d/$attr.ls + + # Close the fifo so that "cp" can continue. But output first, + # before exiting, otherwise some shells would optimize away the file + # descriptor that holds the fifo open. + echo foo + ) >$attr/fifo + + ls_output=`cat d/$attr.ls` || fail=1 + case $attr,$ls_output in + ownership,d???--[-S]--[-S]* | \ + mode,d????-??-?* | \ + mode,d??[-x]?w[-x]?-[-x]* ) + ;; + *) + fail=1;; + esac + + wait $cp_pid || fail=1 +done (exit $fail); exit $fail -- cgit v1.2.3-70-g09d2