pset: fix NULL pointer dereference when deserializing malformed PSETs over RPC #1416
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ubsan doesn't like assigning arbitrary uint8_t values to bool. It's easy to avoid doing, so do it. (We do this specifically in PSET since that's Elements-specific code, but the same issue is present in Bitcoin in the Unserialize impl for bool in serialize.h. Upstream this is only used in the wallet database, where it may be that non 0/1 values are impossible (absent a corrupt wallet).
There is a crashing bug in psbt.h which works as follows. This occurs in the psbt_deserialize_input fuzz test, which deserializes an input and then tries to reserialize it. Here a PSET input may contain a mainchain transaction, which is where our trouble is. The process is as follows: 1. On line 901, we create a CTransactionRef, which is a newtype around std::shared_ptr<CTransaction> which defaults to being null. 2. On line 903 we then call `UnserializeFromVector` to populate this, where this is a helper function which attempts to read some number of objects from a byte vector, i.e. a length-prefixed blob. 3. HOWEVER, `UnserializeFromVector` when given an empty vector, decides that it has successfully deserialized zero elements, and returns. 4. Then, on line 904 we assign the CTransactionRef, which is a valid std::shared_ptr whose internal pointer is NULL, to `m_peg_in_tx`, which is a variant of monostate, Bitcoin::CTransactionRef, and CTransactionRef. Its variant changes from the default monostate to CTransactionRef. 5. Then, on line 419, we call `std::get_if<CTransactionRef>` on this object, which returns a std::optional<CTransactionRef>. Because `m_peg_in_tx` is in the `CTransactionRef` variant, this succeeds, returning a true std::optional containing a valid std::shared_ptr which contains a NULL pointer. 6. Then, on line 420, we call `if (peg_in_tx)`, which is true, because we have a true std::optional. We then dereference it on line 423, which is perfectly legal, to get our std::shared_ptr, and pass this shared pointer to SerializeToVector. 7. SerializeToVector passes through like 6 layers of serialize.h obfuscation and eventually dereferences the shared pointer, but because it's NULL, this is a NULL pointer dereference, and we get a crash. There are two lessons here: 1. Don't use C++. As I say in acf709b, where I introduced some of the offending code here (but only by replacing boost stuff with their STL equivalents; the bug existed before I did this), "what a trainwreck of a language". 2. Don't use `UnserializeFromVector` and expect it to throw if the data you're deserializing is malformed. If it's malformed in the sense of being empty, it will "succeed" and silently do nothing. I glanced at every other instance of UnserializeFromVector to check what will happen when it's passed an empty string. I believe there are no other cases where it will fail to initialize a NULL pointer, so I believe that this will not cause other crashes. But I also believe that the behavior in this case is almost always wrong and that we parse malformed PSETs in crazy and incorrect ways all over the place. If anybody has a problem with this, I encourage you to go review the Bitcoin PSBT2 PRs, which this stuff is based on, which have been languishing in rebase hell for the better part of a decade. Don't blame the author for not writing perfect code in a hostile language with no support. After this PR I ran the fuzzer for 16 hours on a 192-thread machine (so 3072 CPU-hours) and didn't see any more crashes.
|
Will undraft once I'm done double-checking locally with all sanitizers on. |
delta1
approved these changes
Feb 18, 2025
delta1
added a commit
that referenced
this pull request
Feb 18, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
I mentioned in one of my fuzz seed PRs that I couldn't provide seeds for the
psbt_input_deserializetarget because they were crashing.This PR fixes the crash. I will PR to the fuzz seeds repo shortly.