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.

Sign up for GitHub

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

Jump to bottom

Add opt in reentrancy to soroban #1491

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Add opt in reentrancy to soroban #1491

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
43a3ed8
add opt in reentrancy to soroban
heytdep Nov 6, 2024
efd3537
change supported protocol
heytdep Nov 6, 2024
22f5770
fmt
heytdep Nov 6, 2024
d3bc92b
allow caller contract to specify reentry rules
heytdep Nov 7, 2024
450cbf3
add test where caller does not enable reentry
heytdep Nov 7, 2024
abe39c3
fmt
heytdep Nov 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion rust-toolchain.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
[toolchain]
channel = "stable"
channel = "1.81"
targets = ["wasm32-unknown-unknown"]
components = ["rustc", "cargo", "rustfmt", "clippy", "rust-src"]
55 changes: 55 additions & 0 deletions soroban-env-common/env.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1560,6 +1560,61 @@
],
"return": "Val",
"docs": "Calls a function in another contract with arguments contained in vector `args`, returning either the result of the called function or an `Error` if the called function failed. The returned error is either a custom `ContractError` that the called contract returns explicitly, or an error with type `Context` and code `InvalidAction` in case of any other error in the called contract (such as a host function failure that caused a trap). `try_call` might trap in a few scenarios where the error can't be meaningfully recovered from, such as running out of budget."
},
{
"export": "1",
"name": "call_reentrant",
"args": [
{
"name": "contract",
"type": "AddressObject"
},
{
"name": "func",
"type": "Symbol"
},
{
"name": "args",
"type": "VecObject"
}
],
"return": "Val",
"docs": "Calls a function in another contract with arguments contained in vector `args`. If the call is successful, returns the result of the called function. Traps otherwise. This functions enables re-entrancy in the immediate cross-contract call.",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This functions enables re-entrancy in the immediate cross-contract call.

In an outgoing call graph that looks like this:

flowchart LR
    A --> B --> C --> D
Loading

Does it mean 1️⃣ that reentry into A is only allowed from B? For example:

flowchart LR
    A --> B --> C --> D
    B --> A
Loading

Or 2️⃣ that reentry into A is allowed from any contract that is executing further down the stack than A's call out? For example:

flowchart LR
    A --> B --> C --> D
    B --> A
    C --> A
    D --> A
Loading

I think 2️⃣ fits better with narrative on what it means to call out to another contract. What that other contract does, whether it is a monolith or a micro component is irrelevant to the calling contract. The calling contract merely needs to acknowledge that it is capable to handle being re-entered during the call. By who, is mostly irrelevant.

Are there cases where the who is important?

If I'm completely misunderstanding, please correct me 😄.

cc @dmkozh @sisuresh

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this was also partially discussed on the thread. Currently the implementation is fairly unsafe because when reentrancy is enabled the called contract can be reentrant on any contract down the stack. This means that scenario 2 (which is what I think we should go for) is possible, but it also makes possible a scenario where e.g D is reentrant on B which didn't have reentrancy enabled.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wanted to confirm that I think we should go with 2 (reentry is enabled into A only, but from any downstream contract in the reentry scope). One extension we've discussed in the thread is to also limit the depth of reentry explicitly (i.e. don't specify who can reenter, but have an ability to limit the reentry e.g. to the direct calls), but I'm not sure yet if that's necessary.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 I think depth is unnecessary and somewhat breaks the abstraction and possibly interop. For eg someone puts an arbitrary depth and then some pluggavle contract invocations work while others don't.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW I think for depth the only useful options are 1 (self-reentry), 2 (call a contract that re-enters into me) and 'infinity' (for arbitrary reentrance). Self-reentry is something we do for auth, so it's definitely not useless, but I'm not sure if there are use cases for it in the regular contracts (likely it can be better achieved just by factoring out the function logic into regular functions). Depth 2 may be useful for some well-defined protocols that want to protect themselves from a 'man-in-the-middle' scenarios (e.g. imagine contract A calls contract B and it trusts contract B to reenter contract A, but doesn't trust any contracts C,D,... that B may call). My intuition is that A->B->A reentrance may be useful quite often and tight coupling between A and B is actually intentional for that. Anything above 2 is probably too fine-grained and should just fall into 'infinity' bucket. So if I were to implement depth, I'd probably go with these 3 options. That said, I'm also ok with leaving the depth out completely.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not strong opposed to a depth parameter, but I don't yet grasp where it would move the needle on safety. It looks like it could be used to limit exposure, but on its own doesn't actually make contracts reentrant safe, and may harm interop and contract composability.

I'm saying this mostly because when it comes to reentry, A should not trust any dependency to reenter in an unsafe way. A should be internally defensively organised so that its state, irrespective of when or how it is reentered, cannot be moved into a state that is invalid.

The only exception I can think of is when A and B are components of a single system, sharing the same developer. But the presence of an intentional vulnerability in A that B promises not to exploit still doesn't feel right.

Use cases and concrete examples are needed to work out if depth is needed, or not.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could work through some case studies to determine usefulness.

"min_supported_protocol": 21
},
{
"export": "2",
"name": "try_call_reentrant",
"args": [
{
"name": "contract",
"type": "AddressObject"
},
{
"name": "func",
"type": "Symbol"
},
{
"name": "args",
"type": "VecObject"
}
],
"return": "Val",
"docs": "Calls a function in another contract with arguments contained in vector `args`, returning either the result of the called function or an `Error` if the called function failed. The returned error is either a custom `ContractError` that the called contract returns explicitly, or an error with type `Context` and code `InvalidAction` in case of any other error in the called contract (such as a host function failure that caused a trap). `try_call` might trap in a few scenarios where the error can't be meaningfully recovered from, such as running out of budget. This functions enables re-entrancy in the immediate cross-contract call.",
"min_supported_protocol": 21
},
{
"export": "3",
"name": "set_reentrant",
"args": [
{
"name": "enabled",
"type": "Bool"
}
],
"return": "Void",
"docs": "Enables the current contract to specify the reentrancy rules.",
"min_supported_protocol": 21
Comment on lines +1607 to +1617
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the idea with set_reentrant that it provides an alternative way to enable reentry on all the following regular call / try_call host fns?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Set reentrant just instructs the host that the contract in this context (or better, currently it sets the flag on the whole host). The current implementation requires two specialized host functions to have the callparam option with the reentry enabled (while call and try_call always have it disabled), and set_reentrant is an additional guard to make sure that if it's set to false call_reentrant will fail. So set_reentrant(true) and call in the current implementation would result in a trap anyways. This would change if we inherit the reentry guard from the context i.e what dmytro brought up in the discord.

}
]
},
Expand Down
105 changes: 40 additions & 65 deletions soroban-env-host/src/host.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,9 @@ struct HostImpl {
#[doc(hidden)]
#[cfg(any(test, feature = "recording_mode"))]
need_to_build_module_cache: RefCell<bool>,

// Enables calling modules that link functions that call with reentry.
enable_reentrant: RefCell<bool>,
}

// Host is a newtype on Rc<HostImpl> so we can impl Env for it below.
Expand Down Expand Up @@ -382,6 +385,8 @@ impl Host {
suppress_diagnostic_events: RefCell::new(false),
#[cfg(any(test, feature = "recording_mode"))]
need_to_build_module_cache: RefCell::new(false),

enable_reentrant: RefCell::new(false),
}))
}

Expand Down Expand Up @@ -2369,23 +2374,8 @@ impl VmCallerEnv for Host {
func: Symbol,
args: VecObject,
) -> Result<Val, HostError> {
let argvec = self.call_args_from_obj(args)?;
// this is the recommended path of calling a contract, with `reentry`
// always set `ContractReentryMode::Prohibited`
let res = self.call_n_internal(
&self.contract_id_from_address(contract_address)?,
func,
argvec.as_slice(),
CallParams::default_external_call(),
);
if let Err(e) = &res {
self.error(
e.error,
"contract call failed",
&[func.to_val(), args.to_val()],
);
}
res
let call_params = CallParams::default_external_call();
self.call_with_params(contract_address, func, args, call_params)
}

// Notes on metering: covered by the components.
Expand All @@ -2396,54 +2386,39 @@ impl VmCallerEnv for Host {
func: Symbol,
args: VecObject,
) -> Result<Val, HostError> {
let argvec = self.call_args_from_obj(args)?;
// this is the "loosened" path of calling a contract.
// TODO: A `reentry` flag will be passed from `try_call` into here.
// For now, we are passing in `ContractReentryMode::Prohibited` to disable
// reentry.
let res = self.call_n_internal(
&self.contract_id_from_address(contract_address)?,
func,
argvec.as_slice(),
CallParams::default_external_call(),
);
match res {
Ok(rv) => Ok(rv),
Err(e) => {
self.error(
e.error,
"contract try_call failed",
&[func.to_val(), args.to_val()],
);
// Only allow to gracefully handle the recoverable errors.
// Non-recoverable errors should still cause guest to panic and
// abort execution.
if e.is_recoverable() {
// Pass contract error _codes_ through, while switching
// from Err(ce) to Ok(ce), i.e. recovering.
if e.error.is_type(ScErrorType::Contract) {
Ok(e.error.to_val())
} else {
// Narrow all the remaining host errors down to a single
// error type. We don't want to expose the granular host
// errors to the guest, consistently with how every
// other host function works. This reduces the risk of
// implementation being 'locked' into specific error
// codes due to them being exposed to the guest and
// hashed into blockchain.
// The granular error codes are still observable with
// diagnostic events.
Ok(Error::from_type_and_code(
ScErrorType::Context,
ScErrorCode::InvalidAction,
)
.to_val())
}
} else {
Err(e)
}
}
}
let call_params = CallParams::default_external_call();
self.try_call_with_params(contract_address, func, args, call_params)
}

fn call_reentrant(
&self,
_vmcaller: &mut VmCaller<Host>,
contract_address: AddressObject,
func: Symbol,
args: VecObject,
) -> Result<Val, Self::Error> {
let call_params = CallParams::reentrant_external_call();
self.call_with_params(contract_address, func, args, call_params)
}

fn try_call_reentrant(
&self,
_vmcaller: &mut VmCaller<Host>,
contract_address: AddressObject,
func: Symbol,
args: VecObject,
) -> Result<Val, Self::Error> {
let call_params = CallParams::reentrant_external_call();
self.try_call_with_params(contract_address, func, args, call_params)
}

fn set_reentrant(
&self,
_vmcaller: &mut VmCaller<Host>,
enabled: Bool,
) -> Result<Void, HostError> {
*self.0.enable_reentrant.borrow_mut() = enabled.try_into()?;
Ok(Void::from(()))
}

// endregion: "call" module functions
Expand Down
118 changes: 114 additions & 4 deletions soroban-env-host/src/host/frame.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use soroban_env_common::VecObject;

use crate::{
auth::AuthorizationManagerSnapshot,
budget::AsBudget,
Expand Down Expand Up @@ -111,6 +113,14 @@ impl CallParams {
}
}

pub(crate) fn reentrant_external_call() -> Self {
Self {
reentry_mode: ContractReentryMode::Allowed,
internal_host_call: false,
treat_missing_function_as_noop: false,
}
}

#[allow(unused)]
pub(crate) fn default_internal_call() -> Self {
Self {
Expand Down Expand Up @@ -174,6 +184,10 @@ impl Frame {
}

impl Host {
pub(crate) fn get_reentrancy_flag(&self) -> Result<bool, HostError> {
Ok(*self.0.enable_reentrant.borrow())
}

/// Returns if the host currently has a frame on the stack.
///
/// A frame being on the stack usually indicates that a contract is currently
Expand Down Expand Up @@ -676,7 +690,7 @@ impl Host {
let args_vec = args.to_vec();
match &instance.executable {
ContractExecutable::Wasm(wasm_hash) => {
let vm = self.instantiate_vm(id, wasm_hash)?;
let vm = self.instantiate_vm(id, wasm_hash, true)?;
let relative_objects = Vec::new();
self.with_frame(
Frame::ContractVM {
Expand All @@ -699,7 +713,12 @@ impl Host {
}
}

fn instantiate_vm(&self, id: &Hash, wasm_hash: &Hash) -> Result<Rc<Vm>, HostError> {
fn instantiate_vm(
&self,
id: &Hash,
wasm_hash: &Hash,
reentry_guard: bool,
) -> Result<Rc<Vm>, HostError> {
#[cfg(any(test, feature = "recording_mode"))]
{
if !self.in_storage_recording_mode()? {
Expand Down Expand Up @@ -792,7 +811,14 @@ impl Host {
#[cfg(not(any(test, feature = "recording_mode")))]
let cost_mode = crate::vm::ModuleParseCostMode::Normal;

Vm::new_with_cost_inputs(self, contract_id, code.as_slice(), costs, cost_mode)
Vm::new_with_cost_inputs(
self,
contract_id,
code.as_slice(),
costs,
cost_mode,
reentry_guard,
)
}

pub(crate) fn get_contract_protocol_version(
Expand All @@ -807,13 +833,97 @@ impl Host {
let instance = self.retrieve_contract_instance_from_storage(&storage_key)?;
match &instance.executable {
ContractExecutable::Wasm(wasm_hash) => {
let vm = self.instantiate_vm(contract_id, wasm_hash)?;
let vm = self.instantiate_vm(contract_id, wasm_hash, false)?;
Ok(vm.module.proto_version)
}
ContractExecutable::StellarAsset => self.get_ledger_protocol_version(),
}
}

pub(crate) fn call_with_params(
&self,
contract_address: AddressObject,
func: Symbol,
args: VecObject,
call_params: CallParams,
) -> Result<Val, HostError> {
let argvec = self.call_args_from_obj(args)?;
// this is the recommended path of calling a contract, with `reentry`
// always set `ContractReentryMode::Prohibited` unless the reentrant
// flag is enabled.
let res = self.call_n_internal(
&self.contract_id_from_address(contract_address)?,
func,
argvec.as_slice(),
call_params,
);
if let Err(e) = &res {
self.error(
e.error,
"contract call failed",
&[func.to_val(), args.to_val()],
);
}
res
}

pub(crate) fn try_call_with_params(
&self,
contract_address: AddressObject,
func: Symbol,
args: VecObject,
call_params: CallParams,
) -> Result<Val, HostError> {
let argvec = self.call_args_from_obj(args)?;
// this is the "loosened" path of calling a contract.
// TODO: A `reentry` flag will be passed from `try_call` into here.
// Default behaviour is to pass in `ContractReentryMode::Prohibited` to disable
// reentry, but it is the `call_data` parameter that controls this mode.
let res = self.call_n_internal(
&self.contract_id_from_address(contract_address)?,
func,
argvec.as_slice(),
call_params,
);
match res {
Ok(rv) => Ok(rv),
Err(e) => {
self.error(
e.error,
"contract try_call failed",
&[func.to_val(), args.to_val()],
);
// Only allow to gracefully handle the recoverable errors.
// Non-recoverable errors should still cause guest to panic and
// abort execution.
if e.is_recoverable() {
// Pass contract error _codes_ through, while switching
// from Err(ce) to Ok(ce), i.e. recovering.
if e.error.is_type(ScErrorType::Contract) {
Ok(e.error.to_val())
} else {
// Narrow all the remaining host errors down to a single
// error type. We don't want to expose the granular host
// errors to the guest, consistently with how every
// other host function works. This reduces the risk of
// implementation being 'locked' into specific error
// codes due to them being exposed to the guest and
// hashed into blockchain.
// The granular error codes are still observable with
// diagnostic events.
Ok(Error::from_type_and_code(
ScErrorType::Context,
ScErrorCode::InvalidAction,
)
.to_val())
}
} else {
Err(e)
}
}
}
}

// Notes on metering: this is covered by the called components.
pub(crate) fn call_n_internal(
&self,
Expand Down
Loading
Loading