From 8ff5e0ec497613e578219c9c3d49053c550b9269 Mon Sep 17 00:00:00 2001 From: Zack Weinberg Date: Fri, 14 Aug 2015 09:21:44 -0400 Subject: [PATCH] stpncpy: fix size checking [BZ #18975] I think the last clause of the conditional, || __n <= __bos (__dest) may be backward. The code 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. --- ChangeLog | 10 +++ NEWS | 4 +- debug/tst-chk1.c | 169 +++++++++++++++++++++++++++++++++++++++++++++++--- string/bits/string3.h | 2 +- 4 files changed, 172 insertions(+), 13 deletions(-) diff --git a/ChangeLog b/ChangeLog index f510bea..67d3517 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,13 @@ +2015-08-15 Zack Weinberg + + [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. + 2015-08-14 Joseph Myers [BZ #18824] diff --git a/NEWS b/NEWS index 088969c..fbbcddb 100644 --- a/NEWS +++ b/NEWS @@ -10,8 +10,8 @@ Version 2.23 * The following bugs are resolved with this release: 16517, 16519, 16520, 16734, 17905, 18086, 18265, 18480, 18525, 18618, - 18647, 18661, 18674, 18778, 18781, 18787, 18789, 18790, 18820, 18824. - + 18647, 18661, 18674, 18778, 18781, 18787, 18789, 18790, 18820, 18824, + 18975. Version 2.22 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.7.4