Hi guys,
Internal Storage Bug
There is an issue in the commonly used ic_cdk
storage that can lead to memory corruption if special care is not taken when using storage. ic_cdk
storage internally uses a static mut
binary tree to store all values. Thus it’s possible to obtain two mutable references to the same value. This is unsafe Rust, and I wonder if it’s needed, especially since with heap-allocated types, changing such a value using one reference can invalidate another reference. This is problematic:
-
When accessing the storage from different methods. For example we can load the value from storage, and then call another function which will also load the same value from the storage. In this case, when the execution returns to the first function, the reference to its value can be broken (as demonstrated in the bug report).
-
When
await
ing on async calls in update methods. Awaiting switches execution context, so another call can access and modify the stored value while we await the call.
To prevent unintentionally creating dangling references and possible some memory-related vulnerabilities, we wrote own implementation of the canister state storage. It uses RefCell
to control access to the stored value. This allows us not use static mut
at all for storage, so we don’t rely on unsafe
Rust code, taking full advantage of Rust safety guarantees.
Here is our implementation: ic-helpers/ic-storage at main · infinity-swap/ic-helpers · GitHub
Cheers,
InfinitySwap
PS - an example of code with the vulnerability:
let transactions = ic_cdk::storage::get_mut::<Ledger>();
transactions.add(from, to, amount);
charge_fee(from);
return transactions.len(); // this line can fail with memory corruption
fn charge_fee(from: Principal) {
ic_cdk::storage::get_mut::<Ledger>().add(from, owner(), fee());
}