From aa2ae1066818c1776bd268932fbd3be09f21bf0e Mon Sep 17 00:00:00 2001 From: Brian Pearce Date: Tue, 7 Nov 2023 09:12:25 +0100 Subject: [PATCH] fix(tariscript): protect compare and check height from underflows (#5872) Description --- This PR corrects the operand position of the `handle_compare_height` function. This was identified as incorrect based on the op code description. Additionally, protect it from underflows, and write test cases against all possible returns and errors. I added underflow protection in the `handle_check_height` function as well but after some thought this function wouldn't currently be possible to underflow. The values passed into the function would never be lower than 0, and then converted to i64 which would handle subtraction from 0 fine. In opposition, the `handle_compare_height` uses a value from the stack that could be i64::MIN and then have i64::MAX subtracted from it. Which was the originally identified problem. This is all to say the functions aren't quite equal so it felt worth a comment for future readers. Motivation and Context --- Closes #5813 How Has This Been Tested? --- Added new tests. What process can a PR reviewer use to test or verify this change? --- Double check the [docs](https://github.com/tari-project/tari/blob/development/infrastructure/tari_script/src/op_codes.rs#L146) to validate the operand switch is a valid change. See the new underflow protection. Breaking Changes --- - [x] None - [ ] Requires data directory on base node to be deleted - [ ] Requires hard fork - [ ] Other - Please specify --- infrastructure/tari_script/src/error.rs | 2 + infrastructure/tari_script/src/op_codes.rs | 2 +- infrastructure/tari_script/src/script.rs | 133 ++++++++++++++++++++- 3 files changed, 134 insertions(+), 3 deletions(-) diff --git a/infrastructure/tari_script/src/error.rs b/infrastructure/tari_script/src/error.rs index 059c979d41..dd82be7783 100644 --- a/infrastructure/tari_script/src/error.rs +++ b/infrastructure/tari_script/src/error.rs @@ -49,6 +49,8 @@ pub enum ScriptError { VerifyFailed, #[error("as_hash requires a Digest function that returns at least 32 bytes")] InvalidDigest, + #[error("A compare opcode failed, aborting the script immediately with reason: `{0}`")] + CompareFailed(String), } impl From for ScriptError { diff --git a/infrastructure/tari_script/src/op_codes.rs b/infrastructure/tari_script/src/op_codes.rs index 895bea560a..e8454bd0bf 100644 --- a/infrastructure/tari_script/src/op_codes.rs +++ b/infrastructure/tari_script/src/op_codes.rs @@ -143,7 +143,7 @@ pub enum Opcode { /// if there is not a valid integer value on top of the stack. Fails with `StackUnderflow` if the stack is empty. /// Fails with `VerifyFailed` if the block height < `height`. CompareHeightVerify, - /// Pops the top of the stack as `height`, then pushes the value of (`height` - the current height) to the stack. + /// Pops the top of the stack as `height`, then pushes the value of (the current height - `height`) to the stack. /// In other words, this opcode replaces the top of the stack with the difference between `height` and the /// current height. Fails with `InvalidInput` if there is not a valid integer value on top of the stack. Fails /// with `StackUnderflow` if the stack is empty. diff --git a/infrastructure/tari_script/src/script.rs b/infrastructure/tari_script/src/script.rs index a8ea847429..7b80f7225b 100644 --- a/infrastructure/tari_script/src/script.rs +++ b/infrastructure/tari_script/src/script.rs @@ -340,7 +340,18 @@ impl TariScript { fn handle_check_height(stack: &mut ExecutionStack, height: u64, block_height: u64) -> Result<(), ScriptError> { let height = i64::try_from(height)?; let block_height = i64::try_from(block_height)?; - let item = StackItem::Number(block_height - height); + + // Due to the conversion of u64 into i64 which would fail above if they overflowed, these + // numbers should never enter a state where a `sub` could fail. As they'd both be within range and 0 or above. + // This differs from compare_height due to a stack number being used, which can be lower than 0 + let item = match block_height.checked_sub(height) { + Some(num) => StackItem::Number(num), + None => { + return Err(ScriptError::CompareFailed( + "Subtraction of given height from current block height failed".to_string(), + )) + }, + }; stack.push(item) } @@ -359,7 +370,16 @@ impl TariScript { let target_height = stack.pop_into_number::()?; let block_height = i64::try_from(block_height)?; - let item = StackItem::Number(block_height - target_height); + // Here it is possible to underflow because the stack can take lower numbers where check + // height does not use a stack number and it's minimum can't be lower than 0. + let item = match block_height.checked_sub(target_height) { + Some(num) => StackItem::Number(num), + None => { + return Err(ScriptError::CompareFailed( + "Couldn't subtract the target height from the current block height".to_string(), + )) + }, + }; stack.push(item) } @@ -1727,4 +1747,113 @@ mod test { let buf = &mut buf.as_slice(); assert!(TariScript::deserialize(buf).is_err()); } + + #[test] + fn test_compare_height_block_height_exceeds_bounds() { + let script = script!(CompareHeight); + + let inputs = inputs!(0); + let ctx = context_with_height(u64::MAX); + let stack_item = script.execute_with_context(&inputs, &ctx); + assert!(matches!(stack_item, Err(ScriptError::ValueExceedsBounds))); + } + + #[test] + fn test_compare_height_underflows() { + let script = script!(CompareHeight); + + let inputs = ExecutionStack::new(vec![Number(i64::MIN)]); + let ctx = context_with_height(i64::MAX as u64); + let stack_item = script.execute_with_context(&inputs, &ctx); + assert!(matches!(stack_item, Err(ScriptError::CompareFailed(_)))); + } + + #[test] + fn test_compare_height_underflows_on_empty_stack() { + let script = script!(CompareHeight); + + let inputs = ExecutionStack::new(vec![]); + let ctx = context_with_height(i64::MAX as u64); + let stack_item = script.execute_with_context(&inputs, &ctx); + assert!(matches!(stack_item, Err(ScriptError::StackUnderflow))); + } + + #[test] + fn test_compare_height_valid_with_uint_result() { + let script = script!(CompareHeight); + + let inputs = inputs!(100); + let ctx = context_with_height(24_u64); + let stack_item = script.execute_with_context(&inputs, &ctx); + assert!(stack_item.is_ok()); + assert_eq!(stack_item.unwrap(), Number(-76)) + } + + #[test] + fn test_compare_height_valid_with_int_result() { + let script = script!(CompareHeight); + + let inputs = inputs!(100); + let ctx = context_with_height(110_u64); + let stack_item = script.execute_with_context(&inputs, &ctx); + assert!(stack_item.is_ok()); + assert_eq!(stack_item.unwrap(), Number(10)) + } + + #[test] + fn test_check_height_block_height_exceeds_bounds() { + let script = script!(CheckHeight(0)); + + let inputs = ExecutionStack::new(vec![]); + let ctx = context_with_height(u64::MAX); + let stack_item = script.execute_with_context(&inputs, &ctx); + assert!(matches!(stack_item, Err(ScriptError::ValueExceedsBounds))); + } + + #[test] + fn test_check_height_exceeds_bounds() { + let script = script!(CheckHeight(u64::MAX)); + + let inputs = ExecutionStack::new(vec![]); + let ctx = context_with_height(10_u64); + let stack_item = script.execute_with_context(&inputs, &ctx); + assert!(matches!(stack_item, Err(ScriptError::ValueExceedsBounds))); + } + + #[test] + fn test_check_height_overflows_on_max_stack() { + let script = script!(CheckHeight(0)); + + let mut inputs = ExecutionStack::new(vec![]); + + for i in 0..255 { + inputs.push(Number(i)).unwrap(); + } + + let ctx = context_with_height(i64::MAX as u64); + let stack_item = script.execute_with_context(&inputs, &ctx); + assert!(matches!(stack_item, Err(ScriptError::StackOverflow))); + } + + #[test] + fn test_check_height_valid_with_uint_result() { + let script = script!(CheckHeight(24)); + + let inputs = ExecutionStack::new(vec![]); + let ctx = context_with_height(100_u64); + let stack_item = script.execute_with_context(&inputs, &ctx); + assert!(stack_item.is_ok()); + assert_eq!(stack_item.unwrap(), Number(76)) + } + + #[test] + fn test_check_height_valid_with_int_result() { + let script = script!(CheckHeight(100)); + + let inputs = ExecutionStack::new(vec![]); + let ctx = context_with_height(24_u64); + let stack_item = script.execute_with_context(&inputs, &ctx); + assert!(stack_item.is_ok()); + assert_eq!(stack_item.unwrap(), Number(-76)) + } }