Code vulnerable to segfault after a network fork #5698

Hello,

I'm Evan Duffield, the lead developer of Darkcoin. We recently re-forked from Bitcoin 0.9.3 away from litecoin and after which we had a minor network fork, with 2 valid chains for a period of time. This lead many clients getting segfaults, after hitting this line of code:

https://github.com/bitcoin/bitcoin/blob/0.10/src/main.cpp#L1274

I've looked through updates done to Bitcoin since 0.9.3 and this looks like an unknown issue in the Bitcoin codebase. This is important and must get fixed, because a good portion of the network is on vulnerable versions and my understanding is they'll all segfault when a fork condition is met.

Here's the stack trace from Darkcoin.

darkcoind 0.11.0.11

(gdb) bt #0 0x000000000046a83e in ToString (this=<error reading variable: Cannot access memory at address 0x0>) at uint256.h:343 #1 CheckForkWarningConditions () at main.cpp:1749 #2 0x000000000047e3d5 in AddToBlockIndex (block=..., state=..., pos=...) at main.cpp:2734 #3 0x000000000047e9d0 in AcceptBlock (block=..., state=..., dbp=dbp@entry=0x0) at main.cpp:3065 #4 0x000000000047f991 in ProcessBlock (state=..., pfrom=pfrom@entry=0xb6ed40, pblock=pblock@entry=0x7fffdeffc460, dbp=dbp@entry=0x0) at main.cpp:3184 #5 0x000000000048475f in ProcessMessage (pfrom=pfrom@entry=0xb6ed40, strCommand=..., vRecv=...) at main.cpp:4468 #6 0x0000000000486b53 in ProcessMessages (pfrom=0xb6ed40) at main.cpp:4780 #7 0x000000000050a71f in operator() (a0=<optimized out>, this=<optimized out>) at /usr/include/boost/function/function_template.hpp:760 #8 m_invoke (connectionBody=..., this=0x7fffdeffca60) at /usr/include/boost/signals2/detail/signal_template.hpp:368 #9 operator() (connectionBody=..., this=0x7fffdeffca60) at /usr/include/boost/signals2/detail/signal_template.hpp:345 #10 dereference (this=<synthetic pointer>) at /usr/include/boost/signals2/detail/slot_call_iterator.hpp:82 #11 dereference<boost::signals2::detail::slot_call_iterator_t<boost::signals2::detail::signal1_impl<bool, CNode, boost::signals2::optional_last_value<bool>, int, std::less<int>, boost::function<bool(CNode)>, boost::function<bool(const boost::signals2::connection&, CNode)>, boost::signals2::mutex>::slot_invoker, std::_List_iterator<boost::shared_ptr<boost::signals2::detail::connection_body<std::pair<boost::signals2::detail::slot_meta_group, boost::optional<int> >, boost::signals2::slot1<bool, CNode, boost::function<bool(CNode)> >, boost::signals2::mutex> > >, boost::signals2::detail::connection_body<std::pair<boost::signals2::detail::slot_meta_group, boost::optional<int> >, boost::signals2::slot1<bool, CNode, boost::function<bool(CNode)> >, boost::signals2::mutex> > > (f=<synthetic pointer>) at /usr/include/boost/iterator/iterator_facade.hpp:517 #12 operator\ (this=<synthetic pointer>) at /usr/include/boost/iterator/iterator_facade.hpp:643 #13 operator()<boost::signals2::detail::slot_call_iterator_t<boost::signals2::detail::signal1_impl<bool, CNode, boost::signals2::optional_last_value<bool>, int, std::less<int>, boost::function<bool(CNode)>, boost::function<bool(const boost::signals2::connection&, CNode)>, boost::signals2::mutex>::slot_invoker, std::_List_iterator<boost::shared_ptr<boost::signals2::detail::connection_body<std::pair<boost::signals2::detail::slot_meta_group, boost::optional<int> >, boost::signals2::slot1<bool, CNode, boost::function<bool(CNode)> >, boost::signals2::mutex> > >, boost::signals2::detail::connection_body<std::pair<boost::signals2::detail::slot_meta_group, boost::optional<int> >, boost::signals2::slot1<bool, CNode, boost::function<bool(CNode)> >, boost::signals2::mutex> > > (first=..., this=<optimized out>, last=...) at /usr/include/boost/signals2/optional_last_value.hpp:34 #14 operator()boost::signals2::optional_last_value<bool, boost::signals2::detail::slot_call_iterator_t<boost::signals2::detail::signal1_impl<bool, CNode, boost::signals2::optional_last_value<bool>, int, std::less<int>, boost::function<bool(CNode)>, boost::function<bool(const boost::signals2::connection&, CNode)>, boost::signals2::mutex>::slot_invoker, std::_List_iterator<boost::shared_ptr<boost::signals2::detail::connection_body<std::pair<boost::signals2::detail::slot_meta_group, boost::optional<int> >, boost::signals2::slot1<bool, CNode, boost::function<bool(CNode)> >, boost::signals2::mutex> > >, boost::signals2::detail::connection_body<std::pair<boost::signals2::detail::slot_meta_group, boost::optional<int> >, boost::signals2::slot1<bool, CNode, boost::function<bool(CNode)> >, boost::signals2::mutex> > > (first=..., this=<optimized out>, combiner=..., last=...) at /usr/include/boost/signals2/detail/result_type_wrapper.hpp:53 #15 boost::signals2::detail::signal1_impl<bool, CNode, boost::signals2::optional_last_value<bool>, int, std::less<int>, boost::function<bool (CNode)>, boost::function<bool (boost::signals2::connection const&, CNode)>, boost::signals2::mutex>::operator()(CNode) (this=<optimized out>, arg1=arg1@entry=0xb6ed40) at /usr/include/boost/signals2/detail/signal_template.hpp:246 #16 0x00000000004f0155 in operator() (arg1=0xb6ed40, this=0xb240d8) at /usr/include/boost/signals2/detail/signal_template.hpp:695 #17 ThreadMessageHandler () at net.cpp:1529 #18 0x00000000004f7bee in TraceThread<void (*)()> (name=0x74f2d1 "msghand", func=0x4effa0 <ThreadMessageHandler()>) at util.h:575 #19 0x00007ffff7f55629 in ?? () from /usr/lib/libboost_thread.so.1.49.0 #20 0x00007ffff6839b50 in start_thread () from /lib/x86_64-linux-gnu/libpthread.so.0 #21 0x00007ffff65837bd in clone () from /lib/x86_64-linux-gnu/libc.so.6 #22 0x0000000000000000 in ?? ()

Notice the segfault is hitting pindexBestForkBase->phashBlock and trying to turn that into a String, the vulnerability still is present is 0.9.4 and 0.10.

#0 0x000000000046a83e in ToString (this=) at uint256.h:343 #1 CheckForkWarningConditions () at main.cpp:1749

https://github.com/darkcoin/darkcoin/commit/90ad641d39d36e9576cd62b737565fc6480ca15e

I believe you've misunderstood the behavior. No block can be in the block index there without a hash, if one is there is an error elsewhere. See main.cpp: AddToBlockIndex. (Also-- I'm unable to reproduce, the warnings act fine.)

As an aside your response of posting something you believed to be a "vulnerability" on reddit instead of reporting it to the security contact is irresponsible.

I didn't submit it to reddit all, I just made an issue and a pull request. I didn't believe this was incredibly serious, due to requiring a 12 hour long fork to cause issues on the Bitcoin network. However, there's no checks at all in 0.9, so regardless of how you patch it, it needs to get patched. Thanks.

Feel free to keep reporting problems you find, nonetheless!