Fix some asmap issues #18023

pull sipa wants to merge 4 commits into bitcoin:master from sipa:202001_asmap_nits changing 6 files +89 −63
  1. 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).
  2. fanquake added the label P2P on Jan 29, 2020
  3. 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.
  4. fanquake commented at 0:39 am on January 30, 2020: member
    cc @naumenkogs
  5. 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.

  6. sipa force-pushed on Jan 30, 2020
  7. sanjaykdragon commented at 3:15 pm on January 30, 2020: contributor
    utack - looks nice, preventing unnecessary copies
  8. 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” :)

  9. sipa commented at 0:54 am on January 31, 2020: member
    Added a commit to resolve #18033.
  10. sipa added the label Bug on Jan 31, 2020
  11. MarcoFalke removed the label Bug on Jan 31, 2020
  12. MarcoFalke added this to the milestone 0.20.0 on Jan 31, 2020
  13. MarcoFalke renamed this:
    Some asmap improvements
    Fix some asmap issues
    on Jan 31, 2020
  14. jonatack commented at 11:16 am on January 31, 2020: member
    Concept ACK, will review shortly.
  15. naumenkogs commented at 2:31 pm on January 31, 2020: member
    utACK 868647e5d72f2718e41b723dc7e15594ffb1d7b6 Thank you!
  16. 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.
  17. Avoid asmap copies in initialization d58bcdc4b5
  18. Mark asmap const in statistics code 6f8c937312
  19. Use ASNs for mapped IPv4 addresses correctly 38c2395d7a
  20. Make asmap Interpret tolerant of malicious map data c86bc14408
  21. 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)) {
  22. sipa force-pushed on Jan 31, 2020
  23. sipa commented at 10:53 pm on January 31, 2020: member
    @practicalswift Nice catch, addressed I think.
  24. 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

  25. 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
  26. 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 ...
  27. 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"-
  28. naumenkogs commented at 10:33 am on February 5, 2020: member
    utACK c86bc14
  29. laanwj commented at 12:59 pm on February 5, 2020: member
    ACK c86bc144081f960347232546f7d22deb65d27deb
  30. laanwj referenced this in commit adea5e1b54 on Feb 5, 2020
  31. laanwj merged this on Feb 5, 2020
  32. laanwj closed this on Feb 5, 2020
  33. sidhujag referenced this in commit 018782881c on Feb 9, 2020
  34. luke-jr referenced this in commit 30849d52ec on Feb 16, 2020
  35. luke-jr referenced this in commit 5fad305c58 on Feb 16, 2020
  36. luke-jr referenced this in commit 9400fa235e on Feb 16, 2020
  37. luke-jr referenced this in commit 0d7ea707fc on Feb 16, 2020
  38. jasonbcox referenced this in commit e4c59a2382 on Oct 30, 2020
  39. sidhujag referenced this in commit 47f20209ac on Nov 10, 2020
  40. kittywhiskers referenced this in commit 7cb3e60d5d on Mar 14, 2021
  41. kittywhiskers referenced this in commit 5edb8b1d2b on Mar 14, 2021
  42. kittywhiskers referenced this in commit 22646a11c2 on Mar 14, 2021
  43. kittywhiskers referenced this in commit 9be6d8a887 on Mar 14, 2021
  44. kittywhiskers referenced this in commit 74f271963f on Mar 18, 2021
  45. kittywhiskers referenced this in commit c772391b49 on Mar 18, 2021
  46. kittywhiskers referenced this in commit a343f996c1 on Mar 18, 2021
  47. kittywhiskers referenced this in commit 3282b1cb65 on Mar 18, 2021
  48. kittywhiskers referenced this in commit ad5c4a2182 on Mar 19, 2021
  49. kittywhiskers referenced this in commit 923ea8e59a on Mar 19, 2021
  50. kittywhiskers referenced this in commit a5d7726dc3 on Mar 19, 2021
  51. kittywhiskers referenced this in commit fadc735329 on Mar 19, 2021
  52. kittywhiskers referenced this in commit 567ef7707c on Mar 19, 2021
  53. kittywhiskers referenced this in commit f0c91883cd on Apr 21, 2021
  54. kittywhiskers referenced this in commit 204d6ee44f on Apr 21, 2021
  55. kittywhiskers referenced this in commit 344e7dc476 on Apr 21, 2021
  56. kittywhiskers referenced this in commit cf7ce59305 on Apr 23, 2021
  57. kittywhiskers referenced this in commit 518dd0254d on Apr 23, 2021
  58. kittywhiskers referenced this in commit 1e7b9e31fc on Apr 23, 2021
  59. kittywhiskers referenced this in commit 550e63cd03 on Apr 23, 2021
  60. kittywhiskers referenced this in commit 74dee9e401 on May 11, 2021
  61. kittywhiskers referenced this in commit cc80d540f5 on May 11, 2021
  62. kittywhiskers referenced this in commit d9ef41b1f9 on May 11, 2021
  63. UdjinM6 referenced this in commit 9973fac521 on May 15, 2021
  64. kittywhiskers referenced this in commit 734024d60e on May 18, 2021
  65. kittywhiskers referenced this in commit 68aceda35f on May 18, 2021
  66. UdjinM6 referenced this in commit 0ab6d79fda on May 19, 2021
  67. kittywhiskers referenced this in commit 354a4b16e8 on May 19, 2021
  68. kittywhiskers referenced this in commit f3819c4eef on May 19, 2021
  69. PastaPastaPasta referenced this in commit 2f555f87d1 on May 19, 2021
  70. kittywhiskers referenced this in commit dd5f28a923 on May 20, 2021
  71. random-zebra referenced this in commit 85f000ecbe on Jul 30, 2021
  72. random-zebra referenced this in commit b4751e10ce on Aug 11, 2021
  73. DrahtBot locked this on Feb 15, 2022


sipa practicalswift fanquake DrahtBot sanjaykdragon jonatack naumenkogs laanwj

Labels
P2P

Milestone
0.20.0

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-18 12:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me