Repository: stellar/rs-soroban-env (rust)

Issue:
Host::check_val_integrity could/should error on bad tags
`Host::check_val_integrity` is called on all Vals passing through the Host. This avoids contracts receiving adversarial "weird" Vals from other contracts.

At the moment it appears to only reject Vals that are objects that have invalid references.

Another type of unexpected Val is one with an invalid tag (Tag::Bad). The host could also reject these.

cc "adversarial" vals for fuzzing https://github.com/stellar/rs-soroban-sdk/issues/1052


Pull request:
Reject vals with invalid tags, fix #1029
This addresses #1029 and also adds more calls to check_val_integrity (hopefully all, but it's hard to be sure) on incoming args to host functions.

(NB: any args that are typed more precisely than `Val` do not need such a check, as they will not type-convert correctly in the macro-generated dispatch function)

Patch:
diff --git a/soroban-env-host/src/host.rs b/soroban-env-host/src/host.rs
index a1fee7d3..22a4a003 100644
--- a/soroban-env-host/src/host.rs
+++ b/soroban-env-host/src/host.rs
@@ -1269,6 +1269,8 @@ impl VmCallerEnv for Host {
 
     // Metered: covered by `visit`.
     fn obj_cmp(&self, _vmcaller: &mut VmCaller<Host>, a: Val, b: Val) -> Result<i64, HostError> {
+        self.check_val_integrity(a)?;
+        self.check_val_integrity(b)?;
         let res = match {
             match (Object::try_from(a), Object::try_from(b)) {
                 // We were given two objects: compare them.
@@ -1334,6 +1336,7 @@ impl VmCallerEnv for Host {
         topics: VecObject,
         data: Val,
     ) -> Result<Void, HostError> {
+        self.check_val_integrity(data)?;
         self.record_contract_event(ContractEventType::Contract, topics, data)?;
         Ok(Val::VOID)
     }
@@ -1616,6 +1619,7 @@ impl VmCallerEnv for Host {
         m: MapObject,
         k: Val,
     ) -> Result<Val, HostError> {
+        self.check_val_integrity(k)?;
         self.visit_obj(m, move |hm: &HostMap| {
             hm.get(&k, self)?.copied().ok_or_else(|| {
                 self.err(
@@ -1634,6 +1638,7 @@ impl VmCallerEnv for Host {
         m: MapObject,
         k: Val,
     ) -> Result<MapObject, HostError> {
+        self.check_val_integrity(k)?;
         match self.visit_obj(m, |hm: &HostMap| hm.remove(&k, self))? {
             Some((mnew, _)) => Ok(self.add_host_object(mnew)?),
             None => Err(self.err(
@@ -1656,6 +1661,7 @@ impl VmCallerEnv for Host {
         m: MapObject,
         k: Val,
     ) -> Result<Bool, HostError> {
+        self.check_val_integrity(k)?;
         self.visit_obj(m, move |hm: &HostMap| Ok(hm.contains_key(&k, self)?.into()))
     }
 
@@ -1975,6 +1981,7 @@ impl VmCallerEnv for Host {
         v: VecObject,
         x: Val,
     ) -> Result<Val, Self::Error> {
+        self.check_val_integrity(x)?;
         self.visit_obj(v, |hv: &HostVec| {
             Ok(
                 match hv.first_index_of(|other| self.compare(&x, other), self.as_budget())? {
@@ -1991,6 +1998,7 @@ impl VmCallerEnv for Host {
         v: VecObject,
         x: Val,
     ) -> Result<Val, Self::Error> {
+        self.check_val_integrity(x)?;
         self.visit_obj(v, |hv: &HostVec| {
             Ok(
                 match hv.last_index_of(|other| self.compare(&x, other), self.as_budget())? {
@@ -2007,6 +2015,7 @@ impl VmCallerEnv for Host {
         v: VecObject,
         x: Val,
     ) -> Result<u64, Self::Error> {
+        self.check_val_integrity(x)?;
         self.visit_obj(v, |hv: &HostVec| {
             let res = hv.binary_search_by(|probe| self.compare(probe, &x), self.as_budget())?;
             self.u64_from_binary_search_result(res)
@@ -2089,6 +2098,7 @@ impl VmCallerEnv for Host {
         k: Val,
         t: StorageType,
     ) -> Result<Bool, HostError> {
+        self.check_val_integrity(k)?;
         let res = match t {
             StorageType::Temporary | StorageType::Persistent => {
                 let key = self.storage_key_from_rawval(k, t.try_into()?)?;
@@ -2111,6 +2121,7 @@ impl VmCallerEnv for Host {
         k: Val,
         t: StorageType,
     ) -> Result<Val, HostError> {
+        self.check_val_integrity(k)?;
         match t {
             StorageType::Temporary | StorageType::Persistent => {
                 let key = self.storage_key_from_rawval(k, t.try_into()?)?;
@@ -2151,6 +2162,7 @@ impl VmCallerEnv for Host {
         k: Val,
         t: StorageType,
     ) -> Result<Void, HostError> {
+        self.check_val_integrity(k)?;
         match t {
             StorageType::Temporary | StorageType::Persistent => {
                 let key = self.contract_data_key_from_rawval(k, t.try_into()?)?;
@@ -2180,6 +2192,7 @@ impl VmCallerEnv for Host {
         low_expiration_watermark: U32Val,
         high_expiration_watermark: U32Val,
     ) -> Result<Void, HostError> {
+        self.check_val_integrity(k)?;
         if matches!(t, StorageType::Instance) {
             return Err(self.err(
                 ScErrorType::Storage,
@@ -2448,6 +2461,7 @@ impl VmCallerEnv for Host {
         _vmcaller: &mut VmCaller<Host>,
         v: Val,
     ) -> Result<BytesObject, HostError> {
+        self.check_val_integrity(v)?;
         let scv = self.from_host_val(v)?;
         let mut buf = Vec::<u8>::new();
         metered_write_xdr(self.budget_ref(), &scv, &mut buf)?;
diff --git a/soroban-env-host/src/host_object.rs b/soroban-env-host/src/host_object.rs
index cbaf6c1d..d3b2b98e 100644
--- a/soroban-env-host/src/host_object.rs
+++ b/soroban-env-host/src/host_object.rs
@@ -379,10 +379,18 @@ impl Host {
     }
 
     pub(crate) fn check_val_integrity(&self, val: Val) -> Result<(), HostError> {
-        if let Ok(obj) = Object::try_from(val) {
-            self.check_obj_integrity(obj)?;
+        if val.get_tag() == Tag::Bad {
+            Err(self.err(
+                ScErrorType::Value,
+                ScErrorCode::InvalidInput,
+                "bad value tag",
+                &[val],
+            ))
+        } else if let Ok(obj) = Object::try_from(val) {
+            self.check_obj_integrity(obj)
+        } else {
+            Ok(())
         }
-        Ok(())
     }
 
     pub(crate) fn check_obj_integrity(&self, obj: Object) -> Result<(), HostError> {


Test patch:
diff --git a/soroban-env-host/src/test/budget_metering.rs b/soroban-env-host/src/test/budget_metering.rs
index 9e3c379c..6ec649b0 100644
--- a/soroban-env-host/src/test/budget_metering.rs
+++ b/soroban-env-host/src/test/budget_metering.rs
@@ -229,10 +229,16 @@ fn map_insert_key_vec_obj() -> Result<(), HostError> {
     host.map_put(m, k1.into(), v1)?;
 
     host.with_budget(|budget| {
-        // 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);
+        // 12 visit-objs =
+        //    1 to ensure value integrity of key for first map-put
+        //  + 1 to get map to do first map-put
+        //  + 1 to ensure value integrity of key for second map-put
+        //  + 1 to get map to do second map-put
+        //  + 2 to check integrity of k0 and k1 for obj_cmp during lookup
+        //  + 2 for actually doing comparison in obj_cmp
+        //  + 4 more to do same 2+2 visits when validating order of new map
+        // = 12
+        assert_eq!(budget.get_tracker(ContractCostType::VisitObject)?.0, 12);
         // 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/hostile.rs b/soroban-env-host/src/test/hostile.rs
index 956fb2a3..c193181e 100644
--- a/soroban-env-host/src/test/hostile.rs
+++ b/soroban-env-host/src/test/hostile.rs
@@ -189,6 +189,18 @@ fn hostile_forged_objects_trap() -> Result<(), HostError> {
         (ScErrorType::Object, ScErrorCode::UnexpectedType)
     ));
 
+    // Here we call a function that passes an argument to a host function
+    // with a bad val tag. This should fail in check_val_integrity.
+    let res = host.call(
+        contract_id_obj,
+        Symbol::try_from_small_str("badtag")?,
+        host.vec_new_from_slice(&[absolute_vec.to_val()])?,
+    );
+    assert!(HostError::result_matches_err(
+        res.clone(),
+        (ScErrorType::Value, ScErrorCode::InvalidInput)
+    ));
+
     Ok(())
 }
 
diff --git a/soroban-test-wasms/wasm-workspace/Cargo.lock b/soroban-test-wasms/wasm-workspace/Cargo.lock
index aa833d08..a17ac096 100644
--- a/soroban-test-wasms/wasm-workspace/Cargo.lock
+++ b/soroban-test-wasms/wasm-workspace/Cargo.lock
@@ -1043,7 +1043,7 @@ dependencies = [
  "soroban-env-macros",
  "soroban-wasmi",
  "static_assertions",
- "stellar-xdr 0.0.17 (git+https://github.com/stellar/rs-stellar-xdr?rev=936273b737b99d79eee7a28dd4823436d0bd0abb)",
+ "stellar-xdr",
 ]
 
 [[package]]
@@ -1059,12 +1059,9 @@ name = "soroban-env-host"
 version = "0.0.17"
 dependencies = [
  "backtrace",
- "curve25519-dalek",
  "ed25519-dalek",
  "getrandom",
- "hex",
  "k256",
- "log",
  "num-derive",
  "num-integer",
  "num-traits",
@@ -1088,7 +1085,7 @@ dependencies = [
  "quote",
  "serde",
  "serde_json",
- "stellar-xdr 0.0.17 (git+https://github.com/stellar/rs-stellar-xdr?rev=936273b737b99d79eee7a28dd4823436d0bd0abb)",
+ "stellar-xdr",
  "syn",
 ]
 
@@ -1143,7 +1140,7 @@ dependencies = [
  "soroban-env-common",
  "soroban-spec",
  "soroban-spec-rust",
- "stellar-xdr 0.0.17 (git+https://github.com/stellar/rs-stellar-xdr?rev=e2a9cbf72d94941de1bde6ba34a38e1f49328567)",
+ "stellar-xdr",
  "syn",
 ]
 
@@ -1152,7 +1149,7 @@ name = "soroban-spec"
 version = "0.9.2"
 dependencies = [
  "base64 0.13.1",
- "stellar-xdr 0.0.17 (git+https://github.com/stellar/rs-stellar-xdr?rev=e2a9cbf72d94941de1bde6ba34a38e1f49328567)",
+ "stellar-xdr",
  "thiserror",
  "wasmparser",
 ]
@@ -1166,7 +1163,7 @@ dependencies = [
  "quote",
  "sha2",
  "soroban-spec",
- "stellar-xdr 0.0.17 (git+https://github.com/stellar/rs-stellar-xdr?rev=e2a9cbf72d94941de1bde6ba34a38e1f49328567)",
+ "stellar-xdr",
  "syn",
  "thiserror",
 ]
@@ -1224,7 +1221,7 @@ dependencies = [
 [[package]]
 name = "stellar-xdr"
 version = "0.0.17"
-source = "git+https://github.com/stellar/rs-stellar-xdr?rev=936273b737b99d79eee7a28dd4823436d0bd0abb#936273b737b99d79eee7a28dd4823436d0bd0abb"
+source = "git+https://github.com/stellar/rs-stellar-xdr?rev=39904e09941046dab61e6e35fc89e31bf2dea1cd#39904e09941046dab61e6e35fc89e31bf2dea1cd"
 dependencies = [
  "arbitrary",
  "base64 0.13.1",
@@ -1234,17 +1231,6 @@ dependencies = [
  "serde_with",
 ]
 
-[[package]]
-name = "stellar-xdr"
-version = "0.0.17"
-source = "git+https://github.com/stellar/rs-stellar-xdr?rev=e2a9cbf72d94941de1bde6ba34a38e1f49328567#e2a9cbf72d94941de1bde6ba34a38e1f49328567"
-dependencies = [
- "crate-git-revision",
- "hex",
- "serde",
- "serde_with",
-]
-
 [[package]]
 name = "strsim"
 version = "0.10.0"
diff --git a/soroban-test-wasms/wasm-workspace/hostile/src/lib.rs b/soroban-test-wasms/wasm-workspace/hostile/src/lib.rs
index 703fe8ec..5aa3093b 100644
--- a/soroban-test-wasms/wasm-workspace/hostile/src/lib.rs
+++ b/soroban-test-wasms/wasm-workspace/hostile/src/lib.rs
@@ -83,4 +83,11 @@ impl Contract {
         );
         v.push_back(b)
     }
+
+    // Pass a value with a bad tag.
+    #[allow(unused_mut)]
+    pub fn badtag(_env: Env, mut v: Vec<Val>) {
+        let bad = Val::from_payload((v.to_val().get_payload() & !0xff) | 32);
+        v.push_back(bad)
+    }
 }