From patchwork Fri Aug 14 13:21:44 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Zack Weinberg X-Patchwork-Id: 8204 Received: (qmail 99482 invoked by alias); 14 Aug 2015 13:22:00 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 99466 invoked by uid 89); 14 Aug 2015 13:21:59 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.8 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-qk0-f175.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=subject:to:references:from:message-id:date:user-agent:mime-version :in-reply-to:content-type; bh=phD6/38/PbmRktKRXwhsynUH3pyZVc6lezkIT9GJcjA=; b=Fw8gbBCKCOAgdbg0Hlyqpa/jk3t3D+/G+SZE4BJqzPKDVVVICE3Eag8dd4bJRKmCAv L6+tSByNDugEdD2sgzBgyW1veC/61e6V0wEXLClcLYAiy/BMPoKfDBjujkDoceF6bdtW HB7i/Xtvn77zRyfSo4qWcSfD7CoBKh3xWBXzyzmUAMXfcXvhaPkojGuumm49Zl70/Z1L pKyS8A/6mlAD1PY5OTRCyUV0pjRmqW/5NYe5SqwS+qym/eNESy0MF2kZr7FZMWqdatkh HVyxX9C34N27liEc2BxhEGn4zVOPzhOJrjqpMaG/E8r8EiLwCdjFGwRXMtYUrja36z1O cgQQ== X-Received: by 10.55.40.162 with SMTP id o34mr53458705qko.106.1439558514653; Fri, 14 Aug 2015 06:21:54 -0700 (PDT) Subject: Re: Possible bug in fortified stpncpy To: GNU C Library References: <20150809033233.GO30077@vapier> <55C76182.2090503@panix.com> <20150814050835.GD17656@vapier> From: Zack Weinberg Message-ID: <55CDEB68.1060101@panix.com> Date: Fri, 14 Aug 2015 09:21:44 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Icedove/38.1.0 MIME-Version: 1.0 In-Reply-To: <20150814050835.GD17656@vapier> 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 From 16d9536daba72470108c88e1bd96393e5261c57b Mon Sep 17 00:00:00 2001 From: Zack Weinberg 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