Re: [Bug-tar] invalid archive provokes segfault

[Paul Eggert]

Jim Meyering <address@hidden> writes:

>   $ touch g
>   $ tar --pax-option=exthdr.name=X -c -f k g
>   $ perl -pi -e 's/30 ....../99999999 /;s/=/./g' k
>   $ ./tar tf k
>   [Exit 139 (SEGV)]

Thanks for reporting this!  I went over decode_record
with a fine-toothed comb and installed the following
more-extensive patch.

2005-04-02  Paul Eggert  <address@hidden>

        * src/xheader.c (decode_record): Don't dump core when given
        a corrupted extended header.  Problem reported by Jim Meyering.
        Also, check for other ways that the header might be invalid,
        e.g., missing newline at end.  Do not allow keys with nulls.
        Allow blanks before and after length, as POSIX requires.
        Do not allow leading "-" in length.  Check for length overflow.
        (xheader_decode, xheader_decode_global): Let decode_record
        check for exhaustion of record.
        (xheader_read): Null-terminate the extended record;
        decode_record relies on this.

Index: src/xheader.c
===================================================================
RCS file: /cvsroot/tar/tar/src/xheader.c,v
retrieving revision 1.23
diff -p -u -r1.23 xheader.c
--- src/xheader.c       3 Feb 2005 16:28:37 -0000       1.23
+++ src/xheader.c       2 Apr 2005 06:33:58 -0000
@@ -1,6 +1,6 @@
 /* POSIX extended headers for tar.
 
-   Copyright (C) 2003, 2004 Free Software Foundation, Inc.
+   Copyright (C) 2003, 2004, 2005 Free Software Foundation, Inc.
 
    This program is free software; you can redistribute it and/or modify it
    under the terms of the GNU General Public License as published by the
@@ -463,51 +463,70 @@ xheader_protected_keyword_p (const char 
   return false;
 }
 
-/* Decodes a single extended header record. Advances P to the next
-   record.
-   Returns true on success, false otherwise. */
+/* Decode a single extended header record, advancing *PTR to the next record.
+   Return true on success, false otherwise.  */
 static bool
-decode_record (char **p,
+decode_record (char **ptr,
               void (*handler) (void *, char const *, char const *),
               void *data)
 {
-  size_t len;
+  char *start = *ptr;
+  char *p = start;
+  unsigned long int len;
+  char *len_lim;
   char const *keyword;
-  char *start = *p;
-  char endc;
+  char *nextp;
+  size_t len_max = extended_header.buffer + extended_header.size - start;
 
-  if (**p == 0)
-    return false;
+  while (*p == ' ' || *p == '\t')
+    p++;
 
-  len = strtoul (*p, p, 10);
-  if (**p != ' ')
+  if (! ISDIGIT (*p))
     {
-      ERROR ((0, 0,
-       _("Malformed extended header: missing whitespace after the length")));
+      if (*p)
+       ERROR ((0, 0, _("Malformed extended header: missing length")));
       return false;
     }
 
-  keyword = ++*p;
-  for (;*p < start + len; ++*p)
-    if (**p == '=')
-      break;
+  errno = 0;
+  len = strtoul (p, &len_lim, 10);
 
-  if (**p != '=')
+  if (len_max < len || (len == ULONG_MAX && errno == ERANGE))
     {
-      ERROR ((0, 0, _("Malformed extended header: missing equal sign")));
+      ERROR ((0, 0, _("Malformed extended header: length out of range")));
       return false;
     }
 
-  **p = 0;
+  nextp = start + len;
 
-  endc = start[len-1];
-  start[len-1] = 0;
+  for (p = len_lim; *p == ' ' || *p == '\t'; p++)
+    continue;
+  if (p == len_lim)
+    {
+      ERROR ((0, 0,
+             _("Malformed extended header: missing blank after length")));
+      return false;
+    }
 
-  handler (data, keyword, *p + 1);
+  keyword = p;
+  p = strchr (p, '=');
+  if (! (p && p < nextp))
+    {
+      ERROR ((0, 0, _("Malformed extended header: missing equal sign")));
+      return false;
+    }
+
+  if (nextp[-1] != '\n')
+    {
+      ERROR ((0, 0, _("Malformed extended header: missing newline")));
+      return false;
+    }
 
-  start[len-1] = endc;
-  **p = '=';
-  *p = &start[len];
+  *p = nextp[-1] = '\0';
+  handler (data, keyword, p + 1);
+  *p = '=';
+  nextp[-1] = '\n';
+  *ptr = nextp;
   return true;
 }
 
@@ -546,11 +565,8 @@ xheader_decode (struct tar_stat_info *st
   if (extended_header.size)
     {
       char *p = extended_header.buffer + BLOCKSIZE;
-      char *endp = &extended_header.buffer[extended_header.size-1];
-
-      while (p < endp)
-       if (!decode_record (&p, decx, st))
-         break;
+      while (decode_record (&p, decx, st))
+       continue;
     }
   run_override_list (keyword_override_list, st);
 }
@@ -568,12 +584,10 @@ xheader_decode_global (void)
   if (extended_header.size)
     {
       char *p = extended_header.buffer + BLOCKSIZE;
-      char *endp = &extended_header.buffer[extended_header.size-1];
 
       xheader_list_destroy (&global_header_override_list);
-      while (p < endp)
-       if (!decode_record (&p, decg, &global_header_override_list))
-         break;
+      while (decode_record (&p, decg, &global_header_override_list))
+       continue;
     }
 }
 
@@ -615,6 +629,7 @@ xheader_read (union block *p, size_t siz
   extended_header.size = size;
   nblocks = (size + BLOCKSIZE - 1) / BLOCKSIZE;
   extended_header.buffer = xmalloc (size + 1);
+  extended_header.buffer[size] = '\0';
 
   do
     {