Merge pull request from GHSA-xqqc-c5gw-c5r5

* Add the is_matching_chain_id() predicate * Add TrustedBlockState::chain_id field * Implement matching chain-id check in verifier * Add test * Add test for verifier * Bump light client verifier and dependent crates to v0.28.0-pre.1 Signed-off-by: Thane Thomson <[email protected]> * Bump kvstore test light client dependency Signed-off-by: Thane Thomson <[email protected]> * Bump version to v0.28.0 Signed-off-by: Thane Thomson <[email protected]> * Add changelog entries for security fix Signed-off-by: Thane Thomson <[email protected]> * Prepare v0.28.0 release changelog Signed-off-by: Thane Thomson <[email protected]> * Rebuild changelog Signed-off-by: Thane Thomson <[email protected]> * Update release date in changelog Signed-off-by: Thane Thomson <[email protected]> Signed-off-by: Thane Thomson <[email protected]> Co-authored-by: Thane Thomson <[email protected]>
informalsystems · Dec 14, 2022 · 5c32f31 · 5c32f31
1 parent 60d003b
commit 5c32f31
Show file tree

Hide file tree

Showing 30 changed files with 210 additions and 41 deletions.
diff --git a/.changelog/v0.28.0/breaking/1249-light-client-verification-preds.md b/.changelog/v0.28.0/breaking/1249-light-client-verification-preds.md
@@ -0,0 +1,3 @@
+- `[tendermint-light-client-verifier]` Add `is_matching_chain_id`
+  method to the `VerificationPredicates` trait
+  ([#1249](https://github.com/informalsystems/tendermint-rs/pull/1249))
diff --git a/.changelog/v0.28.0/breaking/1249-light-client-verifier.md b/.changelog/v0.28.0/breaking/1249-light-client-verifier.md
@@ -0,0 +1,3 @@
+- `[tendermint-light-client-verifier]` Add a
+  `chain_id` field to the `TrustedBlockState` struct
+  ([#1249](https://github.com/informalsystems/tendermint-rs/pull/1249))
diff --git a/...d/improvements/1242-serde-wasm-bindgen.md → ...0/improvements/1242-serde-wasm-bindgen.md b/...d/improvements/1242-serde-wasm-bindgen.md → ...0/improvements/1242-serde-wasm-bindgen.md
diff --git a/.changelog/v0.28.0/security/1249-light-client-security.md b/.changelog/v0.28.0/security/1249-light-client-security.md
@@ -0,0 +1,3 @@
+- `[tendermint-light-client]` Fix an issue where the light client was not
+  checking that the chain ID of the trusted and untrusted headers match
+  ([#1249](https://github.com/informalsystems/tendermint-rs/pull/1249))
diff --git a/.changelog/v0.28.0/summary.md b/.changelog/v0.28.0/summary.md
@@ -0,0 +1,7 @@
+*Dec 13, 2022*
+
+This is primarily a security-related release, and although it's a breaking
+release, the breaking changes are relatively minor.
+
+It is highly recommended that all tendermint-rs light client users upgrade to
+this version immediately.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,5 +1,35 @@
 # CHANGELOG
 
+## v0.28.0
+
+*Dec 13, 2022*
+
+This is primarily a security-related release, and although it's a breaking
+release, the breaking changes are relatively minor.
+
+It is highly recommended that all tendermint-rs light client users upgrade to
+this version immediately.
+
+### BREAKING
+
+- `[tendermint-light-client-verifier]` Add a
+  `chain_id` field to the `TrustedBlockState` struct
+  ([#1249](https://github.com/informalsystems/tendermint-rs/pull/1249))
+- `[tendermint-light-client-verifier]` Add `is_matching_chain_id`
+  method to the `VerificationPredicates` trait
+  ([#1249](https://github.com/informalsystems/tendermint-rs/pull/1249))
+
+### IMPROVEMENTS
+
+- `[tendermint-light-client-js]` Switch to serde-wasm-bindgen for marshalling
+  JS values ([#1242](https://github.com/informalsystems/tendermint-rs/pull/1242))
+
+### SECURITY
+
+- `[tendermint-light-client]` Fix an issue where the light client was not
+  checking that the chain ID of the trusted and untrusted headers match
+  ([#1249](https://github.com/informalsystems/tendermint-rs/pull/1249))
+
 ## v0.27.0
 
 *Nov 28, 2022*

diff --git a/abci/Cargo.toml b/abci/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name        = "tendermint-abci"
-version     = "0.27.0"
+version     = "0.28.0"
 authors     = ["Informal Systems <[email protected]>"]
 edition     = "2018"
 license     = "Apache-2.0"
@@ -33,7 +33,7 @@ binary = [
 [dependencies]
 bytes = { version = "1.0", default-features = false }
 prost = { version = "0.11", default-features = false }
-tendermint-proto = { version = "0.27.0", default-features = false, path = "../proto" }
+tendermint-proto = { version = "0.28.0", default-features = false, path = "../proto" }
 tracing = { version = "0.1", default-features = false }
 flex-error = { version = "0.4.4", default-features = false }
 structopt = { version = "0.3", optional = true, default-features = false }

diff --git a/config/Cargo.toml b/config/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name       = "tendermint-config"
-version    = "0.27.0" # Also update `html_root_url` in lib.rs and
+version    = "0.28.0" # Also update `html_root_url` in lib.rs and
                                # depending crates (rpc, light-node, ..) when bumping this
 license    = "Apache-2.0"
 homepage   = "https://www.tendermint.com/"
@@ -25,7 +25,7 @@ all-features = true
 rustdoc-args = ["--cfg", "docsrs"]
 
 [dependencies]
-tendermint = { version = "0.27.0", default-features = false, path = "../tendermint" }
+tendermint = { version = "0.28.0", default-features = false, path = "../tendermint" }
 flex-error = { version = "0.4.4", default-features = false }
 serde = { version = "1", features = ["derive"] }
 serde_json = "1"

diff --git a/light-client-js/Cargo.toml b/light-client-js/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name        = "tendermint-light-client-js"
-version     = "0.27.0"
+version     = "0.28.0"
 authors     = ["Informal Systems <[email protected]>"]
 edition     = "2018"
 license     = "Apache-2.0"
@@ -22,8 +22,8 @@ default = ["console_error_panic_hook"]
 [dependencies]
 serde = { version = "1.0", default-features = false, features = [ "derive" ] }
 serde_json = { version = "1.0", default-features = false }
-tendermint = { version = "0.27.0", default-features = false, path = "../tendermint" }
-tendermint-light-client-verifier = { version = "0.27.0", default-features = false, path = "../light-client-verifier" }
+tendermint = { version = "0.28.0", default-features = false, path = "../tendermint" }
+tendermint-light-client-verifier = { version = "0.28.0", default-features = false, path = "../light-client-verifier" }
 wasm-bindgen = { version = "0.2.63", default-features = false, features = [ "serde-serialize" ] }
 serde-wasm-bindgen = { version = "0.4.5", default-features = false }
 

diff --git a/light-client-verifier/Cargo.toml b/light-client-verifier/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name       = "tendermint-light-client-verifier"
-version    = "0.27.0"
+version    = "0.28.0"
 edition    = "2021"
 license    = "Apache-2.0"
 readme     = "README.md"
@@ -26,7 +26,7 @@ rustdoc-args = ["--cfg", "docsrs"]
 default = ["flex-error/std", "flex-error/eyre_tracer"]
 
 [dependencies]
-tendermint = { version = "0.27.0", path = "../tendermint", default-features = false }
+tendermint = { version = "0.28.0", path = "../tendermint", default-features = false }
 
 derive_more = { version = "0.99.5", default-features = false, features = ["display"] }
 serde = { version = "1.0.106", default-features = false }

diff --git a/light-client-verifier/src/errors.rs b/light-client-verifier/src/errors.rs
@@ -112,6 +112,16 @@ define_error! {
                     e.got, e.expected)
             },
 
+        ChainIdMismatch
+            {
+                got: String,
+                expected: String,
+            }
+            | e | {
+                format_args!("chain-id mismatch: got={0} expected={1}",
+                    e.got, e.expected)
+            },
+
         NonMonotonicBftTime
             {
                 header_bft_time: Time,

diff --git a/light-client-verifier/src/predicates.rs b/light-client-verifier/src/predicates.rs
@@ -2,7 +2,7 @@
 
 use core::time::Duration;
 
-use tendermint::{block::Height, hash::Hash};
+use tendermint::{block::Height, chain::Id as ChainId, hash::Hash};
 
 use crate::{
     errors::VerificationError,
@@ -160,6 +160,22 @@ pub trait VerificationPredicates: Send + Sync {
         }
     }
 
+    /// Check that the chain-ids of the trusted header and the untrusted one are the same
+    fn is_matching_chain_id(
+        &self,
+        untrusted_chain_id: &ChainId,
+        trusted_chain_id: &ChainId,
+    ) -> Result<(), VerificationError> {
+        if untrusted_chain_id == trusted_chain_id {
+            Ok(())
+        } else {
+            Err(VerificationError::chain_id_mismatch(
+                untrusted_chain_id.to_string(),
+                trusted_chain_id.to_string(),
+            ))
+        }
+    }
+
     /// Check that there is enough validators overlap between the trusted validator set
     /// and the untrusted signed header.
     fn has_sufficient_validators_overlap(
@@ -282,6 +298,30 @@ mod tests {
         }
     }
 
+    #[test]
+    fn test_is_matching_chain_id() {
+        let val = vec![Validator::new("val-1")];
+        let header_one = Header::new(&val).chain_id("chaina-1").generate().unwrap();
+        let header_two = Header::new(&val).chain_id("chainb-1").generate().unwrap();
+
+        let vp = ProdPredicates::default();
+
+        // 1. ensure valid header verifies
+        let result_ok = vp.is_matching_chain_id(&header_one.chain_id, &header_one.chain_id);
+        assert!(result_ok.is_ok());
+
+        // 2. ensure header with different chain-id fails
+        let result_err = vp.is_matching_chain_id(&header_one.chain_id, &header_two.chain_id);
+
+        match result_err {
+            Err(VerificationError(VerificationErrorDetail::ChainIdMismatch(e), _)) => {
+                assert_eq!(e.got, header_one.chain_id.to_string());
+                assert_eq!(e.expected, header_two.chain_id.to_string());
+            },
+            _ => panic!("expected ChainIdMismatch error"),
+        }
+    }
+
     #[test]
     fn test_is_within_trust_period() {
         let val = Validator::new("val-1");

diff --git a/light-client-verifier/src/types.rs b/light-client-verifier/src/types.rs
@@ -8,6 +8,7 @@ use tendermint::{
         header::Header as TMHeader, signed_header::SignedHeader as TMSignedHeader,
         Commit as TMCommit,
     },
+    chain::Id as ChainId,
     trust_threshold::TrustThresholdFraction,
     validator::{Info as TMValidatorInfo, Set as TMValidatorSet},
 };
@@ -78,6 +79,7 @@ impl Status {
 
 /// Trusted block parameters needed for light client verification.
 pub struct TrustedBlockState<'a> {
+    pub chain_id: &'a ChainId,
     pub header_time: Time,
     pub height: Height,
     pub next_validators: &'a ValidatorSet,
@@ -143,6 +145,7 @@ impl LightBlock {
     /// trusted state.
     pub fn as_trusted_state(&self) -> TrustedBlockState<'_> {
         TrustedBlockState {
+            chain_id: &self.signed_header.header.chain_id,
             header_time: self.signed_header.header.time,
             height: self.signed_header.header.height,
             next_validators: &self.next_validators,

diff --git a/light-client-verifier/src/verifier.rs b/light-client-verifier/src/verifier.rs
@@ -195,6 +195,11 @@ where
             .predicates
             .is_monotonic_bft_time(untrusted.signed_header.header.time, trusted.header_time));
 
+        // Check that the chain-id of the untrusted block matches that of the trusted state
+        verdict!(self
+            .predicates
+            .is_matching_chain_id(&untrusted.signed_header.header.chain_id, trusted.chain_id));
+
         let trusted_next_height = trusted.height.increment();
 
         if untrusted.height() == trusted_next_height {
@@ -287,3 +292,68 @@ where
 /// The default production implementation of the [`PredicateVerifier`].
 pub type ProdVerifier =
     PredicateVerifier<ProdPredicates, ProdVotingPowerCalculator, ProdCommitValidator, ProdHasher>;
+
+#[cfg(test)]
+mod tests {
+    use alloc::{borrow::ToOwned, string::ToString};
+    use core::{ops::Sub, time::Duration};
+
+    use tendermint::Time;
+    use tendermint_testgen::{light_block::LightBlock as TestgenLightBlock, Generator};
+
+    use crate::{
+        errors::VerificationErrorDetail, options::Options, types::LightBlock, ProdVerifier,
+        Verdict, Verifier,
+    };
+
+    #[test]
+    fn test_verification_failure_on_chain_id_mismatch() {
+        let now = Time::now();
+
+        // Create a default light block with a valid chain-id for height `1` with a timestamp 20
+        // secs before now (to be treated as trusted state)
+        let light_block_1: LightBlock = TestgenLightBlock::new_default_with_time_and_chain_id(
+            "chain-1".to_owned(),
+            now.sub(Duration::from_secs(20)).unwrap(),
+            1u64,
+        )
+        .generate()
+        .unwrap()
+        .into();
+
+        // Create another default block with a different chain-id for height `2` with a timestamp 10
+        // secs before now (to be treated as untrusted state)
+        let light_block_2: LightBlock = TestgenLightBlock::new_default_with_time_and_chain_id(
+            "forged-chain".to_owned(),
+            now.sub(Duration::from_secs(10)).unwrap(),
+            2u64,
+        )
+        .generate()
+        .unwrap()
+        .into();
+
+        let vp = ProdVerifier::default();
+        let opt = Options {
+            trust_threshold: Default::default(),
+            trusting_period: Duration::from_secs(60),
+            clock_drift: Default::default(),
+        };
+
+        let verdict = vp.verify(
+            light_block_2.as_untrusted_state(),
+            light_block_1.as_trusted_state(),
+            &opt,
+            Time::now(),
+        );
+
+        match verdict {
+            Verdict::Invalid(VerificationErrorDetail::ChainIdMismatch(e)) => {
+                let chain_id_1 = light_block_1.signed_header.header.chain_id;
+                let chain_id_2 = light_block_2.signed_header.header.chain_id;
+                assert_eq!(e.got, chain_id_2.to_string());
+                assert_eq!(e.expected, chain_id_1.to_string());
+            },
+            v => panic!("expected ChainIdMismatch error, got: {:?}", v),
+        }
+    }
+}
diff --git a/light-client/Cargo.toml b/light-client/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name       = "tendermint-light-client"
-version    = "0.27.0"
+version    = "0.28.0"
 edition    = "2018"
 license    = "Apache-2.0"
 readme     = "README.md"
@@ -34,9 +34,9 @@ unstable = []
 mbt = []
 
 [dependencies]
-tendermint = { version = "0.27.0", path = "../tendermint", default-features = false }
-tendermint-rpc = { version = "0.27.0", path = "../rpc", default-features = false }
-tendermint-light-client-verifier = { version = "0.27.0", path = "../light-client-verifier", default-features = false }
+tendermint = { version = "0.28.0", path = "../tendermint", default-features = false }
+tendermint-rpc = { version = "0.28.0", path = "../rpc", default-features = false }
+tendermint-light-client-verifier = { version = "0.28.0", path = "../light-client-verifier", default-features = false }
 
 contracts = { version = "0.6.2", default-features = false }
 crossbeam-channel = { version = "0.4.2", default-features = false }

diff --git a/light-client/src/lib.rs b/light-client/src/lib.rs
@@ -9,7 +9,7 @@
     nonstandard_style
 )]
 #![doc(
-    html_root_url = "https://docs.rs/tendermint-light-client/0.27.0",
+    html_root_url = "https://docs.rs/tendermint-light-client/0.28.0",
     html_logo_url = "https://raw.githubusercontent.com/informalsystems/tendermint-rs/master/img/logo-tendermint-rs_3961x4001.png"
 )]
 #![cfg_attr(docsrs, feature(doc_cfg))]

diff --git a/p2p/Cargo.toml b/p2p/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name        = "tendermint-p2p"
-version     = "0.27.0"
+version     = "0.28.0"
 edition     = "2018"
 license     = "Apache-2.0"
 repository  = "https://github.com/informalsystems/tendermint-rs"
@@ -44,9 +44,9 @@ aead = { version = "0.4.1", default-features = false }
 flex-error = { version = "0.4.4", default-features = false }
 
 # path dependencies
-tendermint = { path = "../tendermint", version = "0.27.0", default-features = false }
-tendermint-proto = { path = "../proto", version = "0.27.0", default-features = false }
-tendermint-std-ext = { path = "../std-ext", version = "0.27.0", default-features = false }
+tendermint = { path = "../tendermint", version = "0.28.0", default-features = false }
+tendermint-proto = { path = "../proto", version = "0.28.0", default-features = false }
+tendermint-std-ext = { path = "../std-ext", version = "0.28.0", default-features = false }
 
 # optional dependencies
 prost-derive = { version = "0.11", optional = true }
diff --git a/p2p/src/lib.rs b/p2p/src/lib.rs
@@ -20,7 +20,7 @@
     unused_qualifications
 )]
 #![doc(
-    html_root_url = "https://docs.rs/tendermint-p2p/0.27.0",
+    html_root_url = "https://docs.rs/tendermint-p2p/0.28.0",
     html_logo_url = "https://raw.githubusercontent.com/informalsystems/tendermint-rs/master/img/logo-tendermint-rs_3961x4001.png"
 )]
 

diff --git a/pbt-gen/Cargo.toml b/pbt-gen/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "tendermint-pbt-gen"
-version = "0.27.0"
+version = "0.28.0"
 authors = ["Informal Systems <[email protected]>"]
 edition = "2018"
 license     = "Apache-2.0"

diff --git a/proto/Cargo.toml b/proto/Cargo.toml
@@ -1,6 +1,6 @@
 [package]
 name = "tendermint-proto"
-version = "0.27.0"
+version = "0.28.0"
 authors = ["Informal Systems <[email protected]>"]
 edition = "2018"
 license = "Apache-2.0"

diff --git a/proto/src/lib.rs b/proto/src/lib.rs
@@ -4,7 +4,7 @@
 #![deny(warnings, trivial_casts, trivial_numeric_casts, unused_import_braces)]
 #![allow(clippy::large_enum_variant)]
 #![forbid(unsafe_code)]
-#![doc(html_root_url = "https://docs.rs/tendermint-proto/0.27.0")]
+#![doc(html_root_url = "https://docs.rs/tendermint-proto/0.28.0")]
 
 extern crate alloc;