diff --git a/aptos-move/aptos-vm/src/transaction_arg_validation.rs b/aptos-move/aptos-vm/src/transaction_arg_validation.rs index a65c2aab6ecdb..dac6eb60120c9 100644 --- a/aptos-move/aptos-vm/src/transaction_arg_validation.rs +++ b/aptos-move/aptos-vm/src/transaction_arg_validation.rs @@ -11,25 +11,17 @@ use crate::{ VMStatus, }; use move_deps::{ - move_binary_format::file_format_common::read_uleb128_as_u64, move_core_types::{account_address::AccountAddress, value::MoveValue, vm_status::StatusCode}, move_vm_runtime::session::LoadedFunctionInstantiation, move_vm_types::loaded_data::runtime_types::Type, }; use once_cell::sync::Lazy; -use std::collections::BTreeMap; -use std::io::{Cursor, Read}; +use std::collections::BTreeSet; -// A map which contains the structs allowed as transaction input and the -// validation function for those, if one was needed (None otherwise). -// The validation function takes the serialized argument and returns -// an error if the validation fails. -type ValidateArg = fn(&[u8]) -> Result<(), VMStatus>; - -static ALLOWED_STRUCTS: Lazy>> = Lazy::new(|| { - [("0x1::string::String", Some(check_string as ValidateArg))] - .into_iter() - .map(|(s, validator)| (s.to_string(), validator)) +static ALLOWED_STRUCTS: Lazy> = Lazy::new(|| { + ["0x1::string::String"] + .iter() + .map(|s| s.to_string()) .collect() }); @@ -64,11 +56,8 @@ pub(crate) fn validate_combine_signer_and_txn_args( } } // validate all non_signer params - let mut needs_validation = false; for ty in func.parameters[signer_param_cnt..].iter() { - let (valid, validation) = is_valid_txn_arg(session, ty); - needs_validation = needs_validation || validation; - if !valid { + if !is_valid_txn_arg(session, ty) { return Err(VMStatus::Error(StatusCode::INVALID_MAIN_FUNCTION_SIGNATURE)); } } @@ -94,116 +83,22 @@ pub(crate) fn validate_combine_signer_and_txn_args( .chain(args) .collect() }; - if needs_validation { - validate_args(session, &combined_args, func)?; - } Ok(combined_args) } -// Return whether the argument is valid/allowed and whether it needs validation. -// Validation is only needed for String arguments at the moment and vectors of them. -fn is_valid_txn_arg(session: &SessionExt, typ: &Type) -> (bool, bool) { +fn is_valid_txn_arg(session: &SessionExt, typ: &Type) -> bool { use move_deps::move_vm_types::loaded_data::runtime_types::Type::*; match typ { - Bool | U8 | U64 | U128 | Address => (true, false), + Bool | U8 | U64 | U128 | Address => true, Vector(inner) => is_valid_txn_arg(session, inner), Struct(idx) | StructInstantiation(idx, _) => { if let Some(st) = session.get_struct_type(*idx) { let full_name = format!("{}::{}", st.module.short_str_lossless(), st.name); - match ALLOWED_STRUCTS.get(&full_name) { - None => (false, false), - Some(validator) => (true, validator.is_some()), - } + ALLOWED_STRUCTS.contains(&full_name) } else { - (false, false) - } - } - Signer | Reference(_) | MutableReference(_) | TyParam(_) => (false, false), - } -} - -// Validate arguments. Walk through the arguments and according to the signature -// validate arguments that require so. -// TODO: This needs a more solid story and a tighter integration with the VM. -// Validation at the moment is only for Strings and Vector of them, so we -// manually walk the serialized args until we find a string. -// This is obviously brittle and something to change at some point soon. -fn validate_args( - session: &SessionExt, - args: &[Vec], - func: &LoadedFunctionInstantiation, -) -> Result<(), VMStatus> { - for (ty, arg) in func.parameters.iter().zip(args.iter()) { - let mut cursor = Cursor::new(&arg[..]); - validate_arg(session, ty, &mut cursor)?; - } - Ok(()) -} - -// Validate a single arg. A Cursor is used to walk the serialized arg manually and correctly. -fn validate_arg( - session: &SessionExt, - ty: &Type, - cursor: &mut Cursor<&[u8]>, -) -> Result<(), VMStatus> { - use move_deps::move_vm_types::loaded_data::runtime_types::Type::*; - Ok(match ty { - Vector(inner) => { - // get the vector length and iterate over each element - let mut len = get_len(cursor)?; - while len > 0 { - validate_arg(session, inner, cursor)?; - len -= 1; - } - } - // only strings are validated, and given we are here only if one was present - // (`is_valid_txn_arg`), this match arm must be for a string - Struct(idx) | StructInstantiation(idx, _) => { - // load the struct name, we use `expect()` because that check was already - // performed in `is_valid_txn_arg` - let st = session - .get_struct_type(*idx) - .expect("unrachable, type must exist"); - let full_name = format!("{}::{}", st.module.short_str_lossless(), st.name); - // load the serialized string - let len = get_len(cursor)?; - let mut s = vec![0u8; len]; - cursor - .read_exact(&mut s) - .map_err(|_| VMStatus::Error(StatusCode::FAILED_TO_DESERIALIZE_ARGUMENT))?; - // validate the struct value, we use `expect()` because that check was already - // performed in `is_valid_txn_arg` - let option_validator = ALLOWED_STRUCTS - .get(&full_name) - .expect("unreachable: struct must be allowed"); - if let Some(validator) = option_validator { - validator(&s)?; + false } } - // nothing to validate - Bool | U8 | U64 | U128 | Address | Signer | Reference(_) | MutableReference(_) - | TyParam(_) => (), - }) -} - -// String is a vector of bytes, so both string and vector carry a length in the serialized format. -// Length of vectors in BCS uses uleb128 as a compression format. -fn get_len(cursor: &mut Cursor<&[u8]>) -> Result { - match read_uleb128_as_u64(cursor) { - Err(_) => Err(VMStatus::Error(StatusCode::FAILED_TO_DESERIALIZE_ARGUMENT)), - Ok(len) => Ok(len as usize), - } -} - -// -// Argument validation functions -// - -// Check if a string is valid. This code is copied from string.rs in the stdlib. -// TODO: change the move VM code (string.rs) to expose a function that does validation. -fn check_string(s: &[u8]) -> Result<(), VMStatus> { - match std::str::from_utf8(s) { - Ok(_) => Ok(()), - Err(_) => Err(VMStatus::Error(StatusCode::FAILED_TO_DESERIALIZE_ARGUMENT)), + Signer | Reference(_) | MutableReference(_) | TyParam(_) => false, } } diff --git a/aptos-move/e2e-move-tests/tests/string_args.data/pack/sources/hello.move b/aptos-move/e2e-move-tests/tests/string_args.data/pack/sources/hello.move index dd47002877ef0..be448384a8b52 100644 --- a/aptos-move/e2e-move-tests/tests/string_args.data/pack/sources/hello.move +++ b/aptos-move/e2e-move-tests/tests/string_args.data/pack/sources/hello.move @@ -1,7 +1,6 @@ module 0xCAFE::test { use std::signer; use std::string::String; - use std::vector; struct ModuleData has key, store { state: String, @@ -15,22 +14,4 @@ module 0xCAFE::test { borrow_global_mut(addr).state = msg; } } - - public entry fun hi_vec(sender: &signer, msgs: vector, i: u64) acquires ModuleData { - let addr = signer::address_of(sender); - if (!exists(addr)) { - move_to(sender, ModuleData{state: *vector::borrow(&msgs, i)}) - } else { - borrow_global_mut(addr).state = *vector::borrow(&msgs, i); - } - } - - public entry fun more_hi_vec(sender: &signer, msgs: vector>, i: u64, j: u64) acquires ModuleData { - let addr = signer::address_of(sender); - if (!exists(addr)) { - move_to(sender, ModuleData{state: *vector::borrow(vector::borrow(&msgs, i), j)}) - } else { - borrow_global_mut(addr).state = *vector::borrow(vector::borrow(&msgs, i), j); - } - } } diff --git a/aptos-move/e2e-move-tests/tests/string_args.rs b/aptos-move/e2e-move-tests/tests/string_args.rs index b57f7ad4755e1..1f26e625dbbd6 100644 --- a/aptos-move/e2e-move-tests/tests/string_args.rs +++ b/aptos-move/e2e-move-tests/tests/string_args.rs @@ -2,8 +2,8 @@ // SPDX-License-Identifier: Apache-2.0 use aptos_types::account_address::AccountAddress; -use e2e_move_tests::{assert_success, assert_vm_status, MoveHarness}; -use move_deps::move_core_types::{parser::parse_struct_tag, vm_status::StatusCode}; +use e2e_move_tests::{assert_success, MoveHarness}; +use move_deps::move_core_types::parser::parse_struct_tag; use serde::{Deserialize, Serialize}; mod common; @@ -44,399 +44,3 @@ fn string_args() { "hi there!" ); } - -#[test] -fn string_args_vec() { - let mut h = MoveHarness::new(); - - // Load the code - let acc = h.new_account_at(AccountAddress::from_hex_literal("0xcafe").unwrap()); - assert_success!(h.publish_package(&acc, &common::test_dir_path("string_args.data/pack"))); - - let module_data = parse_struct_tag("0xCAFE::test::ModuleData").unwrap(); - - // Check in initial state, resource does not exist. - assert!(!h.exists_resource(acc.address(), module_data.clone())); - - let s_vec = vec![ - "hi there!".as_bytes(), - "hello".as_bytes(), - "world".as_bytes(), - ]; - // Now send hi_vec transaction, after that resource should exist and carry value - let mut i = 0u64; - assert_success!(h.run_entry_function( - &acc, - str::parse("0xcafe::test::hi_vec").unwrap(), - vec![], - vec![bcs::to_bytes(&s_vec).unwrap(), bcs::to_bytes(&i).unwrap()], - )); - assert_eq!( - String::from_utf8( - h.read_resource::(acc.address(), module_data.clone()) - .unwrap() - .state - ) - .unwrap(), - "hi there!" - ); - // Now send hi_vec transaction, after that resource should exist and carry value - i = 1u64; - assert_success!(h.run_entry_function( - &acc, - str::parse("0xcafe::test::hi_vec").unwrap(), - vec![], - vec![bcs::to_bytes(&s_vec).unwrap(), bcs::to_bytes(&i).unwrap()], - )); - assert_eq!( - String::from_utf8( - h.read_resource::(acc.address(), module_data.clone()) - .unwrap() - .state - ) - .unwrap(), - "hello" - ); - // Now send hi_vec transaction, after that resource should exist and carry value - i = 2u64; - assert_success!(h.run_entry_function( - &acc, - str::parse("0xcafe::test::hi_vec").unwrap(), - vec![], - vec![bcs::to_bytes(&s_vec).unwrap(), bcs::to_bytes(&i).unwrap()], - )); - assert_eq!( - String::from_utf8( - h.read_resource::(acc.address(), module_data) - .unwrap() - .state - ) - .unwrap(), - "world" - ); -} - -#[test] -fn string_args_vec_vec() { - let mut h = MoveHarness::new(); - - // Load the code - let acc = h.new_account_at(AccountAddress::from_hex_literal("0xcafe").unwrap()); - assert_success!(h.publish_package(&acc, &common::test_dir_path("string_args.data/pack"))); - - let module_data = parse_struct_tag("0xCAFE::test::ModuleData").unwrap(); - - // Check in initial state, resource does not exist. - assert!(!h.exists_resource(acc.address(), module_data.clone())); - - let s_vec = vec![ - vec![ - "hi there!".as_bytes(), - "hello".as_bytes(), - "world".as_bytes(), - ], - vec![ - "hello".as_bytes(), - "world".as_bytes(), - "hi there!".as_bytes(), - ], - vec![ - "world".as_bytes(), - "hi there!".as_bytes(), - "hello".as_bytes(), - ], - ]; - // Now send more_hi_vec transaction, after that resource should exist and carry value - let i = 0u64; - let j = 0u64; - assert_success!(h.run_entry_function( - &acc, - str::parse("0xcafe::test::more_hi_vec").unwrap(), - vec![], - vec![ - bcs::to_bytes(&s_vec).unwrap(), - bcs::to_bytes(&i).unwrap(), - bcs::to_bytes(&j).unwrap() - ], - )); - assert_eq!( - String::from_utf8( - h.read_resource::(acc.address(), module_data.clone()) - .unwrap() - .state - ) - .unwrap(), - "hi there!" - ); - // Now send more_hi_vec transaction, after that resource should exist and carry value - let i = 1u64; - let j = 2u64; - assert_success!(h.run_entry_function( - &acc, - str::parse("0xcafe::test::more_hi_vec").unwrap(), - vec![], - vec![ - bcs::to_bytes(&s_vec).unwrap(), - bcs::to_bytes(&i).unwrap(), - bcs::to_bytes(&j).unwrap() - ], - )); - assert_eq!( - String::from_utf8( - h.read_resource::(acc.address(), module_data.clone()) - .unwrap() - .state - ) - .unwrap(), - "hi there!" - ); - // Now send more_hi_vec transaction, after that resource should exist and carry value - let i = 2u64; - let j = 1u64; - assert_success!(h.run_entry_function( - &acc, - str::parse("0xcafe::test::more_hi_vec").unwrap(), - vec![], - vec![ - bcs::to_bytes(&s_vec).unwrap(), - bcs::to_bytes(&i).unwrap(), - bcs::to_bytes(&j).unwrap() - ], - )); - assert_eq!( - String::from_utf8( - h.read_resource::(acc.address(), module_data) - .unwrap() - .state - ) - .unwrap(), - "hi there!" - ); -} - -#[test] -fn string_args_bad_1() { - let mut h = MoveHarness::new(); - - // Load the code - let acc = h.new_account_at(AccountAddress::from_hex_literal("0xcafe").unwrap()); - assert_success!(h.publish_package(&acc, &common::test_dir_path("string_args.data/pack"))); - - let module_data = parse_struct_tag("0xCAFE::test::ModuleData").unwrap(); - - // Check in initial state, resource does not exist. - assert!(!h.exists_resource(acc.address(), module_data)); - - // Now send hi transaction, after that resource should exist and carry value - let status = h.run_entry_function( - &acc, - str::parse("0xcafe::test::hi").unwrap(), - vec![], - vec![bcs::to_bytes(&[0xf0, 0x28, 0x8c, 0xbc]).unwrap()], - ); - assert_vm_status!(status, StatusCode::FAILED_TO_DESERIALIZE_ARGUMENT) -} - -#[test] -fn string_args_bad_2() { - let mut h = MoveHarness::new(); - - // Load the code - let acc = h.new_account_at(AccountAddress::from_hex_literal("0xcafe").unwrap()); - assert_success!(h.publish_package(&acc, &common::test_dir_path("string_args.data/pack"))); - - let module_data = parse_struct_tag("0xCAFE::test::ModuleData").unwrap(); - - // Check in initial state, resource does not exist. - assert!(!h.exists_resource(acc.address(), module_data)); - - // Now send hi transaction, after that resource should exist and carry value - let status = h.run_entry_function( - &acc, - str::parse("0xcafe::test::hi").unwrap(), - vec![], - vec![bcs::to_bytes(&[0xc3, 0x28]).unwrap()], - ); - assert_vm_status!(status, StatusCode::FAILED_TO_DESERIALIZE_ARGUMENT) -} - -#[test] -fn string_args_bad_vec1() { - let mut h = MoveHarness::new(); - - // Load the code - let acc = h.new_account_at(AccountAddress::from_hex_literal("0xcafe").unwrap()); - assert_success!(h.publish_package(&acc, &common::test_dir_path("string_args.data/pack"))); - - let module_data = parse_struct_tag("0xCAFE::test::ModuleData").unwrap(); - - // Check in initial state, resource does not exist. - assert!(!h.exists_resource(acc.address(), module_data)); - - let bad = vec![0xc3, 0x28]; - let s_vec = vec![&bad[..], "hello".as_bytes(), "world".as_bytes()]; - // Now send hi_vec transaction, after that resource should exist and carry value - let i = 0u64; - let status = h.run_entry_function( - &acc, - str::parse("0xcafe::test::hi_vec").unwrap(), - vec![], - vec![bcs::to_bytes(&s_vec).unwrap(), bcs::to_bytes(&i).unwrap()], - ); - assert_vm_status!(status, StatusCode::FAILED_TO_DESERIALIZE_ARGUMENT) -} - -#[test] -fn string_args_bad_vec2() { - let mut h = MoveHarness::new(); - - // Load the code - let acc = h.new_account_at(AccountAddress::from_hex_literal("0xcafe").unwrap()); - assert_success!(h.publish_package(&acc, &common::test_dir_path("string_args.data/pack"))); - - let module_data = parse_struct_tag("0xCAFE::test::ModuleData").unwrap(); - - // Check in initial state, resource does not exist. - assert!(!h.exists_resource(acc.address(), module_data)); - - let bad = vec![0xff, 0xfe]; - let s_vec = vec!["hello".as_bytes(), "world".as_bytes(), &bad[..]]; - // Now send hi_vec transaction, after that resource should exist and carry value - let i = 0u64; - let status = h.run_entry_function( - &acc, - str::parse("0xcafe::test::hi_vec").unwrap(), - vec![], - vec![bcs::to_bytes(&s_vec).unwrap(), bcs::to_bytes(&i).unwrap()], - ); - assert_vm_status!(status, StatusCode::FAILED_TO_DESERIALIZE_ARGUMENT) -} - -#[test] -fn string_args_bad_vec_vec_1() { - let mut h = MoveHarness::new(); - - // Load the code - let acc = h.new_account_at(AccountAddress::from_hex_literal("0xcafe").unwrap()); - assert_success!(h.publish_package(&acc, &common::test_dir_path("string_args.data/pack"))); - - let module_data = parse_struct_tag("0xCAFE::test::ModuleData").unwrap(); - - // Check in initial state, resource does not exist. - assert!(!h.exists_resource(acc.address(), module_data)); - - let bad = vec![0x40, 0xfe]; - let s_vec = vec![ - vec![&bad[..], "hello".as_bytes(), "world".as_bytes()], - vec![ - "hello".as_bytes(), - "world".as_bytes(), - "hi there!".as_bytes(), - ], - vec![ - "world".as_bytes(), - "hi there!".as_bytes(), - "hello".as_bytes(), - ], - ]; - // Now send more_hi_vec transaction, after that resource should exist and carry value - let i = 0u64; - let j = 0u64; - let status = h.run_entry_function( - &acc, - str::parse("0xcafe::test::more_hi_vec").unwrap(), - vec![], - vec![ - bcs::to_bytes(&s_vec).unwrap(), - bcs::to_bytes(&i).unwrap(), - bcs::to_bytes(&j).unwrap(), - ], - ); - assert_vm_status!(status, StatusCode::FAILED_TO_DESERIALIZE_ARGUMENT) -} - -#[test] -fn string_args_bad_vec_vec_2() { - let mut h = MoveHarness::new(); - - // Load the code - let acc = h.new_account_at(AccountAddress::from_hex_literal("0xcafe").unwrap()); - assert_success!(h.publish_package(&acc, &common::test_dir_path("string_args.data/pack"))); - - let module_data = parse_struct_tag("0xCAFE::test::ModuleData").unwrap(); - - // Check in initial state, resource does not exist. - assert!(!h.exists_resource(acc.address(), module_data)); - - let bad = vec![0xf0, 0x28, 0x8c, 0x28]; - let s_vec = vec![ - vec![ - "hi there!".as_bytes(), - "hello".as_bytes(), - "world".as_bytes(), - ], - vec!["hello".as_bytes(), &bad[..], "hi there!".as_bytes()], - vec![ - "world".as_bytes(), - "hi there!".as_bytes(), - "hello".as_bytes(), - ], - ]; - // Now send more_hi_vec transaction, after that resource should exist and carry value - let i = 0u64; - let j = 0u64; - let status = h.run_entry_function( - &acc, - str::parse("0xcafe::test::more_hi_vec").unwrap(), - vec![], - vec![ - bcs::to_bytes(&s_vec).unwrap(), - bcs::to_bytes(&i).unwrap(), - bcs::to_bytes(&j).unwrap(), - ], - ); - assert_vm_status!(status, StatusCode::FAILED_TO_DESERIALIZE_ARGUMENT) -} - -#[test] -fn string_args_bad_vec_vec_3() { - let mut h = MoveHarness::new(); - - // Load the code - let acc = h.new_account_at(AccountAddress::from_hex_literal("0xcafe").unwrap()); - assert_success!(h.publish_package(&acc, &common::test_dir_path("string_args.data/pack"))); - - let module_data = parse_struct_tag("0xCAFE::test::ModuleData").unwrap(); - - // Check in initial state, resource does not exist. - assert!(!h.exists_resource(acc.address(), module_data)); - - let bad = vec![0x40, 0xff]; - let s_vec = vec![ - vec![ - "hi there!".as_bytes(), - "hello".as_bytes(), - "world".as_bytes(), - ], - vec![ - "hello".as_bytes(), - "world".as_bytes(), - "hi there!".as_bytes(), - ], - vec!["world".as_bytes(), "hi there!".as_bytes(), &bad[..]], - ]; - // Now send more_hi_vec transaction, after that resource should exist and carry value - let i = 0u64; - let j = 0u64; - let status = h.run_entry_function( - &acc, - str::parse("0xcafe::test::more_hi_vec").unwrap(), - vec![], - vec![ - bcs::to_bytes(&s_vec).unwrap(), - bcs::to_bytes(&i).unwrap(), - bcs::to_bytes(&j).unwrap(), - ], - ); - assert_vm_status!(status, StatusCode::FAILED_TO_DESERIALIZE_ARGUMENT) -}