smite: change MineBlocks(0) panic into noop

[ekzyis]

I have noticed a crash in BitcoinCli::mine_blocks when executing the operation MineBlocks(0). I found this on 099c6cb with the LND IR scenario and without custom mutator, so running afl-fuzz like this:

$ afl-fuzz -X -i /tmp/smite-seeds -o /tmp/smite-out -- /tmp/smite-nyx

Crash input IR program (MineBlocks(0)):

$ xxd -p crash
0113000005152d0f

# to recreate:
$ echo -n "0113000005152d0f" | xxd -r -p > crash

$ cat crash.log
panicked at smite/src/bitcoin.rs:79:9:
bitcoin-cli -generate 0 failed: error: the first argument (number of blocks to generate, default: 1) must be an integer value greater than zero

Local reproduction:

$ docker run --rm -v $PWD/crash:/input.bin -e SMITE_INPUT=/input.bin smite-lnd-ir /lnd-scenario
INFO  [smite_scenarios::targets::bitcoind] Starting bitcoind...
INFO  [smite_scenarios::targets::bitcoind] Waiting for bitcoind to be ready...
INFO  [smite_scenarios::targets::bitcoind] bitcoind is ready
INFO  [smite_scenarios::targets::lnd] Starting lnd...
INFO  [smite_scenarios::targets::lnd] Waiting for lnd to be ready and synced...
INFO  [smite_scenarios::targets::lnd] LND identity pubkey: 03857c20775de69a6247eaf074fcd8fd77067930af43c125bf387bf4b3e1167fd8, blockheight: 101, synced_to_chain: false
DEBUG [smite_scenarios::targets::lnd] lnd not yet synced (blockheight=101, synced_to_chain=false)
INFO  [smite_scenarios::targets::lnd] LND identity pubkey: 03857c20775de69a6247eaf074fcd8fd77067930af43c125bf387bf4b3e1167fd8, blockheight: 101, synced_to_chain: true
INFO  [smite_scenarios::targets::lnd] lnd synced (blockheight=101)
INFO  [smite_scenarios::targets::lnd] Both daemons are running, ready to fuzz
DEBUG [smite_scenarios::scenarios] Handshake complete, received target init
INFO  [smite::scenarios] Scenario initialized! Executing input...
INFO  [smite::runners] Reading input from "/input.bin"
DEBUG [smite_scenarios::scenarios::ir] [1.866µs] Executing IR program (1 instructions, 8 input bytes)

thread 'main' (1) panicked at smite/src/bitcoin.rs:79:9:
bitcoin-cli -generate 0 failed: error: the first argument (number of blocks to generate, default: 1) must be an integer value greater than zero

note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
DEBUG [smite::process] lnd: dropping running process, attempting shutdown
DEBUG [smite::process] lnd: sending SIGTERM to process group 55
DEBUG [smite::process] lnd: exited with exit status: 0
DEBUG [smite::process] bitcoind: dropping running process, attempting shutdown
DEBUG [smite::process] bitcoind: sending SIGTERM to process group 7
DEBUG [smite::process] bitcoind: exited with exit status: 0

This showed up as a crash in the fuzzer, but I think it shouldn't, since the target didn't crash. I think these programs should be skipped.

As far as I can tell, without the custom mutator, we don't validate the program before executing it. We only validate the program inside afl_custom_fuzz with decode_and_validate:

smite/smite-ir-mutator/src/lib.rs

Lines 159 to 191 in 099c6cb

	pub unsafe extern "C" fn afl_custom_fuzz(
	data: *mut c_void,
	buf: *mut u8,
	buf_size: usize,
	out_buf: *mut *const u8,
	_add_buf: *mut u8,
	_add_buf_size: usize,
	max_size: usize,
	) -> usize {
	let state = unsafe { &mut *data.cast::<MutatorState>() };
	let generate_fresh = buf_size == 0 || state.rng.random_range(0..20) == 0;
	let program = if generate_fresh {
	state.generate_fresh()
	} else {
	let input = unsafe { slice::from_raw_parts(buf, buf_size) };
	match decode_and_validate(input) {
	Some(mut p) => {
	state.mutate_stacked(&mut p);
	p
	}
	None => state.generate_fresh(),
	}
	};
	let len = if state.serialize(&program, max_size) {
	state.out_buf.len()
	} else {
	0
	};
	unsafe { *out_buf = state.out_buf.as_ptr() };
	len
	}

However, I think this would happen even with the custom mutator, even though I wasn't able to reproduce the crash while fuzzing with it. Program::validate currently does not check if the input to mine_blocks is > 0. This PR fixes that.

Additionally, I'm wondering if we should also validate in IrScenario::run:

diff --git a/smite-scenarios/src/scenarios/ir.rs b/smite-scenarios/src/scenarios/ir.rs
index f171aa3..7492cc8 100644
--- a/smite-scenarios/src/scenarios/ir.rs
+++ b/smite-scenarios/src/scenarios/ir.rs
@@ -41,6 +41,11 @@ impl<T: Target, S: SnapshotSetup<T>> Scenario for IrScenario<T, S> {
             return ScenarioResult::Skip;
         };

+        if program.validate().is_err() {
+            log::debug!("Invalid program, skipping");
+            return ScenarioResult::Skip;
+        }
+
         log::debug!(
             "[{:?}] Executing IR program ({} instructions, {} input bytes)",
             start.elapsed(),

[morehouse]

It's unfortunate that bitcoin-cli -generate 0 returns an error...

Perhaps we should just change MineBlocks(u8) to MineBlocks(NonZeroU8), which would make it impossible to postcard-deserialize a MineBlocks(0).

[NishantBansal2003]

I have noticed a crash in BitcoinCli::mine_blocks when executing the operation MineBlocks(0). I found this on 099c6cb with the LND IR scenario and without custom mutator, so running afl-fuzz like this:

I think IR programs are generally expected to be run with a custom mutator. Also, I was aware that -generate 0 returns an error, which is why I clamped it to > 0 in:

smite/smite-ir/src/mutators/operation_param.rs

Line 77 in 269699e

*v = rng.random_range(1..=16);

I was also thinking of following the same range in generators when appending operations. With the custom mutator in place, I don’t think this panic should occur. WDYT?

I think adding a NonZeroU8 variable type will also boil down to the approach that I suggested above

[morehouse]

I guess the main question is whether it's OK for the executor to panic when input is invalid. Originally I tried to make the executor skip invalid programs rather than reporting an error. The idea was to allow users to hand-create seed programs or to use AFL++'s default mutators if desired (though it would be much less effective).

But I actually broke that contract with ShutdownScriptVariant, which panics on invalid version or program length. MineBlocks is now another step in that direction since MineBlocks(0) causes a panic.

Maybe these two things were mistakes and we should go back to reporting errors instead of panicking. But then, again, the ShutdownScriptVariant panic allowed us to detect the AFL++ bug in #82 which would probably have gone unnoticed without the panics.

It should also be noted that if we decide to lean into the panic strategy, we should probably rewrite portions of the IR scenario to start panicking instead of skipping invalid programs. And we may also want to remove Program::validate altogether and just let the program builder panic on bad inputs to mutators.

[ekzyis]

With the custom mutator in place, I don’t think this panic should occur. WDYT?

Ahh, yes, that's why I didn't encounter this when fuzzing with the custom mutator. I didn't consider it couldn't mutate to MineBlocks(0), only that program validation wouldn't catch it.

Perhaps we should just change MineBlocks(u8) to MineBlocks(NonZeroU8), which would make it impossible to postcard-deserialize a MineBlocks(0).

Oh, cool, didn't know about NonZeroU8. I checked this out, and noticed it's quite an invasive change:

patch

diff --git a/smite-ir/src/mutators/operation_param.rs b/smite-ir/src/mutators/operation_param.rs
index 6ce1306..fc0a25a 100644
--- a/smite-ir/src/mutators/operation_param.rs
+++ b/smite-ir/src/mutators/operation_param.rs
@@ -1,5 +1,7 @@
 //! Mutator that tweaks embedded literal values in Load and Extract operations.
 
+use std::num::NonZero;
+
 use rand::seq::IteratorRandom;
 use rand::{Rng, RngExt};
 use smite::bolt::MAX_MESSAGE_SIZE;
@@ -74,7 +76,7 @@ fn mutate_operation(op: &mut Operation, rng: &mut impl Rng) -> bool {
             // MineBlocks(100): 157ms
             // MineBlocks(200): 359ms
             // MineBlocks(255): 468ms
-            *v = rng.random_range(1..=16);
+            *v = NonZero::new(rng.random_range(1..=16)).unwrap();
             true
         }
         Operation::ExtractAcceptChannel(field) => mutate_extract_field(field, rng),
diff --git a/smite-ir/src/operation.rs b/smite-ir/src/operation.rs
index 7e56ed9..0b0531b 100644
--- a/smite-ir/src/operation.rs
+++ b/smite-ir/src/operation.rs
@@ -8,8 +8,8 @@
 //! - `Build` - Construct a BOLT message from input variables.
 //! - `Act` - Perform a side effect against the target.
 
-use std::fmt;
 use std::fmt::Write;
+use std::{fmt, num::NonZeroU8};
 
 use bitcoin::{opcodes::all as opcodes, script::Builder, script::PushBytes};
 use rand::{Rng, RngExt};
@@ -101,7 +101,7 @@ pub enum Operation {
     /// Produces an `AcceptChannel` compound variable.
     RecvAcceptChannel,
     /// Mines the given number of blocks on the Bitcoin network.
-    MineBlocks(u8),
+    MineBlocks(NonZeroU8),
 }
 
 /// A BOLT 2 compliant `upfront_shutdown_script` template.
diff --git a/smite-ir/src/tests.rs b/smite-ir/src/tests.rs
index 9e907b3..b7b345b 100644
--- a/smite-ir/src/tests.rs
+++ b/smite-ir/src/tests.rs
@@ -1,5 +1,7 @@
 //! Tests for IR types.
 
+use std::num::NonZeroU8;
+
 use rand::RngExt;
 use rand::SeedableRng;
 use rand::rngs::SmallRng;
@@ -231,7 +233,7 @@ fn postcard_roundtrip() {
                 inputs: vec![],
             },
             Instruction {
-                operation: Operation::MineBlocks(6),
+                operation: Operation::MineBlocks(NonZeroU8::new(6).unwrap()),
                 inputs: vec![],
             },
         ],
@@ -244,7 +246,7 @@ fn postcard_roundtrip() {
 
 #[test]
 fn mine_blocks_operation() {
-    let op = Operation::MineBlocks(8);
+    let op = Operation::MineBlocks(NonZeroU8::new(8).unwrap());
     assert_eq!(op.input_types(), vec![]);
     assert_eq!(op.output_type(), None);
     assert!(op.is_param_mutable());
@@ -254,7 +256,7 @@ fn mine_blocks_operation() {
 fn displays_mine_blocks_program() {
     let program = Program {
         instructions: vec![Instruction {
-            operation: Operation::MineBlocks(6),
+            operation: Operation::MineBlocks(NonZeroU8::new(6).unwrap()),
             inputs: vec![],
         }],
     };
@@ -267,7 +269,7 @@ fn displays_mine_blocks_program() {
 fn validate_accepts_mine_blocks() {
     let program = Program {
         instructions: vec![Instruction {
-            operation: Operation::MineBlocks(8),
+            operation: Operation::MineBlocks(NonZeroU8::new(8).unwrap()),
             inputs: vec![],
         }],
     };
@@ -283,7 +285,7 @@ fn validate_rejects_mine_blocks_with_wrong_input() {
                 inputs: vec![],
             },
             Instruction {
-                operation: Operation::MineBlocks(6),
+                operation: Operation::MineBlocks(NonZeroU8::new(6).unwrap()),
                 inputs: vec![0],
             },
         ],
@@ -636,7 +638,10 @@ fn generate_fresh_produces_distinct_indices() {
 fn generate_mine_blocks_program() {
     let mut rng = SmallRng::seed_from_u64(0);
     let mut builder = ProgramBuilder::new();
-    let mb = builder.append(Operation::MineBlocks(rng.random_range(1..=16)), &[]);
+    let mb = builder.append(
+        Operation::MineBlocks(NonZeroU8::new(rng.random_range(1..=16)).unwrap()),
+        &[],
+    );
     let program = builder.build();
     program.validate().expect("MineBlocks should validate");
 
@@ -957,7 +962,7 @@ fn param_mutator_changes_values() {
 fn param_mutator_changes_mined_num_blocks() {
     let original = Program {
         instructions: vec![Instruction {
-            operation: Operation::MineBlocks(42),
+            operation: Operation::MineBlocks(NonZeroU8::new(42).unwrap()),
             inputs: vec![],
         }],
     };
@@ -973,7 +978,7 @@ fn param_mutator_changes_mined_num_blocks() {
         let Operation::MineBlocks(num_blocks) = program.instructions[0].operation else {
             panic!("OperationParamMutator changed the operation type");
         };
-        assert!((1..=16).contains(&num_blocks));
+        assert!((1..=16).contains(&num_blocks.get()));
         if program != original {
             diff_count += 1;
         }
diff --git a/smite-scenarios/src/executor.rs b/smite-scenarios/src/executor.rs
index 83f98a6..017f7a2 100644
--- a/smite-scenarios/src/executor.rs
+++ b/smite-scenarios/src/executor.rs
@@ -3,6 +3,8 @@
 //! Executes an IR program against a target node over an established connection,
 //! producing side effects (sending/receiving messages).
 
+use std::num::NonZeroU8;
+
 use bitcoin::secp256k1::{PublicKey, Secp256k1, SecretKey};
 use smite::bitcoin::BitcoinCli;
 use smite::bolt::{
@@ -15,11 +17,11 @@ use smite_ir::{Operation, Program, Variable, VariableType};
 /// Abstraction over bitcoin-cli operations, allowing mock implementations in tests.
 pub trait BitcoinRpc {
     /// Mines the given number of blocks.
-    fn mine_blocks(&mut self, num_blocks: u8);
+    fn mine_blocks(&mut self, num_blocks: NonZeroU8);
 }
 
 impl BitcoinRpc for BitcoinCli {
-    fn mine_blocks(&mut self, num_blocks: u8) {
+    fn mine_blocks(&mut self, num_blocks: NonZeroU8) {
         BitcoinCli::mine_blocks(self, num_blocks);
     }
 }
@@ -443,7 +445,7 @@ fn nonempty_or_none(bytes: &[u8]) -> Option<Vec<u8>> {
 
 #[cfg(test)]
 mod tests {
-    use std::collections::VecDeque;
+    use std::{collections::VecDeque, num::NonZeroU8};
 
     use super::*;
     use bitcoin::secp256k1::{Secp256k1, SecretKey};
@@ -487,11 +489,11 @@ mod tests {
 
     #[derive(Default)]
     struct MockBitcoinCli {
-        mine_blocks_calls: Vec<u8>,
+        mine_blocks_calls: Vec<NonZeroU8>,
     }
 
     impl BitcoinRpc for MockBitcoinCli {
-        fn mine_blocks(&mut self, num_blocks: u8) {
+        fn mine_blocks(&mut self, num_blocks: NonZeroU8) {
             self.mine_blocks_calls.push(num_blocks);
         }
     }
@@ -1048,7 +1050,7 @@ mod tests {
     #[test]
     fn execute_mine_blocks_invokes_cli() {
         let instrs = vec![Instruction {
-            operation: Operation::MineBlocks(6),
+            operation: Operation::MineBlocks(NonZeroU8::new(6).unwrap()),
             inputs: vec![],
         }];
         let program = Program {
@@ -1067,7 +1069,7 @@ mod tests {
         .unwrap();
 
         // Verify that mine_blocks was called with the correct number
-        assert_eq!(mock_cli.mine_blocks_calls, vec![6]);
+        assert_eq!(mock_cli.mine_blocks_calls, vec![NonZeroU8::new(6).unwrap()]);
     }
 
     #[test]
@@ -1078,7 +1080,7 @@ mod tests {
                 inputs: vec![],
             },
             Instruction {
-                operation: Operation::MineBlocks(6),
+                operation: Operation::MineBlocks(NonZeroU8::new(6).unwrap()),
                 inputs: vec![0],
             },
         ];
diff --git a/smite/src/bitcoin.rs b/smite/src/bitcoin.rs
index 83a7848..12c3132 100644
--- a/smite/src/bitcoin.rs
+++ b/smite/src/bitcoin.rs
@@ -2,6 +2,7 @@
 //! `bitcoind` instances via `bitcoin-cli`.
 
 use std::cmp::Ordering;
+use std::num::NonZeroU8;
 use std::path::PathBuf;
 use std::process::Command;
 use std::str::FromStr;
@@ -69,7 +70,7 @@ impl BitcoinCli {
     ///
     /// If the `bitcoin-cli -generate` command fails to execute or returns
     /// a non-success exit status.
-    pub fn mine_blocks(&self, num_blocks: u8) {
+    pub fn mine_blocks(&self, num_blocks: NonZeroU8) {
         let mine_out = self
             .run()
             .arg("-generate")

An alternative would be to gracefully handle 0 ourselves in BitcoinCli::mine_blocks instead of passing it to bitcoin-cli and crashing:

diff --git a/smite/src/bitcoin.rs b/smite/src/bitcoin.rs
index 83a7848..1848a9f 100644
--- a/smite/src/bitcoin.rs
+++ b/smite/src/bitcoin.rs
@@ -70,6 +70,9 @@ impl BitcoinCli {
     /// If the `bitcoin-cli -generate` command fails to execute or returns
     /// a non-success exit status.
     pub fn mine_blocks(&self, num_blocks: u8) {
+        if num_blocks == 0 {
+            return;
+        }
         let mine_out = self
             .run()
             .arg("-generate")

So MineBlocks(0) would be a noop (as it suggests). Could be an easy fix for a non-urgent problem, while we reconsider such cases in general. WDYT?

edit: did this in 62ce1af

It should also be noted that if we decide to lean into the panic strategy, we should probably rewrite portions of the IR scenario to start panicking instead of skipping invalid programs

I thought we're skipping invalid programs to distinguish fuzzer crashes from target crashes. Is that not the case, or could we still do that?

[Chand-ra]

I thought we're skipping invalid programs to distinguish fuzzer crashes from target crashes. Is that not the case, or could we still do that?

Depends on what we mean by "invalid". We should and do skip structurally invalid programs (like message fields not matching the required type), but should not skip all semantically invalid programs (like sending certain messages out of order) as they may reveal hidden edge cases.

[ekzyis]

We should and do skip structurally invalid programs (like message fields not matching the required type)

Yes, that's what I meant with "invalid". We skip these. If MineBlocks(0) would continue to panic, it will show up as a crash:

And I thought we only want target crashes to show up there.