[Bug-tar] invalid archive provokes segfault

[Jim Meyering]

A corrupt extended-header archive causes tar to segfault:

  $ 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)]

I've tested this with what's in CVS and with tar-1.14.
The problem is in xheader.c's decode_record function.
If the strtoul-parsed length is longer than the header
and there is no `=' to stop the for-loop, then you lose.

BTW, using `*p' and `p' as arguments to strtoul is odd.
Doesn't that rely on undefined behavior?  Sort of like
using memcpy with overlapping regions?
The proposed patch fixes that, too.

2005-03-31  Jim Meyering  <address@hidden>

        * src/xheader.c (decode_record): New parameter, endp.
        Use it to ensure that the parsed record length is reasonable.
        Update callers.
        Don't rely on undefined behavior from strtoul.

Index: src/xheader.c
===================================================================
RCS file: /cvsroot/tar/tar/src/xheader.c,v
retrieving revision 1.23
diff -u -p -r1.23 xheader.c
--- src/xheader.c       3 Feb 2005 16:28:37 -0000       1.23
+++ src/xheader.c       31 Mar 2005 11:25:37 -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
@@ -468,6 +468,7 @@ xheader_protected_keyword_p (const char 
    Returns true on success, false otherwise. */
 static bool
 decode_record (char **p,
+              char *endp,
               void (*handler) (void *, char const *, char const *),
               void *data)
 {
@@ -475,18 +476,27 @@ decode_record (char **p,
   char const *keyword;
   char *start = *p;
   char endc;
+  char *tail;
 
   if (**p == 0)
     return false;
 
-  len = strtoul (*p, p, 10);
-  if (**p != ' ')
+  len = strtoul (*p, &tail, 10);
+  if (*tail != ' ')
     {
       ERROR ((0, 0,
        _("Malformed extended header: missing whitespace after the length")));
       return false;
     }
 
+  if (endp - *p < len)
+    {
+      ERROR ((0, 0,
+             _("Malformed extended header: invalid header record length")));
+      return false;
+    }
+  *p = tail;
+
   keyword = ++*p;
   for (;*p < start + len; ++*p)
     if (**p == '=')
@@ -549,7 +559,7 @@ xheader_decode (struct tar_stat_info *st
       char *endp = &extended_header.buffer[extended_header.size-1];
 
       while (p < endp)
-       if (!decode_record (&p, decx, st))
+       if (!decode_record (&p, endp + 1, decx, st))
          break;
     }
   run_override_list (keyword_override_list, st);
@@ -572,7 +582,7 @@ xheader_decode_global (void)
 
       xheader_list_destroy (&global_header_override_list);
       while (p < endp)
-       if (!decode_record (&p, decg, &global_header_override_list))
+       if (!decode_record (&p, endp + 1, decg, &global_header_override_list))
          break;
     }
 }
        Don't rely on undefined behavior from strtoul.