From 0bff9b06f7086e83aca2fb00dabe35a68eb61eae Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Fri, 14 Feb 2025 02:30:18 +0000 Subject: [PATCH] psbt: guard against empty PSBT transaction 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 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` on this object, which returns a std::optional. 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 acf709b3ab4bf992482b6bbb93ed46681765638d, 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. --- src/psbt.h | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/psbt.h b/src/psbt.h index aef15127ef..5271adfecf 100644 --- a/src/psbt.h +++ b/src/psbt.h @@ -901,12 +901,16 @@ struct PSBTInput Sidechain::Bitcoin::CTransactionRef tx; OverrideStream os(&s, s.GetType(), s.GetVersion()); UnserializeFromVector(os, tx); - m_peg_in_tx = tx; + if (tx) { + m_peg_in_tx = tx; + } } else { CTransactionRef tx; OverrideStream os(&s, s.GetType(), s.GetVersion()); UnserializeFromVector(os, tx); - m_peg_in_tx = tx; + if (tx) { + m_peg_in_tx = tx; + } } break; }