From ee4f7fbc13ec0caf322ad80a145b4156159c14d5 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 Cherry-picked from: 8ebbef01 Conflicts: missing MakeUnique, replaced with manual new CNode() --- src/test/net_tests.cpp | 40 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/src/test/net_tests.cpp b/src/test/net_tests.cpp index b9ed4952b..35b1b0440 100644 --- a/src/test/net_tests.cpp +++ b/src/test/net_tests.cpp @@ -156,7 +156,7 @@ BOOST_AUTO_TEST_CASE(cnode_simple_test) in_addr ipv4Addr; ipv4Addr.s_addr = 0xa0b0c001; - + CAddress addr = CAddress(CService(ipv4Addr, 7777), NODE_NETWORK); std::string pszDest = ""; bool fInboundIn = false; @@ -172,4 +172,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(new CNode(0, NODE_NETWORK, 0, INVALID_SOCKET, addr, 0, 0, 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 b31dbd9dbaa155f9c598e65d7c1d735202c0fff3 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. Cherry-picked from: b7b36dec --- 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 ab07270f3..7646d25fe 100644 --- a/src/netaddress.cpp +++ b/src/netaddress.cpp @@ -18,7 +18,6 @@ static const unsigned char pchOnionCat[] = {0xFD,0x87,0xD8,0x7E,0xEB,0x43}; void CNetAddr::Init() { memset(ip, 0, sizeof(ip)); - scopeId = 0; } void CNetAddr::SetIP(const CNetAddr& ipIn) diff --git a/src/netaddress.h b/src/netaddress.h index a85c2b745..f823bae95 100644 --- a/src/netaddress.h +++ b/src/netaddress.h @@ -31,7 +31,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();