From 8ebbef016928811756e46b9086067d1c826797a8 Mon Sep 17 00:00:00 2001 From: Kaz Wesley Date: Thu, 15 Nov 2018 12:34:46 -0800 Subject: [PATCH 1/2] add test demonstrating addrLocal UB --- src/test/net_tests.cpp | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp index 213afed730..c1ee231d8a 100644 --- a/src/test/net_tests.cpp +++ b/src/test/net_tests.cpp @@ -189,4 +189,42 @@ BOOST_AUTO_TEST_CASE(cnode_simple_test) BOOST_CHECK(pnode2->fFeeler == 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() From b7b36decaf878a8c1dcfdb4a27196c730043474b Mon Sep 17 00:00:00 2001 From: Kaz Wesley Date: Wed, 14 Nov 2018 11:53:27 -0800 Subject: [PATCH 2/2] fix uninitialized read when stringifying an addrLocal Reachable from either place where SetIP is used when our best-guess addrLocal for a peer is IPv4, but the peer tells us it's reaching us at an IPv6 address. 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. --- src/netaddress.cpp | 1 - src/netaddress.h | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/netaddress.cpp b/src/netaddress.cpp index e1af4eff62..10e446392f 100644 --- a/src/netaddress.cpp +++ b/src/netaddress.cpp @@ -17,7 +17,6 @@ static const unsigned char g_internal_prefix[] = { 0xFD, 0x6B, 0x88, 0xC0, 0x87, 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 cc0e4d4f12..dc55d8b1a8 100644 --- a/src/netaddress.h +++ b/src/netaddress.h @@ -33,7 +33,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();