From 816314bfb3da13e05ac826710a281369c41db55c Mon Sep 17 00:00:00 2001 From: steelgeek091 Date: Mon, 8 Apr 2024 13:07:28 +0800 Subject: [PATCH] security fixing for multiple codes --- .../rooch_framework/crypto/decoding.rs | 10 ++- .../rooch_framework/crypto/ecdsa_k1.rs | 10 ++- .../rooch-rpc-server/src/server/btc_server.rs | 14 +++- .../src/server/rooch_server.rs | 75 +++++++++++++++---- .../src/natives/moveos_stdlib/wasm.rs | 59 ++++++++++++--- moveos/moveos-types/src/moveos_std/tx_meta.rs | 32 +++++--- moveos/moveos-verifier/src/metadata.rs | 1 - moveos/moveos/src/vm/moveos_vm.rs | 9 ++- moveos/moveos/src/vm/tx_argument_resolver.rs | 6 +- 9 files changed, 168 insertions(+), 48 deletions(-) diff --git a/crates/rooch-framework/src/natives/rooch_framework/crypto/decoding.rs b/crates/rooch-framework/src/natives/rooch_framework/crypto/decoding.rs index f16e620c12..5e832ba0e6 100644 --- a/crates/rooch-framework/src/natives/rooch_framework/crypto/decoding.rs +++ b/crates/rooch-framework/src/natives/rooch_framework/crypto/decoding.rs @@ -108,9 +108,15 @@ pub fn native_bech32( gas_params.base + (gas_params.per_byte * NumBytes::new(data.as_bytes_ref().len() as u64)); let binding = data.as_bytes_ref(); - let data_str = std::str::from_utf8(&binding).unwrap(); + let data_str = match std::str::from_utf8(&binding) { + Ok(v) => v, + Err(_) => return Ok(NativeResult::err(cost, E_DECODE_FAILED)), + }; - let (hrp, public_key, variant) = bech32::decode(data_str).unwrap(); + let (hrp, public_key, variant) = match bech32::decode(data_str) { + Ok((v1, v2, v3)) => (v1, v2, v3), + Err(_) => return Ok(NativeResult::err(cost, E_DECODE_FAILED)), + }; if hrp != "bech32" && hrp != "bech32m" { return Ok(NativeResult::err(cost, E_DECODE_FAILED)); diff --git a/crates/rooch-framework/src/natives/rooch_framework/crypto/ecdsa_k1.rs b/crates/rooch-framework/src/natives/rooch_framework/crypto/ecdsa_k1.rs index 19e3c3d34d..5bf6c1bd53 100644 --- a/crates/rooch-framework/src/natives/rooch_framework/crypto/ecdsa_k1.rs +++ b/crates/rooch-framework/src/natives/rooch_framework/crypto/ecdsa_k1.rs @@ -43,9 +43,11 @@ pub fn native_ecrecover( let signature = pop_arg!(args, VectorRef); let msg_ref = msg.as_bytes_ref(); let signature_ref = signature.as_bytes_ref(); + let msg_size = msg_ref.len(); + let signature_len = signature_ref.len(); - // TODO(Gas): Charge the arg size dependent costs - let cost = gas_params.base; + let cost = + gas_params.base + gas_params.per_byte * NumBytes::new((msg_size + signature_len) as u64); let Ok(sig) = ::from_bytes(&signature_ref) else { return Ok(NativeResult::err(cost, E_INVALID_SIGNATURE)); @@ -78,8 +80,8 @@ pub fn native_decompress_pubkey( let pubkey = pop_arg!(args, VectorRef); let pubkey_ref = pubkey.as_bytes_ref(); - // TODO(Gas): Charge the arg size dependent costs - let cost = gas_params.base; + let pubkey_ref_len = pubkey_ref.len(); + let cost = gas_params.base + gas_params.per_byte * NumBytes::new(pubkey_ref_len as u64); match Secp256k1PublicKey::from_bytes(&pubkey_ref) { Ok(pubkey) => { diff --git a/crates/rooch-rpc-server/src/server/btc_server.rs b/crates/rooch-rpc-server/src/server/btc_server.rs index 5f5a27918e..0357700c72 100644 --- a/crates/rooch-rpc-server/src/server/btc_server.rs +++ b/crates/rooch-rpc-server/src/server/btc_server.rs @@ -66,9 +66,14 @@ impl BtcAPIServer for BtcServer { let global_state_filter = UTXOFilterView::into_global_state_filter(filter, resolve_address)?; + let limit_value = limit_of + .checked_add(1) + .ok_or(jsonrpsee::core::Error::Custom( + "limit value overflow".to_string(), + ))?; let states = self .rpc_service - .query_object_states(global_state_filter, cursor, limit_of + 1, descending_order) + .query_object_states(global_state_filter, cursor, limit_value, descending_order) .await?; let mut data = self @@ -121,9 +126,14 @@ impl BtcAPIServer for BtcServer { let global_state_filter = InscriptionFilterView::into_global_state_filter(filter, resolve_address)?; + let limit_value = limit_of + .checked_add(1) + .ok_or(jsonrpsee::core::Error::Custom( + "limit value overflow".to_string(), + ))?; let states = self .rpc_service - .query_object_states(global_state_filter, cursor, limit_of + 1, descending_order) + .query_object_states(global_state_filter, cursor, limit_value, descending_order) .await?; let mut data = self diff --git a/crates/rooch-rpc-server/src/server/rooch_server.rs b/crates/rooch-rpc-server/src/server/rooch_server.rs index d974f3620a..8a7554635c 100644 --- a/crates/rooch-rpc-server/src/server/rooch_server.rs +++ b/crates/rooch-rpc-server/src/server/rooch_server.rs @@ -231,11 +231,16 @@ impl RoochAPIServer for RoochServer { Some(key_state_str) => Some(KeyState::from_str(key_state_str.as_str())?), None => None, }; + let limit_value = limit_of + .checked_add(1) + .ok_or(jsonrpsee::core::Error::Custom( + "limit value overflow".to_string(), + ))?; let mut data: Vec = if state_option.decode || show_display { let is_object = access_path.0.is_object(); let (key_states, states): (Vec, Vec) = self .rpc_service - .list_annotated_states(access_path.into(), cursor_of, limit_of + 1) + .list_annotated_states(access_path.into(), cursor_of, limit_value) .await? .into_iter() .unzip(); @@ -266,7 +271,7 @@ impl RoochAPIServer for RoochServer { } } else { self.rpc_service - .list_states(access_path.into(), cursor_of, limit_of + 1) + .list_states(access_path.into(), cursor_of, limit_value) .await? .into_iter() .map(|(key_state, state)| { @@ -303,7 +308,12 @@ impl RoochAPIServer for RoochServer { // NOTE: fetch one more object to check if there is next page let limit_of = min(limit.unwrap_or(DEFAULT_RESULT_LIMIT), MAX_RESULT_LIMIT); - let limit = limit_of + 1; + let limit_value = limit_of + .checked_add(1) + .ok_or(jsonrpsee::core::Error::Custom( + "limit value overflow".to_string(), + ))?; + let limit = limit_value; let mut data = if event_options.decode { self.rpc_service .get_annotated_events_by_event_handle( @@ -379,15 +389,27 @@ impl RoochAPIServer for RoochServer { let tx_orders = if descending_order { let start = cursor.unwrap_or(last_sequencer_order + 1); - let end = if start >= limit_of { - start - limit_of - } else { - 0 - }; + let start_sub = start + .checked_sub(limit_of) + .ok_or(jsonrpsee::core::Error::Custom( + "cursor value overflow".to_string(), + ))?; + let end = if start >= limit_of { start_sub } else { 0 }; (end..start).rev().collect::>() } else { let start = cursor.unwrap_or(0); - let end = min(start + (limit_of + 1), last_sequencer_order + 1); + let limit_value = limit_of + .checked_add(1) + .ok_or(jsonrpsee::core::Error::Custom( + "limit value overflow".to_string(), + ))?; + let start_plus = + start + .checked_add(limit_value) + .ok_or(jsonrpsee::core::Error::Custom( + "cursor value overflow".to_string(), + ))?; + let end = min(start_plus, last_sequencer_order + 1); (start..end).collect::>() }; @@ -453,9 +475,14 @@ impl RoochAPIServer for RoochServer { None => None, }; + let limit_value = limit_of + .checked_add(1) + .ok_or(jsonrpsee::core::Error::Custom( + "limit value overflow".to_string(), + ))?; let mut data = self .aggregate_service - .get_balances(account_addr.into(), cursor_of, limit_of + 1) + .get_balances(account_addr.into(), cursor_of, limit_value) .await?; let has_next_page = data.len() > limit_of; @@ -491,9 +518,14 @@ impl RoochAPIServer for RoochServer { let cursor = cursor.map(|v| v.0); let descending_order = descending_order.unwrap_or(true); + let limit_value = limit_of + .checked_add(1) + .ok_or(jsonrpsee::core::Error::Custom( + "limit value overflow".to_string(), + ))?; let mut data = self .rpc_service - .query_transactions(filter.into(), cursor, limit_of + 1, descending_order) + .query_transactions(filter.into(), cursor, limit_value, descending_order) .await?; let has_next_page = data.len() > limit_of; @@ -527,9 +559,14 @@ impl RoochAPIServer for RoochServer { ); let descending_order = descending_order.unwrap_or(true); + let limit_value = limit_of + .checked_add(1) + .ok_or(jsonrpsee::core::Error::Custom( + "limit value overflow".to_string(), + ))?; let mut data = self .rpc_service - .query_events(filter.into(), cursor, limit_of + 1, descending_order) + .query_events(filter.into(), cursor, limit_value, descending_order) .await? .into_iter() .map(IndexerEventView::from) @@ -581,9 +618,14 @@ impl RoochAPIServer for RoochServer { }; let global_state_filter = ObjectStateFilterView::into_object_state_filter(filter, resolve_address); + let limit_value = limit_of + .checked_add(1) + .ok_or(jsonrpsee::core::Error::Custom( + "limit value overflow".to_string(), + ))?; let mut data = self .rpc_service - .query_object_states(global_state_filter, cursor, limit_of + 1, descending_order) + .query_object_states(global_state_filter, cursor, limit_value, descending_order) .await? .into_iter() .map(IndexerObjectStateView::try_new_from_global_state) @@ -616,9 +658,14 @@ impl RoochAPIServer for RoochServer { ); let descending_order = descending_order.unwrap_or(true); + let limit_value = limit_of + .checked_add(1) + .ok_or(jsonrpsee::core::Error::Custom( + "limit value overflow".to_string(), + ))?; let mut data = self .rpc_service - .query_field_states(filter.into(), cursor, limit_of + 1, descending_order) + .query_field_states(filter.into(), cursor, limit_value, descending_order) .await? .into_iter() .map(IndexerFieldStateView::try_new_from_table_state) diff --git a/moveos/moveos-stdlib/src/natives/moveos_stdlib/wasm.rs b/moveos/moveos-stdlib/src/natives/moveos_stdlib/wasm.rs index 345650589c..83ba25dc53 100644 --- a/moveos/moveos-stdlib/src/natives/moveos_stdlib/wasm.rs +++ b/moveos/moveos-stdlib/src/natives/moveos_stdlib/wasm.rs @@ -32,6 +32,11 @@ const E_WASM_FUNCTION_NOT_FOUND: u64 = 6; const E_WASM_MEMORY_ACCESS_FAILED: u64 = 7; const E_JSON_UNMARSHAL_FAILED: u64 = 8; pub const E_CBOR_MARSHAL_FAILED: u64 = 9; +pub const E_EMPTY_RETURN_VALUE: u64 = 10; +pub const E_VALUE_NOT_I32: u64 = 11; +pub const E_MEMORY_NOT_FOUND: u64 = 12; +pub const E_INCORRECT_LENGTH_OF_ARGS: u64 = 13; +pub const E_CBOR_UNMARSHAL_FAILED: u64 = 14; #[derive(Debug, Clone)] pub struct WASMCreateInstanceGasParameters { @@ -131,8 +136,10 @@ fn native_create_cbor_values( } } - let cbor_value: ciborium::Value = - ciborium::from_reader(cbor_buffer.as_slice()).expect("cbor unmarshal failed"); + let cbor_value: ciborium::Value = match ciborium::from_reader(cbor_buffer.as_slice()) { + Ok(v) => v, + Err(_) => return build_err(gas_params.base, E_CBOR_UNMARSHAL_FAILED), + }; log::debug!( "native_create_cbor_values -> mint_args_array {:?}", cbor_value @@ -242,11 +249,10 @@ fn native_create_wasm_args_in_memory( )); } Some(instance) => { - let stack_alloc_func = instance - .instance - .exports - .get_function("stackAlloc") - .unwrap(); + let stack_alloc_func = match instance.instance.exports.get_function("stackAlloc") { + Ok(v) => v, + Err(_) => return build_err(gas_params.base_create_args, E_WASM_FUNCTION_NOT_FOUND), + }; for arg in func_args.iter() { let c_arg = unsafe { CString::from_vec_unchecked(arg.clone()) }; @@ -288,6 +294,10 @@ impl WASMExecuteGasParameters { } } +fn build_err(cost: InternalGas, abort_code: u64) -> PartialVMResult { + return Ok(NativeResult::err(cost, abort_code)); +} + // native_execute_wasm_function #[inline] fn native_execute_wasm_function( @@ -322,8 +332,24 @@ fn native_execute_wasm_function( match calling_function.call(&mut instance.store, wasm_func_args.as_slice()) { Ok(ret) => { - let return_value = ret.deref().get(0).unwrap(); - let offset = return_value.i32().unwrap(); + let return_value = match ret.deref().get(0) { + Some(v) => v, + None => { + return build_err( + gas_params.base_create_execution, + E_EMPTY_RETURN_VALUE, + ) + } + }; + let offset = match return_value.i32() { + Some(v) => v, + None => { + return build_err( + gas_params.base_create_execution, + E_VALUE_NOT_I32, + ) + } + }; let ret_val = Value::u64(offset as u64); let mut cost = gas_params.base_create_execution; @@ -381,7 +407,10 @@ fn native_read_data_length( let ret = match instance_pool.lock().unwrap().get_mut(&instance_id) { None => Ok(NativeResult::err(gas_params.base, E_INSTANCE_NO_EXISTS)), Some(instance) => { - let memory = instance.instance.exports.get_memory("memory").unwrap(); + let memory = match instance.instance.exports.get_memory("memory") { + Ok(v) => v, + Err(_) => return build_err(gas_params.base, E_MEMORY_NOT_FOUND), + }; let memory_view = memory.view(&instance.store); let mut length_bytes: [u8; 4] = [0; 4]; match memory_view.read(data_ptr, length_bytes.as_mut_slice()) { @@ -435,7 +464,10 @@ fn native_read_data_from_heap( let ret = match instance_pool.lock().unwrap().get_mut(&instance_id) { None => Ok(NativeResult::err(gas_params.base, E_INSTANCE_NO_EXISTS)), Some(instance) => { - let memory = instance.instance.exports.get_memory("memory").unwrap(); + let memory = match instance.instance.exports.get_memory("memory") { + Ok(v) => v, + Err(_) => return build_err(gas_params.base, E_MEMORY_NOT_FOUND), + }; let memory_view = memory.view(&instance.store); if data_length > 0 { let mut data = vec![0; data_length as usize]; @@ -506,7 +538,10 @@ fn native_release_wasm_instance( _ty_args: Vec, mut args: VecDeque, ) -> PartialVMResult { - let value = args.pop_back().unwrap(); + let value = match args.pop_back() { + Some(v) => v, + None => return build_err(gas_params.base, E_INCORRECT_LENGTH_OF_ARGS), + }; let mut fiedls = value.value_as::()?.unpack()?; let val = fiedls.next().ok_or_else(|| { PartialVMError::new(StatusCode::TYPE_RESOLUTION_FAILURE) diff --git a/moveos/moveos-types/src/moveos_std/tx_meta.rs b/moveos/moveos-types/src/moveos_std/tx_meta.rs index f69362ffac..ac8c3e42cf 100644 --- a/moveos/moveos-types/src/moveos_std/tx_meta.rs +++ b/moveos/moveos-types/src/moveos_std/tx_meta.rs @@ -44,17 +44,27 @@ impl TxMeta { pub fn new_from_move_action(move_action: &MoveAction) -> Self { let function_meta = match move_action { - MoveAction::Function(call) => Some(FunctionCallMeta { - module_address: *call.function_id.module_id.address(), - module_name: MoveAsciiString::new( - call.function_id.module_id.name().as_bytes().to_vec(), - ) - .expect("module name must be valid ascii"), - function_name: MoveAsciiString::new( - call.function_id.function_name.as_bytes().to_vec(), - ) - .expect("module name must be valid ascii"), - }), + MoveAction::Function(call) => { + let module_name = + MoveAsciiString::new(call.function_id.module_id.name().as_bytes().to_vec()); + let function_name = + MoveAsciiString::new(call.function_id.function_name.as_bytes().to_vec()); + if module_name.is_err() || function_name.is_err() { + None + } else { + Some(FunctionCallMeta { + module_address: *call.function_id.module_id.address(), + module_name: MoveAsciiString::new( + call.function_id.module_id.name().as_bytes().to_vec(), + ) + .expect("module name must be valid ascii"), + function_name: MoveAsciiString::new( + call.function_id.function_name.as_bytes().to_vec(), + ) + .expect("module name must be valid ascii"), + }) + } + } _ => None, }; Self { diff --git a/moveos/moveos-verifier/src/metadata.rs b/moveos/moveos-verifier/src/metadata.rs index f731f7628c..252f07c459 100644 --- a/moveos/moveos-verifier/src/metadata.rs +++ b/moveos/moveos-verifier/src/metadata.rs @@ -195,7 +195,6 @@ impl<'a> ExtendedChecker<'a> { // Inspect the bytecode of every function, and if an instruction is CallGeneric, // verify that it calls a function with the private_generics attribute as detected earlier. // Then, ensure that the generic parameters of the CallGeneric instruction are valid. - // #TODO: Ensure that every generic function call is fully checked. check_call_generics(self.env, module, view); for (private_generics_func_name, types_list) in type_name_indices { diff --git a/moveos/moveos/src/vm/moveos_vm.rs b/moveos/moveos/src/vm/moveos_vm.rs index c0455e040b..3e5fd4bd51 100644 --- a/moveos/moveos/src/vm/moveos_vm.rs +++ b/moveos/moveos/src/vm/moveos_vm.rs @@ -234,6 +234,14 @@ where MoveAction::ModuleBundle(module_bundle) => { let compiled_modules = deserialize_modules(&module_bundle)?; + self.vm + .runtime + .loader() + .verify_module_bundle_for_publication( + compiled_modules.as_slice(), + &self.session.data_cache, + )?; + let mut init_function_modules = vec![]; for module in &compiled_modules { let result = moveos_verifier::verifier::verify_module(module, self.remote); @@ -247,7 +255,6 @@ where } } - //TODO add more module verifier. Ok(VerifiedMoveAction::ModuleBundle { module_bundle, init_function_modules, diff --git a/moveos/moveos/src/vm/tx_argument_resolver.rs b/moveos/moveos/src/vm/tx_argument_resolver.rs index bff7218d07..c0bcb9d495 100644 --- a/moveos/moveos/src/vm/tx_argument_resolver.rs +++ b/moveos/moveos/src/vm/tx_argument_resolver.rs @@ -138,7 +138,11 @@ where //Other pure value Struct args //If the session is read_only, only allow any pure value struct, otherwise, only allow the allowed struct if self.read_only || is_allowed_argument_struct(&struct_arg_type) { - let arg = args.next().expect("argument length mismatch"); + let arg = args.next().ok_or_else(|| { + PartialVMError::new(StatusCode::NUMBER_OF_ARGUMENTS_MISMATCH) + .with_message("Argument length mismatch".to_string()) + .finish(location.clone()) + })?; resolved_args.push(ResolvedArg::pure(arg)); } else { return Err(PartialVMError::new(StatusCode::TYPE_MISMATCH)