[Bug-cpio] Re: Ubuntu patch for cpio CAN-2005-1111 and CAN-2005-1229

[t takahashi]

(gmail bugs.  i hope this gets posted only once.)

hi, i'm the original debian submitter (didn't know about CAN at the
time but somebody changed the title to include the CAN for me).

in my bug, i reported, in addition to absolute and .. pathnames, a
related and imho nastier exploit, in which you unpack *symlinks* that
point to ../../../../../../... but whose paths do not themselves have
to contain .., then unpack relative paths that do also do not have to
contain .. .

this is imho nastier because it is harder to detect.  it doesn't show
up with cpio -t; you have to use cpio -tv and grep.  it is also
nastier because most people think to check for absolute and .. but not
for symlinks.

is that nastier bug, in the same debian bug report, also fixed with
this patch?  if so, is it activated with --n-a-p or is symlink
behavior changed completely?

my layman's reading of the previous post suggests that it does not
address my second post to the debian bug report.

thanks.


On 9/29/05, Clint Adams <address@hidden> wrote:
> > They have it here:
> >
> > http://patches.ubuntu.com/patches/cpio.CAN-2005-1111_1229.diff
>
> This should be it ported forward to 2.6.  Savannah seems to have choked,
> so I can't check CVS HEAD.
>
> --- orig/src/copyin.c
> +++ mod/src/copyin.c
> @@ -389,19 +389,20 @@
>         continue;
>       }
>
> -      if (close (out_file_des) < 0)
> -     error (0, errno, "%s", d->header.c_name);
> -
>        /* File is now copied; set attributes.  */
>        if (!no_chown_flag)
> -     if ((chown (d->header.c_name,
> +     if ((fchown (out_file_des,
>                   set_owner_flag ? set_owner : d->header.c_uid,
>              set_group_flag ? set_group : d->header.c_gid) < 0)
>           && errno != EPERM)
>         error (0, errno, "%s", d->header.c_name);
>        /* chown may have turned off some permissions we wanted. */
> -      if (chmod (d->header.c_name, (int) d->header.c_mode) < 0)
> +      if (fchmod (out_file_des, (int) d->header.c_mode) < 0)
>       error (0, errno, "%s", d->header.c_name);
> +
> +      if (close (out_file_des) < 0)
> +     error (0, errno, "%s", d->header.c_name);
> +
>        if (retain_time_flag)
>       {
>         times.actime = times.modtime = d->header.c_mtime;
> @@ -1335,6 +1336,48 @@
>      }
>  }
>
> +/* Return a safer suffix of FILE_NAME, or "." if it has no safer
> +   suffix.  Check for fully specified file names and other atrocities.  */
> +static const char *
> +safer_name_suffix (char const *file_name)
> +{
> +  char const *p;
> +
> +  /* Skip leading file name components that contain "..", and leading
> slashes.  */
> +  size_t prefix_len = 0;
> +
> +  for (p = file_name; *p;)
> +    {
> +      if (p[0] == '.' && p[1] == '.' && ((p[2] == '/') || !p[2]))
> +     prefix_len = p + 2 - file_name;
> +
> +      do
> +     {
> +       if (*p++ == '/')
> +         break;
> +     }
> +      while (*p);
> +    }
> +
> +  for (p = file_name + prefix_len; *p == '/'; p++)
> +    continue;
> +  prefix_len = p - file_name;
> +
> +  if (prefix_len)
> +    {
> +      char *prefix = alloca (prefix_len + 1);
> +      memcpy (prefix, file_name, prefix_len);
> +      prefix[prefix_len] = '\0';
> +
> +      error (0, 0, ("Removing leading `%s' from member names"), prefix);
> +    }
> +
> +  if (!*p)
> +    p = ".";
> +
> +  return p;
> +}
> +
>  /* Read the collection from standard input and create files
>     in the file system.  */
>
> @@ -1445,18 +1488,11 @@
>
>        /* Do we have to ignore absolute paths, and if so, does the filename
>           have an absolute path?  */
> -      if (no_abs_paths_flag && file_hdr.c_name && file_hdr.c_name [0] ==
> '/')
> +      if (no_abs_paths_flag && file_hdr.c_name && file_hdr.c_name [0])
>       {
> -       char *p;
> +       const char *p = safer_name_suffix (file_hdr.c_name);
>
> -       p = file_hdr.c_name;
> -       while (*p == '/')
> -         ++p;
> -       if (*p == '\0')
> -         {
> -           strcpy (file_hdr.c_name, ".");
> -         }
> -       else
> +       if (p != file_hdr.c_name)
>           {
>                /* Debian hack: file_hrd.c_name is sometimes set to
>                   point to static memory by code in tar.c.  This
>
>
> --- orig/src/copypass.c
> +++ mod/src/copypass.c
> @@ -181,19 +181,20 @@
>               }
>             if (close (in_file_des) < 0)
>               error (0, errno, "%s", input_name.ds_string);
> -           if (close (out_file_des) < 0)
> -             error (0, errno, "%s", output_name.ds_string);
> -
>             /* Set the attributes of the new file.  */
>             if (!no_chown_flag)
> -             if ((chown (output_name.ds_string,
> +             if ((fchown (out_file_des,
>                           set_owner_flag ? set_owner : in_file_stat.st_uid,
>                     set_group_flag ? set_group : in_file_stat.st_gid) < 0)
>                   && errno != EPERM)
>                 error (0, errno, "%s", output_name.ds_string);
>             /* chown may have turned off some permissions we wanted. */
> -           if (chmod (output_name.ds_string, in_file_stat.st_mode) < 0)
> +           if (fchmod (out_file_des, in_file_stat.st_mode) < 0)
> +             error (0, errno, "%s", output_name.ds_string);
> +             
> +           if (close (out_file_des) < 0)
>               error (0, errno, "%s", output_name.ds_string);
> +
>             if (reset_time_flag)
>               {
>                 times.actime = in_file_stat.st_atime;
>
>
>
>
>
> _______________________________________________
> Bug-cpio mailing list
> address@hidden
> http://lists.gnu.org/mailman/listinfo/bug-cpio
>


--
Webmaster: do you believe that people will switch browsers to view
your page instead of going to your competitor?