From 44ccd1c4657703b15971b0670b9716a25244a358 Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Mon, 18 Sep 2017 18:54:52 -0700 Subject: [PATCH] copy: check for vulnerable target dirs * NEWS, doc/coreutils.texi (Target directory): Document this. * src/cp.c, src/install.c, src/ln.c, src/mv.c: Include targetdir.h. (target_directory_operand): Use the new targetdir_operand_type function to check for vulnerable target directories. * src/cp.c (stat_target_operand): New function. (target_directory_operand, do_copy): Use it. * src/local.mk (noinst_HEADERS): Add src/targetdir.h. (src_ginstall_SOURCES, src_cp_SOURCES, src_ln_SOURCES) (src_mv_SOURCES): Add src/targetdir.c. * src/targetdir.c, src/targetdir.h: New files. * tests/mv/vulnerable-target.sh: New test. * tests/local.mk (all_root_tests): Add 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 | 100 ++++++++++++++++++++++++++++++++++++++++++ src/targetdir.h | 4 ++ tests/local.mk | 1 + tests/mv/vulnerable-target.sh | 38 ++++++++++++++++ 11 files changed, 280 insertions(+), 45 deletions(-) create mode 100644 src/targetdir.c create mode 100644 src/targetdir.h create mode 100755 tests/mv/vulnerable-target.sh diff --git a/NEWS b/NEWS index ca36063..9a005c5 100644 --- a/NEWS +++ b/NEWS @@ -2,6 +2,17 @@ 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 eb17462..0c6e2c5 100644 --- a/doc/coreutils.texi +++ b/doc/coreutils.texi @@ -1282,7 +1282,38 @@ 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. Sometimes this behavior is not exactly +@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 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 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 6949a67..f84eac8 100644 --- a/src/cp.c +++ b/src/cp.c @@ -33,6 +33,7 @@ #include "ignore-value.h" #include "quote.h" #include "stat-time.h" +#include "targetdir.h" #include "utimens.h" #include "acl.h" @@ -567,6 +568,20 @@ 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, @@ -577,17 +592,17 @@ make_dir_parents_private (char const *const_dir, size_t src_offset, Otherwise, set *NEW_DST. */ static bool -target_directory_operand (char const *file, struct stat *st, bool *new_dst) +target_directory_operand (char *file, struct stat *st, bool *new_dst) { - 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; + 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; } /* Scan the arguments, and copy each by calling copy. @@ -623,7 +638,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. */ - ignore_value (target_directory_operand (file[n_files -1], &sb, &new_dst)); + stat_target_operand (file[n_files -1], &sb, &new_dst); } else if (!target_directory) { diff --git a/src/install.c b/src/install.c index 5b68261..6756667 100644 --- a/src/install.c +++ b/src/install.c @@ -42,6 +42,7 @@ #include "savewd.h" #include "selinux.h" #include "stat-time.h" +#include "targetdir.h" #include "utimens.h" #include "xstrtol.h" @@ -395,20 +396,29 @@ setdefaultfilecon (char const *file) directory if it referred to anything at all. */ static bool -target_directory_operand (char const *file) +target_directory_operand (char *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 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"), + 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)"), quoteaf (file)); - return is_a_dir; + return ty != TARGETDIR_NOT; } /* Report that directory DIR was made, if OPTIONS requests this. */ diff --git a/src/ln.c b/src/ln.c index e86f581..122be26 100644 --- a/src/ln.c +++ b/src/ln.c @@ -32,6 +32,7 @@ #include "hash-triple.h" #include "relpath.h" #include "same.h" +#include "targetdir.h" #include "yesno.h" #include "canonicalize.h" @@ -120,22 +121,33 @@ errno_nonexisting (int err) directory if it referred to anything at all. */ static bool -target_directory_operand (char const *file) +target_directory_operand (char *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)); - 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"), + 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)"), quoteaf (file)); - return is_a_dir; + return ty != TARGETDIR_NOT; } /* Return FROM represented as relative to the dir of TARGET. diff --git a/src/local.mk b/src/local.mk index 1cb6859..e0d66b7 100644 --- a/src/local.mk +++ b/src/local.mk @@ -59,6 +59,7 @@ noinst_HEADERS = \ src/remove.h \ src/set-fields.h \ src/system.h \ + src/targetdir.h \ src/uname.h EXTRA_DIST += \ @@ -344,8 +345,8 @@ copy_sources = \ transform = s/ginstall/install/;/libstdbuf/!$(program_transform_name) -src_ginstall_SOURCES = src/install.c src/prog-fprintf.c $(copy_sources) \ - $(selinux_sources) +src_ginstall_SOURCES = src/install.c src/prog-fprintf.c src/targetdir.c \ + $(copy_sources) $(selinux_sources) # This is for the '[' program. Automake transliterates '[' and '/' to '_'. src___SOURCES = src/lbracket.c @@ -353,7 +354,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 $(copy_sources) $(selinux_sources) +src_cp_SOURCES = src/cp.c src/targetdir.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 @@ -361,14 +362,16 @@ 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/relpath.c src/relpath.h \ + src/targetdir.c 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 $(copy_sources) $(selinux_sources) +src_mv_SOURCES = src/mv.c src/remove.c src/targetdir.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 fc1fca4..6df4c5d 100644 --- a/src/mv.c +++ b/src/mv.c @@ -32,6 +32,7 @@ #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). */ @@ -148,14 +149,23 @@ cp_option_init (struct cp_options *x) than nonexistence (errno == ENOENT). */ static bool -target_directory_operand (char const *file) +target_directory_operand (char *file) { struct stat st; - 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; + 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; } /* Move SOURCE onto DEST. Handles cross-file-system moves. diff --git a/src/targetdir.c b/src/targetdir.c new file mode 100644 index 0000000..e1ef16e --- /dev/null +++ b/src/targetdir.c @@ -0,0 +1,100 @@ +/* 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 + +#include +#include +#include +#include + +/* 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. */ + +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]) + || strcmp (lc, ".") == 0 || strcmp (lc, "..") == 0) + 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 new file mode 100644 index 0000000..29ea867 --- /dev/null +++ b/src/targetdir.h @@ -0,0 +1,4 @@ +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 732ec99..d9d3350 100644 --- a/tests/local.mk +++ b/tests/local.mk @@ -130,6 +130,7 @@ all_root_tests = \ tests/mkdir/smack-root.sh \ tests/mv/hardlink-case.sh \ tests/mv/sticky-to-xpart.sh \ + tests/mv/vulnerable-target.sh \ tests/rm/fail-2eperm.sh \ tests/rm/no-give-up.sh \ tests/rm/one-file-system.sh \ diff --git a/tests/mv/vulnerable-target.sh b/tests/mv/vulnerable-target.sh new file mode 100755 index 0000000..d3b99ce --- /dev/null +++ b/tests/mv/vulnerable-target.sh @@ -0,0 +1,38 @@ +#!/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_ + +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.7.4