wallet: fix buffer over-read in SQLite file magic check #20216
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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, themagic
buffer would be over-read in thestd::string
constructor formagic_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:
$ python3 -c "print('A'*512)" > /tmp/corrupt_wallet
magic_str
string in case the magic check failsIsSQLiteFile
with the corrupt wallet filemagic_str
after 16A
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 usememcmp
, strings that include null bytes are pretty confusing.