* 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.

diff --git a/ChangeLog b/ChangeLog
index cdeceacc4d36a11f74cf2c95730231f30ccfb637..6e0db4ac0588d6bf0ff98c64d613986570b7cf09 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,22 @@
+2006-12-04  Paul Eggert  <eggert@cs.ucla.edu>
+
+       * 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  <jim@meyering.net>
 
        * doc/tar.texi (Long Options): Remove doubled word.

diff --git a/NEWS b/NEWS
index 47ef203a0b7c2f1b90c396eed0af43f36ce87cd1..fa096500758cc21d3e7e47b2450cb99d233c09b1 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -9,6 +9,10 @@ Please send GNU tar bug reports to <bug-tar@gnu.org>
   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 d391e3e3372c0340b58e398203f2b5e6197ce5fe..c699cb239abff2828ec24f44576f6d4b9588f0b6 100644 (file)
--- 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, &current_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, &current_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, &current_stat_info, NULL, 0,
+    set_stat (file_name, &current_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, &current_stat_info, NULL, 0,
+    set_stat (file_name, &current_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, &current_stat_info, NULL, 0,
+    set_stat (file_name, &current_stat_info, NULL, invert_permissions,
              ARCHIVED_PERMSTATUS, typeflag);
   else
     mkfifo_error (file_name);