Skip to content

Conversation

theStack
Copy link
Contributor

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.

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.
@achow101
Copy link
Member

ACK 56a461f

@practicalswift
Copy link
Contributor

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

ACK 56a461f: patch looks correct

@maflcko maflcko added this to the 0.21.0 milestone Oct 22, 2020
Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK 56a461f.

@fanquake fanquake merged commit 9af7c19 into bitcoin:master Oct 23, 2020
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 23, 2020
…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
@theStack theStack deleted the 20201022-wallet-fix-sqlite-magic-buffer-overread branch December 1, 2020 09:54
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants