Our current decoding does a very rough approximation of the size of the vector before allocating it.
In the impl_vec! macro here:
|
let len = VarInt::consensus_decode(&mut d)?.0; |
|
let byte_size = (len as usize) |
|
.checked_mul(mem::size_of::<$type>()) |
|
.ok_or(self::Error::ParseFailed("Invalid length"))?; |
|
if byte_size > MAX_VEC_SIZE { |
|
return Err(self::Error::OversizedVectorAllocation { requested: byte_size, max: MAX_VEC_SIZE }) |
In #340, we introduced a very strict constant for the message size. In the current decoding code, we are assuming that the decoded memory size in rust would be less than the consensus serialized block size (4Mb). From a superficial look, it looks like bitcoin core also has some slack space here by allowing 5Mb (See: https://github.com/bitcoin/bitcoin/blob/833add0f48b0fad84d7b8cf9373a349e7aef20b4/src/serialize.h#L34)
We are using mem::size_of which is architecture-dependent and a very rough way to do the size check. This works fine in most cases for Vec<Transaction> mem::sizeof(&Transaction) is 56 bytes(two 4 bytes and 2 vectors with 24 bytes each) on 64 machines. And most transactions would be more than 56 bytes.
When dealing with Vec<TxOuts>/Vec<TxIns, I am fairly certain that there are some valid blocks that rust-bitcoin cannot decode.
As an extreme example, I am fairly certain that if we create a Block with just a single big transaction, one input(no witness), and all outputs with all empty script pubkeys, this will crash.
All the examples that I can come up with would like not to happen in practice, but it is still good to fix the bug in case someone does a malicious deliberate attack.
Overall, I think we should
- Come with better logic to estimate size before decoding.
- Cut some slack and increase the limit to a higher limit. I am really not sure what the number here should be as it depends on the exact way we estimate
Our current decoding does a very rough approximation of the size of the vector before allocating it.
In the
impl_vec!macro here:rust-bitcoin/src/consensus/encode.rs
Lines 560 to 565 in 50d9394
In #340, we introduced a very strict constant for the message size. In the current decoding code, we are assuming that the decoded memory size in rust would be less than the consensus serialized block size (4Mb). From a superficial look, it looks like bitcoin core also has some slack space here by allowing 5Mb (See: https://github.com/bitcoin/bitcoin/blob/833add0f48b0fad84d7b8cf9373a349e7aef20b4/src/serialize.h#L34)
We are using mem::size_of which is architecture-dependent and a very rough way to do the size check. This works fine in most cases for
Vec<Transaction>mem::sizeof(&Transaction)is 56 bytes(two 4 bytes and 2 vectors with 24 bytes each) on 64 machines. And most transactions would be more than 56 bytes.When dealing with
Vec<TxOuts>/Vec<TxIns, I am fairly certain that there are some valid blocks that rust-bitcoin cannot decode.As an extreme example, I am fairly certain that if we create a Block with just a single big transaction, one input(no witness), and all outputs with all empty script pubkeys, this will crash.
All the examples that I can come up with would like not to happen in practice, but it is still good to fix the bug in case someone does a malicious deliberate attack.
Overall, I think we should