diff mbox

Possible bug in fortified stpncpy

Message ID 55C76182.2090503@panix.com
State New
Headers show

Commit Message

Zack Weinberg Aug. 9, 2015, 2:19 p.m. UTC 
On 08/08/2015 11:32 PM, Mike Frysinger wrote:
> On 08 Aug 2015 17:06, Zack Weinberg wrote:
>> [stpncpy] should call the runtime-checking function
>> if __n is not constant, or if __n is known to be LARGER
>> than the size of the destination.  Ne?
> 
> agreed.  feel like sending a patch ? :)

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.

(Is there a way to run just one subdirectory's tests?  This was tedious
to develop, what with string.h getting used basically everywhere.)

zw

Comments

Joseph Myers Aug. 9, 2015, 2:44 p.m. UTC | #1 
On Sun, 9 Aug 2015, Zack Weinberg wrote:

> (Is there a way to run just one subdirectory's tests?  This was tedious
> to develop, what with string.h getting used basically everywhere.)

I routinely use "make PARALLELMFLAGS=-j8 math/tests".

However, if changing a header such as string.h I think it's a good idea to 
run the full glibc build and full testsuite at least once, when you 
believe your patch is working.  That's the case even if you've verified 
that the patch doesn't change the installed shared libraries at all (for 
patches that aren't meant to change the installed libraries), because 
header changes can break building testcases - I found several times when 
fixing linknamespace bugs that a patch built glibc OK and passed conform/ 
tests but failed building a testcase in another directory.
Zack Weinberg Aug. 9, 2015, 2:47 p.m. UTC | #2 
On 08/09/2015 10:44 AM, Joseph Myers wrote:
> On Sun, 9 Aug 2015, Zack Weinberg wrote:
> 
>> (Is there a way to run just one subdirectory's tests?  This was tedious
>> to develop, what with string.h getting used basically everywhere.)
> 
> I routinely use "make PARALLELMFLAGS=-j8 math/tests".
> 
> However, if changing a header such as string.h I think it's a good idea to 
> run the full glibc build and full testsuite at least once, when you 
> believe your patch is working.

Well, yes, of course.  I was just looking for quicker development
turnaround in case the enhanced test case had revealed more bugs of this
type - cycle on debug/tests until *that* works, then run the whole
thing.  (In the event, no more bugs were found.)

zw
Mike Frysinger Aug. 10, 2015, 3:34 a.m. UTC | #3 
On 09 Aug 2015 10:19, Zack Weinberg wrote:
> On 08/08/2015 11:32 PM, Mike Frysinger wrote:
> > On 08 Aug 2015 17:06, Zack Weinberg wrote:
> >> [stpncpy] should call the runtime-checking function
> >> if __n is not constant, or if __n is known to be LARGER
> >> than the size of the destination.  Ne?
> > 
> > agreed.  feel like sending a patch ? :)
> 
> Patch is attached.  In addition to the actual two-character bugfix, I
> enhanced debug/tst-chk1.c to catch this and similar bugs.

looks fine to me.  if no one else has feedback i'll push it in a bit.

> I suspect this needs to get backported as widely as possible.

i don't see why.  it's simply expanding checking coverage, not fixing a
function call that actively exploits things.  it's also a fix to the API
so any code that wants to take advantage of it needs to be recompiled.

i would cherry pick to 2.22 because that was just branched, but that's it.
-mike
Mike Frysinger Aug. 14, 2015, 5:08 a.m. UTC | #4 
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.
-mike
diff mbox

Patch

	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/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);
 }
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,13 +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;
-- 
2.5.0