diff --git a/aptos-move/aptos-vm/src/aptos_vm.rs b/aptos-move/aptos-vm/src/aptos_vm.rs index f4a7dc0afe3af..aaed71a379e95 100644 --- a/aptos-move/aptos-vm/src/aptos_vm.rs +++ b/aptos-move/aptos-vm/src/aptos_vm.rs @@ -426,6 +426,12 @@ impl AptosVM { TransactionPayload::Script(script) => { let loaded_func = session.load_script(script.code(), script.ty_args().to_vec())?; + // Gerardo: consolidate the extended validation to verifier. + verifier::event_validation::verify_no_event_emission_in_script( + script.code(), + session.get_vm_config().max_binary_format_version, + )?; + let args = verifier::transaction_arg_validation::validate_combine_signer_and_txn_args( &mut session, diff --git a/aptos-move/aptos-vm/src/verifier/event_validation.rs b/aptos-move/aptos-vm/src/verifier/event_validation.rs index b618b36593504..d3de58596b6e8 100644 --- a/aptos-move/aptos-vm/src/verifier/event_validation.rs +++ b/aptos-move/aptos-vm/src/verifier/event_validation.rs @@ -4,10 +4,10 @@ use crate::move_vm_ext::SessionExt; use aptos_framework::RuntimeModuleMetadataV1; use move_binary_format::{ - access::ModuleAccess, + access::{ModuleAccess, ScriptAccess}, errors::{Location, PartialVMError, VMError, VMResult}, file_format::{ - Bytecode, + Bytecode, CompiledScript, SignatureToken::{Struct, StructInstantiation}, }, CompiledModule, @@ -25,12 +25,12 @@ fn metadata_validation_err(msg: &str) -> Result<(), VMError> { } fn metadata_validation_error(msg: &str) -> VMError { - PartialVMError::new(StatusCode::CONSTRAINT_NOT_SATISFIED) + PartialVMError::new(StatusCode::EVENT_METADATA_VALIDATION_ERROR) .with_message(format!("metadata and code bundle mismatch: {}", msg)) .finish(Location::Undefined) } -/// Validate resource group metadata on modules one by one: +/// Validate event metadata on modules one by one: /// * Extract the event metadata /// * Verify all changes are compatible upgrades (existing event attributes cannot be removed) pub(crate) fn validate_module_events( @@ -77,8 +77,7 @@ pub(crate) fn validate_emit_calls( let module_addr = module.address_identifier_at(module_handle.address); let module_name = module.identifier_at(module_handle.name); let func_name = module.identifier_at(func_handle.name); - if module_addr - != &AccountAddress::from_hex_literal("0x1").expect("valid address") + if module_addr != &AccountAddress::ONE || module_name.as_str() != EVENT_MODULE_NAME || func_name.as_str() != EVENT_EMIT_FUNCTION_NAME { @@ -147,3 +146,39 @@ pub(crate) fn extract_event_metadata( } Ok(event_structs) } + +pub(crate) fn verify_no_event_emission_in_script( + script_code: &[u8], + max_binary_format_version: u32, +) -> VMResult<()> { + let script = match CompiledScript::deserialize_with_max_version( + script_code, + max_binary_format_version, + ) { + Ok(script) => script, + Err(err) => { + let msg = format!("[VM] deserializer for script returned error: {:?}", err); + return Err(PartialVMError::new(StatusCode::CODE_DESERIALIZATION_ERROR) + .with_message(msg) + .finish(Location::Script)); + }, + }; + for bc in &script.code().code { + if let Bytecode::CallGeneric(index) = bc { + let func_instantiation = &script.function_instantiation_at(*index); + let func_handle = script.function_handle_at(func_instantiation.handle); + let module_handle = script.module_handle_at(func_handle.module); + let module_addr = script.address_identifier_at(module_handle.address); + let module_name = script.identifier_at(module_handle.name); + let func_name = script.identifier_at(func_handle.name); + if module_addr == &AccountAddress::ONE + && module_name.as_str() == EVENT_MODULE_NAME + && func_name.as_str() == EVENT_EMIT_FUNCTION_NAME + { + return Err(PartialVMError::new(StatusCode::INVALID_OPERATION_IN_SCRIPT) + .finish(Location::Script)); + } + } + } + Ok(()) +} diff --git a/aptos-move/e2e-testsuite/src/tests/scripts.rs b/aptos-move/e2e-testsuite/src/tests/scripts.rs index 0da2e5110332f..9c11ed2ea56d3 100644 --- a/aptos-move/e2e-testsuite/src/tests/scripts.rs +++ b/aptos-move/e2e-testsuite/src/tests/scripts.rs @@ -9,8 +9,9 @@ use aptos_types::{ transaction::{ExecutionStatus, Script, TransactionStatus}, }; use move_binary_format::file_format::{ - empty_script, AbilitySet, AddressIdentifierIndex, Bytecode, FunctionHandle, - FunctionHandleIndex, IdentifierIndex, ModuleHandle, ModuleHandleIndex, SignatureIndex, + empty_script, Ability, AbilitySet, AddressIdentifierIndex, Bytecode, FunctionHandle, + FunctionHandleIndex, FunctionInstantiation, FunctionInstantiationIndex, IdentifierIndex, + ModuleHandle, ModuleHandleIndex, Signature, SignatureIndex, SignatureToken, }; use move_core_types::{ identifier::Identifier, @@ -434,3 +435,83 @@ fn script_nested_type_argument_module_does_not_exist() { assert_eq!(balance, updated_sender_balance.coin()); assert_eq!(11, updated_sender.sequence_number()); } + +#[test] +fn forbid_script_emitting_events() { + let mut executor = FakeExecutor::from_head_genesis(); + + // create and publish sender + let sender = executor.create_raw_account_data(1_000_000, 10); + executor.add_account_data(&sender); + + // create an event-emitting script + let mut script = empty_script(); + script.code.code = vec![ + Bytecode::LdTrue, + Bytecode::CallGeneric(FunctionInstantiationIndex(0)), + Bytecode::Ret, + ]; + script.function_instantiations.push(FunctionInstantiation { + handle: FunctionHandleIndex(0), + type_parameters: SignatureIndex(2), + }); + script.function_handles.push(FunctionHandle { + module: ModuleHandleIndex(0), + name: IdentifierIndex(1), + parameters: SignatureIndex(1), + return_: SignatureIndex(0), + type_parameters: vec![ + AbilitySet::singleton(Ability::Store) | AbilitySet::singleton(Ability::Drop), + ], + }); + script.module_handles.push(ModuleHandle { + address: AddressIdentifierIndex(0), + name: IdentifierIndex(0), + }); + script.address_identifiers.push(AccountAddress::ONE); + script.identifiers = vec![ + Identifier::new("event").unwrap(), + Identifier::new("emit").unwrap(), + ]; + // dummy signatures + script.signatures = vec![ + Signature(vec![]), + Signature(vec![SignatureToken::TypeParameter(0)]), + Signature(vec![SignatureToken::Bool]), + ]; + let mut blob = vec![]; + script.serialize(&mut blob).expect("script must serialize"); + let txn = sender + .account() + .transaction() + .script(Script::new(blob, vec![], vec![])) + .sequence_number(10) + .gas_unit_price(1) + .sign(); + // execute transaction + let output = &executor.execute_transaction(txn); + let status = output.status(); + match status { + TransactionStatus::Keep(_) => (), + _ => panic!("TransactionStatus must be Keep"), + } + assert_eq!( + status.status(), + Ok(ExecutionStatus::MiscellaneousError(Some( + StatusCode::INVALID_OPERATION_IN_SCRIPT + ))) + ); + executor.apply_write_set(output.write_set()); + + // Check that numbers in store are correct. + let gas = output.gas_used(); + let balance = 1_000_000 - gas; + let updated_sender = executor + .read_account_resource(sender.account()) + .expect("sender must exist"); + let updated_sender_balance = executor + .read_coin_store_resource(sender.account()) + .expect("sender balance must exist"); + assert_eq!(balance, updated_sender_balance.coin()); + assert_eq!(11, updated_sender.sequence_number()); +} diff --git a/aptos-move/framework/src/natives/event.rs b/aptos-move/framework/src/natives/event.rs index 995131c8fb3a5..a9756e0851c8d 100644 --- a/aptos-move/framework/src/natives/event.rs +++ b/aptos-move/framework/src/natives/event.rs @@ -13,8 +13,6 @@ use aptos_types::contract_event::ContractEvent; use aptos_types::event::EventKey; use better_any::{Tid, TidAble}; use move_binary_format::errors::PartialVMError; -#[cfg(feature = "testing")] -use move_binary_format::errors::PartialVMResult; use move_core_types::{language_storage::TypeTag, vm_status::StatusCode}; use move_vm_runtime::native_functions::NativeFunction; #[cfg(feature = "testing")] @@ -35,11 +33,7 @@ impl NativeEventContext { } #[cfg(feature = "testing")] - fn emitted_v1_events( - &self, - event_key: &EventKey, - ty_tag: &TypeTag, - ) -> PartialVMResult> { + fn emitted_v1_events(&self, event_key: &EventKey, ty_tag: &TypeTag) -> Vec<&[u8]> { let mut events = vec![]; for event in self.events.iter() { if let ContractEvent::V1(e) = event { @@ -48,11 +42,11 @@ impl NativeEventContext { } } } - Ok(events) + events } #[cfg(feature = "testing")] - fn emitted_v2_events(&self, ty_tag: &TypeTag) -> PartialVMResult> { + fn emitted_v2_events(&self, ty_tag: &TypeTag) -> Vec<&[u8]> { let mut events = vec![]; for event in self.events.iter() { if let ContractEvent::V2(e) = event { @@ -61,7 +55,7 @@ impl NativeEventContext { } } } - Ok(events) + events } } @@ -94,7 +88,7 @@ fn native_write_to_event_store( let ty_layout = context.type_to_type_layout(&ty)?; let blob = msg.simple_serialize(&ty_layout).ok_or_else(|| { SafeNativeError::InvariantViolation(PartialVMError::new( - StatusCode::VALUE_DESERIALIZATION_ERROR, + StatusCode::UNKNOWN_INVARIANT_VIOLATION_ERROR, )) })?; let key = bcs::from_bytes(guid.as_slice()).map_err(|_| { @@ -147,12 +141,12 @@ fn native_emitted_events_by_handle( let ty_layout = context.type_to_type_layout(&ty)?; let ctx = context.extensions_mut().get_mut::(); let events = ctx - .emitted_v1_events(&key, &ty_tag)? + .emitted_v1_events(&key, &ty_tag) .into_iter() .map(|blob| { Value::simple_deserialize(blob, &ty_layout).ok_or_else(|| { SafeNativeError::InvariantViolation(PartialVMError::new( - StatusCode::VALUE_DESERIALIZATION_ERROR, + StatusCode::UNKNOWN_INVARIANT_VIOLATION_ERROR, )) }) }) @@ -175,7 +169,7 @@ fn native_emitted_events( let ty_layout = context.type_to_type_layout(&ty)?; let ctx = context.extensions_mut().get_mut::(); let events = ctx - .emitted_v2_events(&ty_tag)? + .emitted_v2_events(&ty_tag) .into_iter() .map(|blob| { Value::simple_deserialize(blob, &ty_layout).ok_or_else(|| { @@ -207,7 +201,7 @@ fn native_write_module_event_to_store( let type_tag = context.type_to_type_tag(&ty)?; - // Runtime check for script/module call. + // Additional runtime check for module call. if let (Some(id), _, _) = context .stack_frames(1) .stack_trace() @@ -229,15 +223,11 @@ fn native_write_module_event_to_store( StatusCode::INTERNAL_TYPE_ERROR, ))); } - } else { - return Err(SafeNativeError::InvariantViolation(PartialVMError::new( - StatusCode::INVALID_OPERATION_IN_SCRIPT, - ))); } let layout = context.type_to_type_layout(&ty)?; let blob = msg.simple_serialize(&layout).ok_or_else(|| { SafeNativeError::InvariantViolation( - PartialVMError::new(StatusCode::VALUE_SERIALIZATION_ERROR) + PartialVMError::new(StatusCode::UNKNOWN_INVARIANT_VIOLATION_ERROR) .with_message("Event serialization failure".to_string()), ) })?; diff --git a/third_party/move/move-bytecode-verifier/src/dependencies.rs b/third_party/move/move-bytecode-verifier/src/dependencies.rs index a3876ec34bc8e..a38f1e900cf64 100644 --- a/third_party/move/move-bytecode-verifier/src/dependencies.rs +++ b/third_party/move/move-bytecode-verifier/src/dependencies.rs @@ -252,7 +252,7 @@ fn verify_imported_structs(context: &Context) -> PartialVMResult<()> { StatusCode::LOOKUP_FAILED, IndexKind::StructHandle, idx as TableIndex, - )) + )); }, } } diff --git a/third_party/move/move-core/types/src/vm_status.rs b/third_party/move/move-core/types/src/vm_status.rs index b52c27ad7a32a..a2412fedb8512 100644 --- a/third_party/move/move-core/types/src/vm_status.rs +++ b/third_party/move/move-core/types/src/vm_status.rs @@ -701,7 +701,7 @@ pub enum StatusCode { MAX_FIELD_DEFINITIONS_REACHED = 1121, // Reserved error code for future use TOO_MANY_BACK_EDGES = 1122, - RESERVED_VERIFICATION_ERROR_1 = 1123, + EVENT_METADATA_VALIDATION_ERROR = 1123, RESERVED_VERIFICATION_ERROR_2 = 1124, RESERVED_VERIFICATION_ERROR_3 = 1125, RESERVED_VERIFICATION_ERROR_4 = 1126, diff --git a/third_party/move/move-vm/runtime/src/session.rs b/third_party/move/move-vm/runtime/src/session.rs index fe42ffbecf02e..8fbd61eb03780 100644 --- a/third_party/move/move-vm/runtime/src/session.rs +++ b/third_party/move/move-vm/runtime/src/session.rs @@ -3,7 +3,7 @@ // SPDX-License-Identifier: Apache-2.0 use crate::{ - data_cache::TransactionDataCache, loader::LoadedFunction, move_vm::MoveVM, + config::VMConfig, data_cache::TransactionDataCache, loader::LoadedFunction, move_vm::MoveVM, native_extensions::NativeContextExtensions, }; use move_binary_format::{ @@ -422,6 +422,10 @@ impl<'r, 'l> Session<'r, 'l> { pub fn get_move_vm(&self) -> &'l MoveVM { self.move_vm } + + pub fn get_vm_config(&self) -> &'l VMConfig { + self.move_vm.runtime.loader().vm_config() + } } pub struct LoadedFunctionInstantiation {