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

1. 
   MarcoFalke commented at 1:44 pm on July 19, 2021: member

   Otherwise CAddrMan::_Check() will complain.

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

2. MarcoFalke marked this as a draft on Jul 19, 2021
3. 
   MarcoFalke commented at 1:45 pm on July 19, 2021: member

   Converted to draft to avoid merge conflicts for now
4. MarcoFalke added this to the milestone Future on Jul 19, 2021
5. 
   vasild commented at 2:05 pm on July 19, 2021: member

   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....
6. 
   MarcoFalke commented at 2:09 pm on July 19, 2021: member

   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?

7. DrahtBot added the label P2P on Jul 19, 2021
8. vasild approved
9. 
   vasild commented at 2:39 pm on July 19, 2021: member

   ACK fa52c7e2c20ca6a4c3f217f9a00c165e5fff7e91

   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:

   0-        nKey = insecure_rand.rand256();
   1+        do {
   2+            nKey = insecure_rand.rand256();
   3+        } while (nKey.IsNull());
   

   to make it explicit.

10. 
    sipa commented at 2:44 pm on July 19, 2021: member

    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.
11. 
    vasild commented at 2:58 pm on July 19, 2021: member

    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.

12. 
    MarcoFalke commented at 3:15 pm on July 19, 2021: member

    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.

13. 
    sipa commented at 3:17 pm on July 19, 2021: member

    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.
14. 
    MarcoFalke commented at 3:28 pm on July 19, 2021: member

    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?
15. 
    sipa commented at 3:32 pm on July 19, 2021: member

    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.

16. 
    DrahtBot commented at 6:56 pm on July 19, 2021: member

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

    Conflicts

    No conflicts as of last run.

17. MarcoFalke force-pushed on Jul 20, 2021
18. 
    MarcoFalke commented at 6:01 am on July 20, 2021: member

    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.
19. MarcoFalke marked this as ready for review on Jul 20, 2021
20. MarcoFalke removed this from the milestone Future on Jul 20, 2021
21. MarcoFalke marked this as a draft on Jul 20, 2021
22. 
    vasild commented at 9:39 am on July 20, 2021: member

    addrman_tests/remove_invalid and addrman_tests/addrman_serialization are failing due to the new check: “Corrupt CAddrMan serialization, m_key is all zeros”
23. 
    lsilva01 commented at 6:18 pm on July 20, 2021: contributor

    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

24. 
    practicalswift commented at 7:41 pm on July 24, 2021: contributor

    Concept ACK
25. MarcoFalke force-pushed on Aug 16, 2021
26. 
    jnewbery commented at 12:32 pm on August 17, 2021: member

    Concept ACK
27. MarcoFalke renamed this:
    Check that CAddrMan::m_key is not null after deserialize
    Check that CAddrMan::nKey is not null after deserialize
    on Aug 17, 2021
28. Check that CAddrMan::nKey is not null

    Otherwise CAddrMan::_Check() will complain.

    fa5ee2f95a
29. MarcoFalke force-pushed on Aug 17, 2021
30. 
    MarcoFalke commented at 7:23 am on August 18, 2021: member

    See #22734
31. MarcoFalke closed this on Aug 18, 2021

    ---

32. MarcoFalke deleted the branch on Aug 18, 2021
33. MarcoFalke referenced this in commit a8a272ac32 on Sep 21, 2021
34. sidhujag referenced this in commit 43bc87779b on Sep 21, 2021
35. DrahtBot locked this on Aug 18, 2022

Contributors
MarcoFalke vasild sipa DrahtBot lsilva01 practicalswift jnewbery

Labels
P2P