* 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.
 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.
 
   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
 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 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
   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;
 
     {
       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.  */
         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;
          && 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
 /* 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.
    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
        }
 
       /* 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)
     }
 
   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.  */
         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 (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
        {
 #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)
 }
 
 /* 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 =
          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;
        }
          data->permstatus = ARCHIVED_PERMSTATUS;
          return;
        }


@@ -626,8 +638,9 @@ extract_dir (char *file_name, int typeflag)
   else if (typeflag == GNUTYPE_DUMPDIR)
     skip_member ();
 
   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)))
     {
 
   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,
     {
       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,
                        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
 
 
 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));
 {
   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
 
 #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;
   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.  */
 
 
   /* FIXME: deal with protection issues.  */
 


@@ -745,7 +761,7 @@ extract_file (char *file_name, int typeflag)
   else
     {
       do
   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)
       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
   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);
              (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;
 {
   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
 
   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
                    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;
 }
              ARCHIVED_PERMSTATUS, typeflag);
   return status;
 }


@@ -1009,13 +1028,16 @@ extract_fifo (char *file_name, int typeflag)
 {
   int status;
   int interdir_made = 0;
 {
   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)
     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);
              ARCHIVED_PERMSTATUS, typeflag);
   else
     mkfifo_error (file_name);