Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tariscript OP_COMPARE_VERIFY can crash with overflow #5813

Closed
SWvheerden opened this issue Sep 26, 2023 · 1 comment · Fixed by #5872
Closed

tariscript OP_COMPARE_VERIFY can crash with overflow #5813

SWvheerden opened this issue Sep 26, 2023 · 1 comment · Fixed by #5872
Assignees
Labels
release-blocker Something that needs to be fixed before a release can be made

Comments

@SWvheerden
Copy link
Collaborator

An attacker can craft a transaction with a malicious script that crashes any node
which attempts to execute it.
The crash is triggered by an underflow in the OP_COMPARE_VERIFY opcode:

fn handle_compare_height(stack: &mut ExecutionStack, block_height:
u64) -> Result<(), ScriptError> {
let target_height = stack.pop_into_number::<i64>()?;
let block_height = i64::try_from(block_height)?;
let item = StackItem::Number(block_height - target_height);
stack.push(item)
}

An attacker creates a script which puts into the stack the i64::MIN. Any
subtraction will then cause the underflow.
let script = TariScript::new(vec![PushInt(i64::MIN), CompareHeight]);

@SWvheerden SWvheerden added the release-blocker Something that needs to be fixed before a release can be made label Sep 26, 2023
@brianp brianp self-assigned this Oct 10, 2023
@brianp
Copy link
Contributor

brianp commented Oct 25, 2023

Just a note about the description of this task. The op codes in question are actually CompareHeight and CheckHeight.

CompareHeightVerify, and CheckHeightVerify are fine.

SWvheerden pushed a commit that referenced this issue Nov 7, 2023
)

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-blocker Something that needs to be fixed before a release can be made
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants