From 02aac6b0f821be6aaff68c2d4e78805bc869f5aa Mon Sep 17 00:00:00 2001 From: fanquake Date: Tue, 25 Feb 2020 09:51:13 +0800 Subject: [PATCH] Merge #18162: util: Avoid potential uninitialized read in FormatISO8601DateTime(int64_t) by checking gmtime_s/gmtime_r return value MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 12a2f377185a413b740460db36812de22ee2e041 util: Avoid potential uninitialized read in FormatISO8601DateTime(int64_t nTime) by checking gmtime_s/gmtime_r return value (practicalswift) Pull request description: Avoid potential uninitialized read in `FormatISO8601DateTime(int64_t)` by checking `gmtime_s`/`gmtime_r` return value. Before this patch `FormatISO8601DateTime(67768036191676800)` resulted in: ``` ==5930== Conditional jump or move depends on uninitialised value(s) ==5930== at 0x4F44C0A: std::ostreambuf_iterator > std::num_put > >::_M_insert_int(std::ostreambuf_iterator >, std::ios_base&, char, long) const (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.25) ==5930== by 0x4F511A4: std::ostream& std::ostream::_M_insert(long) (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.25) ==5930== by 0x4037C3: void tinyformat::formatValue(std::ostream&, char const*, char const*, int, int const&) (tinyformat.h:358) ==5930== by 0x403725: void tinyformat::detail::FormatArg::formatImpl(std::ostream&, char const*, char const*, int, void const*) (tinyformat.h:543) ==5930== by 0x402E02: tinyformat::detail::FormatArg::format(std::ostream&, char const*, char const*, int) const (tinyformat.h:528) ==5930== by 0x401B16: tinyformat::detail::formatImpl(std::ostream&, char const*, tinyformat::detail::FormatArg const*, int) (tinyformat.h:907) ==5930== by 0x4017AE: tinyformat::vformat(std::ostream&, char const*, tinyformat::FormatList const&) (tinyformat.h:1054) ==5930== by 0x401765: void tinyformat::format(std::ostream&, char const*, int const&, int const&, int const&, int const&, int const&, int const&) (tinyformat.h:1064) ==5930== by 0x401656: std::__cxx11::basic_string, std::allocator > tinyformat::format(char const*, int const&, int const&, int const&, int const&, int const&, int const&) (tinyformat.h:1073) ==5930== by 0x4014CC: FormatISO8601DateTime[abi:cxx11](long) (…) ``` The same goes for other very large positive and negative arguments. Fix by simply checking the `gmtime_s`/`gmtime_r` return value :) ACKs for top commit: MarcoFalke: ACK 12a2f377185a413b740460db36812de22ee2e041 theStack: re-ACK https://github.com/bitcoin/bitcoin/commit/12a2f377185a413b740460db36812de22ee2e041 elichai: re ACK 12a2f377185a413b740460db36812de22ee2e041 Tree-SHA512: 066142670d9bf0944d41fa3f3c702b1a460b5471b93e76a619b1e818ff9bb9c09fe14c4c37e9536a04c99533f7f21d1b08ac141e1b829ff87ee54c80d0e61d48 --- ci/test/00_setup_env_native_fuzz_with_valgrind.sh | 2 +- src/util/time.cpp | 12 ++++++++---- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/ci/test/00_setup_env_native_fuzz_with_valgrind.sh b/ci/test/00_setup_env_native_fuzz_with_valgrind.sh index 6e1c400d50..45b13a669d 100644 --- a/ci/test/00_setup_env_native_fuzz_with_valgrind.sh +++ b/ci/test/00_setup_env_native_fuzz_with_valgrind.sh @@ -12,7 +12,7 @@ export NO_DEPENDS=1 export RUN_UNIT_TESTS=false export RUN_FUNCTIONAL_TESTS=false export RUN_FUZZ_TESTS=true -export FUZZ_TESTS_CONFIG="--exclude integer,parse_iso8601 --valgrind" +export FUZZ_TESTS_CONFIG="--valgrind" export GOAL="install" export BITCOIN_CONFIG="--enable-fuzz --with-sanitizers=fuzzer CC=clang-8 CXX=clang++-8" # Use clang-8, instead of default clang on bionic, which is clang-6 and does not come with libfuzzer on aarch64 diff --git a/src/util/time.cpp b/src/util/time.cpp index b41ad42b5a..bbc2ec931d 100644 --- a/src/util/time.cpp +++ b/src/util/time.cpp @@ -77,10 +77,12 @@ std::string FormatISO8601DateTime(int64_t nTime) { struct tm ts; time_t time_val = nTime; #ifdef HAVE_GMTIME_R - gmtime_r(&time_val, &ts); + if (gmtime_r(&time_val, &ts) == nullptr) { #else - gmtime_s(&ts, &time_val); + if (gmtime_s(&ts, &time_val) != 0) { #endif + return {}; + } return strprintf("%04i-%02i-%02iT%02i:%02i:%02iZ", ts.tm_year + 1900, ts.tm_mon + 1, ts.tm_mday, ts.tm_hour, ts.tm_min, ts.tm_sec); } @@ -88,10 +90,12 @@ std::string FormatISO8601Date(int64_t nTime) { struct tm ts; time_t time_val = nTime; #ifdef HAVE_GMTIME_R - gmtime_r(&time_val, &ts); + if (gmtime_r(&time_val, &ts) == nullptr) { #else - gmtime_s(&ts, &time_val); + if (gmtime_s(&ts, &time_val) != 0) { #endif + return {}; + } return strprintf("%04i-%02i-%02i", ts.tm_year + 1900, ts.tm_mon + 1, ts.tm_mday); }