From 0211eecb14d568c0ab90e3a667d6553ea4e6b14f Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Tue, 9 Jun 2020 19:06:54 -0500 Subject: [PATCH] Merge #14728: fix uninitialized read when stringifying an addrLocal b7b36decaf878a8c1dcfdb4a27196c730043474b fix uninitialized read when stringifying an addrLocal (Kaz Wesley) 8ebbef016928811756e46b9086067d1c826797a8 add test demonstrating addrLocal UB (Kaz Wesley) Pull request description: Reachable from either place where SetIP is used when all of: - our best-guess addrLocal for a peer is IPv4 - the peer tells us it's reaching us at an IPv6 address - NET logging is enabled In that case, SetIP turns an IPv4 address into an IPv6 address without setting the scopeId, which is subsequently read in GetSockAddr during CNetAddr::ToStringIP and passed to getnameinfo. Fix by ensuring every constructor initializes the scopeId field with something. Tree-SHA512: 8f0159750995e08b985335ccf60a273ebd09003990bcf2c3838b550ed8dc2659552ac7611650e6dd8e29d786fe52ed57674f5880f2e18dc594a7a863134739e3 --- src/netaddress.cpp | 1 - src/netaddress.h | 2 +- src/test/net_tests.cpp | 38 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 39 insertions(+), 2 deletions(-) diff --git a/src/netaddress.cpp b/src/netaddress.cpp index 6fa7077b12..ed1ecdc034 100644 --- a/src/netaddress.cpp +++ b/src/netaddress.cpp @@ -20,7 +20,6 @@ bool fAllowPrivateNet = DEFAULT_ALLOWPRIVATENET; CNetAddr::CNetAddr() { memset(ip, 0, sizeof(ip)); - scopeId = 0; } void CNetAddr::SetIP(const CNetAddr& ipIn) diff --git a/src/netaddress.h b/src/netaddress.h index 50f52ee369..37e69ca2c7 100644 --- a/src/netaddress.h +++ b/src/netaddress.h @@ -34,7 +34,7 @@ class CNetAddr { protected: unsigned char ip[16]; // in network byte order - uint32_t scopeId; // for scoped/link-local ipv6 addresses + uint32_t scopeId{0}; // for scoped/link-local ipv6 addresses public: CNetAddr(); diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp index bfe3358c7e..c87e336661 100644 --- a/src/test/net_tests.cpp +++ b/src/test/net_tests.cpp @@ -201,4 +201,42 @@ BOOST_AUTO_TEST_CASE(PoissonNextSend) g_mock_deterministic_tests = false; } +// prior to PR #14728, this test triggers an undefined behavior +BOOST_AUTO_TEST_CASE(ipv4_peer_with_ipv6_addrMe_test) +{ + // set up local addresses; all that's necessary to reproduce the bug is + // that a normal IPv4 address is among the entries, but if this address is + // !IsRoutable the undefined behavior is easier to trigger deterministically + { + LOCK(cs_mapLocalHost); + in_addr ipv4AddrLocal; + ipv4AddrLocal.s_addr = 0x0100007f; + CNetAddr addr = CNetAddr(ipv4AddrLocal); + LocalServiceInfo lsi; + lsi.nScore = 23; + lsi.nPort = 42; + mapLocalHost[addr] = lsi; + } + + // create a peer with an IPv4 address + in_addr ipv4AddrPeer; + ipv4AddrPeer.s_addr = 0xa0b0c001; + CAddress addr = CAddress(CService(ipv4AddrPeer, 7777), NODE_NETWORK); + std::unique_ptr pnode = MakeUnique(0, NODE_NETWORK, 0, INVALID_SOCKET, addr, 0, 0, CAddress{}, std::string{}, false); + pnode->fSuccessfullyConnected.store(true); + + // the peer claims to be reaching us via IPv6 + in6_addr ipv6AddrLocal; + memset(ipv6AddrLocal.s6_addr, 0, 16); + ipv6AddrLocal.s6_addr[0] = 0xcc; + CAddress addrLocal = CAddress(CService(ipv6AddrLocal, 7777), NODE_NETWORK); + pnode->SetAddrLocal(addrLocal); + + // before patch, this causes undefined behavior detectable with clang's -fsanitize=memory + AdvertiseLocal(&*pnode); + + // suppress no-checks-run warning; if this test fails, it's by triggering a sanitizer + BOOST_CHECK(1); +} + BOOST_AUTO_TEST_SUITE_END()