wallet: fix buffer over-read in SQLite file magic check #20216

1. 
   theStack commented at 2:37 AM on October 22, 2020: member

   Looking at our new SQLite database code, I noticed that there is a potential problem in the method IsSQLiteFile(): If there is no terminating zero within the first 16 bytes of the file, the magic buffer would be over-read in the std::string constructor for magic_str. Fixed by using the "from buffer" variant of the string ctor (that also takes a size) rather than the "from c-string" variant (see http://www.cplusplus.com/reference/string/string/string/).

   The behaviour can be reproduced by the following steps:

   • Creating a file of at least 512 bytes in size (to pass the minimum size check) that doesn't contain zero bytes in the magic area, e.g. simply: $ python3 -c "print('A'*512)" > /tmp/corrupt_wallet
   • Showing content and size of the magic_str string in case the magic check fails
   • Create a simple unit test that simply calls IsSQLiteFile with the corrupt wallet file
   • Run the unit test and see the random gibberish output of magic_str after 16 As :-)

   Or, TLDR variant, just get the branch https://github.com/theStack/bitcoin/tree/reproduce_sqlite_magic_overread, compile unit Tests and run the script ./reproduce_sqlite_magic_overread.sh.

   Note that this is the minimal diff, probably it would be better to avoid std::string at all in this case and just use memcmp, strings that include null bytes are pretty confusing.

2. wallet: fix buffer over-read in SQLite file magic check

   If there is no terminating zero within the 16 magic bytes, the buffer would be
   over-read in the std::string constructor. Fixed by using the "from buffer"
   variant of the ctor (that also takes a size) rather than the "from c-string"
   variant.

   56a461f727
3. fanquake added the label Wallet on Oct 22, 2020
4. 
   achow101 commented at 3:16 AM on October 22, 2020: member

   ACK 56a461f72796ca60de28e78f144741eb1a4f5213

5. 
   practicalswift commented at 6:06 AM on October 22, 2020: contributor

   Great find @theStack! Thanks for making Bitcoin Core stronger! 💪

   ACK 56a461f72796ca60de28e78f144741eb1a4f5213: patch looks correct

6. MarcoFalke added this to the milestone 0.21.0 on Oct 22, 2020
7. 
   promag commented at 8:29 PM on October 22, 2020: member

   Code review ACK 56a461f72796ca60de28e78f144741eb1a4f5213.

8. fanquake merged this on Oct 23, 2020
9. fanquake closed this on Oct 23, 2020

   ---

10. sidhujag referenced this in commit 2973260631 on Oct 23, 2020
11. theStack deleted the branch on Dec 1, 2020
12. DrahtBot locked this on Feb 15, 2022

Milestone
0.21.0