NetBSD Problem Report #37957

From yx@fxp0-660.lrou.x.ua  Mon Feb  4 21:22:43 2008
Return-Path: <yx@fxp0-660.lrou.x.ua>
Received: from mail.netbsd.org (mail.netbsd.org [204.152.190.11])
	by narn.NetBSD.org (Postfix) with ESMTP id 1E91763B101
	for <gnats-bugs@gnats.NetBSD.org>; Mon,  4 Feb 2008 21:22:43 +0000 (UTC)
Message-Id: <200802042122.m14LMc5w020613@fxp0-660.lrou.x.ua>
Date: Mon, 4 Feb 2008 23:22:38 +0200 (EET)
From: Yakovetsky Vladimir <yx@x.ua>
Reply-To: yx@x.ua
To: gnats-bugs@gnats.NetBSD.org
Subject: non-critical buffer overflow in libbfd
X-Send-Pr-Version: 3.95

>Number:         37957
>Category:       lib
>Synopsis:       non-critical buffer overflow in libbfd
>Confidential:   no
>Severity:       non-critical
>Priority:       low
>Responsible:    lib-bug-people
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Mon Feb 04 21:25:00 +0000 2008
>Closed-Date:    Sun Jun 12 02:26:03 +0000 2016
>Last-Modified:  Sun Jun 12 02:26:03 +0000 2016
>Originator:     Yakovetsky Vladimir
>Release:        NetBSD 4.99.51
>Organization:
>Environment:
System: NetBSD lrou.x.ua 4.99.51 NetBSD 4.99.51 (lrou-1.880) #0: Tue Jan 29 17:27:05 EET 2008 Yakovetsky Vladimir <yx@x.ua>@lrou.x.ua:/sys/arch/i386/compile/lrou i386
Architecture: i386
Machine: i386

>Description:
	for example - broken `ar' tool from fortified system
	in result of non-critical buffer overflow in libbfd.

>How-To-Repeat:

1) build && install distribution with USE_FORT=yes
2) for example, try to use `ar' tool


	example:
# cd /usr/src && ./build distribution
...
rm -f libnbcompat.a
ar cq libnbcompat.a atoll.lo basename.lo dirname.lo fgetln.lo flock.lo fparseln.lo getmode.lo getopt_long.lo gettemp.lo heapsort.lo issetugid.lo lchflags.lo lchmod.lo lchown.lo libyywrap.lo md2.lo md2hl.lo md4c.lo md4hl.lo md5c.lo md5hl.lo mkdtemp.lo mkstemp.lo pread.lo putc_unlocked.lo pwcache.lo pwrite.lo pw_scan.lo raise_default_signal.lo rmd160.lo rmd160hl.lo setenv.lo setgroupent.lo setpassent.lo setprogname.lo sha1.lo sha1hl.lo sha2.lo sha256hl.lo sha384hl.lo sha512hl.lo snprintf.lo stat_flags.lo strlcat.lo strlcpy.lo strmode.lo strndup.lo strsep.lo strsuftoll.lo strtoll.lo unvis.lo vis.lo err.lo errx.lo verr.lo verrx.lo vwarn.lo vwarnx.lo warn.lo warnx.lo fts.lo glob.lo efun.lo bt_close.lo bt_conv.lo bt_debug.lo bt_delete.lo bt_get.lo bt_open.lo bt_overflow.lo bt_page.lo bt_put.lo bt_search.lo bt_seq.lo bt_split.lo bt_utils.lo db.lo hash.lo hash_bigkey.lo hash_buf.lo hash_func.lo hash_log2.lo hash_page.lo mpool.lo rec_close.lo rec_delete.lo rec_get.lo rec_open.lo rec_!
 put.lo rec_search.lo rec_seq.lo rec_utils.lo
[1]   Abort trap (core dumped) ar cq libnbcompa...

*** Failed target:  libnbcompat.a
*** Failed command: ar cq libnbcompat.a atoll.lo basename.lo dirname.lo fgetln.lo flock.lo fparseln.lo getmode.lo getopt_long.lo gettemp.lo heapsort.lo issetugid.lo lchflags.lo lchmod.lo lchown.lo libyywrap.lo md2.lo md2hl.lo md4c.lo md4hl.lo md5c.lo md5hl.lo mkdtemp.lo mkstemp.lo pread.lo putc_unlocked.lo pwcache.lo pwrite.lo pw_scan.lo raise_default_signal.lo rmd160.lo rmd160hl.lo setenv.lo setgroupent.lo setpassent.lo setprogname.lo sha1.lo sha1hl.lo sha2.lo sha256hl.lo sha384hl.lo sha512hl.lo snprintf.lo stat_flags.lo strlcat.lo strlcpy.lo strmode.lo strndup.lo strsep.lo strsuftoll.lo strtoll.lo unvis.lo vis.lo err.lo errx.lo verr.lo verrx.lo vwarn.lo vwarnx.lo warn.lo warnx.lo fts.lo glob.lo efun.lo bt_close.lo bt_conv.lo bt_debug.lo bt_delete.lo bt_get.lo bt_open.lo bt_overflow.lo bt_page.lo bt_put.lo bt_search.lo bt_seq.lo bt_split.lo bt_utils.lo db.lo hash.lo hash_bigkey.lo hash_buf.lo hash_func.lo hash_log2.lo hash_page.lo mpool.lo rec_close.lo rec_delete.lo rec_get!
 .lo rec_open.lo rec_put.lo rec_search.lo rec_seq.lo rec_utils.lo
*** Error code 134

Stop.
nbmake: stopped in /usr/src/tools/compat


	and message from kernel log:
Feb  4 18:51:44 lrou ar: buffer overflow detected; terminated


	and log from debug session:
...
Breakpoint 3, coff_write_armap (arch=0xbb9191c0, elength=0, map=0xbb92c000, 
    symbol_count=0, stridx=0) at /usr/src/gnu/dist/binutils/bfd/archive.c:2088
2088	  sprintf (hdr.ar_size, "%-10d", (int) mapsize);

(gdb) p sizeof(hdr.ar_size)
$1 = 10

(gdb) n
Program received signal SIGABRT, Aborted.
0xbbab1d47 in kill () from /usr/lib/libc.so.12



>Fix:
_bfd_ar_spacepad() instead sprintf() from bfd/archive.c,v 1.37 2005/03/11
(http://sourceware.org/cgi-bin/cvsweb.cgi/src/bfd/archive.c.diff?r1=1.36&r2=1.37&cvsroot=src)


--- gnu/dist/binutils/bfd/archive.c.orig
+++ gnu/dist/binutils/bfd/archive.c
@@ -159,6 +159,22 @@
 static const char * normalize (bfd *, const char *);

 
+void
+_bfd_ar_spacepad (char *p, size_t n, const char *fmt, long val)
+{
+  static char buf[20];
+  size_t len;
+  snprintf (buf, sizeof (buf), fmt, val);
+  len = strlen (buf);
+  if (len < n)
+    {
+      memcpy (p, buf, len);
+      memset (p + len, ' ', n - len);
+    }
+  else
+    memcpy (p, buf, n);
+}
+
 bfd_boolean
 _bfd_generic_mkarchive (bfd *abfd)
 {
@@ -1266,17 +1282,8 @@
 	      strptr[thislen + 1] = '\012';
 	    }
 	  hdr->ar_name[0] = ar_padchar (current);
-	  /* We know there will always be enough room (one of the few
-	     cases where you may safely use sprintf).  */
-	  sprintf ((hdr->ar_name) + 1, "%-d", (unsigned) (strptr - *tabloc));
-	  /* Kinda Kludgy.  We should just use the returned value of
-	     sprintf but not all implementations get this right.  */
-	  {
-	    char *temp = hdr->ar_name + 2;
-	    for (; temp < hdr->ar_name + maxname; temp++)
-	      if (*temp == '\0')
-		*temp = ' ';
-	  }
+          _bfd_ar_spacepad (hdr->ar_name + 1, maxname - 1, "%-ld",
+                            strptr - *tabloc);
 	  strptr += thislen + 1;
 	  if (trailing_slash)
 	    ++strptr;
@@ -1351,10 +1358,10 @@
   /* ar headers are space padded, not null padded!  */
   memset (hdr, ' ', sizeof (struct ar_hdr));

-  strncpy (hdr->ar_fmag, ARFMAG, 2);
+  memcpy (hdr->ar_fmag, ARFMAG, 2);

-  /* Goddamned sprintf doesn't permit MAXIMUM field lengths.  */
-  sprintf ((hdr->ar_date), "%-12ld", (long) status.st_mtime);
+  _bfd_ar_spacepad (hdr->ar_date, sizeof (hdr->ar_date), "%-12ld",
+                    status.st_mtime);
 #ifdef HPUX_LARGE_AR_IDS
   /* HP has a very "special" way to handle UID/GID's with numeric values
      > 99999.  */
@@ -1362,7 +1369,8 @@
     hpux_uid_gid_encode (hdr->ar_gid, (long) status.st_uid);
   else
 #endif
-    sprintf ((hdr->ar_uid), "%ld", (long) status.st_uid);
+    _bfd_ar_spacepad (hdr->ar_uid, sizeof (hdr->ar_uid), "%ld",
+                      status.st_uid);
 #ifdef HPUX_LARGE_AR_IDS
   /* HP has a very "special" way to handle UID/GID's with numeric values
      > 99999.  */
@@ -1370,20 +1378,13 @@
     hpux_uid_gid_encode (hdr->ar_uid, (long) status.st_gid);
   else
 #endif
-  sprintf ((hdr->ar_gid), "%ld", (long) status.st_gid);
-  sprintf ((hdr->ar_mode), "%-8o", (unsigned int) status.st_mode);
-  sprintf ((hdr->ar_size), "%-10ld", (long) status.st_size);
-  /* Correct for a lossage in sprintf whereby it null-terminates.  I cannot
-     understand how these C losers could design such a ramshackle bunch of
-     IO operations.  */
-  temp = (char *) hdr;
-  temp1 = temp + sizeof (struct ar_hdr) - 2;
-  for (; temp < temp1; temp++)
-    {
-      if (*temp == '\0')
-	*temp = ' ';
-    }
-  strncpy (hdr->ar_fmag, ARFMAG, 2);
+    _bfd_ar_spacepad (hdr->ar_gid, sizeof (hdr->ar_gid), "%ld",
+                      status.st_gid);
+  _bfd_ar_spacepad (hdr->ar_mode, sizeof (hdr->ar_mode), "%-8lo",
+                    status.st_mode);
+  _bfd_ar_spacepad (hdr->ar_size, sizeof (hdr->ar_size), "%-10ld",
+                    status.st_size);
+  memcpy (hdr->ar_fmag, ARFMAG, 2);
   ared->parsed_size = status.st_size;
   ared->arch_header = (char *) hdr;

@@ -1661,15 +1662,12 @@
     {
       struct ar_hdr hdr;

-      memset (&hdr, 0, sizeof (struct ar_hdr));
-      strcpy (hdr.ar_name, ename);
+      memset (&hdr, ' ', sizeof (struct ar_hdr));
+      memcpy (hdr.ar_name, ename, strlen (ename));
       /* Round size up to even number in archive header.  */
-      sprintf (&(hdr.ar_size[0]), "%-10d",
-	       (int) ((elength + 1) & ~(bfd_size_type) 1));
-      strncpy (hdr.ar_fmag, ARFMAG, 2);
-      for (i = 0; i < sizeof (struct ar_hdr); i++)
-	if (((char *) (&hdr))[i] == '\0')
-	  (((char *) (&hdr))[i]) = ' ';
+      _bfd_ar_spacepad (hdr.ar_size, sizeof (hdr.ar_size), "%-10ld",
+                        (elength + 1) & ~(bfd_size_type) 1);
+      memcpy (hdr.ar_fmag, ARFMAG, 2);
       if ((bfd_bwrite (&hdr, sizeof (struct ar_hdr), arch)
 	   != sizeof (struct ar_hdr))
 	  || bfd_bwrite (etable, elength, arch) != elength)
@@ -1918,20 +1916,18 @@
   firstreal = mapsize + elength + sizeof (struct ar_hdr) + SARMAG;

   stat (arch->filename, &statbuf);
-  memset (&hdr, 0, sizeof (struct ar_hdr));
-  sprintf (hdr.ar_name, RANLIBMAG);
+  memset (&hdr, ' ', sizeof (struct ar_hdr));
+  memcpy (hdr.ar_name, RANLIBMAG, strlen (RANLIBMAG));
   /* Remember the timestamp, to keep it holy.  But fudge it a little.  */
   bfd_ardata (arch)->armap_timestamp = statbuf.st_mtime + ARMAP_TIME_OFFSET;
   bfd_ardata (arch)->armap_datepos = (SARMAG
 				      + offsetof (struct ar_hdr, ar_date[0]));
-  sprintf (hdr.ar_date, "%ld", bfd_ardata (arch)->armap_timestamp);
-  sprintf (hdr.ar_uid, "%ld", (long) getuid ());
-  sprintf (hdr.ar_gid, "%ld", (long) getgid ());
-  sprintf (hdr.ar_size, "%-10d", (int) mapsize);
-  strncpy (hdr.ar_fmag, ARFMAG, 2);
-  for (i = 0; i < sizeof (struct ar_hdr); i++)
-    if (((char *) (&hdr))[i] == '\0')
-      (((char *) (&hdr))[i]) = ' ';
+  _bfd_ar_spacepad (hdr.ar_date, sizeof (hdr.ar_date), "%ld",
+                    bfd_ardata (arch)->armap_timestamp);
+  _bfd_ar_spacepad (hdr.ar_uid, sizeof (hdr.ar_uid), "%ld", getuid ());
+  _bfd_ar_spacepad (hdr.ar_gid, sizeof (hdr.ar_gid), "%ld", getgid ());
+  _bfd_ar_spacepad (hdr.ar_size, sizeof (hdr.ar_size), "%-10ld", mapsize);
+  memcpy (hdr.ar_fmag, ARFMAG, 2);
   if (bfd_bwrite (&hdr, sizeof (struct ar_hdr), arch)
       != sizeof (struct ar_hdr))
     return FALSE;
@@ -2019,11 +2015,9 @@
   bfd_ardata (arch)->armap_timestamp = archstat.st_mtime + ARMAP_TIME_OFFSET;

   /* Prepare an ASCII version suitable for writing.  */
-  memset (hdr.ar_date, 0, sizeof (hdr.ar_date));
-  sprintf (hdr.ar_date, "%ld", bfd_ardata (arch)->armap_timestamp);
-  for (i = 0; i < sizeof (hdr.ar_date); i++)
-    if (hdr.ar_date[i] == '\0')
-      (hdr.ar_date)[i] = ' ';
+  memset (hdr.ar_date, ' ', sizeof (hdr.ar_date));
+  _bfd_ar_spacepad (hdr.ar_date, sizeof (hdr.ar_date), "%ld",
+                    bfd_ardata (arch)->armap_timestamp);

   /* Write it into the file.  */
   bfd_ardata (arch)->armap_datepos = (SARMAG
@@ -2083,19 +2077,17 @@
 			     + sizeof (struct ar_hdr)
 			     + SARMAG);

-  memset (&hdr, 0, sizeof (struct ar_hdr));
+  memset (&hdr, ' ', sizeof (struct ar_hdr));
   hdr.ar_name[0] = '/';
-  sprintf (hdr.ar_size, "%-10d", (int) mapsize);
-  sprintf (hdr.ar_date, "%ld", (long) time (NULL));
+  _bfd_ar_spacepad (hdr.ar_size, sizeof (hdr.ar_size), "%-10ld",
+                    mapsize);
+  _bfd_ar_spacepad (hdr.ar_date, sizeof (hdr.ar_date), "%ld",
+                    time (NULL));
   /* This, at least, is what Intel coff sets the values to.  */
-  sprintf ((hdr.ar_uid), "%d", 0);
-  sprintf ((hdr.ar_gid), "%d", 0);
-  sprintf ((hdr.ar_mode), "%-7o", (unsigned) 0);
-  strncpy (hdr.ar_fmag, ARFMAG, 2);
-
-  for (i = 0; i < sizeof (struct ar_hdr); i++)
-    if (((char *) (&hdr))[i] == '\0')
-      (((char *) (&hdr))[i]) = ' ';
+  _bfd_ar_spacepad (hdr.ar_uid, sizeof (hdr.ar_uid), "%ld", 0);
+  _bfd_ar_spacepad (hdr.ar_gid, sizeof (hdr.ar_gid), "%ld", 0);
+  _bfd_ar_spacepad (hdr.ar_mode, sizeof (hdr.ar_mode), "%-7lo", 0);
+  memcpy (hdr.ar_fmag, ARFMAG, 2);

   /* Write the ar header for this item and the number of symbols.  */
   if (bfd_bwrite (&hdr, sizeof (struct ar_hdr), arch)

>Release-Note:

>Audit-Trail:

State-Changed-From-To: open->closed
State-Changed-By: dholland@NetBSD.org
State-Changed-When: Sun, 12 Jun 2016 02:26:03 +0000
State-Changed-Why:
Since this patch was from upstream I'm going to assume that the binutils
we have nowadays integrated it.


>Unformatted:

Home	PR Database Search