Check that CAddrMan::nKey is not null after deserialize #22498

maflcko · 2021-07-19T13:44:04Z

Otherwise CAddrMan::_Check() will complain.

An alternative would be to remove the check from CAddrMan::_Check().

maflcko · 2021-07-19T13:45:51Z

Converted to draft to avoid merge conflicts for now

vasild · 2021-07-19T14:05:25Z

nKey is a random integer. Why treat 0 specially? It is a possible valid value and no less or more likely to be set in nKey = FastRandomContext::rand256(); than any other value, e.g. 0xFFFF... or 0x5555....

maflcko · 2021-07-19T14:09:50Z

Zero is a special value because it is used to clear the memory before destructing addrman. Though, I see your point, which is why I mentioned the alternative in OP:

An alternative would be to remove the check from CAddrMan::_Check().

Are you saying I should remove the check from _Check?

vasild

ACK fa52c7e

Needs rebase.

Are you saying I should remove the check from _Check?

I don't have strong opinion and this is so minor that it will work either way. Personally I would remove it from Check() and remove the SetNull() from addrman destructor. Nowadays there are better ways to detect use-after-free than setting select members to magic values in the destructor.

Or add a comment to m_key to explain the magic value or make the magic value a static member constant or:

-        nKey = insecure_rand.rand256();
+        do {
+            nKey = insecure_rand.rand256();
+        } while (nKey.IsNull());

to make it explicit.

sipa · 2021-07-19T14:44:28Z

There is no need for that. It's a 256-bit value. If we're worried about the chance that it can randomly be 0, we should first be worried about hash collisions in blocks, and randomly guessing private keys.

vasild · 2021-07-19T14:58:51Z

to make it explicit

... to the reader that 0 is treated specially, it would be more of a documentation than avoiding a 1/2^256 chance.

maflcko · 2021-07-19T15:15:53Z

Needs rebase.

Not if it is merged after the pull that it conflicts with ;)

I don't have strong opinion and this is so minor that it will work either way. Personally I would remove it from Check() and remove the SetNull() from addrman destructor. Nowadays there are better ways to detect use-after-free than setting select members to magic values in the destructor.

Makes sense. Will do, because addrman is only destructed on shutdown, at which point is seems unlikely that someone successfully pulls an attack after exploiting a use-after-free on m_key.

sipa · 2021-07-19T15:17:32Z

The reason for wiping the key is to avoid keeping secret key material longer in memory than needed, by the way. The fact that it allows use-after-free detection is a side effect.

maflcko · 2021-07-19T15:28:14Z

The secret is written to disk in plain text and thus kept in the cache in memory. With that in mind, is it important to still set it to zero on shutdown?

sipa · 2021-07-19T15:32:16Z

I'd still say so, yes. This is no different from wallet private keys in an unencrypted wallet.

Leaving it in memory means it risks ending up in uninitialized memory that's allocated, where it may leak in case of other bugs. The addrman key is of course not nearly as sensitive as wallet keys, and the fact that it remains in use almost the entire lifetime of the process makes it less of an issue further, but I think it's good practice in general to wipe key material that is unused.

We could go further and allocate it using the locked pool, actually.

DrahtBot · 2021-07-19T18:56:25Z

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

Conflicts

No conflicts as of last run.

maflcko · 2021-07-20T06:01:15Z

Ok, I am not removing the m_key.SetNull() in this pull. It seems unrelated anyway. Also, I took this out of draft and force pushed to fixup the tests in the first commit, which I forgot initially.

vasild · 2021-07-20T09:39:44Z

addrman_tests/remove_invalid and addrman_tests/addrman_serialization are failing due to the new check: "Corrupt CAddrMan serialization, m_key is all zeros"

lsilva01

The same errors occurred.

fatal error: in "addrman_tests/addrman_serialization": std::ios_base::failure[abi:cxx11]: Corrupt CAddrMan serialization, m_key is all zeros.: iostream error

fatal error: in "addrman_tests/remove_invalid": std::ios_base::failure[abi:cxx11]: Corrupt CAddrMan serialization, m_key is all zeros.: iostream error

practicalswift · 2021-07-24T19:41:17Z

Concept ACK

jnewbery · 2021-08-17T12:32:41Z

Concept ACK

Otherwise CAddrMan::_Check() will complain.

maflcko · 2021-08-18T07:23:53Z

See #22734

…k after deserialize fa3669f fuzz: Move all addrman fuzz targets to one file (MarcoFalke) fa7a883 addrman: Replace assert with throw on corrupt data (MarcoFalke) fa29897 Refactor: Turn the internal addrman check helper into a forced check (MarcoFalke) fae5c63 move-only: Move CAddrMan::Check to cpp file (MarcoFalke) Pull request description: Assert should only be used for program internal logic errors, not to sanitize external user input. The assert was introduced via the debug-only runtime option `-checkaddrman` in commit 803ef70, thus won't need a backport. Also, it doesn't really make sense to continue when the deserialized addrman doesn't pass the sanity check. For example, if `nLastSuccess` is negative, it would later result in integer overflows. Thus, this patch fixes bitcoin#22931. Also, Fixes bitcoin#22503 Fixes bitcoin#22504 Fixes bitcoin#22519 Closes bitcoin#22498 Steps to test: ``` mkdir -p /tmp/test_235/regtest/ echo 'H4sIAAAAAAAAA/u1f+stZmUGYgELgwPRakfBKBgFo2AUjIJRMApGwSgYBaNgFIyCUTBswdyGpFnLjUKjP9e0bvjYusl6b+L2e7Vs2dd6N//Pua0/xQUALJAn93IQAAA=' | base64 --decode | zcat > /tmp/test_235/regtest/peers.dat ./src/qt/bitcoin-qt -regtest -datadir=/tmp/test_235/ -checkaddrman=1 -printtoconsole | grep -A2 'Loading P2P addresses' ``` Output before: ``` 2021-09-10T11:28:37Z init message: Loading P2P addresses… 2021-09-10T11:28:37Z ADDRMAN CONSISTENCY CHECK FAILED!!! err=-16 bitcoin-qt: addrman.cpp:765: void CAddrMan::Check() const: Assertion `false' failed. (program crashes) ``` Output after: ``` 2021-09-10T11:26:00Z init message: Loading P2P addresses… 2021-09-10T11:26:00Z Error: Invalid or corrupt peers.dat (Corrupt data. Consistency check failed with code -16: iostream error). If you believe this is a bug, please report it to https://github.com/bitcoin/bitcoin/issues. As a workaround, you can move the file ("/tmp/test_235/regtest/peers.dat") out of the way (rename, move, or delete) to have a new one created on the next start. (program exits) ``` ACKs for top commit: naumenkogs: ACK fa3669f jnewbery: Code review ACK fa3669f vasild: ACK fa3669f Tree-SHA512: 687e4a4765bbc66495152fa7a49d28ee84b405dc5370ba87b4016b5593e45f54c4ce5cae579e4d433e0e082d20fc263969fa602679c911accef0adb2d6213bd6

maflcko marked this pull request as draft July 19, 2021 13:45

maflcko added this to the Future milestone Jul 19, 2021

DrahtBot added the P2P label Jul 19, 2021

vasild approved these changes Jul 19, 2021

View reviewed changes

This was referenced Jul 19, 2021

addrman: Remove addrman hotfixes #22496

Merged

Assert in CAddrMan::Check() #22500

Closed

maflcko force-pushed the 2107-addrmanKeyZero branch from fa52c7e to fa64288 Compare July 20, 2021 06:00

maflcko marked this pull request as ready for review July 20, 2021 06:01

maflcko removed this from the Future milestone Jul 20, 2021

maflcko marked this pull request as draft July 20, 2021 06:31

lsilva01 reviewed Jul 20, 2021

View reviewed changes

This was referenced Jul 21, 2021

addrman: Make consistency checks a runtime option #20233

Closed

refactor: Mark CAddrMan::Select and GetAddr const #21940

Merged

maflcko force-pushed the 2107-addrmanKeyZero branch from fa64288 to 0b8e34c Compare August 16, 2021 16:24

DrahtBot mentioned this pull request Aug 16, 2021

addrman: Remove CAddrMan::Clear() function #22697

Merged

maflcko changed the title ~~Check that CAddrMan::m_key is not null after deserialize~~ Check that CAddrMan::nKey is not null after deserialize Aug 17, 2021

Check that CAddrMan::nKey is not null

fa5ee2f

Otherwise CAddrMan::_Check() will complain.

maflcko force-pushed the 2107-addrmanKeyZero branch from 0b8e34c to fa5ee2f Compare August 17, 2021 15:25

maflcko mentioned this pull request Aug 18, 2021

addrman: Avoid crash on corrupt data, Force Check after deserialize #22734

Merged

maflcko closed this Aug 18, 2021

maflcko deleted the 2107-addrmanKeyZero branch August 18, 2021 07:24

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

Check that CAddrMan::nKey is not null after deserialize #22498

Check that CAddrMan::nKey is not null after deserialize #22498

Uh oh!

Conversation

maflcko commented Jul 19, 2021

Uh oh!

maflcko commented Jul 19, 2021

Uh oh!

vasild commented Jul 19, 2021

Uh oh!

maflcko commented Jul 19, 2021

Uh oh!

vasild left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sipa commented Jul 19, 2021

Uh oh!

vasild commented Jul 19, 2021

Uh oh!

maflcko commented Jul 19, 2021

Uh oh!

sipa commented Jul 19, 2021

Uh oh!

maflcko commented Jul 19, 2021

Uh oh!

sipa commented Jul 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jul 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

maflcko commented Jul 20, 2021

Uh oh!

vasild commented Jul 20, 2021

Uh oh!

lsilva01 left a comment

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Jul 24, 2021

Uh oh!

jnewbery commented Aug 17, 2021

Uh oh!

maflcko commented Aug 18, 2021

Uh oh!

Uh oh!

vasild left a comment •

edited

Loading

sipa commented Jul 19, 2021 •

edited

Loading

DrahtBot commented Jul 19, 2021 •

edited

Loading