util: Avoid potential uninitialized read in FormatISO8601DateTime(int64_t) by checking gmtime_s/gmtime_r return value by practicalswift · Pull Request #18162 · bitcoin/bitcoin

practicalswift · 2020-02-16T19:55:09Z

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<char, std::char_traits<char> > std::num_put<char, std::ostreambuf_iterator<char, std::char_traits<char> > >::_M_insert_int<long>(std::ostreambuf_iterator<char, std::char_traits<char> >, 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>(long) (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.25)
==5930==    by 0x4037C3: void tinyformat::formatValue<int>(std::ostream&, char const*, char const*, int, int const&) (tinyformat.h:358)
==5930==    by 0x403725: void tinyformat::detail::FormatArg::formatImpl<int>(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<int, int, int, int, int, int>(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<char, std::char_traits<char>, std::allocator<char> > tinyformat::format<int, int, int, int, int, int>(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 :)

theStack

gmtime_s doesn't return a pointer, but errno_t (see https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/gmtime-s-gmtime32-s-gmtime64-s?view=vs-2019), hence the condition != 0 rather than == nullptr is needed in those cases to detect an error. See AppVeyor build failure.

practicalswift · 2020-02-17T07:19:16Z

@theStack Oh, thanks for notifying! :) Now addressed . Please re-review updated version :)

Empact · 2020-02-17T07:19:45Z

The gmtime_s return value seems to be a windows-specific situation: https://en.cppreference.com/w/c/chrono/gmtime

practicalswift · 2020-02-17T07:22:51Z

@Empact

The gmtime_s return value seems to be a windows-specific situation: https://en.cppreference.com/w/c/chrono/gmtime

Weird: "The implementation of gmtime_s in Microsoft CRT is incompatible with the C standard since it has reversed parameter order." :)

Luckily we use gmtime_s only in the case of _MSC_VER.

fanquake · 2020-02-17T09:11:06Z

The unit tests are not passing:

test/util_tests.cpp(155): error: in "util_tests/util_FormatParseISO8601DateTime": check FormatISO8601DateTime(std::numeric_limits<int64_t>::min()) == "" has failed [1970-01-01T00:00:00Z != ]
test/util_tests.cpp(156): error: in "util_tests/util_FormatParseISO8601DateTime": check FormatISO8601DateTime(std::numeric_limits<int64_t>::max()) == "" has failed [1969-12-31T23:59:59Z != ]
test/util_tests.cpp(157): error: in "util_tests/util_FormatParseISO8601DateTime": check FormatISO8601Date(std::numeric_limits<int64_t>::min()) == "" has failed [1970-01-01 != ]
test/util_tests.cpp(158): error: in "util_tests/util_FormatParseISO8601DateTime": check FormatISO8601Date(std::numeric_limits<int64_t>::max()) == "" has failed [1969-12-31 != ]

DrahtBot · 2020-02-17T12:18:43Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

theStack

Code-Review ACK 6558ec3

practicalswift · 2020-02-17T19:58:00Z

Added a valgrind CI job that would have caught this: see PR #18166 (ci: Run fuzz testing test cases (bitcoin-core/qa-assets) under valgrind to catch memory errors).

elichai

Nice find!
I'd refactor this out to a function that returns Optional<struct tm> but this can be done in a future PR
ACK 6558ec35654d1a9990dcb534b18b5c88c5a4e165

…) under valgrind to catch memory errors f2472f6 tests: Improve test runner output in case of target errors (practicalswift) 733bbec tests: Add --exclude integer,parse_iso8601 (temporarily) to make Travis pass until uninitialized read issue in FormatISO8601DateTime is fixed (practicalswift) 5ea8144 tests: Add support for excluding fuzz targets using -x/--exclude (practicalswift) 555236f tests: Remove -detect_leaks=0 from test/fuzz/test_runner.py - no longer needed (practicalswift) a3b539a ci: Run fuzz testing test cases under valgrind (practicalswift) Pull request description: Run fuzz testing [test cases (bitcoin-core/qa-assets)](https://github.com/bitcoin-core/qa-assets) under `valgrind`. This would have caught `util: Avoid potential uninitialized read in FormatISO8601DateTime(int64_t) by checking gmtime_s/gmtime_r return value` (#18162) and similar cases. ACKs for top commit: MarcoFalke: ACK f2472f6 👼 Tree-SHA512: bb0879d40167cf6906bc0ed31bed39db83c39c7beb46026f7b0ee53f28ff0526ad6fabc3f4cb3f5f18d3b8cafdcbf5f30105b35919f4e83697c71e838ed71493

…64_t nTime) by checking gmtime_s/gmtime_r return value

practicalswift · 2020-02-19T22:42:13Z

Rebased and removed --exclude integer,parse_iso8601 which is not needed after the merge of this PR.

…-assets) under valgrind to catch memory errors f2472f6 tests: Improve test runner output in case of target errors (practicalswift) 733bbec tests: Add --exclude integer,parse_iso8601 (temporarily) to make Travis pass until uninitialized read issue in FormatISO8601DateTime is fixed (practicalswift) 5ea8144 tests: Add support for excluding fuzz targets using -x/--exclude (practicalswift) 555236f tests: Remove -detect_leaks=0 from test/fuzz/test_runner.py - no longer needed (practicalswift) a3b539a ci: Run fuzz testing test cases under valgrind (practicalswift) Pull request description: Run fuzz testing [test cases (bitcoin-core/qa-assets)](https://github.com/bitcoin-core/qa-assets) under `valgrind`. This would have caught `util: Avoid potential uninitialized read in FormatISO8601DateTime(int64_t) by checking gmtime_s/gmtime_r return value` (bitcoin#18162) and similar cases. ACKs for top commit: MarcoFalke: ACK f2472f6 👼 Tree-SHA512: bb0879d40167cf6906bc0ed31bed39db83c39c7beb46026f7b0ee53f28ff0526ad6fabc3f4cb3f5f18d3b8cafdcbf5f30105b35919f4e83697c71e838ed71493

elichai

re ACK 12a2f37

maflcko · 2020-02-24T16:17:13Z

ACK 12a2f37

theStack · 2020-02-24T17:37:39Z

re-ACK 12a2f37

…atISO8601DateTime(int64_t) by checking gmtime_s/gmtime_r return value 12a2f37 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<char, std::char_traits<char> > std::num_put<char, std::ostreambuf_iterator<char, std::char_traits<char> > >::_M_insert_int<long>(std::ostreambuf_iterator<char, std::char_traits<char> >, 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>(long) (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.25) ==5930== by 0x4037C3: void tinyformat::formatValue<int>(std::ostream&, char const*, char const*, int, int const&) (tinyformat.h:358) ==5930== by 0x403725: void tinyformat::detail::FormatArg::formatImpl<int>(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<int, int, int, int, int, int>(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<char, std::char_traits<char>, std::allocator<char> > tinyformat::format<int, int, int, int, int, int>(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 12a2f37 theStack: re-ACK bitcoin@12a2f37 elichai: re ACK 12a2f37 Tree-SHA512: 066142670d9bf0944d41fa3f3c702b1a460b5471b93e76a619b1e818ff9bb9c09fe14c4c37e9536a04c99533f7f21d1b08ac141e1b829ff87ee54c80d0e61d48

…t64_t) by checking gmtime_s/gmtime_r return value (bitcoin#18162)

…-assets) under valgrind to catch memory errors 3f686d1 ci: Run fuzz testing test cases (bitcoin-core/qa-assets) under valgrind to catch memory errors (practicalswift) Pull request description: Re-introduce the Travis valgrind fuzzing job which was removed by PR bitcoin#18899. The removal seems to have been made by accident since the removed job does not appear to be the source of the problem the PR set out to fix. --- Run fuzz testing [test cases (bitcoin-core/qa-assets)](https://github.com/bitcoin-core/qa-assets) under `valgrind`. This would have caught `util: Avoid potential uninitialized read in FormatISO8601DateTime(int64_t) by checking gmtime_s/gmtime_r return value` (bitcoin#18162) and similar cases. This fuzzing job was introduced in bitcoin#18166. Top commit has no ACKs. Tree-SHA512: 6e2681eb0ade6af465c5ea91ac163a337465d2130ec9880ba57a36d9af7c25682734586a32977dc25972d4f78483f339d680ea48c0ae13cf1dfa52b617aae401

…64_t nTime) by checking gmtime_s/gmtime_r return value Summary: ``` The same goes for other very large positive and negative arguments. Fix by simply checking the gmtime_s/gmtime_r return value :) ``` Backport of core [[bitcoin/bitcoin#18162 | PR18162]]. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: PiRK Differential Revision: https://reviews.bitcoinabc.org/D8219

…-assets) under valgrind to catch memory errors f2472f6 tests: Improve test runner output in case of target errors (practicalswift) 733bbec tests: Add --exclude integer,parse_iso8601 (temporarily) to make Travis pass until uninitialized read issue in FormatISO8601DateTime is fixed (practicalswift) 5ea8144 tests: Add support for excluding fuzz targets using -x/--exclude (practicalswift) 555236f tests: Remove -detect_leaks=0 from test/fuzz/test_runner.py - no longer needed (practicalswift) a3b539a ci: Run fuzz testing test cases under valgrind (practicalswift) Pull request description: Run fuzz testing [test cases (bitcoin-core/qa-assets)](https://github.com/bitcoin-core/qa-assets) under `valgrind`. This would have caught `util: Avoid potential uninitialized read in FormatISO8601DateTime(int64_t) by checking gmtime_s/gmtime_r return value` (bitcoin#18162) and similar cases. ACKs for top commit: MarcoFalke: ACK f2472f6 👼 Tree-SHA512: bb0879d40167cf6906bc0ed31bed39db83c39c7beb46026f7b0ee53f28ff0526ad6fabc3f4cb3f5f18d3b8cafdcbf5f30105b35919f4e83697c71e838ed71493

…atISO8601DateTime(int64_t) by checking gmtime_s/gmtime_r return value 12a2f37 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<char, std::char_traits<char> > std::num_put<char, std::ostreambuf_iterator<char, std::char_traits<char> > >::_M_insert_int<long>(std::ostreambuf_iterator<char, std::char_traits<char> >, 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>(long) (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.25) ==5930== by 0x4037C3: void tinyformat::formatValue<int>(std::ostream&, char const*, char const*, int, int const&) (tinyformat.h:358) ==5930== by 0x403725: void tinyformat::detail::FormatArg::formatImpl<int>(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<int, int, int, int, int, int>(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<char, std::char_traits<char>, std::allocator<char> > tinyformat::format<int, int, int, int, int, int>(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 12a2f37 theStack: re-ACK bitcoin@12a2f37 elichai: re ACK 12a2f37 Tree-SHA512: 066142670d9bf0944d41fa3f3c702b1a460b5471b93e76a619b1e818ff9bb9c09fe14c4c37e9536a04c99533f7f21d1b08ac141e1b829ff87ee54c80d0e61d48

f3f3d6c util: fix compilation with mingw-w64 7.0.0 (Fuzzbawls) f24da7d util: Avoid potential uninitialized read in FormatISO8601DateTime (Fuzzbawls) 120a12a util: Add type safe GetTime (Fuzzbawls) be48766 Fix for utiltime to compile with msvc. (Aaron Clauson) 2833406 Avoid std::locale/imbue in DateTimeStrFormat (Pieter Wuille) 3331c25 Format automatic wallet backup file names in ISO 8601 basic format (Fuzzbawls) c32a208 Format timestamps using ISO 8601 formatting (e.g. "2018-02-28T12:34:56Z") (practicalswift) f8bd173 [logging] log system time and mock time (John Newbery) Pull request description: This is a backport of a collection of upstream PRs to bring our time utility more up to date. The following upstream PRs are included: - bitcoin#10383 - bitcoin#12567 - bitcoin#12973 - bitcoin#13031 - bitcoin#16046 - bitcoin#18162 - bitcoin#18358 The two user-facing notable changes here have been documented in the release notes template. I've also connected the functionality that `-logtimemicros` was supposed to provide. ACKs for top commit: furszy: code review ACK f3f3d6c random-zebra: utACK f3f3d6c after rebase, and merging... Tree-SHA512: 64f9cc1f7fc65c192f3b3ab869c057a81e007cf0fae82ecd23993c6b51830e4bc80656c699ba40bb8e019115a24d1ea88a618be0c0d7112749d9ce125d62ce44

practicalswift changed the title ~~util: Avoid potential uninitialized read in FormatISO8601DateTime(int64_t n) by checking gmtime_s/gmtime_r return value~~ util: Avoid potential uninitialized read in FormatISO8601DateTime(int64_t) by checking gmtime_s/gmtime_r return value Feb 16, 2020

fanquake added the Utils/log/libs label Feb 16, 2020

theStack suggested changes Feb 16, 2020

View reviewed changes

fanquake added the Waiting for author label Feb 17, 2020

practicalswift force-pushed the FormatISO8601DateTime-unitialized-read branch from 5fe2368 to 6558ec3 Compare February 17, 2020 07:10

fanquake removed the Waiting for author label Feb 17, 2020

DrahtBot mentioned this pull request Feb 17, 2020

wallet: Pass in transactions and messages for signing instead of exporting the private keys #18115

Merged

practicalswift force-pushed the FormatISO8601DateTime-unitialized-read branch from 3025336 to 6558ec3 Compare February 17, 2020 13:19

This was referenced Feb 17, 2020

refactor: deduplicate the message sign/verify code #17577

Merged

Native Descriptor Wallets using DescriptorScriptPubKeyMan #16528

Merged

theStack approved these changes Feb 17, 2020

View reviewed changes

practicalswift mentioned this pull request Feb 17, 2020

ci: Run fuzz testing test cases (bitcoin-core/qa-assets) under valgrind to catch memory errors #18166

Merged

elichai approved these changes Feb 19, 2020

View reviewed changes

util: Avoid potential uninitialized read in FormatISO8601DateTime(int…

12a2f37

…64_t nTime) by checking gmtime_s/gmtime_r return value

practicalswift force-pushed the FormatISO8601DateTime-unitialized-read branch from 6558ec3 to 12a2f37 Compare February 19, 2020 22:41

elichai approved these changes Feb 24, 2020

View reviewed changes

fanquake merged commit a674e89 into bitcoin:master Feb 25, 2020

practicalswift mentioned this pull request Mar 8, 2020

build: Add MemorySanitizer (MSan) in Travis to detect use of uninitialized memory #18288

Merged

practicalswift mentioned this pull request Mar 24, 2020

Some fuzzing crashes when running under ASan and/or UBSan #18046

Closed

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

MarkLTZ added a commit to litecoinz-core/litecoinz that referenced this pull request Apr 19, 2020

[util] Avoid potential uninitialized read in FormatISO8601DateTime(in…

e274ffa

…t64_t) by checking gmtime_s/gmtime_r return value (bitcoin#18162)

practicalswift mentioned this pull request May 8, 2020

ci: Run fuzz testing test cases (bitcoin-core/qa-assets) under valgrind to catch memory errors #18912

Merged

practicalswift mentioned this pull request Oct 1, 2020

signet: Fix uninitialized read in validation #20035

Merged

practicalswift mentioned this pull request Nov 11, 2020

ci: Remove redundant valgrind fuzz task #20368

Merged

Fuzzbawls mentioned this pull request Apr 8, 2021

[Util] Time utility updates PIVX-Project/PIVX#2300

Merged

practicalswift deleted the FormatISO8601DateTime-unitialized-read branch April 10, 2021 19:39

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

Conversation

practicalswift commented Feb 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Feb 17, 2020

Uh oh!

Empact commented Feb 17, 2020

Uh oh!

practicalswift commented Feb 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented Feb 17, 2020

Uh oh!

DrahtBot commented Feb 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Feb 17, 2020

Uh oh!

elichai left a comment

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Feb 19, 2020

Uh oh!

elichai left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Feb 24, 2020

Uh oh!

theStack commented Feb 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

practicalswift commented Feb 16, 2020 •

edited

Loading

practicalswift commented Feb 17, 2020 •

edited

Loading

DrahtBot commented Feb 17, 2020 •

edited

Loading