search

ickb / v1-core

iCKB scripts and utilities for interacting with them
MIT License
2 stars 0 forks source link

Handling of LengthNotEnough Error #14

Open jlguochn opened 1 week ago

jlguochn commented 1 week ago

In several functions, such as: https://github.com/ickb/v1-core/blob/92be3c3ea31226dece0f7235b12cf5b96766a8a3/scripts/contracts/utils/src/utils.rs#L41 https://github.com/ickb/v1-core/blob/92be3c3ea31226dece0f7235b12cf5b96766a8a3/scripts/contracts/utils/src/utils.rs#L57 https://github.com/ickb/v1-core/blob/92be3c3ea31226dece0f7235b12cf5b96766a8a3/scripts/contracts/ickb_logic/src/utils.rs#L21

It seems that the LengthNotEnough error is not being handled in a specific way. While this approach might be valid in certain contexts, it could potentially allow incomplete data to be treated as valid without additional checks.

Perhaps it would be worth considering a more explicit handling of the LengthNotEnough error to ensure that the data is fully valid before proceeding. Alternatively, logging such occurrences could help in diagnosing potential issues.

phroi commented 1 week ago

Hey @jlguochn, thank you for publicly expressing your interest in iCKB by auditing the proposal and L1 scripts source code as part of the Scalebit external audit, I personally appreciate a lot!! 🙏

Background

In the beginning I was using the high-level calls (which internally handles this error) until @XuJiandong raised an issue on the high level calls usage, that made me switch to the low-level syscalls. At that point I raised this issue on the LengthNotEnough(N) lacks of documentation in ckb-std.

Back to us, as per Partial Loading section in VM Syscalls RFC: LengthNotEnough(N) literally means data in the field was longer than the provided buffer. When this error occurs, the following happens:

Usage of low-level syscall in the iCKB Scripts

Let's take a step back and examine all the low-level syscalls in the current v1-core version:

user@host:~/ickb/v1-core/scripts/contracts$ grep --recursive --line-number -i 'syscalls' .
./limit_order/src/entry.rs:12:    syscalls::load_cell_data,
./ickb_logic/src/utils.rs:5:    syscalls::{load_cell_data, SysError},
./ickb_logic/src/celltype.rs:11:    syscalls::SysError,
./owned_owner/src/entry.rs:7:    syscalls::{load_cell_data, SysError},
./utils/src/utils.rs:9:    syscalls::{load_cell_data, load_header, load_input_by_field},
./utils/src/dao.rs:4:    syscalls::{load_cell_data, SysError},

Any of the following calls could potentially raise a LengthNotEnough:

So let's examine all their uses in the current v1-core version:

user@host:~/ickb/v1-core/scripts/contracts$ grep --recursive --line-number -E 'load_cell_data\(|load_header\(|load_input_by_field\(' .
./limit_order/src/entry.rs:166:    if load_cell_data(&mut data, 0, index, source)? != data.len() {
./ickb_logic/src/utils.rs:20:    let mut raw_data = match load_cell_data(&mut data, 0, index, source) {
./owned_owner/src/entry.rs:80:    let d = match load_cell_data(&mut data, 0, index, source) {
./utils/src/utils.rs:40:    match load_cell_data(&mut data, 0, index, source) {
./utils/src/utils.rs:56:    match load_header(&mut data, AR_OFFSET, index, source) {
./utils/src/utils.rs:76:    match load_input_by_field(&mut d, 0, index, source, InputField::OutPoint) {
./utils/src/dao.rs:19:    match load_cell_data(&mut data, 0, index, source) {
./utils/src/dao.rs:28:    match load_cell_data(&mut data, 0, index, source) {

Let's group the use-cases by expected data length.

Data must be at least of the expected length

./ickb_logic/src/utils.rs:20

let mut data = [0u8; RECEIPT_SIZE];
let mut raw_data = match load_cell_data(&mut data, 0, index, source) {
    Ok(RECEIPT_SIZE) | Err(SysError::LengthNotEnough(_)) => data.as_slice(),
    Ok(_) => return Err(Error::Encoding),
    Err(err) => return Err(Error::from(err)),
};

Receipt data must be at least RECEIPT_SIZE, if it's RECEIPT_SIZE or more (possibly the lock is memorizing data), then we return exactly the data. If it is less we raise an Encoding error. If any other error arises we return it.

./owned_owner/src/entry.rs:81

let mut data = [0u8; OWNED_DISTANCE_SIZE];
let d = match load_cell_data(&mut data, 0, index, source) {
    Ok(OWNED_DISTANCE_SIZE) | Err(SysError::LengthNotEnough(_)) => i32::from_le_bytes(data),
    Ok(_) => return Err(Error::Encoding),
    Err(err) => return Err(Error::from(err)),
};

Owner data must be at least OWNED_DISTANCE_SIZE, if it's OWNED_DISTANCE_SIZE or more (possibly the lock is memorizing data), then we return exactly the data. If it is less we raise an Encoding error. If any other error arises we return it.

./utils/src/utils.rs:40:

let mut data = [0u8; UDT_SIZE];
match load_cell_data(&mut data, 0, index, source) {
    Ok(UDT_SIZE) | Err(SysError::LengthNotEnough(_)) => Ok(u128::from_le_bytes(data)),
    Ok(_) => Err(SysError::Encoding),
    Err(err) => Err(err),
}

UDT data must be at least UDT_SIZE, if it's UDT_SIZE or more (possibly the lock is memorizing data), then we return exactly the data. If it is less we raise an Encoding error. If any other error arises we return it.

./utils/src/utils.rs:56

let mut data = [0u8; AR_SIZE];
match load_header(&mut data, AR_OFFSET, index, source) {
    Ok(AR_SIZE) | Err(SysError::LengthNotEnough(_)) => Ok(u64::from_le_bytes(data)),
    Ok(_) => Err(SysError::Encoding),
    Err(err) => Err(err),
}

AR data must be at least AR_SIZE, if it's AR_SIZE or likely more, then we return exactly the data. If it is less we raise an Encoding error. If any other error arises we return it.

Data must be of the exact expected length

./limit_order/src/entry.rs:166

let mut data = [0u8; UDT_SIZE + ORDER_SIZE];

if load_cell_data(&mut data, 0, index, source)? != data.len() {
    return Err(Error::Encoding);
}

The data length must be exactly of UDT_SIZE + ORDER_SIZE. If cell data contains more or less data an error must be raised, respectively LengthNotEnough (implicit) and Encoding.

./utils/src/utils.rs:76

let mut d = [0u8; OUT_POINT_SIZE];
match load_input_by_field(&mut d, 0, index, source, InputField::OutPoint) {
    Ok(OUT_POINT_SIZE) => Ok(MetaPoint {
        tx_hash: Some(d[..TX_HASH_SIZE].try_into().unwrap()),
        index: i64::from(u32::from_le_bytes(d[TX_HASH_SIZE..].try_into().unwrap())),
    }),
    Ok(_) => Err(SysError::Encoding),
    Err(err) => Err(err),
}

The data length must be exactly of OUT_POINT_SIZE. If the returned data contains more or less data an error must be raised, respectively LengthNotEnough (implicit) and Encoding. The previous implementation assumed that the VM already checked that the outpoint data is valid, now it's explicitly checked, even if not needed.

./utils/src/dao.rs:19

pub fn is_deposit_data(index: usize, source: Source) -> bool {
    let mut data = DAO_DEPOSIT_DATA;
    match load_cell_data(&mut data, 0, index, source) {
        Ok(DAO_DEPOSIT_DATA_SIZE) => data == DAO_DEPOSIT_DATA,
        _ => false,
    }
}

Already discussed in: https://github.com/ickb/v1-core/issues/9

The is_deposit_data functionality is the following:

As per NervosDAO RFC, the deposit data:

MUST have [exactly] 8 bytes length cell data, filled with all zeros.

Given this reasoning, by all means if the cell data is not exactly of 8 bytes filled with all zeros, then is not a deposit and this function must return false. Say such a cell exists anyway Nervos DAO will not validate the transaction, due to this cell.

./utils/src/dao.rs:28:

pub fn is_withdrawal_request_data(index: usize, source: Source) -> bool {
    let mut data = DAO_DEPOSIT_DATA;
    match load_cell_data(&mut data, 0, index, source) {
        Ok(DAO_DEPOSIT_DATA_SIZE) => data != DAO_DEPOSIT_DATA,
        _ => false,
    }
}

Similarly to before, the is_withdrawal_request_data functionality is the following:

As per NervosDAO RFC, the withdrawal request data:

The withdrawing cell MUST also have 8 bytes length cell data, but instead of 8 zero, it MUST store the block number of the deposit cell's including block. The number MUST be packed in 64-bit unsigned little-endian integer format.

Given this reasoning, by all means if the cell data is not exactly of 8 bytes (not all zeroed), then is not a withdrawal request and this function must return false. Say such a cell exists anyway Nervos DAO will not validate the transaction, due to this cell.

Discussion

While this approach might be valid in certain contexts, it could potentially allow incomplete data to be treated as valid without additional checks.

@jlguochn could you point out where and how exactly?

jlguochn commented 1 week ago

Thank you for your detailed explanation regarding the handling of the LengthNotEnough error and background. Your breakdown is very detailed and clear.

The reason I initially raised this issue is that I am not certain whether all instances that lead to a LengthNotEnough error necessarily indicate that the data meets expectations. To me, LengthNotEnough seems more like a potential error rather than an expected condition. In my view, this behavior essentially treats the truncated data as valid without any further checks, but I am unsure whether all truncated data necessarily meets expectations, which might introduce potential risks or deviate from best practices. Based on your detailed description, I haven't found direct exploitation vectors or security vulnerabilities.

phroi commented 1 week ago

this behavior essentially treats the truncated data as valid without any further checks

Well, that's the thing, we don't know which other data will be there: anyone can develop a lock on top of our script that needs to store additional data for whatever reason. There is no way to check this additional data validity, so we need to restrict our checks to the data owned by our Script.

Underlying idea: we need to validate just the right amount of data, not too little (as it could result in vulnerabilities), not too much (as it could result in future compatibility issues with other scripts).