Skip to content

Commit

Permalink
Merge bitcoin#20216: wallet: fix buffer over-read in SQLite file magi…
Browse files Browse the repository at this point in the history
…c check

56a461f wallet: fix buffer over-read in SQLite file magic check (Sebastian Falbesoner)

Pull request description:

  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 `A`s :-)

  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.

ACKs for top commit:
  promag:
    Code review ACK 56a461f.
  practicalswift:
    ACK 56a461f: patch looks correct
  achow101:
    ACK 56a461f

Tree-SHA512: a7aadd4d38eb92337e6281df2980f4bde744dbb6cf112b9cd0f2cab8772730e302db9123a8fe7ca4e7e844c47e68957487adb2bed4518c40b4bed6a69d7922b4
  • Loading branch information
fanquake committed Oct 23, 2020
2 parents 9453fbf + 56a461f commit 9af7c19
Showing 1 changed file with 2 additions and 2 deletions.
4 changes: 2 additions & 2 deletions src/wallet/sqlite.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -619,8 +619,8 @@ bool IsSQLiteFile(const fs::path& path)
file.close();

// Check the magic, see https://sqlite.org/fileformat2.html
std::string magic_str(magic);
if (magic_str != std::string("SQLite format 3")) {
std::string magic_str(magic, 16);
if (magic_str != std::string("SQLite format 3", 16)) {
return false;
}

Expand Down

0 comments on commit 9af7c19

Please sign in to comment.