Merge "FORTIFY_SOURCE: introduce __strncpy_chk2"

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<size_t>(s - src);
+    if (__predict_false(s_copy_len > src_len)) {
+      __fortify_chk_fail("strncpy read beyond end of src buffer", 0);
+    }
+  }
+
+  return dst;
 }

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

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]);
+}