From 4626ccfcaaf71ee364e71447b2a548d5f3f504c4 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Wed, 8 Feb 2023 17:35:51 +0000 Subject: [PATCH] hs time handling: Make TimePeriod contain length as IntegerMinutes Locally, the only functional effect is that now we refuse to handle non-whole-number-of-minutes lengths - but since the consensus parameter can't represent those, there's no overall functional change. --- Cargo.lock | 1 + crates/tor-hscrypto/Cargo.toml | 1 + crates/tor-hscrypto/src/pk.rs | 5 ++++- crates/tor-hscrypto/src/time.rs | 29 ++++++++++++++++++--------- crates/tor-netdir/src/hsdir_params.rs | 4 ++-- 5 files changed, 28 insertions(+), 12 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 60ba2f0b1..f01b65afd 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3950,6 +3950,7 @@ dependencies = [ "thiserror", "tor-basic-utils", "tor-llcrypto", + "tor-units", ] [[package]] diff --git a/crates/tor-hscrypto/Cargo.toml b/crates/tor-hscrypto/Cargo.toml index 3b40134e9..95b8ea2d2 100644 --- a/crates/tor-hscrypto/Cargo.toml +++ b/crates/tor-hscrypto/Cargo.toml @@ -26,6 +26,7 @@ tor-llcrypto = { version = "0.4.1", path = "../tor-llcrypto", features = [ "hsv3-client", "hsv3-service", ] } +tor-units = { path = "../tor-units", version = "0.4.1" } [dev-dependencies] hex-literal = "0.3" diff --git a/crates/tor-hscrypto/src/pk.rs b/crates/tor-hscrypto/src/pk.rs index 624daa9a2..4b8c9edd5 100644 --- a/crates/tor-hscrypto/src/pk.rs +++ b/crates/tor-hscrypto/src/pk.rs @@ -142,7 +142,10 @@ impl HsIdKey { h.update(ED25519_BASEPOINT); h.update(b"key-blind"); h.update(cur_period.interval_num.to_be_bytes()); - h.update(u64::from(cur_period.length_in_sec).to_be_bytes()); + // TODO hs: shouldn't this be the period length in *minutes*, not seconds ? + // The spec just says `period_length` which elsewhere means minutes. + // https://gitlab.torproject.org/tpo/core/arti/-/issues/768 + h.update((u64::from(cur_period.length.as_minutes()) * 60).to_be_bytes()); h.finalize().into() } diff --git a/crates/tor-hscrypto/src/time.rs b/crates/tor-hscrypto/src/time.rs index 4ce718a96..fe6c70233 100644 --- a/crates/tor-hscrypto/src/time.rs +++ b/crates/tor-hscrypto/src/time.rs @@ -2,6 +2,8 @@ use std::time::{Duration, SystemTime}; +use tor_units::IntegerMinutes; + /// A period of time, as used in the onion service system. /// /// A `TimePeriod` is defined as a duration (in seconds), and the number of such @@ -25,8 +27,10 @@ use std::time::{Duration, SystemTime}; pub struct TimePeriod { /// Index of the time periods that have passed since the unix epoch. pub(crate) interval_num: u64, - /// The length of a time period, in seconds. - pub(crate) length_in_sec: u32, + /// The length of a time period, in **minutes**. + /// + /// The spec admits only periods which are a whole number of minutes. + pub(crate) length: IntegerMinutes, /// Our offset from the epoch, in seconds. pub(crate) offset_in_sec: u32, } @@ -35,7 +39,7 @@ pub struct TimePeriod { /// same interval length and offset. impl PartialOrd for TimePeriod { fn partial_cmp(&self, other: &Self) -> Option { - if self.length_in_sec == other.length_in_sec && self.offset_in_sec == other.offset_in_sec { + if self.length == other.length && self.offset_in_sec == other.offset_in_sec { Some(self.interval_num.cmp(&other.interval_num)) } else { None @@ -46,7 +50,8 @@ impl PartialOrd for TimePeriod { impl TimePeriod { /// Construct a time period of a given `length` that contains `when`. /// - /// The `length` value is rounded down to the nearest second. + /// The `length` value is rounded down to the nearest second, + /// and must then be a whole number of minutes. /// /// The `epoch_offset` value is the amount of time after the Unix epoch when /// our epoch begins. It is also rounded down to the nearest second. @@ -68,6 +73,11 @@ impl TimePeriod { pub fn new(length: Duration, when: SystemTime, epoch_offset: Duration) -> Option { // The algorithm here is specified in rend-spec-v3 section 2.2.1 let length_in_sec = u32::try_from(length.as_secs()).ok()?; + if length_in_sec % 60 != 0 { + return None; + } + let length_in_minutes = length_in_sec / 60; + let length = IntegerMinutes::new(length_in_minutes); let offset_in_sec = u32::try_from(epoch_offset.as_secs()).ok()?; let interval_num = when .duration_since(SystemTime::UNIX_EPOCH + epoch_offset) @@ -76,7 +86,7 @@ impl TimePeriod { / u64::from(length_in_sec); Some(TimePeriod { interval_num, - length_in_sec, + length, offset_in_sec, }) } @@ -116,8 +126,9 @@ impl TimePeriod { /// Return None if this time period contains any times that can be /// represented as a `SystemTime`. pub fn range(&self) -> Option> { - let start_sec = u64::from(self.length_in_sec).checked_mul(self.interval_num)?; - let end_sec = start_sec.checked_add(self.length_in_sec.into())?; + let length_in_sec = u64::from(self.length.as_minutes()) * 60; + let start_sec = length_in_sec.checked_mul(self.interval_num)?; + let end_sec = start_sec.checked_add(length_in_sec)?; let epoch_offset = Duration::new(self.offset_in_sec.into(), 0); let start = (SystemTime::UNIX_EPOCH + epoch_offset).checked_add(Duration::from_secs(start_sec))?; @@ -138,8 +149,8 @@ impl TimePeriod { /// /// This function should only be used when encoding the time period for /// cryptographic purposes. - pub fn length_in_sec(&self) -> u64 { - self.length_in_sec.into() + pub fn length(&self) -> IntegerMinutes { + self.length } } diff --git a/crates/tor-netdir/src/hsdir_params.rs b/crates/tor-netdir/src/hsdir_params.rs index a0db6cc85..dc3ab9440 100644 --- a/crates/tor-netdir/src/hsdir_params.rs +++ b/crates/tor-netdir/src/hsdir_params.rs @@ -120,7 +120,7 @@ fn disaster_srv(period: TimePeriod) -> SharedRandVal { use digest::Digest; let mut d = tor_llcrypto::d::Sha3_256::new(); d.update(b"shared-random-disaster"); - d.update((period.length_in_sec() / 60).to_be_bytes()); + d.update(u64::from(period.length().as_minutes()).to_be_bytes()); d.update(period.interval_num().to_be_bytes()); let v: [u8; 32] = d.finalize().into(); @@ -406,7 +406,7 @@ mod test { use digest::Digest; use tor_llcrypto::d::Sha3_256; let period = TimePeriod::new(d("1 day"), t("1970-01-02T17:33:00Z"), d("12 hours")).unwrap(); - assert_eq!(period.length_in_sec(), 86400); + assert_eq!(period.length().as_minutes(), 86400 / 60); assert_eq!(period.interval_num(), 1); let dsrv = disaster_srv(period);