From 65c99de2cb7a569ea17ca35e2f8f1e033421864b Mon Sep 17 00:00:00 2001 From: Nick Kralevich Date: Wed, 9 Oct 2013 13:44:38 -0700 Subject: [PATCH] FORTIFY_SOURCE: fortify read() Change-Id: I3d7b4ec86d04efb865117ce7629a2e26917f3331 --- libc/Android.mk | 1 + libc/bionic/__read_chk.cpp | 43 ++++++++++++++++++++++++++++++++++++++ libc/include/unistd.h | 33 +++++++++++++++++++++++++++++ tests/fortify_test.cpp | 10 +++++++++ 4 files changed, 87 insertions(+) create mode 100644 libc/bionic/__read_chk.cpp diff --git a/libc/Android.mk b/libc/Android.mk index f4fdb4c37..b63de889a 100644 --- a/libc/Android.mk +++ b/libc/Android.mk @@ -198,6 +198,7 @@ libc_common_src_files += \ bionic/__memcpy_chk.cpp \ bionic/__memmove_chk.cpp \ bionic/__memset_chk.cpp \ + bionic/__read_chk.cpp \ bionic/__recvfrom_chk.cpp \ bionic/__strcat_chk.cpp \ bionic/__strchr_chk.cpp \ diff --git a/libc/bionic/__read_chk.cpp b/libc/bionic/__read_chk.cpp new file mode 100644 index 000000000..d7133f1cd --- /dev/null +++ b/libc/bionic/__read_chk.cpp @@ -0,0 +1,43 @@ +/* + * Copyright (C) 2013 The Android Open Source Project + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions + * are met: + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * * Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in + * the documentation and/or other materials provided with the + * distribution. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS + * FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE + * COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, + * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, + * BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS + * OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED + * AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, + * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT + * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF + * SUCH DAMAGE. + */ + +#undef _FORTIFY_SOURCE +#include +#include "libc_logging.h" + +extern "C" ssize_t __read_chk(int fd, void* buf, size_t count, size_t buf_size) { + if (__predict_false(count > buf_size)) { + __fortify_chk_fail("read prevented write past end of buffer", 0); + } + + if (__predict_false(count > SSIZE_MAX)) { + __fortify_chk_fail("read count > SSIZE_MAX", 0); + } + + return read(fd, buf, count); +} diff --git a/libc/include/unistd.h b/libc/include/unistd.h index 60964f0f7..fb1f663da 100644 --- a/libc/include/unistd.h +++ b/libc/include/unistd.h @@ -202,6 +202,39 @@ extern int setdomainname(const char *, size_t); } while (_rc == -1 && errno == EINTR); \ _rc; }) +#if defined(__BIONIC_FORTIFY) +extern ssize_t __read_chk(int, void*, size_t, size_t); +__errordecl(__read_dest_size_error, "read called with size bigger than destination"); +__errordecl(__read_count_toobig_error, "read called with count > SSIZE_MAX"); +extern ssize_t __read_real(int, void*, size_t) + __asm__(__USER_LABEL_PREFIX__ "read"); + +__BIONIC_FORTIFY_INLINE +ssize_t read(int fd, void* buf, size_t count) { + size_t bos = __bos0(buf); + +#if !defined(__clang__) + if (__builtin_constant_p(count) && (count > SSIZE_MAX)) { + __read_count_toobig_error(); + } + + if (bos == __BIONIC_FORTIFY_UNKNOWN_SIZE) { + return __read_real(fd, buf, count); + } + + if (__builtin_constant_p(count) && (count > bos)) { + __read_dest_size_error(); + } + + if (__builtin_constant_p(count) && (count <= bos)) { + return __read_real(fd, buf, count); + } +#endif + + return __read_chk(fd, buf, count, bos); +} +#endif /* defined(__BIONIC_FORTIFY) */ + __END_DECLS #endif /* _UNISTD_H_ */ diff --git a/tests/fortify_test.cpp b/tests/fortify_test.cpp index d514a3d10..b42c6b781 100644 --- a/tests/fortify_test.cpp +++ b/tests/fortify_test.cpp @@ -21,6 +21,7 @@ #include #include #include +#include // We have to say "DeathTest" here so gtest knows to run this test (which exits) // in its own process. Unfortunately, the C preprocessor doesn't give us an @@ -568,6 +569,15 @@ TEST(DEATHTEST, FD_ZERO_fortified) { ASSERT_EXIT(FD_ZERO(set), testing::KilledBySignal(SIGABRT), ""); } +TEST(DEATHTEST, read_fortified) { + ::testing::FLAGS_gtest_death_test_style = "threadsafe"; + char buf[1]; + size_t ct = atoi("2"); // prevent optimizations + int fd = open("/dev/null", O_RDONLY); + ASSERT_EXIT(read(fd, buf, ct), testing::KilledBySignal(SIGABRT), ""); + close(fd); +} + extern "C" char* __strncat_chk(char*, const char*, size_t, size_t); extern "C" char* __strcat_chk(char*, const char*, size_t);