addrman: change internal id counting to int64_t

[mzumsande]

With nIdCount being incremented for each addr received, an attacker could cause an overflow in the past, see https://bitcoincore.org/en/2024/07/31/disclose-addrman-int-overflow/
Even though that attack was made infeasible indirectly by addr rate-limiting (PR #22387), to be on the safe side and prevent any regressions change the nIds used internally to int64_t.
This is being done by first introducing a user-defined type for nIds in the first commit, and then updating it to int64_t (thanks sipa for help with this!).

Note that nId is only used internally, it is not part of the serialization, so peers.dat should not be affected by this.

I assume that the only reason this was not done in the past is to not draw attention to this previously undisclosed issue.

[DrahtBot]

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	naumenkogs, stratospher, achow101
Concept ACK	dergoegge, brunoerg, BrandonOdiwuor, hebasto, Christewart
Approach ACK	tdb3

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #29536 (fuzz: fuzz connman with non-empty addrman + ASMap by brunoerg)

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.

[dergoegge]

Concept ACK

[brunoerg]

Concept ACK

[mzumsande]

Updated the PR to have a refactor-only commit that introduces a user-defined type for all spots that use nId as an int, and then change that to int64_t in the second commit.

[BrandonOdiwuor]

Concept ACK

[hebasto]

Concept ACK.

[hebasto]

Good to see that a signed type is used, rather than an unsigned one in the initial version of this PR.

[mzumsande]

interesting pdf, thanks! In this particular case there was no choice because nIds in vvNew and vvTried are initialised to -1.

[sipa]

I think you'll want std::vector<std::pair<int, nid_type>> bucket_entries; as well in the serializer below (not strictly necessary as the number can't actually overflow an int, but morally, the second pair element is an id). Same with const nid_type entry_index{bucket_entry.second}; that's derived from it (and possibly other places).

[mzumsande]

Hmm, can you explain the "morally" part a bit more?

mapUnkIds maps nid_type to an int, let's call that counter ser_count. ser_count is part of the serialization and is bounded by the limited slots of the addrman tables - even if the internal id would overflow an int, ser_count wouldn't be affected by that.

During deserialization, ser_count is read into int entry_index (which must be int), and then makes it into bucket_entries and later into vvNew. But I'm unsure when is the best step to convert it into an int64_t - why is it better to already do that in the step entry_index -> bucket_entries rather than the way it is now?

[naumenkogs]

I assume this is about the introduced index mismatch at AddrInfo& info = mapInfo[entry_index];, specifically the keys of mapInfo.

It is technically safe because we always go from a narrower type to the wide one... but that's what is immoral i guess :)

I think it's fine they way this commit works, at most we could add a comment explaining this mismatch.

[stratospher]

I'm fine with the version on the branch now because Serialise() and Unserialise() are symmetrical. nIndex is written as int into peers.dat in Serialise() and read as int from peers.dat in Unserialise().

[stratospher]

Concept ACK. if you retouch, there's also:

bitcoin/src/test/fuzz/addrman.cpp

Line 189 in c2d15d9

auto IdsReferToSameAddress = [&](int id, int other_id) EXCLUSIVE_LOCKS_REQUIRED(m_impl->cs, other.m_impl->cs) {

[mzumsande]

Concept ACK. if you retouch, there's also:

Good catch, fixed!

[tdb3]

Concept ACK.
Briefly reviewed, and will return to review in more detail. Love the approach to introduce nid_type. Makes things safer moving forward.

[Christewart]

Concept ACK

So presumably test cases are in #22387 to directly test the vuln?

I had a hard time deciphering what is meant to test the new rate limiting logic in #22387 and what test cases were intended to prevent a regression for being introduced in the future that re-introduces this vulnerability.

[stratospher]

I had a hard time deciphering what is meant to test the new rate limiting logic in #22387 and what test cases were intended to prevent a regression for being introduced in the future that re-introduces this vulnerability.

Main thing is to make sure that nId values are distinct. #22387 limits the nId which the addrman can possibly construct(functional tests which test this behaviour). This PR makes the nId space so large (64 bits) that the values are distinct. so even in the absence of the rate limiting mechanism, with this PR the attacker would need to insert > 2**64 addresses to make the node crash.

(you could change the type of nIdCount to int8_t to see the undesirable effects of duplicate nId in action!)

I looked into the assertion crash since I was confused how it happened. Sharing details in case it's helpful to anyone.

32 bit nIdCount could overflow and wrap around if addr messages were spammed (nIdCount would go from 0 to INT32_MAX, then become negative and go from INT32_MIN to 0 and loop again). So nId's aren't distinct anymore. Create() is written with an assumption that every nId it receives is distinct. This line ends up overwriting the nId-> AddrInfo map without checking if it exists. This can lead to corruptions and assertion failures.

An example scenario:

• suppose AddrInfo1 with nId=36 is inserted into a bucket-position in vvNew(new table) and mapInfo now contains nId=36 -> AddrInfo1 mapping
• when nId=36 happens again (because nId wraps around due to overflow) and another address AddrInfo2 is inserted into the new table, there is no clean deletion of existing nId in vvNew /mapAddr and the nId=36 in vvNew means AddrInfo2 now.
• It's also possible that we don't want to insert AddrInfo2 into new table and nId=36 address can get deleted later.
• when the same bucket-position happens again, and we want to rewrite it's contents(nId=36) which meant the overwritten old address -> that info isn't present in mapInfo and nRefCount = 0 (because it got deleted!)
• this will cause an assertion failure like the one below and the node crashes.

bitcoind: addrman.cpp:494: void AddrManImpl::ClearNew(int, int): Assertion `infoDelete.nRefCount > 0' failed.
Aborted (core dumped)

[tdb3]

Approach ACK
Reviewed a bit more. Just nits.

[tdb3]

nit: The return type in the Doxygen content could be adjusted.

-     *  @return  int The nid of the entry. If the addrman position is empty or not found, returns -1.
+     *  @return  nid_type The nid of the entry. If the addrman position is empty or not found, returns -1.
      * */
     nid_type GetEntry(bool use_tried, size_t bucket, size_t position) const EXCLUSIVE_LOCKS_REQUIRED(cs);

nit / thinking out loud: Returning -1 is a remnant of the previous type being a plain int. To keep with the theme of using the user-defined nid_type, would it be worth creating a macro (or constexpr, or similar) to name the invalid case (e.g. INVALID_NID = -1)? Not sure, as I'm not seeing a need to adjust nid_type to anything more complicated than int64_t, and GetEntry() in this case is just indicating empty/not found.

[mzumsande]

Fixed the return type.

Yes, also not sure about INVALID_NID - I don't really see why we would want to change it from -1 to another value, but happy to add it if others like it... I'll keep this thread open for a while.

[mzumsande]

1ded29d to 51f7668:
Rebased and fixed doxygen comment.

So presumably test cases are in #22387 to directly test the vuln?

No, there is no test case that directly tests the vulnerability. For that, we'd need to add approximately 2 * INT_MAX addrs to addrman (see the explanation by @stratospher above) - which I think would be too slow to be practical for the test suite. However, one simple way to recreate the vulnerability (with this branch) is to locally change nid_type to int8_t - if you then run a node, it will crash quickly.

[naumenkogs]

ACK 51f7668

The reasoning in the original post makes sense, this is an improvement. I reviewed the code visually.

[stratospher]

ACK 51f7668. I think it's a good change to make the nId space large(64 bits) so that the nId values are distinct.

[achow101]

ACK 51f7668