fix uninitialized read when stringifying an addrLocal #14728

kazcw · 2018-11-14T22:53:39Z

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.

sipa · 2018-11-14T23:15:31Z

Nice catch. Can you add a test that tests for the old behavior?

promag

utACK 9151f78.

src/netaddress.cpp

practicalswift · 2018-11-15T08:48:27Z

Excellent catch and thanks for another great contribution!

utACK 9151f783ca53cde718a3895324bd6389e5de6318 modulo @promag's nit

How did you find this issue?

I can't believe I didn't catch this issue as part of my routine static analysis runs :-\ I checked now and I had this specific case in multiple analysis reports, but it appears the signal drowned in the noise.

Would be nice to have the offending code path executed in a test to allow for catching it via dynamic analysis too.

practicalswift · 2018-11-15T21:08:07Z

utACK 04ae6807bc09d52c4738836243cb3dad8845b405

kazcw · 2018-11-15T21:28:30Z

I was looking at the code that relates to a recent bug report. I didn't see the cause of the bug, but I did see this.

I added a test that successfully fails under -fsanitize=memory without this patch.

src/test/net_tests.cpp

practicalswift · 2018-11-15T21:43:55Z

utACK d0b673176b3214e16e4df50dcb65352fbd07969f modulo typo fix :-)

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.

promag · 2018-11-16T16:03:00Z

utACK b7b36de.

practicalswift · 2018-11-16T16:08:58Z

utACK 8ebbef0

maflcko · 2018-11-16T19:00:37Z

utACK b7b36de

Makes the code adhere to our developer cpp style guide and also fixes the warning that valgrind yells at me when running valgrind ./src/test/test_bitcoin -t net_tests/ipv4_peer_with_ipv6_addrMe_test.

wailo

I would suggest applying the same change to the ip array unsigned char ip[16] = {0}; and remove the memset call from the constructor. This leaves an empty constructor which can then be marked default.

laanwj · 2018-11-23T08:15:26Z

utACK b7b36de

@wailo's comment could be done later, but is not related to the fix for UB here

laanwj · 2018-11-23T08:52:21Z

re-utACK 8ebbef0

You're not acking the top commit (b7b36de) is this intentional? (I guess not)

b7b36de fix uninitialized read when stringifying an addrLocal (Kaz Wesley) 8ebbef0 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

practicalswift · 2018-11-23T08:57:47Z

@laanwj Oh sorry I got the ordering wrong. I meant to re-utACK the latest commit :-)

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. Github-Pull: bitcoin#14728 Rebased-From: b7b36de

Github-Pull: bitcoin#14728 Rebased-From: 8ebbef0

fanquake · 2018-11-29T10:44:11Z

Backported in #14835.

f9db08e qa: Avoid race in p2p_invalid_block by waiting for the block request (MarcoFalke) 7935881 Add SAFE_CHARS[SAFE_CHARS_URI]: Chars allowed in URIs (RFC 3986) (practicalswift) 9666dba rpc: Make HTTP RPC debug logging more informative (practicalswift) b901578 add test demonstrating addrLocal UB (Kaz Wesley) 6f04264 fix uninitialized read when stringifying an addrLocal (Kaz Wesley) 5782fdc Throw error if CPubKey is invalid during PSBT keypath serialization (Gregory Sanders) de5e48a Bugfix: RPC: Add address_type named param for createmultisig (Luke Dashjr) df5131b gui: explicitly disable "Dark Mode" appearance on macOS (fanquake) Pull request description: Backports #14593, #14596, #14618, #14690 and #14728 to the 0.17 branch. Tree-SHA512: fcda4b75fcb71bb80cc8bde2a2b98ff5c0239dfa754ac980b1a91a90409502ac7678326399a4fc03a773074339dbf8b3d11750c91fe4302741a954745acfcca1

fa2510d Use C++11 default member initializers (MarcoFalke) Pull request description: Changes: * Remove unused constructors that leave some members uninitialized * Remove manual initialization in each constructor and prefer C++11 default member initializers This is not a stylistic change, but a change that avoids bugs such as: * fix uninitialized read when stringifying an addrLocal #14728 * qt: Initialize members in WalletModel #12426 * net: correctly initialize nMinPingUsecTime #6636 * ... Tree-SHA512: 0f896f3b9fcc464d5fc7525f7c86343ef9ce9fb13425fbc68e9a9728fd8710c2b4e2fd039ee08279ea41ff20fd92b7185cf5cca95a0bcb6a5340a1e6f03cae6b

fac2f5e Use C++11 default member initializers (MarcoFalke) Pull request description: The second and last change on this topic (c.f. #15109). Split up because the diff would otherwise interleave, making review harder than necessary. This is not a stylistic change, but a change that avoids bugs such as: * fix uninitialized read when stringifying an addrLocal #14728 * qt: Initialize members in WalletModel #12426 * net: correctly initialize nMinPingUsecTime #6636 * ... Tree-SHA512: 547ae72b87aeaed5890eb5fdcff612bfc93354632b238d89e1e1c0487187f39609bcdc537ef21345e0aea8cfcf1ea48da432d672c5386dd87cf58742446a86b1

… addrLocal Summary: This is a backport of Core PR14728 As above. bitcoin/bitcoin#14728 Test Plan: ninja check Reviewers: jasonbcox, O1 Bitcoin ABC, #bitcoin_abc, deadalnix Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D4570

…Local b7b36de fix uninitialized read when stringifying an addrLocal (Kaz Wesley) 8ebbef0 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

… addrLocal Summary: This is a backport of Core PR14728 As above. bitcoin/bitcoin#14728 Test Plan: ninja check Reviewers: jasonbcox, O1 Bitcoin ABC, #bitcoin_abc, deadalnix Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D4570

fcdf87a Populate services in GetLocalAddress (Alex Morcos) a0a079f Return early in IsBanned. (Gregory Maxwell) 7772138 Remove redundant nullptr checks before deallocation (practicalswift) 5390862 net: remove unimplemented functions. (furszy) 74d0482 Limit setAskFor and retire requested entries only when a getdata returns. (Gregory Maxwell) 3b3bf63 prevent peer flooding request queue for an inv (kazcw) c814967 Fix subscript[0] potential bugs in key.cpp (Jeremy Rubin) 8e2e79e Remove unnecessary branches in utilstrencodings string constructors. (Jeremy Rubin) 25a16b3 Fix subscript[0] in utilstrencodings.cpp (Jeremy Rubin) df0584e Fix subscript[0] in streams.h (Jeremy Rubin) b7e64a5 Fix subscript[0] in validation.cpp (Jeremy Rubin) 2f7b73b Fix subscript[0] in torcontrol (Jeremy Rubin) 3b883eb Fix subscript[0] in netaddress.cpp (Jeremy Rubin) e5f5401 Fix subscript[0] in base58.cpp (Jeremy Rubin) 7d4ec87 Cleanup (safe, it was checked) subscript[0] in MurmurHash3 (and cleanup MurmurHash3 to be more clear). (Jeremy Rubin) 5e14f54 Fix subscript[0] in compressor.cpp (Jeremy Rubin) 48a622d Fix subscript[0] bug in net.cpp if GetGroup returns a 0-sized vector (Jeremy Rubin) af4cba3 net: use an internal address for fixed seeds (Cory Fields) 36368c5 net: switch to dummy internal ip for dns seed source (Cory Fields) 8f31295 net: do not allow resolving to an internal address (Cory Fields) 8ba3a40 net: add an internal subnet for representing unresolved hostnames (Cory Fields) c60b875 fix uninitialized read when stringifying an addrLocal (Kaz Wesley) 3d36540 add test demonstrating addrLocal UB (Kaz Wesley) Pull request description: Grouped some net updates (plus a miscellaneous one) coming from upstream. Part of another deep rabbit hole that i'm doing in parallel of the tier two work. PRs List: * bitcoin#7079. * bitcoin#9804. * bitcoin#10424 * bitcoin#10446. * bitcoin#10564. * bitcoin#14728. ACKs for top commit: random-zebra: Code ACK fcdf87a Tree-SHA512: ee81c834641aa6fdb9ca4396657457358a4b32f7862d60716e914dcfc2d355970937bd0fb4e164faaa0f4ea26d263ca8a2af4d8d5d6615b2db5bf89ec70d15f3

fa2510d Use C++11 default member initializers (MarcoFalke) Pull request description: Changes: * Remove unused constructors that leave some members uninitialized * Remove manual initialization in each constructor and prefer C++11 default member initializers This is not a stylistic change, but a change that avoids bugs such as: * fix uninitialized read when stringifying an addrLocal bitcoin#14728 * qt: Initialize members in WalletModel bitcoin#12426 * net: correctly initialize nMinPingUsecTime dashpay#6636 * ... Tree-SHA512: 0f896f3b9fcc464d5fc7525f7c86343ef9ce9fb13425fbc68e9a9728fd8710c2b4e2fd039ee08279ea41ff20fd92b7185cf5cca95a0bcb6a5340a1e6f03cae6b

fanquake added the P2P label Nov 15, 2018

promag reviewed Nov 15, 2018

View reviewed changes

src/netaddress.cpp Outdated Show resolved Hide resolved

kazcw force-pushed the uninit-scopeid branch from 9151f78 to 04ae680 Compare November 15, 2018 19:50

kazcw force-pushed the uninit-scopeid branch from 04ae680 to 262c0b1 Compare November 15, 2018 21:28

practicalswift reviewed Nov 15, 2018

View reviewed changes

src/test/net_tests.cpp Outdated Show resolved Hide resolved

kazcw added 2 commits November 15, 2018 13:47

add test demonstrating addrLocal UB

8ebbef0

kazcw force-pushed the uninit-scopeid branch from 262c0b1 to b7b36de Compare November 15, 2018 21:50

wailo suggested changes Nov 17, 2018

View reviewed changes

laanwj merged commit b7b36de into bitcoin:master Nov 23, 2018

fanquake added the Needs backport label Nov 29, 2018

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Nov 29, 2018

add test demonstrating addrLocal UB

b901578

Github-Pull: bitcoin#14728 Rebased-From: 8ebbef0

fanquake mentioned this pull request Nov 29, 2018

[0.17] Further Backports #14835

Merged

fanquake removed the Needs backport label Nov 29, 2018

maflcko mentioned this pull request Jan 5, 2019

refactor: Use C++11 default member initializers #15109

Merged

maflcko mentioned this pull request Jan 10, 2019

[refactor] CNode: Use C++11 default member initializers #15144

Merged

furszy mentioned this pull request May 3, 2021

Net code updates [Step 2] PIVX-Project/PIVX#2359

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix uninitialized read when stringifying an addrLocal #14728

fix uninitialized read when stringifying an addrLocal #14728

Uh oh!

kazcw commented Nov 14, 2018

Uh oh!

sipa commented Nov 14, 2018

Uh oh!

promag left a comment

Uh oh!

Uh oh!

practicalswift commented Nov 15, 2018 •

edited

Loading

Uh oh!

practicalswift commented Nov 15, 2018

Uh oh!

kazcw commented Nov 15, 2018

Uh oh!

Uh oh!

practicalswift commented Nov 15, 2018

Uh oh!

promag commented Nov 16, 2018

Uh oh!

practicalswift commented Nov 16, 2018

Uh oh!

maflcko commented Nov 16, 2018

Uh oh!

wailo left a comment

Uh oh!

laanwj commented Nov 23, 2018

Uh oh!

laanwj commented Nov 23, 2018 •

edited

Loading

Uh oh!

practicalswift commented Nov 23, 2018

Uh oh!

fanquake commented Nov 29, 2018

Uh oh!

Uh oh!

fix uninitialized read when stringifying an addrLocal #14728

fix uninitialized read when stringifying an addrLocal #14728

Uh oh!

Conversation

kazcw commented Nov 14, 2018

Uh oh!

sipa commented Nov 14, 2018

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

practicalswift commented Nov 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Nov 15, 2018

Uh oh!

kazcw commented Nov 15, 2018

Uh oh!

Uh oh!

practicalswift commented Nov 15, 2018

Uh oh!

promag commented Nov 16, 2018

Uh oh!

practicalswift commented Nov 16, 2018

Uh oh!

maflcko commented Nov 16, 2018

Uh oh!

wailo left a comment

Choose a reason for hiding this comment

Uh oh!

laanwj commented Nov 23, 2018

Uh oh!

laanwj commented Nov 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Nov 23, 2018

Uh oh!

fanquake commented Nov 29, 2018

Uh oh!

Uh oh!

practicalswift commented Nov 15, 2018 •

edited

Loading

laanwj commented Nov 23, 2018 •

edited

Loading