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

TOB-FUEL-18: Crash when executing CCP (Copy Code) instruction #523

Closed
Dentosal opened this issue Jul 25, 2023 · 0 comments · Fixed by #524
Closed

TOB-FUEL-18: Crash when executing CCP (Copy Code) instruction #523

Dentosal opened this issue Jul 25, 2023 · 0 comments · Fixed by #524
Assignees
Labels
audit-report Issue from the audit report bug Something isn't working fuel-vm Related to the `fuel-vm` crate. mainnet testing

Comments

@Dentosal
Copy link
Member

Dentosal commented Jul 25, 2023

Description

When executing the CCP (Copy Code) instruction with a code offset larger than the code being copied, then the interpreter panics. The following unit test will cause a panic.

Figure 18.1: Unit test which panics while executing the CCP instruction.

let rng = &mut StdRng::seed_from_u64(2322u64);
let gas_limit = 10_000_000;
let asset_id: AssetId = rng.gen();
let call_amount = 10u64;
let mut test_context = TestBuilder::new(2322u64);
let subcontract = vec![op::ret(RegId::BAL)];
let contract_id = test_context.setup_contract(subcontract.clone(), None, None)
    .contract_id;
let (script_ops, offset) = script_with_data_offset!(
    data_offset,
vec![
        op::ret(RegId::ONE),
    ],
    test_context.tx_offset()
);
let script_data: Vec<u8> = [ asset_id.as_ref(), Call::new(contract_id, 0, offset as
Word).to_bytes().as_slice()].into_iter().flatten().copied().collect();
let transfer_tx = test_context.start_script(script_ops, script_data)
  .gas_limit(gas_limit).gas_price(0).coin_input(asset_id, 1000)
  .contract_input(contract_id).contract_output(&contract_id).change_output(asset_id)
  .execute();

The reason for this panic is that the code_copy function does not verify that the parameter c is in bounds of the contract.

Figure 18.2: Code with bug because array bounds are not checked. (fuel-vm/fuel-vm/src/interpreter/blockchain.rs#625–653)

let contract = CheckedMemConstLen::<{ ContractId::LEN }>::new(b)?;
let cd = checked_add_word(c, d)?;
// ...
let (a, c, d) = (a as usize, c as usize, d as usize);
let cd = cd as usize;
// ...
if contract.as_ref().len() < d {
    try_zeroize(a, d, self.owner, self.memory)?;
} else {
    try_mem_write(a, &contract.as_ref()[c..cd], self.owner, self.memory)?;
}
// ...

This issue has been discovered by manual code review, but can also automatically be discovered using the fuzzer from the fuzzing appendix (see appendix E)

Exploit Scenario

An attacker deploys a contract that includes a malicious CCP instruction. The interpreter will panic, and the whole node will crash. Because the same code is executed on multiple nodes maybe the whole network comes to a halt.

Recommendations

Short term, check that the range c..cd is in the bounds of the contract.
Long term, consider to automatically restart fuel-core when it crashed and report an error to the Fuel team. This would increase the reliability of fuel-core nodes. There should be a delay between restarts and also a maximum amount of restarts.
While this could also be solved on a Rust level by making sure to use the Rust unwinding behavior and catching panics at a higher level, a solution which does not depend on specific Rust compiler flags seems more robust.
Also deploy the fuzzer from the fuzzing appendix (see appendix E), which is able to discover this bug automatically.

@Dentosal Dentosal added bug Something isn't working testing fuel-vm Related to the `fuel-vm` crate. labels Jul 25, 2023
@Dentosal Dentosal self-assigned this Jul 25, 2023
@xgreenx xgreenx added the audit-report Issue from the audit report label Aug 28, 2023
@xgreenx xgreenx changed the title Crash in CCP instruction TOB-FUEL-18: Crash when executing CCP (Copy Code) instruction Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
audit-report Issue from the audit report bug Something isn't working fuel-vm Related to the `fuel-vm` crate. mainnet testing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants