From 908d78d2082f605318be6b6a774676e3319cf650 Mon Sep 17 00:00:00 2001 From: Paul Eggert Date: Mon, 4 Dec 2006 21:35:56 +0000 Subject: [PATCH] * NEWS: Fix some race conditions with tar -x --same-owner. * src/extract.c (ARCHIVED_PERMSTATS): Add a comment saying that S_IRWXG | S_IRWXO might be masked out. (set_mode): Set the mode if some bits were masked out originally. (set_stat): Don't chmod before chown, as that might temporarily grant permissions that we don't want to grant. The chmod was there only to work around broken hosts, so add a comment advising users not to use those broken hosts instead. (repair_delayed_set_stat, extract_dir): Remember to mask out current umask before inverting permissions. (extract_dir): If the owner might change, or if the mode has special bits, create the directory 700 at first, but restore it later. (open_output_file): New arg mode; all uses changed. (extract_file, extract_node, extract_fifo): If the owner might change, omit group and other bits at first, but restore them after changing the owner. --- ChangeLog | 19 +++++++++++ NEWS | 4 +++ src/extract.c | 94 +++++++++++++++++++++++++++++++-------------------- 3 files changed, 81 insertions(+), 36 deletions(-) diff --git a/ChangeLog b/ChangeLog index cdeceac..6e0db4a 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,22 @@ +2006-12-04 Paul Eggert + + * NEWS: Fix some race conditions with tar -x --same-owner. + * src/extract.c (ARCHIVED_PERMSTATS): Add a comment saying that + S_IRWXG | S_IRWXO might be masked out. + (set_mode): Set the mode if some bits were masked out originally. + (set_stat): Don't chmod before chown, as that might temporarily + grant permissions that we don't want to grant. The chmod was + there only to work around broken hosts, so add a comment advising + users not to use those broken hosts instead. + (repair_delayed_set_stat, extract_dir): + Remember to mask out current umask before inverting permissions. + (extract_dir): If the owner might change, or if the mode has + special bits, create the directory 700 at first, but restore it later. + (open_output_file): New arg mode; all uses changed. + (extract_file, extract_node, extract_fifo): If the owner might + change, omit group and other bits at first, but restore them after + changing the owner. + 2006-12-04 Jim Meyering * doc/tar.texi (Long Options): Remove doubled word. diff --git a/NEWS b/NEWS index 47ef203..fa09650 100644 --- a/NEWS +++ b/NEWS @@ -9,6 +9,10 @@ Please send GNU tar bug reports to records, but if you have one and you trust its contents, you can decode it with GNU tar 1.16 or earlier. +* Race conditions have been fixed that in some cases briefly allowed + files extracted by 'tar -x --same-owner' (or plain 'tar -x', when + running as root) to be accessed by users that they shouldn't have been. + version 1.16 - Sergey Poznyakoff, 2006-10-21 * After creating an archive, tar exits with code 1 if some files were diff --git a/src/extract.c b/src/extract.c index d391e3e..c699cb2 100644 --- a/src/extract.c +++ b/src/extract.c @@ -37,7 +37,8 @@ enum permstatus /* This file may have existed already; its permissions are unknown. */ UNKNOWN_PERMSTATUS, - /* This file was created using the permissions from the archive. */ + /* This file was created using the permissions from the archive, + except with S_IRWXG | S_IRWXO masked out if 0 < same_owner_option. */ ARCHIVED_PERMSTATUS, /* This is an intermediate directory; the archive did not specify @@ -149,12 +150,15 @@ set_mode (char const *file_name, { mode = stat_info->st_mode; - /* If we created the file and it has a usual mode, then its mode - is normally set correctly already. But on many hosts, some + /* If we created the file and it has a mode that we set already + with O_CREAT, then its mode is often set correctly already. + But if we are changing ownership, the mode's group and and + other permission bits were omitted originally, so it's less + likely that the mode is OK now. Also, on many hosts, some directories inherit the setgid bits from their parents, so we we must set directories' modes explicitly. */ - if (permstatus == ARCHIVED_PERMSTATUS - && ! (mode & ~ MODE_RWX) + if ((permstatus == ARCHIVED_PERMSTATUS + && ! (mode & ~ (0 < same_owner_option ? S_IRWXU : MODE_RWX))) && typeflag != DIRTYPE && typeflag != GNUTYPE_DUMPDIR) return; @@ -217,7 +221,7 @@ check_time (char const *file_name, struct timespec t) /* Restore stat attributes (owner, group, mode and times) for FILE_NAME, using information given in *ST. If CUR_INFO is nonzero, *CUR_INFO is the - file's currernt status. + file's current status. If not restoring permissions, invert the INVERT_PERMISSIONS bits from the file's current permissions. PERMSTATUS specifies the status of the file's permissions. @@ -265,11 +269,11 @@ set_stat (char const *file_name, } /* Some systems allow non-root users to give files away. Once this - done, it is not possible anymore to change file permissions, so we - have to set permissions prior to possibly giving files away. */ - - set_mode (file_name, &st->stat, cur_info, - invert_permissions, permstatus, typeflag); + done, it is not possible anymore to change file permissions. + However, setting file permissions now would be incorrect, since + they would apply to the wrong user, and there would be a race + condition. So, don't use systems that allow non-root users to + give files away. */ } if (0 < same_owner_option && permstatus != INTERDIR_PERMSTATUS) @@ -278,29 +282,36 @@ set_stat (char const *file_name, the symbolic link itself. In this case, a mere chown would change the attributes of the file the symbolic link is pointing to, and should be avoided. */ + int chown_result = 1; if (typeflag == SYMTYPE) { #if HAVE_LCHOWN - if (lchown (file_name, st->stat.st_uid, st->stat.st_gid) < 0) - chown_error_details (file_name, - st->stat.st_uid, st->stat.st_gid); + chown_result = lchown (file_name, st->stat.st_uid, st->stat.st_gid); #endif } else { - if (chown (file_name, st->stat.st_uid, st->stat.st_gid) < 0) - chown_error_details (file_name, - st->stat.st_uid, st->stat.st_gid); - - /* On a few systems, and in particular, those allowing to give files - away, changing the owner or group destroys the suid or sgid bits. - So let's attempt setting these bits once more. */ - if (st->stat.st_mode & (S_ISUID | S_ISGID | S_ISVTX)) - set_mode (file_name, &st->stat, 0, - invert_permissions, permstatus, typeflag); + chown_result = chown (file_name, st->stat.st_uid, st->stat.st_gid); + } + + if (chown_result == 0) + { + /* Changing the owner can flip st_mode bits in some cases, so + ignore cur_info if it might be obsolete now. */ + if (cur_info + && cur_info->st_mode & S_IXUGO + && cur_info->st_mode & (S_ISUID | S_ISGID)) + cur_info = NULL; } + else if (chown_result < 0) + chown_error_details (file_name, + st->stat.st_uid, st->stat.st_gid); } + + if (typeflag != SYMTYPE) + set_mode (file_name, &st->stat, cur_info, + invert_permissions, permstatus, typeflag); } /* Remember to restore stat attributes (owner, group, mode and times) @@ -374,7 +385,8 @@ repair_delayed_set_stat (char const *dir, data->atime = current_stat_info.atime; data->mtime = current_stat_info.mtime; data->invert_permissions = - (MODE_RWX & (current_stat_info.stat.st_mode ^ st.st_mode)); + ((current_stat_info.stat.st_mode ^ st.st_mode) + & MODE_RWX & ~ current_umask); data->permstatus = ARCHIVED_PERMSTATUS; return; } @@ -626,8 +638,9 @@ extract_dir (char *file_name, int typeflag) else if (typeflag == GNUTYPE_DUMPDIR) skip_member (); - mode = (current_stat_info.stat.st_mode | - (we_are_root ? 0 : MODE_WXUSR)) & MODE_RWX; + mode = current_stat_info.stat.st_mode | (we_are_root ? 0 : MODE_WXUSR); + if (0 < same_owner_option || current_stat_info.stat.st_mode & ~ MODE_RWX) + mode &= S_IRWXU; while ((status = mkdir (file_name, mode))) { @@ -670,7 +683,8 @@ extract_dir (char *file_name, int typeflag) { if (status == 0) delay_set_stat (file_name, ¤t_stat_info, - MODE_RWX & (mode ^ current_stat_info.stat.st_mode), + ((mode ^ current_stat_info.stat.st_mode) + & MODE_RWX & ~ current_umask), ARCHIVED_PERMSTATUS); else /* For an already existing directory, invert_perms must be 0 */ delay_set_stat (file_name, ¤t_stat_info, @@ -682,14 +696,13 @@ extract_dir (char *file_name, int typeflag) static int -open_output_file (char *file_name, int typeflag) +open_output_file (char *file_name, int typeflag, mode_t mode) { int fd; int openflag = (O_WRONLY | O_BINARY | O_CREAT | (old_files_option == OVERWRITE_OLD_FILES ? O_TRUNC : O_EXCL)); - mode_t mode = current_stat_info.stat.st_mode & MODE_RWX & ~ current_umask; #if O_CTG /* Contiguous files (on the Masscomp) have to specify the size in @@ -728,6 +741,9 @@ extract_file (char *file_name, int typeflag) size_t count; size_t written; int interdir_made = 0; + mode_t mode = current_stat_info.stat.st_mode & MODE_RWX & ~ current_umask; + mode_t invert_permissions = + 0 < same_owner_option ? mode & (S_IRWXG | S_IRWXO) : 0; /* FIXME: deal with protection issues. */ @@ -745,7 +761,7 @@ extract_file (char *file_name, int typeflag) else { do - fd = open_output_file (file_name, typeflag); + fd = open_output_file (file_name, typeflag, mode ^ invert_permissions); while (fd < 0 && maybe_recoverable (file_name, &interdir_made)); if (fd < 0) @@ -810,7 +826,7 @@ extract_file (char *file_name, int typeflag) if (to_command_option) sys_wait_command (); else - set_stat (file_name, ¤t_stat_info, NULL, 0, + set_stat (file_name, ¤t_stat_info, NULL, invert_permissions, (old_files_option == OVERWRITE_OLD_FILES ? UNKNOWN_PERMSTATUS : ARCHIVED_PERMSTATUS), typeflag); @@ -988,16 +1004,19 @@ extract_node (char *file_name, int typeflag) { int status; int interdir_made = 0; + mode_t mode = current_stat_info.stat.st_mode & ~ current_umask; + mode_t invert_permissions = + 0 < same_owner_option ? mode & (S_IRWXG | S_IRWXO) : 0; do - status = mknod (file_name, current_stat_info.stat.st_mode, + status = mknod (file_name, mode ^ invert_permissions, current_stat_info.stat.st_rdev); while (status && maybe_recoverable (file_name, &interdir_made)); if (status != 0) mknod_error (file_name); else - set_stat (file_name, ¤t_stat_info, NULL, 0, + set_stat (file_name, ¤t_stat_info, NULL, invert_permissions, ARCHIVED_PERMSTATUS, typeflag); return status; } @@ -1009,13 +1028,16 @@ extract_fifo (char *file_name, int typeflag) { int status; int interdir_made = 0; + mode_t mode = current_stat_info.stat.st_mode & ~ current_umask; + mode_t invert_permissions = + 0 < same_owner_option ? mode & (S_IRWXG | S_IRWXO) : 0; - while ((status = mkfifo (file_name, current_stat_info.stat.st_mode))) + while ((status = mkfifo (file_name, mode)) != 0) if (!maybe_recoverable (file_name, &interdir_made)) break; if (status == 0) - set_stat (file_name, ¤t_stat_info, NULL, 0, + set_stat (file_name, ¤t_stat_info, NULL, invert_permissions, ARCHIVED_PERMSTATUS, typeflag); else mkfifo_error (file_name); -- 2.45.2