From 93501d3ab81156bcef251bb817a49e9ca46a6ec1 Mon Sep 17 00:00:00 2001 From: Nick Kralevich Date: Wed, 28 Aug 2013 10:47:43 -0700 Subject: [PATCH] FORTIFY_SOURCE: introduce __strncpy_chk2 This change detects programs reading beyond the end of "src" when calling strncpy. Change-Id: Ie1b42de923385d62552b22c27b2d4713ab77ee03 --- libc/bionic/__strncpy_chk.cpp | 50 +++++++++++++++++++++++++---- libc/include/string.h | 18 +++++++++-- tests/fortify_test.cpp | 60 +++++++++++++++++++++++++++++++++++ 3 files changed, 119 insertions(+), 9 deletions(-) diff --git a/libc/bionic/__strncpy_chk.cpp b/libc/bionic/__strncpy_chk.cpp index b01879cba..6b1a3c77f 100644 --- a/libc/bionic/__strncpy_chk.cpp +++ b/libc/bionic/__strncpy_chk.cpp @@ -41,13 +41,51 @@ * This strncpy check is called if _FORTIFY_SOURCE is defined and * greater than 0. */ -extern "C" char *__strncpy_chk (char *dest, const char *src, +extern "C" char* __strncpy_chk(char* __restrict dest, const char* __restrict src, size_t len, size_t dest_len) { - if (__predict_false(len > dest_len)) { - __fortify_chk_fail("strncpy buffer overflow", - BIONIC_EVENT_STRNCPY_BUFFER_OVERFLOW); - } + if (__predict_false(len > dest_len)) { + __fortify_chk_fail("strncpy dest buffer overflow", + BIONIC_EVENT_STRNCPY_BUFFER_OVERFLOW); + } - return strncpy(dest, src, len); + return strncpy(dest, src, len); +} + +/* + * __strncpy_chk2 + * + * This is a variant of __strncpy_chk, but it also checks to make + * sure we don't read beyond the end of "src". The code for this is + * based on the original version of strncpy, but modified to check + * how much we read from "src" at the end of the copy operation. + */ +extern "C" char* __strncpy_chk2(char* __restrict dst, const char* __restrict src, + size_t n, size_t dest_len, size_t src_len) +{ + if (__predict_false(n > dest_len)) { + __fortify_chk_fail("strncpy dest buffer overflow", + BIONIC_EVENT_STRNCPY_BUFFER_OVERFLOW); + } + if (n != 0) { + char* d = dst; + const char* s = src; + + do { + if ((*d++ = *s++) == 0) { + /* NUL pad the remaining n-1 bytes */ + while (--n != 0) { + *d++ = 0; + } + break; + } + } while (--n != 0); + + size_t s_copy_len = static_cast(s - src); + if (__predict_false(s_copy_len > src_len)) { + __fortify_chk_fail("strncpy read beyond end of src buffer", 0); + } + } + + return dst; } diff --git a/libc/include/string.h b/libc/include/string.h index 7801ee987..540939144 100644 --- a/libc/include/string.h +++ b/libc/include/string.h @@ -119,14 +119,26 @@ char* strcpy(char* __restrict dest, const char* __restrict src) { } __errordecl(__strncpy_error, "strncpy called with size bigger than buffer"); +extern char* __strncpy_chk2(char* __restrict, const char* __restrict, size_t, size_t, size_t); __BIONIC_FORTIFY_INLINE char* strncpy(char* __restrict dest, const char* __restrict src, size_t n) { - size_t bos = __bos(dest); - if (__builtin_constant_p(n) && (n > bos)) { + size_t bos_dest = __bos(dest); + size_t bos_src = __bos(src); + if (__builtin_constant_p(n) && (n > bos_dest)) { __strncpy_error(); } - return __builtin___strncpy_chk(dest, src, n, bos); + + if (bos_src == __BIONIC_FORTIFY_UNKNOWN_SIZE) { + return __builtin___strncpy_chk(dest, src, n, bos_dest); + } + + size_t slen = __builtin_strlen(src); + if (__builtin_constant_p(slen)) { + return __builtin___strncpy_chk(dest, src, n, bos_dest); + } + + return __strncpy_chk2(dest, src, n, bos_dest, bos_src); } __BIONIC_FORTIFY_INLINE diff --git a/tests/fortify_test.cpp b/tests/fortify_test.cpp index d8f0e7619..aa1373617 100644 --- a/tests/fortify_test.cpp +++ b/tests/fortify_test.cpp @@ -48,6 +48,19 @@ TEST(DEATHTEST, strncpy_fortified2) { } #endif +#ifndef __clang__ +// This test is disabled in clang because clang doesn't properly detect +// this buffer overflow. TODO: Fix clang. +TEST(DEATHTEST, strncpy2_fortified2) { + ::testing::FLAGS_gtest_death_test_style = "threadsafe"; + foo myfoo; + memset(&myfoo, 0, sizeof(myfoo)); + myfoo.one[0] = 'A'; // not null terminated string + ASSERT_EXIT(strncpy(myfoo.b, myfoo.one, sizeof(myfoo.b)), + testing::KilledBySignal(SIGABRT), ""); +} +#endif + #ifndef __clang__ // This test is disabled in clang because clang doesn't properly detect // this buffer overflow. TODO: Fix clang. @@ -481,6 +494,14 @@ TEST(DEATHTEST, strncpy_fortified) { ASSERT_EXIT(strncpy(bufb, bufa, n), testing::KilledBySignal(SIGABRT), ""); } +TEST(DEATHTEST, strncpy2_fortified) { + ::testing::FLAGS_gtest_death_test_style = "threadsafe"; + char dest[11]; + char src[10]; + memcpy(src, "0123456789", sizeof(src)); // src is not null terminated + ASSERT_EXIT(strncpy(dest, src, sizeof(dest)), testing::KilledBySignal(SIGABRT), ""); +} + TEST(DEATHTEST, snprintf_fortified) { ::testing::FLAGS_gtest_death_test_style = "threadsafe"; char bufa[15]; @@ -657,3 +678,42 @@ TEST(TEST_NAME, strcat2) { ASSERT_EQ('7', buf[8]); ASSERT_EQ('\0', buf[9]); } + +TEST(TEST_NAME, strncpy) { + char src[10]; + char dst[10]; + memcpy(src, "0123456789", sizeof(src)); // non null terminated string + strncpy(dst, src, sizeof(dst)); + ASSERT_EQ('0', dst[0]); + ASSERT_EQ('1', dst[1]); + ASSERT_EQ('2', dst[2]); + ASSERT_EQ('3', dst[3]); + ASSERT_EQ('4', dst[4]); + ASSERT_EQ('5', dst[5]); + ASSERT_EQ('6', dst[6]); + ASSERT_EQ('7', dst[7]); + ASSERT_EQ('8', dst[8]); + ASSERT_EQ('9', dst[9]); +} + +TEST(TEST_NAME, strncpy2) { + char src[10]; + char dst[15]; + memcpy(src, "012345678\0", sizeof(src)); + strncpy(dst, src, sizeof(dst)); + ASSERT_EQ('0', dst[0]); + ASSERT_EQ('1', dst[1]); + ASSERT_EQ('2', dst[2]); + ASSERT_EQ('3', dst[3]); + ASSERT_EQ('4', dst[4]); + ASSERT_EQ('5', dst[5]); + ASSERT_EQ('6', dst[6]); + ASSERT_EQ('7', dst[7]); + ASSERT_EQ('8', dst[8]); + ASSERT_EQ('\0', dst[9]); + ASSERT_EQ('\0', dst[10]); + ASSERT_EQ('\0', dst[11]); + ASSERT_EQ('\0', dst[12]); + ASSERT_EQ('\0', dst[13]); + ASSERT_EQ('\0', dst[14]); +}