bruno
June 17, 2024, 4:15pm
6
Updates!
We recently added support for btcd and we discovered and could reproduce more bugs.
rust-miniscript : Some miniscripts were unexpectedly being rejected
opened 09:30PM - 11 Dec 23 UTC
closed 10:54PM - 28 Feb 24 UTC
Hi, recently I started developing [bitcoinfuzz](https://github.com/brunoerg/bitc… oinfuzz) - differential fuzzing of Bitcoin implementations and libraries. One of the targets gets a string and checks whether it's a valid miniscript. The code I'm using to check it with `rust-miniscript` is:
```rust
#[no_mangle]
pub extern "C" fn rust_miniscript_from_str(input: *const c_char) -> bool {
if let Ok(data) = unsafe { CStr::from_ptr(input) }.to_str() {
if let Ok(_pol) = Miniscript::<String, Segwitv0>::from_str_insane(data) {
return true
} else if let Ok(_pol) = Miniscript::<String, Tap>::from_str_insane(data) {
return true
}
}
false
}
```
and `bitcoinfuzz` is crashing (rust-miniscript returning invalid) with the following miniscripts (and other ones):
```
nnnnnnnnnnnnnnnln:1
dv:0
lll:0
l:1
```
Could I be missing something in my code or is it a bug?
rust-bitcoin : During witness verification, rust-bitcoin doesn’t check if a transaction has the witness flag but empty witnesses.
opened 07:28PM - 12 Apr 24 UTC
By applying differential fuzzing ([bitcoinfuzz](https://github.com/brunoerg/bitc… oinfuzz)) with rust-bitcoin and Bitcoin Core for block deserialization, I found out a bug during the transaction verification step. Basically, it's illegal to have the witness flag present and all witness stacks empty. That is, it has inputs, they have empty witnesses, but the transaction is flagged as segwit. It seems this case is not being catched properly.
Base64 to reproduce (for block deserialization): //////////+puampqalJSUlJSUlJSUlJSUlJSUlJSUlJSUlJSUlJSUlJSUlJqampKak4ODg4ODg4qampqVdRqampAgICAgIB/QITAAICAgICAgICAAABAABRAgICAgH9AgICAgICAgICAgICAgICAgICAgICAACRAAIAAAAAAAAAAFECAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgAAApECAAIAAAAAAAAAUQICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAICRAAAAAAAAFv////8AAD0A/////y3/////////AP////8AAQAAAAD///8=
btcd : Not a bug but an API mismatch with Bitcoin Core. When decoding a transaction, Bitcoin Core fails if the input was not entirely consumed, btcd doesn’t.
opened 09:13PM - 06 Jun 24 UTC
Using the `decoderawtransaction` command (`btcDecode` function?) we noticed that… it does not check if the entire input was successfully consumed when decoding which is important to keep it consistent.
To reproduce:
```
decoderawtransaction "0a9a9e0101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010100000000010001010101a09a"
```
Obs.: Running same command on Bitcoin Core fails.
Bitcoin Core/rust-miniscript : When validating miniscripts from string, there is an inconsistence between rust-miniscript and Bitcoin Core. We noticed that Bitcoin Core accepts the usage of the + sign, e.g. (l:older(+1) , u:after(+1) ), because of ParseInt64 function. However, rust-miniscript checks the character itself, so it only accepts “1”, “2”, “3”…“9”.
opened 07:35PM - 14 Jun 24 UTC
crash
We got a crash on `miniscript_string` target due to validation of numbers. For `… older()`, `after()`, `thresh()` and maybe other fragments, Bitcoin Core accept the usage of the `+` sign, e.g. (`l:older(+1)`, `u:after(+1)`), because of `ParseInt64` function. However, `rust-miniscript` checks the character itself, so it only accepts "1", "2", "3"..."9".
```rs
if !('1'..='9').contains(&ch) {
return Err(Error::Unexpected("Number must start with a digit 1-9".to_string()));
}
```
There is a test to ensure it doesn't accept those ones:
```rs
fn test_parse_num() {
assert!(parse_num("0").is_ok());
assert!(parse_num("00").is_err());
assert!(parse_num("0000").is_err());
assert!(parse_num("06").is_err());
assert!(parse_num("+6").is_err());
assert!(parse_num("-6").is_err());
}
```
rust-miniscript : The parser function is still recursive while Core isn’t anymore. It makes some miniscripts valid for Core but rejected by rust-miniscript due to the max recursion depth. (We didn’t find this, we just could reproduce by running the harness with the seed corpus from Core)
opened 04:29PM - 14 Jun 24 UTC
Sorry if it's a dumb question, as far as I understand there is a recursion depth… limit on `from_slice_delim` to avoid running out of stack space (based on https://github.com/sipa/miniscript/pull/5) when parsing an expression with round brackets. However, the parse function from sipa/miniscript is not recursive anymore (https://github.com/sipa/miniscript/pull/68). Worths make it non-recursive here as well? I noticed that some miniscripts that can be parsed on Core fails here because of this limit.
```rs
// https://github.com/sipa/miniscript/pull/5 for discussion on this number
const MAX_RECURSION_DEPTH: u32 = 402;
```
```rs
impl<'a> Tree<'a> {
/// Parse an expression with round brackets
pub fn from_slice(sl: &'a str) -> Result<(Tree<'a>, &'a str), Error> {
// Parsing TapTree or just miniscript
Self::from_slice_delim(sl, 0u32, '(')
}
pub(crate) fn from_slice_delim(
mut sl: &'a str,
depth: u32,
delim: char,
) -> Result<(Tree<'a>, &'a str), Error> {
if depth >= MAX_RECURSION_DEPTH {
return Err(Error::MaxRecursiveDepthExceeded);
}
```
4 Likes