Fix some asmap issues

sipa commented at 11:08 pm on January 29, 2020: member

Here are a few things to improve in the asmap implementation. The first two commits are just code improvements. The last one is a bugfix (the exsting code wouldn’t correctly apply ASN lookups to mapped/embedded IPv4 addresses).

fanquake added the label P2P on Jan 29, 2020

in src/netaddress.h:85 in 2630011ca2 outdated

79@@ -80,6 +80,11 @@ class CNetAddr
80         bool GetInAddr(struct in_addr* pipv4Addr) const;
81         uint32_t GetNetClass() const;
82 
83+        //! For IPv4, mapped IPv4, SIIT translated IPv4, Teredo, 6to4 tunneled addresses, return the relevant IPv4 address as a uint32.
84+        bool HasLinkedIPv4() const;
85+        //! Whether this address has a linked IPv4 address (see GetLinkedIPv4()).

practicalswift commented at 11:23 pm on January 29, 2020:

This comment should be on L83 and vice versa? :)

sipa commented at 5:02 am on January 30, 2020:

Fixed.

fanquake commented at 0:39 am on January 30, 2020: member

cc @naumenkogs

DrahtBot commented at 2:25 am on January 30, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17812 (config, test: asmap functional tests and feature refinements by jonatack)
#17428 (p2p: Try to preserve outbound block-relay-only connections during restart by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

sipa force-pushed on Jan 30, 2020

sanjaykdragon commented at 3:15 pm on January 30, 2020: contributor

utack - looks nice, preventing unnecessary copies

practicalswift commented at 11:21 pm on January 30, 2020: contributor

Concept ACK

While improving asmap – please consider increasing the robustness of GetMappedAS(…) by plugging this issue too: “GetMappedAS(…) lookup on an IPv6 address against a maliciously constructed AS-map triggers heap buffer-overflow” :)

sipa commented at 0:54 am on January 31, 2020: member

Added a commit to resolve #18033.

sipa added the label Bug on Jan 31, 2020

MarcoFalke removed the label Bug on Jan 31, 2020

MarcoFalke added this to the milestone 0.20.0 on Jan 31, 2020

MarcoFalke renamed this:
~~Some asmap improvements~~
Fix some asmap issues
on Jan 31, 2020

jonatack commented at 11:16 am on January 31, 2020: member

Concept ACK, will review shortly.

naumenkogs commented at 2:31 pm on January 31, 2020: member

utACK 868647e5d72f2718e41b723dc7e15594ffb1d7b6 Thank you!

in src/util/asmap.cpp:81 in 868647e5d7 outdated

89-            return DecodeASN(pos);
90+            return DecodeASN(pos, endpos);
91         } else if (opcode == 1) {
92-            jump = DecodeJump(pos);
93+            jump = DecodeJump(pos, endpos);
94             if (ip[ip.size() - bits]) {

practicalswift commented at 3:50 pm on January 31, 2020:

A heap buffer overflow will occur here in case of bits == 0?

practicalswift commented at 4:27 pm on January 31, 2020:

0             if (bits == 0) {
1                 return default_asn;
2             }
3             if (ip[ip.size() - bits]) {

sipa commented at 10:53 pm on January 31, 2020:

No, reaching the end of the ip array just means a malformed asmap. It’s probably better to return 0 (also the other location).

sipa commented at 10:53 pm on January 31, 2020:

Yup, fixed.

Avoid asmap copies in initialization d58bcdc4b5

Mark asmap const in statistics code 6f8c937312

Use ASNs for mapped IPv4 addresses correctly 38c2395d7a

Make asmap Interpret tolerant of malicious map data c86bc14408

in src/util/asmap.cpp:91 in 868647e5d7 outdated

 99         } else if (opcode == 2) {
100-            match = DecodeMatch(pos);
101+            match = DecodeMatch(pos, endpos);
102             matchlen = CountBits(match) - 1;
103             for (uint32_t bit = 0; bit < matchlen; bit++) {
104                 if ((ip[ip.size() - bits]) != ((match >> (matchlen - 1 - bit)) & 1)) {

practicalswift commented at 4:27 pm on January 31, 2020:

0                 if (bits == 0 || (ip[ip.size() - bits]) != ((match >> (matchlen - 1 - bit)) & 1)) {

sipa force-pushed on Jan 31, 2020

sipa commented at 10:53 pm on January 31, 2020: member

@practicalswift Nice catch, addressed I think.

practicalswift commented at 8:26 am on February 1, 2020: contributor

@sipa I’m no longer able to trigger the two heap buffer overflows, and the code withstood an eight hour asmap fuzzer (#18033) run (48 million executions): I think the code is robust against malicious map data now :)

ACK c86bc144081f960347232546f7d22deb65d27deb – patch looks correct

in src/netaddress.cpp:519 in c86bc14408

548         nStartByte = 6;
549         nBits = 4;
550-    }
551-    // for he.net, use /36 groups
552-    else if (GetByte(15) == 0x20 && GetByte(14) == 0x01 && GetByte(13) == 0x04 && GetByte(12) == 0x70)
553+    } else if (GetByte(15) == 0x20 && GetByte(14) == 0x01 && GetByte(13) == 0x04 && GetByte(12) == 0x70) {

jonatack commented at 9:22 am on February 1, 2020:

This line could be extracted to a bool CNetAddr::IsHeNet() const function

jonatack commented at 9:49 am on February 1, 2020: member

ACK c86bc144081f960347232546f7d22deb65d27deb code looks correct, built/ran tests, bitcoind with -asmap pointed to asmap/demo.map

Question: is it expected to see as many identical values?

 0$ bitcoin-cli getpeerinfo | grep 'mapped_as' | sort
 1    "mapped_as": 1267,
 2    "mapped_as": 12876,
 3    "mapped_as": 14061,
 4    "mapped_as": 15435,
 5    "mapped_as": 202053,
 6    "mapped_as": 20473,
 7    "mapped_as": 24940,
 8    "mapped_as": 24940,
 9    "mapped_as": 30083,
10    "mapped_as": 31334,
11    "mapped_as": 34878,
12    "mapped_as": 34878,
13    "mapped_as": 39586,
14    "mapped_as": 4713,
15    "mapped_as": 47869,
16    "mapped_as": 54098,
17    "mapped_as": 54098,
18    "mapped_as": 54098,
19    "mapped_as": 54098,
20    "mapped_as": 54098,

Edit: am fuzzing this PR with #18029; currently at 34.6M executions.

0[#34611198](/bitcoin-bitcoin/34611198/) REDUCE cov: 1198 ft: 3529 corp: 197/13577b exec/s: 1125 rss: 450Mb ...

jonatack commented at 6:12 am on February 2, 2020: member

On my side, this PR held up on 72M execs (about 18 hours) of the asmap fuzzer.

0[#72474660](/bitcoin-bitcoin/72474660/)	REDUCE cov: 1198 ft: 3529 corp: 197/12792b exec/s: 1063 rss: 452Mb L: 41/1527 MS: 4 InsertByte-EraseBytes-InsertByte-PersAutoDict- DE: "\x00 \x00\x00\x00\x00\x00\x00"-

naumenkogs commented at 10:33 am on February 5, 2020: member

utACK c86bc14

laanwj commented at 12:59 pm on February 5, 2020: member

ACK c86bc144081f960347232546f7d22deb65d27deb

laanwj referenced this in commit adea5e1b54 on Feb 5, 2020

laanwj merged this on Feb 5, 2020

laanwj closed this on Feb 5, 2020

sidhujag referenced this in commit 018782881c on Feb 9, 2020

luke-jr referenced this in commit 30849d52ec on Feb 16, 2020

luke-jr referenced this in commit 5fad305c58 on Feb 16, 2020

luke-jr referenced this in commit 9400fa235e on Feb 16, 2020

luke-jr referenced this in commit 0d7ea707fc on Feb 16, 2020

jasonbcox referenced this in commit e4c59a2382 on Oct 30, 2020

sidhujag referenced this in commit 47f20209ac on Nov 10, 2020

kittywhiskers referenced this in commit 7cb3e60d5d on Mar 14, 2021

kittywhiskers referenced this in commit 5edb8b1d2b on Mar 14, 2021

kittywhiskers referenced this in commit 22646a11c2 on Mar 14, 2021

kittywhiskers referenced this in commit 9be6d8a887 on Mar 14, 2021

kittywhiskers referenced this in commit 74f271963f on Mar 18, 2021

kittywhiskers referenced this in commit c772391b49 on Mar 18, 2021

kittywhiskers referenced this in commit a343f996c1 on Mar 18, 2021

kittywhiskers referenced this in commit 3282b1cb65 on Mar 18, 2021

kittywhiskers referenced this in commit ad5c4a2182 on Mar 19, 2021

kittywhiskers referenced this in commit 923ea8e59a on Mar 19, 2021

kittywhiskers referenced this in commit a5d7726dc3 on Mar 19, 2021

kittywhiskers referenced this in commit fadc735329 on Mar 19, 2021

kittywhiskers referenced this in commit 567ef7707c on Mar 19, 2021

kittywhiskers referenced this in commit f0c91883cd on Apr 21, 2021

kittywhiskers referenced this in commit 204d6ee44f on Apr 21, 2021

kittywhiskers referenced this in commit 344e7dc476 on Apr 21, 2021

kittywhiskers referenced this in commit cf7ce59305 on Apr 23, 2021

kittywhiskers referenced this in commit 518dd0254d on Apr 23, 2021

kittywhiskers referenced this in commit 1e7b9e31fc on Apr 23, 2021

kittywhiskers referenced this in commit 550e63cd03 on Apr 23, 2021

kittywhiskers referenced this in commit 74dee9e401 on May 11, 2021

kittywhiskers referenced this in commit cc80d540f5 on May 11, 2021

kittywhiskers referenced this in commit d9ef41b1f9 on May 11, 2021

UdjinM6 referenced this in commit 9973fac521 on May 15, 2021

kittywhiskers referenced this in commit 734024d60e on May 18, 2021

kittywhiskers referenced this in commit 68aceda35f on May 18, 2021

UdjinM6 referenced this in commit 0ab6d79fda on May 19, 2021

kittywhiskers referenced this in commit 354a4b16e8 on May 19, 2021

kittywhiskers referenced this in commit f3819c4eef on May 19, 2021

PastaPastaPasta referenced this in commit 2f555f87d1 on May 19, 2021

kittywhiskers referenced this in commit dd5f28a923 on May 20, 2021

random-zebra referenced this in commit 85f000ecbe on Jul 30, 2021

random-zebra referenced this in commit b4751e10ce on Aug 11, 2021

DrahtBot locked this on Feb 15, 2022

Fix some asmap issues #18023

Conflicts