diff mbox

Possible bug in fortified stpncpy

Message ID 55CDEB68.1060101@panix.com
State Committed
Headers

Commit Message

Zack Weinberg Aug. 14, 2015, 1:21 p.m. UTC 
  On 08/14/2015 01:08 AM, Mike Frysinger wrote:
> On 09 Aug 2015 10:19, Zack Weinberg wrote:
>> Patch is attached.  In addition to the actual two-character bugfix, I
>> enhanced debug/tst-chk1.c to catch this and similar bugs.  I also filed
>> https://sourceware.org/bugzilla/show_bug.cgi?id=18795 and I suspect this
>> needs to get backported as widely as possible.
> 
> it seems this patch isn't based on latest master.  can you rebase it and
> repost ?  make sure it doesn't depend on other changes you have locally.

Here you go.  Note the buglist in NEWS is not updated.

zw

Comments

Mike Frysinger Aug. 15, 2015, 2:45 a.m. UTC | #1 
pushed, thanks!
-mike
diff mbox

Patch

From 16d9536daba72470108c88e1bd96393e5261c57b Mon Sep 17 00:00:00 2001
From: Zack Weinberg <zackw@panix.com>
Date: Sun, 9 Aug 2015 10:12:27 -0400
Subject: [PATCH] Correct backwards conditional in stpncpy fortification.

	BZ #18975
	* string/bits/string3.h (stpncpy): Call __stpncpy_chk if the
	buffer length is known to be too large, not if it's known to be
	small enough.
	* debug/tst-chk1.c (do_test): Do all tests for catching a buffer
	overflow at runtime, involving a length parameter, twice: once
	with a compile-time constant length parameter, once without.
---
 debug/tst-chk1.c      | 169 +++++++++++++++++++++++++++++++++++++++++++++++---
 string/bits/string3.h |   2 +-
 2 files changed, 160 insertions(+), 11 deletions(-)

diff --git a/debug/tst-chk1.c b/debug/tst-chk1.c
index 53559e6..bded583 100644
--- a/debug/tst-chk1.c
+++ b/debug/tst-chk1.c
@@ -264,21 +264,39 @@  do_test (void)
 #endif
 
 #if __USE_FORTIFY_LEVEL >= 1
-  /* Now check if all buffer overflows are caught at runtime.  */
+  /* Now check if all buffer overflows are caught at runtime.
+     N.B. All tests involving a length parameter need to be done
+     twice: once with the length a compile-time constant, once without.  */
+
+  CHK_FAIL_START
+  memcpy (buf + 1, "abcdefghij", 10);
+  CHK_FAIL_END
 
   CHK_FAIL_START
   memcpy (buf + 1, "abcdefghij", l0 + 10);
   CHK_FAIL_END
 
   CHK_FAIL_START
+  memmove (buf + 2, buf + 1, 9);
+  CHK_FAIL_END
+
+  CHK_FAIL_START
   memmove (buf + 2, buf + 1, l0 + 9);
   CHK_FAIL_END
 
   CHK_FAIL_START
+  p = (char *) mempcpy (buf + 6, "abcde", 5);
+  CHK_FAIL_END
+
+  CHK_FAIL_START
   p = (char *) mempcpy (buf + 6, "abcde", l0 + 5);
   CHK_FAIL_END
 
   CHK_FAIL_START
+  memset (buf + 9, 'j', 2);
+  CHK_FAIL_END
+
+  CHK_FAIL_START
   memset (buf + 9, 'j', l0 + 2);
   CHK_FAIL_END
 
@@ -291,10 +309,18 @@  do_test (void)
   CHK_FAIL_END
 
   CHK_FAIL_START
+  strncpy (buf + 7, "X", 4);
+  CHK_FAIL_END
+
+  CHK_FAIL_START
   strncpy (buf + 7, "X", l0 + 4);
   CHK_FAIL_END
 
   CHK_FAIL_START
+  stpncpy (buf + 6, "cd", 5);
+  CHK_FAIL_END
+
+  CHK_FAIL_START
   stpncpy (buf + 6, "cd", l0 + 5);
   CHK_FAIL_END
 
@@ -304,6 +330,10 @@  do_test (void)
   CHK_FAIL_END
 
   CHK_FAIL_START
+  snprintf (buf + 8, 3, "%d", num2);
+  CHK_FAIL_END
+
+  CHK_FAIL_START
   snprintf (buf + 8, l0 + 3, "%d", num2);
   CHK_FAIL_END
 
@@ -316,29 +346,50 @@  do_test (void)
   CHK_FAIL_END
 # endif
 
-  memcpy (buf, str1 + 2, l0 + 9);
+  memcpy (buf, str1 + 2, 9);
   CHK_FAIL_START
   strcat (buf, "AB");
   CHK_FAIL_END
 
-  memcpy (buf, str1 + 3, l0 + 8);
+  memcpy (buf, str1 + 3, 8);
+  CHK_FAIL_START
+  strncat (buf, "ZYXWV", 3);
+  CHK_FAIL_END
+
+  memcpy (buf, str1 + 3, 8);
   CHK_FAIL_START
   strncat (buf, "ZYXWV", l0 + 3);
   CHK_FAIL_END
 
   CHK_FAIL_START
+  memcpy (a.buf1 + 1, "abcdefghij", 10);
+  CHK_FAIL_END
+
+  CHK_FAIL_START
   memcpy (a.buf1 + 1, "abcdefghij", l0 + 10);
   CHK_FAIL_END
 
   CHK_FAIL_START
+  memmove (a.buf1 + 2, a.buf1 + 1, 9);
+  CHK_FAIL_END
+
+  CHK_FAIL_START
   memmove (a.buf1 + 2, a.buf1 + 1, l0 + 9);
   CHK_FAIL_END
 
   CHK_FAIL_START
+  p = (char *) mempcpy (a.buf1 + 6, "abcde", 5);
+  CHK_FAIL_END
+
+  CHK_FAIL_START
   p = (char *) mempcpy (a.buf1 + 6, "abcde", l0 + 5);
   CHK_FAIL_END
 
   CHK_FAIL_START
+  memset (a.buf1 + 9, 'j', 2);
+  CHK_FAIL_END
+
+  CHK_FAIL_START
   memset (a.buf1 + 9, 'j', l0 + 2);
   CHK_FAIL_END
 
@@ -357,6 +408,10 @@  do_test (void)
   CHK_FAIL_END
 
   CHK_FAIL_START
+  strncpy (a.buf1 + (O + 6), "X", 4);
+  CHK_FAIL_END
+
+  CHK_FAIL_START
   strncpy (a.buf1 + (O + 6), "X", l0 + 4);
   CHK_FAIL_END
 
@@ -366,16 +421,20 @@  do_test (void)
   CHK_FAIL_END
 
   CHK_FAIL_START
+  snprintf (a.buf1 + (O + 7), 3, "%d", num2);
+  CHK_FAIL_END
+
+  CHK_FAIL_START
   snprintf (a.buf1 + (O + 7), l0 + 3, "%d", num2);
   CHK_FAIL_END
 # endif
 
-  memcpy (a.buf1, str1 + (3 - O), l0 + 8 + O);
+  memcpy (a.buf1, str1 + (3 - O), 8 + O);
   CHK_FAIL_START
   strcat (a.buf1, "AB");
   CHK_FAIL_END
 
-  memcpy (a.buf1, str1 + (4 - O), l0 + 7 + O);
+  memcpy (a.buf1, str1 + (4 - O), 7 + O);
   CHK_FAIL_START
   strncat (a.buf1, "ZYXWV", l0 + 3);
   CHK_FAIL_END
@@ -504,25 +563,47 @@  do_test (void)
 #endif
 
 #if __USE_FORTIFY_LEVEL >= 1
-  /* Now check if all buffer overflows are caught at runtime.  */
+  /* Now check if all buffer overflows are caught at runtime.
+     N.B. All tests involving a length parameter need to be done
+     twice: once with the length a compile-time constant, once without.  */
+
+  CHK_FAIL_START
+  wmemcpy (wbuf + 1, L"abcdefghij", 10);
+  CHK_FAIL_END
 
   CHK_FAIL_START
   wmemcpy (wbuf + 1, L"abcdefghij", l0 + 10);
   CHK_FAIL_END
 
   CHK_FAIL_START
+  wmemcpy (wbuf + 9, L"abcdefghij", 10);
+  CHK_FAIL_END
+
+  CHK_FAIL_START
   wmemcpy (wbuf + 9, L"abcdefghij", l0 + 10);
   CHK_FAIL_END
 
   CHK_FAIL_START
+  wmemmove (wbuf + 2, wbuf + 1, 9);
+  CHK_FAIL_END
+
+  CHK_FAIL_START
   wmemmove (wbuf + 2, wbuf + 1, l0 + 9);
   CHK_FAIL_END
 
   CHK_FAIL_START
+  wp = wmempcpy (wbuf + 6, L"abcde", 5);
+  CHK_FAIL_END
+
+  CHK_FAIL_START
   wp = wmempcpy (wbuf + 6, L"abcde", l0 + 5);
   CHK_FAIL_END
 
   CHK_FAIL_START
+  wmemset (wbuf + 9, L'j', 2);
+  CHK_FAIL_END
+
+  CHK_FAIL_START
   wmemset (wbuf + 9, L'j', l0 + 2);
   CHK_FAIL_END
 
@@ -535,6 +616,10 @@  do_test (void)
   CHK_FAIL_END
 
   CHK_FAIL_START
+  wcsncpy (wbuf + 7, L"X", 4);
+  CHK_FAIL_END
+
+  CHK_FAIL_START
   wcsncpy (wbuf + 7, L"X", l0 + 4);
   CHK_FAIL_END
 
@@ -547,32 +632,52 @@  do_test (void)
   CHK_FAIL_END
 
   CHK_FAIL_START
+  wcpncpy (wbuf + 6, L"cd", 5);
+  CHK_FAIL_END
+
+  CHK_FAIL_START
   wcpncpy (wbuf + 6, L"cd", l0 + 5);
   CHK_FAIL_END
 
-  wmemcpy (wbuf, wstr1 + 2, l0 + 9);
+  wmemcpy (wbuf, wstr1 + 2, 9);
   CHK_FAIL_START
   wcscat (wbuf, L"AB");
   CHK_FAIL_END
 
-  wmemcpy (wbuf, wstr1 + 3, l0 + 8);
+  wmemcpy (wbuf, wstr1 + 3, 8);
   CHK_FAIL_START
   wcsncat (wbuf, L"ZYXWV", l0 + 3);
   CHK_FAIL_END
 
   CHK_FAIL_START
+  wmemcpy (wa.buf1 + 1, L"abcdefghij", 10);
+  CHK_FAIL_END
+
+  CHK_FAIL_START
   wmemcpy (wa.buf1 + 1, L"abcdefghij", l0 + 10);
   CHK_FAIL_END
 
   CHK_FAIL_START
+  wmemmove (wa.buf1 + 2, wa.buf1 + 1, 9);
+  CHK_FAIL_END
+
+  CHK_FAIL_START
   wmemmove (wa.buf1 + 2, wa.buf1 + 1, l0 + 9);
   CHK_FAIL_END
 
   CHK_FAIL_START
+  wp = wmempcpy (wa.buf1 + 6, L"abcde", 5);
+  CHK_FAIL_END
+
+  CHK_FAIL_START
   wp = wmempcpy (wa.buf1 + 6, L"abcde", l0 + 5);
   CHK_FAIL_END
 
   CHK_FAIL_START
+  wmemset (wa.buf1 + 9, L'j', 2);
+  CHK_FAIL_END
+
+  CHK_FAIL_START
   wmemset (wa.buf1 + 9, L'j', l0 + 2);
   CHK_FAIL_END
 
@@ -591,15 +696,19 @@  do_test (void)
   CHK_FAIL_END
 
   CHK_FAIL_START
+  wcsncpy (wa.buf1 + (O + 6), L"X", 4);
+  CHK_FAIL_END
+
+  CHK_FAIL_START
   wcsncpy (wa.buf1 + (O + 6), L"X", l0 + 4);
   CHK_FAIL_END
 
-  wmemcpy (wa.buf1, wstr1 + (3 - O), l0 + 8 + O);
+  wmemcpy (wa.buf1, wstr1 + (3 - O), 8 + O);
   CHK_FAIL_START
   wcscat (wa.buf1, L"AB");
   CHK_FAIL_END
 
-  wmemcpy (wa.buf1, wstr1 + (4 - O), l0 + 7 + O);
+  wmemcpy (wa.buf1, wstr1 + (4 - O), 7 + O);
   CHK_FAIL_START
   wcsncat (wa.buf1, L"ZYXWV", l0 + 3);
   CHK_FAIL_END
@@ -884,6 +993,11 @@  do_test (void)
   if (read (fileno (stdin), buf, sizeof (buf) + 1) != sizeof (buf) + 1)
     FAIL ();
   CHK_FAIL_END
+
+  CHK_FAIL_START
+  if (read (fileno (stdin), buf, l0 + sizeof (buf) + 1) != sizeof (buf) + 1)
+    FAIL ();
+  CHK_FAIL_END
 #endif
 
   if (pread (fileno (stdin), buf, sizeof (buf) - 1, sizeof (buf) - 2)
@@ -904,6 +1018,12 @@  do_test (void)
       != sizeof (buf) + 1)
     FAIL ();
   CHK_FAIL_END
+
+  CHK_FAIL_START
+  if (pread (fileno (stdin), buf, l0 + sizeof (buf) + 1, 2 * sizeof (buf))
+      != sizeof (buf) + 1)
+    FAIL ();
+  CHK_FAIL_END
 #endif
 
   if (pread64 (fileno (stdin), buf, sizeof (buf) - 1, sizeof (buf) - 2)
@@ -924,6 +1044,12 @@  do_test (void)
       != sizeof (buf) + 1)
     FAIL ();
   CHK_FAIL_END
+
+  CHK_FAIL_START
+  if (pread64 (fileno (stdin), buf, l0 + sizeof (buf) + 1, 2 * sizeof (buf))
+      != sizeof (buf) + 1)
+    FAIL ();
+  CHK_FAIL_END
 #endif
 
   if (freopen (temp_filename, "r", stdin) == NULL)
@@ -1435,23 +1561,38 @@  do_test (void)
 
   fd_set s;
   FD_ZERO (&s);
+
   FD_SET (FD_SETSIZE - 1, &s);
 #if __USE_FORTIFY_LEVEL >= 1
   CHK_FAIL_START
   FD_SET (FD_SETSIZE, &s);
   CHK_FAIL_END
+
+  CHK_FAIL_START
+  FD_SET (l0 + FD_SETSIZE, &s);
+  CHK_FAIL_END
 #endif
+
   FD_CLR (FD_SETSIZE - 1, &s);
 #if __USE_FORTIFY_LEVEL >= 1
   CHK_FAIL_START
   FD_CLR (FD_SETSIZE, &s);
   CHK_FAIL_END
+
+  CHK_FAIL_START
+  FD_SET (l0 + FD_SETSIZE, &s);
+  CHK_FAIL_END
 #endif
+
   FD_ISSET (FD_SETSIZE - 1, &s);
 #if __USE_FORTIFY_LEVEL >= 1
   CHK_FAIL_START
   FD_ISSET (FD_SETSIZE, &s);
   CHK_FAIL_END
+
+  CHK_FAIL_START
+  FD_ISSET (l0 + FD_SETSIZE, &s);
+  CHK_FAIL_END
 #endif
 
   struct pollfd fds[1];
@@ -1462,12 +1603,20 @@  do_test (void)
   CHK_FAIL_START
   poll (fds, 2, 0);
   CHK_FAIL_END
+
+  CHK_FAIL_START
+  poll (fds, l0 + 2, 0);
+  CHK_FAIL_END
 #endif
   ppoll (fds, 1, NULL, NULL);
 #if __USE_FORTIFY_LEVEL >= 1
   CHK_FAIL_START
   ppoll (fds, 2, NULL, NULL);
   CHK_FAIL_END
+
+  CHK_FAIL_START
+  ppoll (fds, l0 + 2, NULL, NULL);
+  CHK_FAIL_END
 #endif
 
   return ret;
diff --git a/string/bits/string3.h b/string/bits/string3.h
index f482935..4d11aa6 100644
--- a/string/bits/string3.h
+++ b/string/bits/string3.h
@@ -136,7 +136,7 @@  __fortify_function char *
 __NTH (stpncpy (char *__dest, const char *__src, size_t __n))
 {
   if (__bos (__dest) != (size_t) -1
-      && (!__builtin_constant_p (__n) || __n <= __bos (__dest)))
+      && (!__builtin_constant_p (__n) || __n > __bos (__dest)))
     return __stpncpy_chk (__dest, __src, __n, __bos (__dest));
   return __stpncpy_alias (__dest, __src, __n);
 }
-- 
2.5.0