diff --git a/rs/rosetta-api/icp/src/request_handler/construction_parse.rs b/rs/rosetta-api/icp/src/request_handler/construction_parse.rs index 0e8be5d6438c..6d27428c96a9 100644 --- a/rs/rosetta-api/icp/src/request_handler/construction_parse.rs +++ b/rs/rosetta-api/icp/src/request_handler/construction_parse.rs @@ -787,8 +787,12 @@ mod tests { .duration_since(SystemTime::UNIX_EPOCH) .expect("Could not get system time") .as_nanos() as u64; + // `ingress_start` is kept within a realistic window around `now`: the + // ingress window is now bounded to 24h, and this + // also avoids overflowing `ingress_start + ingress_interval` below. + let gen_ingress_start = proptest::option::of(now..(now + ONE_HOUR_NANOS)); let gen_metadata = proptest::option::of( - (gen_opt_u64.clone(), gen_opt_u64.clone(), gen_opt_u64).prop_flat_map( + (gen_opt_u64.clone(), gen_ingress_start, gen_opt_u64).prop_flat_map( |(created_at_time, ingress_start, memo)| { proptest::option::of(1..ONE_HOUR_NANOS).prop_map(move |ingress_interval| { let ingress_end = ingress_interval.map(|ingress_interval| { diff --git a/rs/rosetta-api/icp/src/request_handler/construction_payloads.rs b/rs/rosetta-api/icp/src/request_handler/construction_payloads.rs index a160a6fe0634..986b027d2598 100644 --- a/rs/rosetta-api/icp/src/request_handler/construction_payloads.rs +++ b/rs/rosetta-api/icp/src/request_handler/construction_payloads.rs @@ -34,6 +34,50 @@ use rosetta_core::convert::principal_id_from_public_key; use std::{collections::HashMap, sync::Arc, time::Duration}; use tracing::log::debug; +/// Maximum permitted ingress window, mirroring the 24-hour bound documented on +/// `ConstructionPayloadsRequestMetadata`. +const MAX_INGRESS_WINDOW: Duration = Duration::from_secs(24 * 60 * 60); + +/// Validate the caller-provided ingress window before it is expanded into a +/// list of ingress expiries. +/// +/// Rejects windows that would make the expiry loop iterate an excessive (or, on +/// arithmetic wraparound, unbounded) number of times. It enforces that: +/// - `ingress_start` is strictly before `ingress_end` (an empty or reversed +/// window would otherwise produce an empty/degenerate set of payloads), +/// - the span `ingress_end - ingress_start` must not exceed 24h, and +/// - `ingress_end` must not be more than 24h in the future, which also rejects +/// the near-`u64::MAX` payloads that would otherwise wrap the loop counter. +fn validate_ingress_window( + now: ic_types::time::Time, + ingress_start: ic_types::time::Time, + ingress_end: ic_types::time::Time, +) -> Result<(), ApiError> { + let max_window = MAX_INGRESS_WINDOW.as_nanos() as u64; + let now = now.as_nanos_since_unix_epoch(); + let start = ingress_start.as_nanos_since_unix_epoch(); + let end = ingress_end.as_nanos_since_unix_epoch(); + + if start >= end { + return Err(ApiError::invalid_request( + "ingress_start must be strictly before ingress_end.", + )); + } + if end.saturating_sub(start) > max_window { + return Err(ApiError::invalid_request(format!( + "The ingress window (ingress_end - ingress_start) must not exceed {} hours.", + MAX_INGRESS_WINDOW.as_secs() / 3600 + ))); + } + if end.saturating_sub(now) > max_window { + return Err(ApiError::invalid_request(format!( + "ingress_end must not be more than {} hours in the future.", + MAX_INGRESS_WINDOW.as_secs() / 3600 + ))); + } + Ok(()) +} + impl RosettaRequestHandler { /// Generate an Unsigned Transaction and Signing Payloads. /// See https://www.rosetta-api.org/docs/ConstructionApi.html#constructionpayloads @@ -96,6 +140,8 @@ impl RosettaRequestHandler { .map(Memo) .unwrap_or_else(|| Memo(rand::thread_rng().r#gen())); + validate_ingress_window(ic_types::time::current_time(), ingress_start, ingress_end)?; + let mut ingress_expiries = vec![]; let mut now = ingress_start; while now < ingress_end { @@ -1110,3 +1156,70 @@ fn neuron_subaccount( } } } + +#[cfg(test)] +mod tests { + use super::*; + use ic_types::time::Time; + + const HOUR_NANOS: u64 = 60 * 60 * 1_000_000_000; + // A realistic "now" (~2023) that is well away from the u64 boundary. + const NOW_NANOS: u64 = 1_700_000_000 * 1_000_000_000; + + fn t(nanos: u64) -> Time { + Time::from_nanos_since_unix_epoch(nanos) + } + + // A window whose span exceeds the documented 24h bound would make the loop + // build a huge list of ingress expiries, so it is rejected before the loop. + #[test] + fn oversized_ingress_window_is_rejected() { + assert!( + validate_ingress_window(t(NOW_NANOS), t(NOW_NANOS), t(NOW_NANOS + 48 * HOUR_NANOS)) + .is_err() + ); + } + + // A near-zero start together with a near-`u64::MAX` end spans almost the + // entire u64 range; that enormous span is rejected (it would otherwise + // iterate billions of times). + #[test] + fn unbounded_ingress_span_is_rejected() { + assert!(validate_ingress_window(t(NOW_NANOS), t(0), t(u64::MAX)).is_err()); + } + + // A window ending at (near) `u64::MAX` has a small span but an end far in the + // future; without the future bound the loop counter would wrap past + // `u64::MAX` and never terminate. The future bound rejects it. + #[test] + fn near_u64_max_ingress_window_is_rejected() { + let start = t(u64::MAX - 50 * 1_000_000_000); + assert!(validate_ingress_window(t(NOW_NANOS), start, t(u64::MAX)).is_err()); + } + + // A reversed or empty window (ingress_end <= ingress_start) is rejected + // rather than silently producing an empty set of payloads. + #[test] + fn reversed_ingress_window_is_rejected() { + assert!(validate_ingress_window(t(NOW_NANOS), t(NOW_NANOS + 1), t(NOW_NANOS)).is_err()); + assert!(validate_ingress_window(t(NOW_NANOS), t(NOW_NANOS), t(NOW_NANOS)).is_err()); + } + + // Negative control: a realistic window (5 minutes ahead of now) is accepted. + #[test] + fn valid_ingress_window_is_accepted() { + let end = t(NOW_NANOS + 5 * 60 * 1_000_000_000); + assert!(validate_ingress_window(t(NOW_NANOS), t(NOW_NANOS), end).is_ok()); + } + + // Boundary: a window of exactly 24h ending exactly 24h from now is accepted, + // one nanosecond more is rejected. + #[test] + fn ingress_window_boundary_is_inclusive() { + let max = MAX_INGRESS_WINDOW.as_nanos() as u64; + assert!(validate_ingress_window(t(NOW_NANOS), t(NOW_NANOS), t(NOW_NANOS + max)).is_ok()); + assert!( + validate_ingress_window(t(NOW_NANOS), t(NOW_NANOS), t(NOW_NANOS + max + 1)).is_err() + ); + } +} diff --git a/rs/rosetta-api/icrc1/src/common/constants.rs b/rs/rosetta-api/icrc1/src/common/constants.rs index d29f5e884eed..dc4a73a98c31 100644 --- a/rs/rosetta-api/icrc1/src/common/constants.rs +++ b/rs/rosetta-api/icrc1/src/common/constants.rs @@ -17,5 +17,8 @@ pub const SPENDER_OPERATION_IDENTIFIER: u64 = 7; pub const FEE_COLLECTOR_OPERATION_IDENTIFIER: u64 = 8; pub const MAX_TRANSACTIONS_PER_SEARCH_TRANSACTIONS_REQUEST: u64 = 10000; pub const INGRESS_INTERVAL_OVERLAP: Duration = Duration::from_secs(120); +/// Maximum permitted ingress window, mirroring the 24-hour bound documented on +/// `ConstructionPayloadsRequestMetadata`. +pub const MAX_INGRESS_WINDOW: Duration = Duration::from_secs(24 * 60 * 60); pub const STATUS_COMPLETED: &str = "COMPLETED"; pub const MAX_BLOCKS_PER_QUERY_BLOCK_RANGE_REQUEST: u64 = 10000; diff --git a/rs/rosetta-api/icrc1/src/construction_api/services.rs b/rs/rosetta-api/icrc1/src/construction_api/services.rs index 2c4a1c29c294..ef2448c46328 100644 --- a/rs/rosetta-api/icrc1/src/construction_api/services.rs +++ b/rs/rosetta-api/icrc1/src/construction_api/services.rs @@ -7,7 +7,7 @@ use super::utils::{ handle_construction_hash, handle_construction_parse, handle_construction_payloads, handle_construction_submit, }; -use crate::common::constants::INGRESS_INTERVAL_OVERLAP; +use crate::common::constants::{INGRESS_INTERVAL_OVERLAP, MAX_INGRESS_WINDOW}; use crate::common::types::Error; use candid::Principal; use ic_base_types::{CanisterId, PrincipalId}; @@ -108,6 +108,38 @@ pub fn construction_combine( .map_err(|err| Error::processing_construction_failed(&err)) } +/// Validate the caller-provided ingress window before it is expanded into a +/// list of ingress expiries. +/// +/// Rejects windows that would make the expiry loop iterate an excessive (or, on +/// arithmetic wraparound, unbounded) number of times. It enforces that: +/// - `ingress_start` is strictly before `ingress_end` (an empty or reversed +/// window would otherwise produce an empty/degenerate set of payloads), +/// - the span `ingress_end - ingress_start` must not exceed 24h, and +/// - `ingress_end` must not be more than 24h in the future, which also rejects +/// the near-`u64::MAX` payloads that would otherwise wrap the loop counter. +fn validate_ingress_window(now: u64, ingress_start: u64, ingress_end: u64) -> Result<(), Error> { + let max_window = MAX_INGRESS_WINDOW.as_nanos() as u64; + if ingress_start >= ingress_end { + return Err(Error::processing_construction_failed(&format!( + "Ingress start should start before ingress end: Start: {ingress_start}, End: {ingress_end}" + ))); + } + if ingress_end.saturating_sub(ingress_start) > max_window { + return Err(Error::processing_construction_failed(&format!( + "The ingress window (ingress_end - ingress_start) must not exceed {} hours: Start: {ingress_start}, End: {ingress_end}", + MAX_INGRESS_WINDOW.as_secs() / 3600 + ))); + } + if ingress_end.saturating_sub(now) > max_window { + return Err(Error::processing_construction_failed(&format!( + "ingress_end must not be more than {} hours in the future: Current time: {now}, End: {ingress_end}", + MAX_INGRESS_WINDOW.as_secs() / 3600 + ))); + } + Ok(()) +} + pub fn construction_payloads( operations: Vec, metadata: Option, @@ -145,18 +177,14 @@ pub fn construction_payloads( .and_then(|meta| meta.memo.clone()) .map(|memo| memo.into()); - if ingress_start >= ingress_end { - return Err(Error::processing_construction_failed(&format!( - "Ingress start should start before ingress end: Start: {ingress_start}, End: {ingress_end}" - ))); - } - if ingress_end < now + ingress_interval { return Err(Error::processing_construction_failed(&format!( "Ingress end should be at least one interval from the current time: Current time: {now}, End: {ingress_end}" ))); } + validate_ingress_window(now, ingress_start, ingress_end)?; + // Every ingress message sent to the IC has an expiry timestamp until which the signature associated with that message is valid // To support a longer overall timeframe than one interval, we can send multiple ingress messages with two signable contents each let mut ingress_expiries = vec![]; @@ -261,6 +289,55 @@ mod tests { const NUM_TEST_CASES: u32 = 100; const NUM_BLOCKS: usize = 1; + const HOUR_NANOS: u64 = 60 * 60 * 1_000_000_000; + // A realistic "now" (~2023) that is well away from the u64 boundary. + const NOW_NANOS: u64 = 1_700_000_000 * 1_000_000_000; + + // A window whose span exceeds the documented 24h bound would make the loop + // build a huge list of ingress expiries, so it is rejected before the loop. + #[test] + fn oversized_ingress_window_is_rejected() { + assert!( + validate_ingress_window(NOW_NANOS, NOW_NANOS, NOW_NANOS + 48 * HOUR_NANOS).is_err() + ); + } + + // A near-zero start together with a near-`u64::MAX` end spans almost the + // entire u64 range; that enormous span is rejected (it would otherwise + // iterate billions of times). + #[test] + fn unbounded_ingress_span_is_rejected() { + assert!(validate_ingress_window(NOW_NANOS, 0, u64::MAX).is_err()); + } + + // A window ending at (near) `u64::MAX` has a small span but an end far in the + // future; without the future bound the loop counter would wrap past + // `u64::MAX` and never terminate. The future bound rejects it. + #[test] + fn near_u64_max_ingress_window_is_rejected() { + assert!( + validate_ingress_window(NOW_NANOS, u64::MAX - 50 * 1_000_000_000, u64::MAX).is_err() + ); + } + + // Negative control: a realistic window (5 minutes ahead of now) is accepted. + #[test] + fn valid_ingress_window_is_accepted() { + assert!( + validate_ingress_window(NOW_NANOS, NOW_NANOS, NOW_NANOS + 5 * 60 * 1_000_000_000) + .is_ok() + ); + } + + // Boundary: a window of exactly 24h ending exactly 24h from now is accepted, + // one nanosecond more is rejected. + #[test] + fn ingress_window_boundary_is_inclusive() { + let max = MAX_INGRESS_WINDOW.as_nanos() as u64; + assert!(validate_ingress_window(NOW_NANOS, NOW_NANOS, NOW_NANOS + max).is_ok()); + assert!(validate_ingress_window(NOW_NANOS, NOW_NANOS, NOW_NANOS + max + 1).is_err()); + } + fn call_construction_derive(key_pair: &T) { let principal_id = key_pair.generate_principal_id().unwrap(); let public_key = ic_rosetta_test_utils::to_public_key(key_pair); @@ -461,6 +538,17 @@ mod tests { } (_, _) => {} } + // Windows wider than the permitted maximum, or with an + // ingress_end too far in the future, are rejected before + // the loop. + let eff_start = payloads_metadata.ingress_start.unwrap_or(now); + let eff_end = payloads_metadata + .ingress_end + .unwrap_or(eff_start + ingress_interval); + if validate_ingress_window(now, eff_start, eff_end).is_err() { + assert!(construction_payloads_response.is_err()); + continue; + } let construction_parse_response = construction_parse( construction_payloads_response .clone()