From b23e7b4cefead420e2892e11a674b83ee466de3e Mon Sep 17 00:00:00 2001 From: Graydon Hoare Date: Tue, 4 Jul 2023 19:29:36 -0700 Subject: [PATCH] Enforce object handle integrity when inserting into containers, fix #569 --- soroban-env-host/src/host.rs | 24 ++++++++++- soroban-env-host/src/host_object.rs | 44 +++++++++++++++++++- soroban-env-host/src/test/budget_metering.rs | 7 ++-- soroban-env-host/src/test/map.rs | 35 ++++++++++++++++ soroban-env-host/src/test/vec.rs | 40 ++++++++++++++++++ 5 files changed, 144 insertions(+), 6 deletions(-) diff --git a/soroban-env-host/src/host.rs b/soroban-env-host/src/host.rs index cf7061854..5ec6fd9f7 100644 --- a/soroban-env-host/src/host.rs +++ b/soroban-env-host/src/host.rs @@ -1000,6 +1000,9 @@ impl EnvBase for Host { for k in keys.iter() { key_syms.push(Symbol::try_from_val(self, k)?); } + for v in vals.iter() { + self.check_val_integrity(*v)?; + } let pair_iter = key_syms .iter() .map(|s| s.to_val()) @@ -1049,8 +1052,11 @@ impl EnvBase for Host { } fn vec_new_from_slice(&self, vals: &[Val]) -> Result { - let map = HostVec::from_exact_iter(vals.iter().cloned(), self.budget_ref())?; - self.add_host_object(map) + let vec = HostVec::from_exact_iter(vals.iter().cloned(), self.budget_ref())?; + for v in vec.iter() { + self.check_val_integrity(*v)?; + } + self.add_host_object(vec) } fn vec_unpack_to_slice(&self, vec: VecObject, vals: &mut [Val]) -> Result { @@ -1481,6 +1487,8 @@ impl VmCallerEnv for Host { k: Val, v: Val, ) -> Result { + self.check_val_integrity(k)?; + self.check_val_integrity(v)?; let mnew = self.visit_obj(m, |hm: &HostMap| hm.insert(k, v, self))?; self.add_host_object(mnew) } @@ -1659,6 +1667,9 @@ impl VmCallerEnv for Host { vals.as_mut_slice(), |buf| Val::from_payload(u64::from_le_bytes(*buf)), )?; + for v in vals.iter() { + self.check_val_integrity(*v)?; + } // Step 3: turn pairs into a map. let pair_iter = key_syms @@ -1741,6 +1752,7 @@ impl VmCallerEnv for Host { x: Val, ) -> Result { let i: u32 = i.into(); + self.check_val_integrity(x)?; let vnew = self.visit_obj(v, move |hv: &HostVec| { self.validate_index_lt_bound(i, hv.len())?; hv.set(i as usize, x, self.as_budget()) @@ -1785,6 +1797,7 @@ impl VmCallerEnv for Host { v: VecObject, x: Val, ) -> Result { + self.check_val_integrity(x)?; let vnew = self.visit_obj(v, move |hv: &HostVec| hv.push_front(x, self.as_budget()))?; self.add_host_object(vnew) } @@ -1804,6 +1817,7 @@ impl VmCallerEnv for Host { v: VecObject, x: Val, ) -> Result { + self.check_val_integrity(x)?; let vnew = self.visit_obj(v, move |hv: &HostVec| hv.push_back(x, self.as_budget()))?; self.add_host_object(vnew) } @@ -1837,6 +1851,7 @@ impl VmCallerEnv for Host { x: Val, ) -> Result { let i: u32 = i.into(); + self.check_val_integrity(x)?; let vnew = self.visit_obj(v, move |hv: &HostVec| { self.validate_index_le_bound(i, hv.len())?; hv.insert(i as usize, x, self.as_budget()) @@ -1941,6 +1956,9 @@ impl VmCallerEnv for Host { vals.as_mut_slice(), |buf| Val::from_payload(u64::from_le_bytes(*buf)), )?; + for v in vals.iter() { + self.check_val_integrity(*v)?; + } self.add_host_object(HostVec::from_vec(vals)) } @@ -1973,6 +1991,8 @@ impl VmCallerEnv for Host { t: StorageType, f: Val, ) -> Result { + self.check_val_integrity(k)?; + self.check_val_integrity(v)?; match t { StorageType::Temporary | StorageType::Persistent => { self.put_contract_data_into_ledger(k, v, t, f)? diff --git a/soroban-env-host/src/host_object.rs b/soroban-env-host/src/host_object.rs index 0c921f45c..004a707fa 100644 --- a/soroban-env-host/src/host_object.rs +++ b/soroban-env-host/src/host_object.rs @@ -1,6 +1,6 @@ use soroban_env_common::{ xdr::ContractCostType, Compare, DurationSmall, I128Small, I256Small, I64Small, SymbolSmall, - SymbolStr, TimepointSmall, U128Small, U256Small, U64Small, + SymbolStr, Tag, TimepointSmall, U128Small, U256Small, U64Small, }; use crate::{ @@ -206,6 +206,48 @@ impl Host { f(r.get(handle as usize)) } + pub(crate) fn check_val_integrity(&self, val: Val) -> Result<(), HostError> { + if let Ok(obj) = Object::try_from(val) { + self.check_obj_integrity(obj)?; + } + Ok(()) + } + + pub(crate) fn check_obj_integrity(&self, obj: Object) -> Result<(), HostError> { + unsafe { + self.unchecked_visit_val_obj(obj, |hopt| match hopt { + None => Err(self.err( + xdr::ScErrorType::Object, + xdr::ScErrorCode::MissingValue, + "unknown object reference", + &[], + )), + Some(hobj) => match (hobj, obj.to_val().get_tag()) { + (HostObject::Vec(_), Tag::VecObject) + | (HostObject::Map(_), Tag::MapObject) + | (HostObject::U64(_), Tag::U64Object) + | (HostObject::I64(_), Tag::I64Object) + | (HostObject::TimePoint(_), Tag::TimepointObject) + | (HostObject::Duration(_), Tag::DurationObject) + | (HostObject::U128(_), Tag::U128Object) + | (HostObject::I128(_), Tag::I128Object) + | (HostObject::U256(_), Tag::U256Object) + | (HostObject::I256(_), Tag::I256Object) + | (HostObject::Bytes(_), Tag::BytesObject) + | (HostObject::String(_), Tag::StringObject) + | (HostObject::Symbol(_), Tag::SymbolObject) + | (HostObject::Address(_), Tag::AddressObject) => Ok(()), + _ => Err(self.err( + xdr::ScErrorType::Object, + xdr::ScErrorCode::UnexpectedType, + "mis-tagged object reference", + &[], + )), + }, + }) + } + } + // Notes on metering: object visiting part is covered by unchecked_visit_val_obj. Closure function // needs to be metered separately. pub(crate) fn visit_obj( diff --git a/soroban-env-host/src/test/budget_metering.rs b/soroban-env-host/src/test/budget_metering.rs index 280cb3260..2364f7561 100644 --- a/soroban-env-host/src/test/budget_metering.rs +++ b/soroban-env-host/src/test/budget_metering.rs @@ -155,9 +155,10 @@ fn map_insert_key_vec_obj() -> Result<(), HostError> { host.map_put(m, k1.into(), v1)?; host.with_budget(|budget| { - // 6 = 1 visit map + 1 visit k1 + (obj_cmp which needs to) 1 visit both k0 and k1 during lookup, - // and then 2 more to validate order of resulting map. - assert_eq!(budget.get_tracker(ContractCostType::VisitObject)?.0, 6); + // 6 = 2 visits to ensure object handle integrity + 1 visit map + 1 + // visit k1 + (obj_cmp which needs to) 1 visit both k0 and k1 during + // lookup, and then 2 more to validate order of resulting map. + assert_eq!(budget.get_tracker(ContractCostType::VisitObject)?.0, 8); // upper bound of number of map-accesses, counting both binary-search, point-access and validate-scan. assert_eq!(budget.get_tracker(ContractCostType::MapEntry)?.0, 8); Ok(()) diff --git a/soroban-env-host/src/test/map.rs b/soroban-env-host/src/test/map.rs index 15a4b10ea..02e826b4c 100644 --- a/soroban-env-host/src/test/map.rs +++ b/soroban-env-host/src/test/map.rs @@ -302,3 +302,38 @@ fn map_stack_no_overflow_65536_boxed_keys_and_vals() { } } } + +#[test] +fn map_build_bad_element_integrity() -> Result<(), HostError> { + use crate::EnvBase; + let host = Host::default(); + let obj = host.map_new()?; + + let ok_val = obj.to_val(); + let payload = ok_val.get_payload(); + + // The low 8 bits of an object-handle payload are the + // tag indicating its type. We just add one to the + // object type here, corrupting it. + let bad_tag = Val::from_payload(payload + 1); + + // the high 32 bits of an object-handle payload are the + // index number of the handle. We corrupt those here with + // an object index far greater than any allocated. + let bad_handle = Val::from_payload(payload | 0xff_u64 << 48); + + // Inserting ok object referejces into maps should work. + assert!(host.map_put(obj, ok_val, ok_val).is_ok()); + assert!(host.map_new_from_slices(&["hi"], &[ok_val]).is_ok()); + + // Inserting corrupt object references into maps should fail. + assert!(host.map_put(obj, ok_val, bad_tag).is_err()); + assert!(host.map_put(obj, bad_tag, ok_val).is_err()); + assert!(host.map_new_from_slices(&["hi"], &[bad_tag]).is_err()); + + assert!(host.map_put(obj, ok_val, bad_handle).is_err()); + assert!(host.map_put(obj, bad_handle, ok_val).is_err()); + assert!(host.map_new_from_slices(&["hi"], &[bad_handle]).is_err()); + + Ok(()) +} diff --git a/soroban-env-host/src/test/vec.rs b/soroban-env-host/src/test/vec.rs index afdaf95a1..480a858be 100644 --- a/soroban-env-host/src/test/vec.rs +++ b/soroban-env-host/src/test/vec.rs @@ -294,3 +294,43 @@ fn vec_binary_search() -> Result<(), HostError> { assert_eq!(u64::from(4u32), res); Ok(()) } + +#[test] +fn vec_build_bad_element_integrity() -> Result<(), HostError> { + use crate::EnvBase; + let host = Host::default(); + let obj = host.test_vec_obj::(&[1, 2, 3])?; + let i = U32Val::from(1); + + let ok_val = obj.to_val(); + let payload = ok_val.get_payload(); + + // The low 8 bits of an object-handle payload are the + // tag indicating its type. We just add one to the + // object type here, corrupting it. + let bad_tag = Val::from_payload(payload + 1); + + // the high 32 bits of an object-handle payload are the + // index number of the handle. We corrupt those here with + // an object index far greater than any allocated. + let bad_handle = Val::from_payload(payload | 0xff_u64 << 48); + + // Inserting ok object referejces into vectors should work. + assert!(host.vec_put(obj, i, ok_val).is_ok()); + assert!(host.vec_push_front(obj, ok_val).is_ok()); + assert!(host.vec_push_back(obj, ok_val).is_ok()); + assert!(host.vec_new_from_slice(&[ok_val]).is_ok()); + + // Inserting corrupt object references into vectors should fail. + assert!(host.vec_put(obj, i, bad_tag).is_err()); + assert!(host.vec_push_front(obj, bad_tag).is_err()); + assert!(host.vec_push_back(obj, bad_tag).is_err()); + assert!(host.vec_new_from_slice(&[bad_tag]).is_err()); + + assert!(host.vec_put(obj, i, bad_handle).is_err()); + assert!(host.vec_push_front(obj, bad_handle).is_err()); + assert!(host.vec_push_back(obj, bad_handle).is_err()); + assert!(host.vec_new_from_slice(&[bad_handle]).is_err()); + + Ok(()) +}