From 9e48f176339108c4caf27821eb584fd0fa46c2e8 Mon Sep 17 00:00:00 2001 From: Romain Ruetschi Date: Thu, 20 Apr 2023 10:45:24 +0200 Subject: [PATCH] Add `Verifier::verify_misbehaviour_header` for verifying headers coming from a misbehaviour evidence (#1300) * Add `Verifier::verify_misbehaviour_header` for verifying headers coming from a misbehaviour evidence * Add changelog entry * Address review comments * Call `validate_against_trusted` directly to better see correspondance with `verify` * Rename `verify` to `verify_update_header` * Add another changelog entry --- .../1294-verify-update-header.md | 4 + .../1294-verify-misbehavior-header.md | 5 ++ light-client-js/src/lib.rs | 2 +- light-client-verifier/src/verifier.rs | 74 ++++++++++++++----- light-client/src/light_client.rs | 2 +- light-client/src/tests.rs | 2 +- 6 files changed, 69 insertions(+), 20 deletions(-) create mode 100644 .changelog/unreleased/breaking-changes/1294-verify-update-header.md create mode 100644 .changelog/unreleased/features/1294-verify-misbehavior-header.md diff --git a/.changelog/unreleased/breaking-changes/1294-verify-update-header.md b/.changelog/unreleased/breaking-changes/1294-verify-update-header.md new file mode 100644 index 000000000..38a7b8372 --- /dev/null +++ b/.changelog/unreleased/breaking-changes/1294-verify-update-header.md @@ -0,0 +1,4 @@ +- [`tendermint-light-client-verifier`] Rename `Verifier::verify` + to `Verifier::verify_update_header` to better describe + its purpose versus `Verifier::verify_misbehaviour_header` + ([\#1294](https://github.com/informalsystems/tendermint-rs/issues/1294)) \ No newline at end of file diff --git a/.changelog/unreleased/features/1294-verify-misbehavior-header.md b/.changelog/unreleased/features/1294-verify-misbehavior-header.md new file mode 100644 index 000000000..dd8408e42 --- /dev/null +++ b/.changelog/unreleased/features/1294-verify-misbehavior-header.md @@ -0,0 +1,5 @@ +- [`tendermint-light-client-verifier`] Add `Verifier::verify_misbehaviour_header` + for verifying headers coming from a misbehaviour evidence. + The verification for these headers is a bit more relaxed in order to catch FLA attacks. + In particular the "header in the future" check for the header should be skipped. + ([\#1294](https://github.com/informalsystems/tendermint-rs/issues/1294)) diff --git a/light-client-js/src/lib.rs b/light-client-js/src/lib.rs index 8b96295ba..38068f39c 100644 --- a/light-client-js/src/lib.rs +++ b/light-client-js/src/lib.rs @@ -32,7 +32,7 @@ pub fn verify(untrusted: JsValue, trusted: JsValue, options: JsValue, now: JsVal let result = deserialize_params(untrusted, trusted, options, now).map( |(untrusted, trusted, options, now)| { let verifier = ProdVerifier::default(); - verifier.verify( + verifier.verify_update_header( untrusted.as_untrusted_state(), trusted.as_trusted_state(), &options, diff --git a/light-client-verifier/src/verifier.rs b/light-client-verifier/src/verifier.rs index efd653bd0..fd2e8826c 100644 --- a/light-client-verifier/src/verifier.rs +++ b/light-client-verifier/src/verifier.rs @@ -51,8 +51,20 @@ impl From> for Verdict { /// - [TMBC-VAL-CONTAINS-CORR.1] /// - [TMBC-VAL-COMMIT.1] pub trait Verifier: Send + Sync { - /// Perform the verification. - fn verify( + /// Verify a header received in a `MsgUpdateClient`. + fn verify_update_header( + &self, + untrusted: UntrustedBlockState<'_>, + trusted: TrustedBlockState<'_>, + options: &Options, + now: Time, + ) -> Verdict; + + /// Verify a header received in `MsgSubmitMisbehaviour`. + /// The verification for these headers is a bit more relaxed in order to catch FLA attacks. + /// In particular the "header in the future" check for the header should be skipped + /// from `validate_against_trusted`. + fn verify_misbehaviour_header( &self, untrusted: UntrustedBlockState<'_>, trusted: TrustedBlockState<'_>, @@ -62,21 +74,21 @@ pub trait Verifier: Send + Sync { } macro_rules! verdict { - ($e:expr) => { + ($e:expr) => {{ let result = $e; if result.is_err() { return result.into(); } - }; + }}; } macro_rules! ensure_verdict_success { - ($e:expr) => { + ($e:expr) => {{ let verdict = $e; if !matches!(verdict, Verdict::Success) { return verdict; } - }; + }}; } /// Predicate verifier encapsulating components necessary to facilitate @@ -146,8 +158,8 @@ where Verdict::Success } - /// Validate an `UntrustedBlockState`, based on the given `TrustedBlockState`, `Options` and - /// current time. + /// Validate an `UntrustedBlockState` coming from a client update, + /// based on the given `TrustedBlockState`, `Options` and current time. pub fn validate_against_trusted( &self, untrusted: &UntrustedBlockState<'_>, @@ -162,13 +174,6 @@ where now, )); - // Ensure the header isn't from a future time - verdict!(self.predicates.is_header_from_past( - untrusted.signed_header.header.time, - options.clock_drift, - now, - )); - // Check that the untrusted block is more recent than the trusted state verdict!(self .predicates @@ -199,6 +204,22 @@ where Verdict::Success } + /// Ensure the header isn't from a future time + pub fn check_header_is_from_past( + &self, + untrusted: &UntrustedBlockState<'_>, + options: &Options, + now: Time, + ) -> Verdict { + verdict!(self.predicates.is_header_from_past( + untrusted.signed_header.header.time, + options.clock_drift, + now, + )); + + Verdict::Success + } + /// Check there is enough overlap between the validator sets of the trusted and untrusted /// blocks. pub fn verify_commit_against_trusted( @@ -257,7 +278,26 @@ where /// `trusted.next_validators.hash() == trusted.next_validators_hash`, /// as typically the `trusted.next_validators` validator set comes from the relayer, /// and `trusted.next_validators_hash` is the hash stored on chain. - fn verify( + fn verify_update_header( + &self, + untrusted: UntrustedBlockState<'_>, + trusted: TrustedBlockState<'_>, + options: &Options, + now: Time, + ) -> Verdict { + ensure_verdict_success!(self.verify_validator_sets(&untrusted)); + ensure_verdict_success!(self.validate_against_trusted(&untrusted, &trusted, options, now)); + ensure_verdict_success!(self.check_header_is_from_past(&untrusted, options, now)); + ensure_verdict_success!(self.verify_commit_against_trusted(&untrusted, &trusted, options)); + ensure_verdict_success!(self.verify_commit(&untrusted)); + + Verdict::Success + } + + /// Verify a header received in `MsgSubmitMisbehaviour`. + /// The verification for these headers is a bit more relaxed in order to catch FLA attacks. + /// In particular the "header in the future" check for the header should be skipped. + fn verify_misbehaviour_header( &self, untrusted: UntrustedBlockState<'_>, trusted: TrustedBlockState<'_>, @@ -329,7 +369,7 @@ mod tests { clock_drift: Default::default(), }; - let verdict = vp.verify( + let verdict = vp.verify_update_header( light_block_2.as_untrusted_state(), light_block_1.as_trusted_state(), &opt, diff --git a/light-client/src/light_client.rs b/light-client/src/light_client.rs index 3b28a57a1..e97f5d948 100644 --- a/light-client/src/light_client.rs +++ b/light-client/src/light_client.rs @@ -211,7 +211,7 @@ impl LightClient { let (current_block, status) = self.get_or_fetch_block(current_height, state)?; // Validate and verify the current block - let verdict = self.verifier.verify( + let verdict = self.verifier.verify_update_header( current_block.as_untrusted_state(), trusted_block.as_trusted_state(), &self.options, diff --git a/light-client/src/tests.rs b/light-client/src/tests.rs index c9382f02c..7818483c8 100644 --- a/light-client/src/tests.rs +++ b/light-client/src/tests.rs @@ -169,7 +169,7 @@ pub fn verify_single( clock_drift, }; - let result = verifier.verify( + let result = verifier.verify_update_header( input.as_untrusted_state(), trusted_block.as_trusted_state(), &options,