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. sipa cross-referenced this on Jan 29, 2020 from issue p2p: supplying and using asmap to improve IP bucketing in addrman by naumenkogs
  3. fanquake added the label P2P on Jan 29, 2020
  4. 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.

  5. fanquake commented at 12:39 AM on January 30, 2020: member

    cc @naumenkogs

  6. DrahtBot commented at 2:25 AM on January 30, 2020: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    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.

  7. sipa force-pushed on Jan 30, 2020
  8. sanjaykdragon commented at 3:15 PM on January 30, 2020: contributor

    utack - looks nice, preventing unnecessary copies

  9. 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" :)

  10. sipa cross-referenced this on Jan 31, 2020 from issue GetMappedAS(...) lookup on an IPv6 address against a maliciously constructed AS-map triggers heap buffer-overflow by practicalswift
  11. sipa commented at 12:54 AM on January 31, 2020: member

    Added a commit to resolve #18033.

  12. sipa added the label Bug on Jan 31, 2020
  13. MarcoFalke removed the label Bug on Jan 31, 2020
  14. MarcoFalke added this to the milestone 0.20.0 on Jan 31, 2020
  15. MarcoFalke renamed this:
    Some asmap improvements
    Fix some asmap issues
    on Jan 31, 2020
  16. jonatack commented at 11:16 AM on January 31, 2020: contributor

    Concept ACK, will review shortly.

  17. naumenkogs commented at 2:31 PM on January 31, 2020: member

    utACK 868647e5d72f2718e41b723dc7e15594ffb1d7b6 Thank you!

  18. 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: 
                 if (bits == 0) {
                 return default_asn;
             }
             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.

  19. Avoid asmap copies in initialization d58bcdc4b5
  20. Mark asmap const in statistics code 6f8c937312
  21. Use ASNs for mapped IPv4 addresses correctly 38c2395d7a
  22. Make asmap Interpret tolerant of malicious map data c86bc14408
  23. 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: 
                     if (bits == 0 || (ip[ip.size() - bits]) != ((match >> (matchlen - 1 - bit)) & 1)) {
  24. sipa force-pushed on Jan 31, 2020
  25. sipa commented at 10:53 PM on January 31, 2020: member

    @practicalswift Nice catch, addressed I think.

  26. 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

  27. 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

  28. jonatack commented at 9:49 AM on February 1, 2020: contributor

    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?

    $ bitcoin-cli getpeerinfo | grep 'mapped_as' | sort
    "mapped_as": 1267,
    "mapped_as": 12876,
    "mapped_as": 14061,
    "mapped_as": 15435,
    "mapped_as": 202053,
    "mapped_as": 20473,
    "mapped_as": 24940,
    "mapped_as": 24940,
    "mapped_as": 30083,
    "mapped_as": 31334,
    "mapped_as": 34878,
    "mapped_as": 34878,
    "mapped_as": 39586,
    "mapped_as": 4713,
    "mapped_as": 47869,
    "mapped_as": 54098,
    "mapped_as": 54098,
    "mapped_as": 54098,
    "mapped_as": 54098,
    "mapped_as": 54098,

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

    [#34611198](/github-metadata-backup-bitcoin-bitcoin/34611198/) REDUCE cov: 1198 ft: 3529 corp: 197/13577b exec/s: 1125 rss: 450Mb ...
  29. jonatack commented at 6:12 AM on February 2, 2020: contributor

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

    [#72474660](/github-metadata-backup-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"-
  30. jonatack cross-referenced this on Feb 2, 2020 from issue tests: Add fuzzing harness for AS-mapping (asmap) by practicalswift
  31. naumenkogs commented at 10:33 AM on February 5, 2020: member

    utACK c86bc14

  32. laanwj commented at 12:59 PM on February 5, 2020: member

    ACK c86bc144081f960347232546f7d22deb65d27deb

  33. laanwj referenced this in commit adea5e1b54 on Feb 5, 2020
  34. laanwj merged this on Feb 5, 2020
  35. laanwj closed this on Feb 5, 2020
  36. laanwj cross-referenced this on Feb 5, 2020 from issue config, net, test: asmap feature refinements and functional tests by jonatack
  37. hebasto cross-referenced this on Feb 5, 2020 from issue p2p: Try to preserve outbound block-relay-only connections during restart by hebasto
  38. sidhujag referenced this in commit 018782881c on Feb 9, 2020
  39. luke-jr referenced this in commit 30849d52ec on Feb 16, 2020
  40. luke-jr referenced this in commit 5fad305c58 on Feb 16, 2020
  41. luke-jr referenced this in commit 9400fa235e on Feb 16, 2020
  42. luke-jr referenced this in commit 0d7ea707fc on Feb 16, 2020
  43. MarkLTZ cross-referenced this on Apr 4, 2020 from issue Bitcoin PR tracking by MarkLTZ
  44. jasonbcox referenced this in commit e4c59a2382 on Oct 30, 2020
  45. sidhujag referenced this in commit 47f20209ac on Nov 10, 2020
  46. xdustinface cross-referenced this on Mar 13, 2021 from issue merge #17812, #16702, #16730, #18023: supplying and using asmap to improve IP bucketing in addrman by kwvg
  47. kwvg referenced this in commit 7cb3e60d5d on Mar 14, 2021
  48. kwvg referenced this in commit 5edb8b1d2b on Mar 14, 2021
  49. kwvg referenced this in commit 22646a11c2 on Mar 14, 2021
  50. kwvg referenced this in commit 9be6d8a887 on Mar 14, 2021
  51. kwvg referenced this in commit 74f271963f on Mar 18, 2021
  52. kwvg referenced this in commit c772391b49 on Mar 18, 2021
  53. kwvg referenced this in commit a343f996c1 on Mar 18, 2021
  54. kwvg referenced this in commit 3282b1cb65 on Mar 18, 2021
  55. kwvg referenced this in commit ad5c4a2182 on Mar 19, 2021
  56. kwvg referenced this in commit 923ea8e59a on Mar 19, 2021
  57. kwvg referenced this in commit a5d7726dc3 on Mar 19, 2021
  58. kwvg referenced this in commit fadc735329 on Mar 19, 2021
  59. kwvg referenced this in commit 567ef7707c on Mar 19, 2021
  60. kwvg referenced this in commit f0c91883cd on Apr 21, 2021
  61. kwvg referenced this in commit 204d6ee44f on Apr 21, 2021
  62. kwvg referenced this in commit 344e7dc476 on Apr 21, 2021
  63. kwvg referenced this in commit cf7ce59305 on Apr 23, 2021
  64. kwvg referenced this in commit 518dd0254d on Apr 23, 2021
  65. kwvg referenced this in commit 1e7b9e31fc on Apr 23, 2021
  66. kwvg referenced this in commit 550e63cd03 on Apr 23, 2021
  67. kwvg referenced this in commit 74dee9e401 on May 11, 2021
  68. kwvg referenced this in commit cc80d540f5 on May 11, 2021
  69. kwvg referenced this in commit d9ef41b1f9 on May 11, 2021
  70. UdjinM6 referenced this in commit 9973fac521 on May 15, 2021
  71. kwvg referenced this in commit 734024d60e on May 18, 2021
  72. kwvg referenced this in commit 68aceda35f on May 18, 2021
  73. UdjinM6 referenced this in commit 0ab6d79fda on May 19, 2021
  74. kwvg referenced this in commit 354a4b16e8 on May 19, 2021
  75. kwvg referenced this in commit f3819c4eef on May 19, 2021
  76. PastaPastaPasta referenced this in commit 2f555f87d1 on May 19, 2021
  77. kwvg referenced this in commit dd5f28a923 on May 20, 2021
  78. furszy cross-referenced this on Jun 7, 2021 from issue Road to Tor v3 support (BIP155) by furszy
  79. furszy cross-referenced this on Jul 15, 2021 from issue [Net] asmap to improve IP bucketing in addrman - backports by furszy
  80. random-zebra referenced this in commit 85f000ecbe on Jul 30, 2021
  81. random-zebra referenced this in commit b4751e10ce on Aug 11, 2021
  82. bitcoin locked this on Feb 15, 2022
Contributors
sipapracticalswiftfanquakeDrahtBotsanjaykdragonjonatacknaumenkogslaanwj
Labels
P2P

Milestone
0.20.0

Linked (view graph)
#16702 p2p: supplying and using asmap to improve IP bucketing in addrman#17428 p2p: Try to preserve outbound block-relay-only connections during restart#17812 config, net, test: asmap feature refinements and functional tests#18029 tests: Add fuzzing harness for AS-mapping (asmap)#18033 GetMappedAS(...) lookup on an IPv6 address against a maliciously constructed AS-map triggers heap buffer-overflow
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: 2026-05-20 06:54 UTC