From 14d8fc718f0c872274b90991ee634b0cd8e1a6f0 Mon Sep 17 00:00:00 2001 From: Sergey Poznyakoff Date: Sat, 8 Feb 2020 13:01:47 +0200 Subject: [PATCH] Fix the --no-overwrite-dir option Given this option, tar failed to preserve permissions of empty directories and to create files under directories owned by the current user that did not have the S_IWUSR bit set. * src/extract.c (fd_chmod): Rename to fd_i_chmod. (fd_chmod): New function. (safe_dir_mode): New function. (extract_dir): Special handling for existing directories in --no-overwrite-dir mode. * tests/extrac23.at: New file. * tests/Makefile.am: Add new test case. * tests/testsuite.at: Likewise. --- src/extract.c | 128 ++++++++++++++++++++++++++++++--------------- tests/Makefile.am | 1 + tests/extrac23.at | 58 ++++++++++++++++++++ tests/testsuite.at | 1 + 4 files changed, 146 insertions(+), 42 deletions(-) create mode 100644 tests/extrac23.at diff --git a/src/extract.c b/src/extract.c index a4a35a57..5a38ba70 100644 --- a/src/extract.c +++ b/src/extract.c @@ -194,7 +194,7 @@ extr_init (void) /* Use fchmod if possible, fchmodat otherwise. */ static int -fd_chmod (int fd, char const *file, mode_t mode, int atflag) +fd_i_chmod (int fd, char const *file, mode_t mode, int atflag) { if (0 <= fd) { @@ -205,6 +205,42 @@ fd_chmod (int fd, char const *file, mode_t mode, int atflag) return fchmodat (chdir_fd, file, mode, atflag); } +/* A version of fd_i_chmod which gracefully handles several common error + conditions. Additional argument TYPEFLAG is the type of file in tar + notation. + */ +static int +fd_chmod(int fd, char const *file_name, int mode, int atflag, int typeflag) +{ + int chmod_errno = fd_i_chmod (fd, file_name, mode, atflag) == 0 ? 0 : errno; + + /* On Solaris, chmod may fail if we don't have PRIV_ALL, because + setuid-root files would otherwise be a backdoor. See + http://opensolaris.org/jive/thread.jspa?threadID=95826 + (2009-09-03). */ + if (chmod_errno == EPERM && (mode & S_ISUID) + && priv_set_restore_linkdir () == 0) + { + chmod_errno = fd_i_chmod (fd, file_name, mode, atflag) == 0 ? 0 : errno; + priv_set_remove_linkdir (); + } + + /* Linux fchmodat does not support AT_SYMLINK_NOFOLLOW, and + returns ENOTSUP even when operating on non-symlinks, try + again with the flag disabled if it does not appear to be + supported and if the file is not a symlink. This + introduces a race, alas. */ + if (atflag && typeflag != SYMTYPE && ! implemented (chmod_errno)) + chmod_errno = fd_i_chmod (fd, file_name, mode, 0) == 0 ? 0 : errno; + + if (chmod_errno && (typeflag != SYMTYPE || implemented (chmod_errno))) + { + errno = chmod_errno; + return -1; + } + return 0; +} + /* Use fchown if possible, fchownat otherwise. */ static int fd_chown (int fd, char const *file, uid_t uid, gid_t gid, int atflag) @@ -259,35 +295,8 @@ set_mode (char const *file_name, if (current_mode != mode) { - int chmod_errno = - fd_chmod (fd, file_name, mode, atflag) == 0 ? 0 : errno; - - /* On Solaris, chmod may fail if we don't have PRIV_ALL, because - setuid-root files would otherwise be a backdoor. See - http://opensolaris.org/jive/thread.jspa?threadID=95826 - (2009-09-03). */ - if (chmod_errno == EPERM && (mode & S_ISUID) - && priv_set_restore_linkdir () == 0) - { - chmod_errno = - fd_chmod (fd, file_name, mode, atflag) == 0 ? 0 : errno; - priv_set_remove_linkdir (); - } - - /* Linux fchmodat does not support AT_SYMLINK_NOFOLLOW, and - returns ENOTSUP even when operating on non-symlinks, try - again with the flag disabled if it does not appear to be - supported and if the file is not a symlink. This - introduces a race, alas. */ - if (atflag && typeflag != SYMTYPE && ! implemented (chmod_errno)) - chmod_errno = fd_chmod (fd, file_name, mode, 0) == 0 ? 0 : errno; - - if (chmod_errno - && (typeflag != SYMTYPE || implemented (chmod_errno))) - { - errno = chmod_errno; - chmod_error_details (file_name, mode); - } + if (fd_chmod (fd, file_name, mode, atflag, typeflag)) + chmod_error_details (file_name, mode); } } } @@ -975,6 +984,26 @@ is_directory_link (const char *file_name) return res; } +/* Given struct stat of a directory (or directory member) whose ownership + or permissions of will be restored later, return the temporary permissions + for that directory, sufficiently restrictive so that in the meantime + processes owned by other users do not inadvertently create files under this + directory that inherit the wrong owner, group, or permissions from the + directory. + + If not root, though, make the directory writeable and searchable at first, + so that files can be created under it. +*/ +static inline int +safe_dir_mode (struct stat const *st) +{ + return ((st->st_mode + & (0 < same_owner_option || 0 < same_permissions_option + ? S_IRWXU + : MODE_RWX)) + | (we_are_root ? 0 : MODE_WXUSR)); +} + /* Extractor functions for various member types */ static int @@ -1004,18 +1033,7 @@ extract_dir (char *file_name, int typeflag) else if (typeflag == GNUTYPE_DUMPDIR) skip_member (); - /* If ownership or permissions will be restored later, create the - directory with restrictive permissions at first, so that in the - meantime processes owned by other users do not inadvertently - create files under this directory that inherit the wrong owner, - group, or permissions from the directory. If not root, though, - make the directory writeable and searchable at first, so that - files can be created under it. */ - mode = ((current_stat_info.stat.st_mode - & (0 < same_owner_option || 0 < same_permissions_option - ? S_IRWXU - : MODE_RWX)) - | (we_are_root ? 0 : MODE_WXUSR)); + mode = safe_dir_mode (¤t_stat_info.stat); for (;;) { @@ -1031,6 +1049,7 @@ extract_dir (char *file_name, int typeflag) if (errno == EEXIST && (interdir_made || keep_directory_symlink_option + || old_files_option == NO_OVERWRITE_DIR_OLD_FILES || old_files_option == DEFAULT_OLD_FILES || old_files_option == OVERWRITE_OLD_FILES)) { @@ -1051,6 +1070,31 @@ extract_dir (char *file_name, int typeflag) repair_delayed_set_stat (file_name, &st); return 0; } + else if (old_files_option == NO_OVERWRITE_DIR_OLD_FILES) + { + /* Temporarily change the directory mode to a safe + value, to be able to create files in it, should + the need be. + */ + mode = safe_dir_mode (&st); + status = fd_chmod(-1, file_name, mode, + AT_SYMLINK_NOFOLLOW, DIRTYPE); + if (status == 0) + { + /* Store the actual directory mode, to be restored + later. + */ + current_stat_info.stat = st; + current_mode = mode & ~ current_umask; + current_mode_mask = MODE_RWX; + atflag = AT_SYMLINK_NOFOLLOW; + break; + } + else + { + chmod_error_details (file_name, mode); + } + } break; } } diff --git a/tests/Makefile.am b/tests/Makefile.am index 0369a950..31ae3460 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -121,6 +121,7 @@ TESTSUITE_AT = \ extrac19.at\ extrac20.at\ extrac21.at\ + extrac23.at\ filerem01.at\ filerem02.at\ dirrem01.at\ diff --git a/tests/extrac23.at b/tests/extrac23.at new file mode 100644 index 00000000..669d18b6 --- /dev/null +++ b/tests/extrac23.at @@ -0,0 +1,58 @@ +# Test suite for GNU tar. -*- Autotest -*- +# Copyright 2020 Free Software Foundation, Inc. +# +# This file is part of GNU tar. +# +# GNU tar 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. +# +# GNU tar 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 . +AT_SETUP([--no-overwrite-dir]) +AT_KEYWORDS([extract extrac23 no-overwrite-dir]) + +# Description: Implementation of the --no-overwrite-dir option was flawed in +# tar versions up to 1.32.90. This option is intended to preserve metadata +# of existing directories. In fact it worked only for non-empty directories. +# Moreover, if the actual directory was owned by the user tar runs as and the +# S_IWUSR bit was not set in its actual permissions, tar failed to create files +# in it. +# +# Reported by: Michael Kaufmann +# References: <20200207112934.Horde.anXzYhAj2CHiwUrw5CuT0G-@webmail.michael-kaufmann.ch>, +# https://lists.gnu.org/archive/html/bug-tar/2020-02/msg00003.html + +AT_TAR_CHECK([ +# Test if the directory permissions are restored properly. +mkdir dir +chmod 755 dir +tar cf a.tar dir +chmod 777 dir +tar -xf a.tar --no-overwrite-dir +genfile --stat=mode.777 dir + +# Test if temprorary permissions are set correctly to allow the owner +# to write to the directory. +genfile --file dir/file +tar cf a.tar dir +rm dir/file +chmod 400 dir +tar -xf a.tar --no-overwrite-dir +genfile --stat=mode.777 dir +chmod 700 dir +find dir +], +[0], +[777 +400 +dir +dir/file +]) +AT_CLEANUP diff --git a/tests/testsuite.at b/tests/testsuite.at index 2cc43a19..0620a3c7 100644 --- a/tests/testsuite.at +++ b/tests/testsuite.at @@ -343,6 +343,7 @@ m4_include([extrac19.at]) m4_include([extrac19.at]) m4_include([extrac20.at]) m4_include([extrac21.at]) +m4_include([extrac23.at]) m4_include([backup01.at]) -- 2.37.3