From 4b8c92b94058ec21ba94b2d48eab7667b3eaee83 Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Sun, 24 Sep 2017 23:05:17 -0700 Subject: [PATCH] copy: revert recent patch for vulnerable dirs I plan to propose a better patch to catch vulnerable parent directories. * NEWS, doc/coreutils.texi (Target directory): Document this. * src/cp.c, src/install.c, src/ln.c, src/mv.c: Do not include targetdir.h. (target_directory_operand): Remove test for vulnerable parents. * src/cp.c (stat_target_operand): Remove. All uses removed. * src/local.mk (noinst_HEADERS): Remove src/targetdir.h. (src_ginstall_SOURCES, src_cp_SOURCES, src_ln_SOURCES) (src_mv_SOURCES): Remove src/targetdir.c. * src/targetdir.c, src/targetdir.h: Remove. * tests/mv/vulnerable-target.sh: Remove. * tests/local.mk (all_root_tests): Remove it. --- NEWS | 11 ---- doc/coreutils.texi | 33 +----------- src/cp.c | 37 ++++---------- src/install.c | 32 ++++-------- src/ln.c | 34 ++++--------- src/local.mk | 13 ++--- src/mv.c | 22 +++----- src/targetdir.c | 96 ----------------------------------- src/targetdir.h | 4 -- tests/local.mk | 1 - tests/mv/vulnerable-target.sh | 38 -------------- 11 files changed, 45 insertions(+), 276 deletions(-) delete mode 100644 src/targetdir.c delete mode 100644 src/targetdir.h delete mode 100755 tests/mv/vulnerable-target.sh diff --git a/NEWS b/NEWS index 15ae40bcbf..5d57c727a3 100644 --- a/NEWS +++ b/NEWS @@ -2,17 +2,6 @@ GNU coreutils NEWS -*- outline -*- * Noteworthy changes in release ?.? (????-??-??) [?] -** Changes in behavior - - cp, install, ln, and mv by default now reject some usages that are - likely vulnerable to hijacking, to foil some attacks on unsafe - shared target directories. For example, if /tmp/risky is - world-writable and /tmp/risky/d is a directory, 'cp f /tmp/risky/d' - now fails because the target directory is vulnerable. To suppress - the vulnerability heuristic, append '/' to the name of the target - directory, or use the -t or -T options, or (for cp, ln, and mv) set - the POSIXLY_CORRECT environment variable. - ** Bug fixes ptx -S no longer infloops for a pattern which returns zero-length matches. diff --git a/doc/coreutils.texi b/doc/coreutils.texi index 9fa680a012..47a9f6d253 100644 --- a/doc/coreutils.texi +++ b/doc/coreutils.texi @@ -1282,38 +1282,7 @@ The @command{cp}, @command{install}, @command{ln}, and @command{mv} commands normally treat the last operand specially when it is a directory or a symbolic link to a directory. For example, @samp{cp source dest} is equivalent to @samp{cp source dest/source} if -@file{dest} is a directory. - -@cindex vulnerable target directory -@vindex POSIXLY_CORRECT -However, if the destination is a directory whose name could be that of -an ordinary file, and the directory's parent appears to be vulnerable -to an attack by another user, then these programs report the -vulnerability and fail. If you are not worried about such an attack -you can suppress this heuristic by appending @samp{/} to the -destination's name, or by using the @option{--target-directory} -(@option{-t}) or @option{--no-target-directory} (@option{-T}) options. -(For @command{cp}, @command{ln}, and @command{mv} you can also -suppress the heuristic by setting the @env{POSIXLY_CORRECT} -environment variable.) For example, if @file{/tmp/risky/d} is a -directory whose parent @file{/tmp/risky} is world-writable and is -not sticky, the command @samp{cp passwd /tmp/risky/d} fails with -a diagnostic reporting a vulnerable target directory, as an attacker -could replace @file{/tmp/risky/d} by a symbolic link to a victim -directory while @command{cp} is running. In this example, you can -suppress the heuristic by issuing one of the following shell commands -instead: - -@example -# These can be hijacked if /tmp/risky is rwxrwxrwx! -cp passwd /tmp/risky/d/ -cp -t /tmp/risky/d passwd -cp -T passwd /tmp/risky/d/passwd -POSIXLY_CORRECT=yes cp passwd /tmp/risky/d -@end example - -As this example illustrates, sometimes the default behavior with -destination directories is not exactly +@file{dest} is a directory. Sometimes this behavior is not exactly what is wanted, so these commands support the following options to allow more fine-grained control: diff --git a/src/cp.c b/src/cp.c index d4efe5c31c..6743f10365 100644 --- a/src/cp.c +++ b/src/cp.c @@ -33,7 +33,6 @@ #include "ignore-value.h" #include "quote.h" #include "stat-time.h" -#include "targetdir.h" #include "utimens.h" #include "acl.h" @@ -568,20 +567,6 @@ make_dir_parents_private (char const *const_dir, size_t src_offset, return true; } -/* Store FILE's status into *ST. If FILE does not exist, set *NEW_DST. - If there is some other error, report it and exit. */ - -static void -stat_target_operand (char const *file, struct stat *st, bool *new_dst) -{ - if (stat (file, st) != 0) - { - if (errno != ENOENT) - die (EXIT_FAILURE, errno, _("failed to access %s"), quoteaf (file)); - *new_dst = true; - } -} - /* FILE is the last operand of this command. Return true if FILE is a directory. But report an error and exit if there is a problem accessing FILE, @@ -592,17 +577,17 @@ stat_target_operand (char const *file, struct stat *st, bool *new_dst) Otherwise, set *NEW_DST. */ static bool -target_directory_operand (char *file, struct stat *st, bool *new_dst) +target_directory_operand (char const *file, struct stat *st, bool *new_dst) { - stat_target_operand (file, st, new_dst); - if (*new_dst || ! S_ISDIR (st->st_mode)) - return false; - enum targetdir ty = targetdir_operand_type (file, NULL); - if (ty == TARGETDIR_VULNERABLE && ! getenv ("POSIXLY_CORRECT")) - die (EXIT_FAILURE, 0, - _("vulnerable target directory %s (append '/' to use anyway)"), - quoteaf (file)); - return ty != TARGETDIR_NOT; + int err = (stat (file, st) == 0 ? 0 : errno); + bool is_a_dir = !err && S_ISDIR (st->st_mode); + if (err) + { + if (err != ENOENT) + die (EXIT_FAILURE, err, _("failed to access %s"), quoteaf (file)); + *new_dst = true; + } + return is_a_dir; } /* Scan the arguments, and copy each by calling copy. @@ -638,7 +623,7 @@ do_copy (int n_files, char **file, const char *target_directory, usage (EXIT_FAILURE); } /* Update NEW_DST and SB, which may be checked below. */ - stat_target_operand (file[n_files -1], &sb, &new_dst); + ignore_value (target_directory_operand (file[n_files -1], &sb, &new_dst)); } else if (!target_directory) { diff --git a/src/install.c b/src/install.c index a66abb0b64..8c4f244f84 100644 --- a/src/install.c +++ b/src/install.c @@ -42,7 +42,6 @@ #include "savewd.h" #include "selinux.h" #include "stat-time.h" -#include "targetdir.h" #include "utimens.h" #include "xstrtol.h" @@ -396,29 +395,20 @@ setdefaultfilecon (char const *file) directory if it referred to anything at all. */ static bool -target_directory_operand (char *file) +target_directory_operand (char const *file) { + char const *b = last_component (file); + size_t blen = strlen (b); + bool looks_like_a_dir = (blen == 0 || ISSLASH (b[blen - 1])); struct stat st; - if (stat (file, &st) != 0) - { - int err = errno; - if (err == ENOENT) - { - char const *b = last_component (file); - size_t blen = strlen (b); - if (blen != 0 && ! ISSLASH (b[blen - 1])) - return false; - } - die (EXIT_FAILURE, err, _("failed to access %s"), quoteaf (file)); - } - if (! S_ISDIR (st.st_mode)) - return false; - enum targetdir ty = targetdir_operand_type (file, NULL); - if (ty == TARGETDIR_VULNERABLE) - die (EXIT_FAILURE, 0, - _("vulnerable target directory %s (append '/' to use anyway)"), + int err = (stat (file, &st) == 0 ? 0 : errno); + bool is_a_dir = !err && S_ISDIR (st.st_mode); + if (err && err != ENOENT) + die (EXIT_FAILURE, err, _("failed to access %s"), quoteaf (file)); + if (is_a_dir < looks_like_a_dir) + die (EXIT_FAILURE, err, _("target %s is not a directory"), quoteaf (file)); - return ty != TARGETDIR_NOT; + return is_a_dir; } /* Report that directory DIR was made, if OPTIONS requests this. */ diff --git a/src/ln.c b/src/ln.c index 67b634d125..0ff8a0b27c 100644 --- a/src/ln.c +++ b/src/ln.c @@ -32,7 +32,6 @@ #include "hash-triple.h" #include "relpath.h" #include "same.h" -#include "targetdir.h" #include "yesno.h" #include "canonicalize.h" @@ -121,33 +120,22 @@ errno_nonexisting (int err) directory if it referred to anything at all. */ static bool -target_directory_operand (char *file) +target_directory_operand (char const *file) { + char const *b = last_component (file); + size_t blen = strlen (b); + bool looks_like_a_dir = (blen == 0 || ISSLASH (b[blen - 1])); struct stat st; int stat_result = (dereference_dest_dir_symlinks ? stat (file, &st) : lstat (file, &st)); - if (stat_result != 0) - { - int err = errno; - if (! errno_nonexisting (err)) - die (EXIT_FAILURE, err, _("failed to access %s"), quoteaf (file)); - char const *b = last_component (file); - size_t blen = strlen (b); - if (blen == 0 || ISSLASH (b[blen - 1])) - die (EXIT_FAILURE, err, _("target %s is not a directory"), - quoteaf (file)); - return false; - } - if (! S_ISDIR (st.st_mode)) - return false; - enum targetdir ty - = targetdir_operand_type (file, - dereference_dest_dir_symlinks ? NULL : &st); - if (ty == TARGETDIR_VULNERABLE && ! getenv ("POSIXLY_CORRECT")) - die (EXIT_FAILURE, 0, - _("%s: vulnerable target directory (append '/' to use anyway)"), + int err = (stat_result == 0 ? 0 : errno); + bool is_a_dir = !err && S_ISDIR (st.st_mode); + if (err && ! errno_nonexisting (errno)) + die (EXIT_FAILURE, err, _("failed to access %s"), quoteaf (file)); + if (is_a_dir < looks_like_a_dir) + die (EXIT_FAILURE, err, _("target %s is not a directory"), quoteaf (file)); - return ty != TARGETDIR_NOT; + return is_a_dir; } /* Return FROM represented as relative to the dir of TARGET. diff --git a/src/local.mk b/src/local.mk index 40061e0d64..38b4bd4499 100644 --- a/src/local.mk +++ b/src/local.mk @@ -59,7 +59,6 @@ noinst_HEADERS = \ src/remove.h \ src/set-fields.h \ src/system.h \ - src/targetdir.h \ src/uname.h EXTRA_DIST += \ @@ -345,8 +344,8 @@ copy_sources = \ transform = s/ginstall/install/;/libstdbuf/!$(program_transform_name) -src_ginstall_SOURCES = src/install.c src/prog-fprintf.c src/targetdir.c \ - $(copy_sources) $(selinux_sources) +src_ginstall_SOURCES = src/install.c src/prog-fprintf.c $(copy_sources) \ + $(selinux_sources) # This is for the '[' program. Automake transliterates '[' and '/' to '_'. src___SOURCES = src/lbracket.c @@ -354,7 +353,7 @@ src___SOURCES = src/lbracket.c nodist_src_coreutils_SOURCES = src/coreutils.h src_coreutils_SOURCES = src/coreutils.c -src_cp_SOURCES = src/cp.c src/targetdir.c $(copy_sources) $(selinux_sources) +src_cp_SOURCES = src/cp.c $(copy_sources) $(selinux_sources) src_dir_SOURCES = src/ls.c src/ls-dir.c src_vdir_SOURCES = src/ls.c src/ls-vdir.c src_id_SOURCES = src/id.c src/group-list.c @@ -362,16 +361,14 @@ src_groups_SOURCES = src/groups.c src/group-list.c src_ls_SOURCES = src/ls.c src/ls-ls.c src_ln_SOURCES = src/ln.c \ src/force-link.c src/force-link.h \ - src/relpath.c src/relpath.h \ - src/targetdir.c + src/relpath.c src/relpath.h src_chown_SOURCES = src/chown.c src/chown-core.c src_chgrp_SOURCES = src/chgrp.c src/chown-core.c src_kill_SOURCES = src/kill.c src/operand2sig.c src_realpath_SOURCES = src/realpath.c src/relpath.c src/relpath.h src_timeout_SOURCES = src/timeout.c src/operand2sig.c -src_mv_SOURCES = src/mv.c src/remove.c src/targetdir.c \ - $(copy_sources) $(selinux_sources) +src_mv_SOURCES = src/mv.c src/remove.c $(copy_sources) $(selinux_sources) src_rm_SOURCES = src/rm.c src/remove.c src_mkdir_SOURCES = src/mkdir.c src/prog-fprintf.c $(selinux_sources) diff --git a/src/mv.c b/src/mv.c index abe72f571b..9e80193dff 100644 --- a/src/mv.c +++ b/src/mv.c @@ -32,7 +32,6 @@ #include "filenamecat.h" #include "remove.h" #include "root-dev-ino.h" -#include "targetdir.h" #include "priv-set.h" /* The official name of this program (e.g., no 'g' prefix). */ @@ -149,23 +148,14 @@ cp_option_init (struct cp_options *x) than nonexistence (errno == ENOENT). */ static bool -target_directory_operand (char *file) +target_directory_operand (char const *file) { struct stat st; - if (stat (file, &st) == 0) - { - if (! S_ISDIR (st.st_mode)) - return false; - enum targetdir ty = targetdir_operand_type (file, NULL); - if (ty == TARGETDIR_VULNERABLE && ! getenv ("POSIXLY_CORRECT")) - die (EXIT_FAILURE, 0, - _("vulnerable target directory %s (append / to use anyway)"), - quoteaf (file)); - return ty != TARGETDIR_NOT; - } - if (errno != ENOENT) - die (EXIT_FAILURE, errno, _("failed to access %s"), quoteaf (file)); - return false; + int err = (stat (file, &st) == 0 ? 0 : errno); + bool is_a_dir = !err && S_ISDIR (st.st_mode); + if (err && err != ENOENT) + die (EXIT_FAILURE, err, _("failed to access %s"), quoteaf (file)); + return is_a_dir; } /* Move SOURCE onto DEST. Handles cross-file-system moves. diff --git a/src/targetdir.c b/src/targetdir.c deleted file mode 100644 index d577893ed1..0000000000 --- a/src/targetdir.c +++ /dev/null @@ -1,96 +0,0 @@ -/* Check target directories for commands like cp and mv. - Copyright 2017 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 - the Free Software Foundation, either version 3 of the License, or - (at your option) any later version. - - This program is distributed in the hope that it will be useful, - but WITHOUT ANY WARRANTY; without even the implied warranty of - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - GNU General Public License for more details. - - You should have received a copy of the GNU General Public License - along with this program. If not, see . */ - -/* Written by Paul Eggert. */ - -#include -#include - -#include -#include - -#include "system.h" - -/* Check whether DIR, which the caller presumably has already verified - is a directory or a symlink to a directory, is likely to be - vulnerable as the target directory of a command like 'cp ... DIR'. - If DIR_LSTAT is nonnull, it is the result of calling lstat on DIR. - - Return TARGETDIR_OK if DIR is OK, which does not necessarily mean - that DIR is a directory or that it is invulnerable to the attack, - only that it satisfies the heuristics. Return TARGETDIR_NOT if DIR - becomes inaccessible or a non-directory while checking things. - Return TARGETDIR_VULNERABLE if the heuristics suggest that DIR is a - likely candidate to be hijacked by a symlink attack. - - This function might temporarily modify the DIR string; it restores - the string to its original value before returning. */ - -extern enum targetdir -targetdir_operand_type (char *restrict dir, - struct stat const *restrict dir_lstat) -{ - char *lc = last_component (dir); - size_t lclen = strlen (lc); - - /* If DIR ends in / or has a last component of . or .. then it is - good enough. */ - if (lclen == 0 || ISSLASH (lc[lclen - 1]) - || STREQ (lc, ".") || STREQ (lc, "..")) - return TARGETDIR_OK; - - char lc0 = *lc; - *lc = '\0'; - struct stat parent_stat; - bool parent_stat_ok = stat (*dir ? dir : ".", &parent_stat) == 0; - *lc = lc0; - - /* If DIR's parent cannot be statted, DIR can't be statted either. */ - if (! parent_stat_ok) - return TARGETDIR_NOT; - - uid_t euid = geteuid (); - if (parent_stat.st_uid == ROOT_UID || parent_stat.st_uid == euid) - { - /* If no other non-root user can write the parent directory, it - is safe. If the parent directory's UID and GID are the same, - assume the common convention of a single-user group with the - same ID, to avoid returning TARGETDIR_VULNERABLE when users - employing this convention have mode-775 directories. */ - if (! (parent_stat.st_mode - & (S_IWOTH - | (parent_stat.st_uid == parent_stat.st_gid ? 0 : S_IWGRP)))) - return TARGETDIR_OK; - - /* If the parent is sticky, and no other non-root user owns - either the parent or DIR, it should be OK. Do not follow - symlinks when checking DIR for this. */ - if (parent_stat.st_mode & S_ISVTX) - { - struct stat st; - if (!dir_lstat) - { - if (lstat (dir, &st) != 0) - return TARGETDIR_NOT; - dir_lstat = &st; - } - if (dir_lstat->st_uid == ROOT_UID || dir_lstat->st_uid == euid) - return TARGETDIR_OK; - } - } - - return TARGETDIR_VULNERABLE; -} diff --git a/src/targetdir.h b/src/targetdir.h deleted file mode 100644 index 29ea867d49..0000000000 --- a/src/targetdir.h +++ /dev/null @@ -1,4 +0,0 @@ -struct stat; -enum targetdir { TARGETDIR_VULNERABLE = -1, TARGETDIR_NOT, TARGETDIR_OK }; -enum targetdir targetdir_operand_type (char *restrict, - struct stat const *restrict); diff --git a/tests/local.mk b/tests/local.mk index 70c49e8b7d..42e247e6f2 100644 --- a/tests/local.mk +++ b/tests/local.mk @@ -662,7 +662,6 @@ all_tests = \ tests/mv/to-symlink.sh \ tests/mv/trailing-slash.sh \ tests/mv/update.sh \ - tests/mv/vulnerable-target.sh \ tests/readlink/can-e.sh \ tests/readlink/can-f.sh \ tests/readlink/can-m.sh \ diff --git a/tests/mv/vulnerable-target.sh b/tests/mv/vulnerable-target.sh deleted file mode 100755 index 49ee363c69..0000000000 --- a/tests/mv/vulnerable-target.sh +++ /dev/null @@ -1,38 +0,0 @@ -#!/bin/sh -# Check that mv diagnoses vulnerable target directories. - -# Copyright 2017 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 -# the Free Software Foundation, either version 3 of the License, or -# (at your option) any later version. - -# This program is distributed in the hope that it will be useful, -# but WITHOUT ANY WARRANTY; without even the implied warranty of -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -# GNU General Public License for more details. - -# You should have received a copy of the GNU General Public License -# along with this program. If not, see . - -. "${srcdir=.}/tests/init.sh"; path_prepend_ ./src -print_ver_ mv - -unset POSIXLY_CORRECT - -mkdir -m a+rwx risky || framework_failure_ -mkdir risky/d || framework_failure_ -echo foo >foo || framework_failure_ - -returns_ 1 mv foo risky/d || fail=1 -mv foo risky/d/ || fail=1 -mv risky/d/foo . || fail=1 -mv -t risky/d foo || fail=1 -mv risky/d/foo . || fail=1 -mv -T foo risky/d/foo || fail=1 -mv risky/d/foo . || fail=1 -POSIXLY_CORRECT=yes mv foo risky/d || fail=1 -mv risky/d/foo . || fail=1 - -Exit $fail -- 2.47.2