From de62173d2a6508387edd9cbbde013906493c20b3 Mon Sep 17 00:00:00 2001 From: Tamme Dittrich Date: Mon, 9 Oct 2023 14:00:58 +0200 Subject: [PATCH 01/28] Start implementation of NTPv5 header parsing --- ntp-proto/Cargo.toml | 1 + ntp-proto/src/packet/mod.rs | 3 + ntp-proto/src/packet/v5.rs | 138 ++++++++++++++++++++++++++++++++++++ 3 files changed, 142 insertions(+) create mode 100644 ntp-proto/src/packet/v5.rs diff --git a/ntp-proto/Cargo.toml b/ntp-proto/Cargo.toml index 934b9dfb0..3fc13d1ab 100644 --- a/ntp-proto/Cargo.toml +++ b/ntp-proto/Cargo.toml @@ -16,6 +16,7 @@ rust-version.workspace = true __internal-fuzz = ["arbitrary", "__internal-api"] __internal-test = ["__internal-api"] __internal-api = [] +ntpv5 = [] [dependencies] # Note: md5 is needed to calculate ReferenceIDs for IPv6 addresses per RFC5905 diff --git a/ntp-proto/src/packet/mod.rs b/ntp-proto/src/packet/mod.rs index 7b0cad27d..fa63cc95f 100644 --- a/ntp-proto/src/packet/mod.rs +++ b/ntp-proto/src/packet/mod.rs @@ -18,6 +18,9 @@ mod error; mod extensionfields; mod mac; +#[cfg(feature = "ntpv5")] +mod v5; + pub use crypto::{ AesSivCmac256, AesSivCmac512, Cipher, CipherHolder, CipherProvider, DecryptError, EncryptResult, NoCipher, diff --git a/ntp-proto/src/packet/v5.rs b/ntp-proto/src/packet/v5.rs new file mode 100644 index 000000000..65389d742 --- /dev/null +++ b/ntp-proto/src/packet/v5.rs @@ -0,0 +1,138 @@ +use crate::packet::error::ParsingError; +use crate::{NtpAssociationMode, NtpDuration, NtpLeapIndicator, NtpTimestamp, ReferenceId}; + +#[repr(u8)] +enum NtpTimescale { + Utc = 0, + Tai = 1, + Ut1 = 2, + LeadSmearedUtc = 3, +} + +impl NtpTimescale { + fn from_bits(bits: u8) -> Option { + Some(match bits { + 0 => Self::Utc, + 1 => Self::Tai, + 2 => Self::Ut1, + 3 => Self::LeadSmearedUtc, + _ => return None, + }) + } +} + +#[derive(Copy, Clone, Default, Ord, PartialOrd, Eq, PartialEq)] +struct NtpEra(pub u8); + +struct NtpFlags(u16); + +impl NtpFlags { + fn from_bits(bits: [u8; 2]) -> Self { + Self(u16::from_be_bytes(bits)) + } + + pub fn unknown_leap(&self) -> bool { + self.0 & 0x01 != 0 + } + + pub fn interleaved_mode(&self) -> bool { + self.0 & 0x02 != 0 + } +} + +struct NtpClientCookie(u64); + +struct NtpServerCookie(u64); + +struct NtpHeaderV5 { + leap: NtpLeapIndicator, + mode: NtpAssociationMode, + stratum: u8, + poll: i8, + precision: i8, + timescale: NtpTimescale, + era: NtpEra, + flags: NtpFlags, + root_delay: NtpDuration, + root_dispersion: NtpDuration, + client_cookie: NtpClientCookie, + server_cookie: NtpServerCookie, + /// Time at the server when the request arrived from the client + receive_timestamp: NtpTimestamp, + /// Time at the server when the response left for the client + transmit_timestamp: NtpTimestamp, +} + +impl NtpHeaderV5 { + fn deserialize(data: &[u8]) -> Result<(Self, usize), ParsingError> { + todo!() + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn round_trip_timescale() { + for i in 0..=u8::MAX { + match NtpTimescale::from_bits(i) { + None => {} + Some(ts) => assert_eq!(ts as u8, i), + } + } + } + + #[test] + fn flags() { + let flags = NtpFlags(0x00); + assert_eq!(flags.unknown_leap(), false); + assert_eq!(flags.interleaved_mode(), false); + + let flags = NtpFlags(0x01); + assert_eq!(flags.unknown_leap(), true); + assert_eq!(flags.interleaved_mode(), false); + + let flags = NtpFlags(0x02); + assert_eq!(flags.unknown_leap(), false); + assert_eq!(flags.interleaved_mode(), true); + + let flags = NtpFlags(0x03); + assert_eq!(flags.unknown_leap(), true); + assert_eq!(flags.interleaved_mode(), true); + } + + #[test] + fn parse() { + #[rustfmt::skip] + let data = [ + // LI VN Mode + 0b_00_101_000, + // Stratum + 0x00, + // Poll + 0x00, + // Precision + 0x00, + // Timescale (0: UTC, 1: TAI, 2: UT1, 3: Leap-smeared UTC) + 0x00, + // Era + 0x00, + // Flags + 0x00, + 0b0000_00_0_0, + // Root Delay + 0x00, 0x00, 0x00, 0x00, + // Root Dispersion + 0x00, 0x00, 0x00, 0x00, + // Server Cookie + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + // Client Cookie + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + // Receive Timestamp + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + // Transmit Timestamp + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + ]; + } +} From 2b4dd00fa6d857c7c792b3e0378ee621b01952b6 Mon Sep 17 00:00:00 2001 From: Marlon Baeten Date: Mon, 9 Oct 2023 14:59:23 +0200 Subject: [PATCH 02/28] Implemented v5 header parsing --- ntp-proto/src/packet/v5.rs | 159 ++++++++++++++++++++++++++++++++++--- 1 file changed, 146 insertions(+), 13 deletions(-) diff --git a/ntp-proto/src/packet/v5.rs b/ntp-proto/src/packet/v5.rs index 65389d742..5993b9552 100644 --- a/ntp-proto/src/packet/v5.rs +++ b/ntp-proto/src/packet/v5.rs @@ -1,7 +1,33 @@ use crate::packet::error::ParsingError; -use crate::{NtpAssociationMode, NtpDuration, NtpLeapIndicator, NtpTimestamp, ReferenceId}; +use crate::{NtpDuration, NtpLeapIndicator, NtpTimestamp}; #[repr(u8)] +#[derive(Debug, PartialEq, Eq)] +pub enum NtpMode { + Request = 3, + Response = 4, +} + +impl NtpMode { + fn from_bits(bits: u8) -> Option { + Some(match bits { + 3 => Self::Request, + 4 => Self::Response, + _ => return None, + }) + } + + pub fn is_request(&self) -> bool { + self == &Self::Request + } + + pub fn is_response(&self) -> bool { + self == &Self::Response + } +} + +#[repr(u8)] +#[derive(Debug, PartialEq, Eq)] enum NtpTimescale { Utc = 0, Tai = 1, @@ -21,9 +47,10 @@ impl NtpTimescale { } } -#[derive(Copy, Clone, Default, Ord, PartialOrd, Eq, PartialEq)] +#[derive(Debug, Copy, Clone, Eq, PartialEq)] struct NtpEra(pub u8); +#[derive(Debug, Copy, Clone, Eq, PartialEq)] struct NtpFlags(u16); impl NtpFlags { @@ -40,13 +67,16 @@ impl NtpFlags { } } -struct NtpClientCookie(u64); +#[derive(Debug, Copy, Clone, Eq, PartialEq)] +struct NtpServerCookie([u8; 8]); -struct NtpServerCookie(u64); +#[derive(Debug, Copy, Clone, Eq, PartialEq)] +struct NtpClientCookie([u8; 8]); +#[derive(Debug)] struct NtpHeaderV5 { leap: NtpLeapIndicator, - mode: NtpAssociationMode, + mode: NtpMode, stratum: u8, poll: i8, precision: i8, @@ -55,8 +85,8 @@ struct NtpHeaderV5 { flags: NtpFlags, root_delay: NtpDuration, root_dispersion: NtpDuration, - client_cookie: NtpClientCookie, server_cookie: NtpServerCookie, + client_cookie: NtpClientCookie, /// Time at the server when the request arrived from the client receive_timestamp: NtpTimestamp, /// Time at the server when the response left for the client @@ -64,8 +94,37 @@ struct NtpHeaderV5 { } impl NtpHeaderV5 { + const LENGTH: usize = 48; + fn deserialize(data: &[u8]) -> Result<(Self, usize), ParsingError> { - todo!() + if data.len() < Self::LENGTH { + return Err(ParsingError::IncorrectLength); + } + + let version = (data[0] >> 3) & 0b111; + if version != 5 { + return Err(ParsingError::InvalidVersion(version)); + } + + Ok(( + Self { + leap: NtpLeapIndicator::from_bits((data[0] & 0xC0) >> 6), + mode: NtpMode::from_bits(data[0] & 0x07).unwrap(), + stratum: data[1], + poll: data[2] as i8, + precision: data[3] as i8, + timescale: NtpTimescale::from_bits(data[4]).unwrap(), + era: NtpEra(data[5]), + flags: NtpFlags::from_bits(data[6..8].try_into().unwrap()), + root_delay: NtpDuration::from_bits_short(data[8..12].try_into().unwrap()), + root_dispersion: NtpDuration::from_bits_short(data[12..16].try_into().unwrap()), + server_cookie: NtpServerCookie(data[16..24].try_into().unwrap()), + client_cookie: NtpClientCookie(data[24..32].try_into().unwrap()), + receive_timestamp: NtpTimestamp::from_bits(data[32..40].try_into().unwrap()), + transmit_timestamp: NtpTimestamp::from_bits(data[40..48].try_into().unwrap()), + }, + Self::LENGTH, + )) } } @@ -103,24 +162,24 @@ mod tests { } #[test] - fn parse() { + fn parse_request() { #[rustfmt::skip] let data = [ // LI VN Mode - 0b_00_101_000, + 0b_00_101_011, // Stratum 0x00, // Poll - 0x00, + 0x05, // Precision 0x00, // Timescale (0: UTC, 1: TAI, 2: UT1, 3: Leap-smeared UTC) - 0x00, + 0x02, // Era 0x00, // Flags 0x00, - 0b0000_00_0_0, + 0b0000_00_1_0, // Root Delay 0x00, 0x00, 0x00, 0x00, // Root Dispersion @@ -128,11 +187,85 @@ mod tests { // Server Cookie 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, // Client Cookie - 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x08, 0x07, 0x06, 0x05, 0x04, 0x03, 0x02, 0x01, // Receive Timestamp 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, // Transmit Timestamp 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, ]; + + let (parsed, len) = NtpHeaderV5::deserialize(&data).unwrap(); + + assert_eq!(len, 48); + assert_eq!(parsed.leap, NtpLeapIndicator::NoWarning); + assert!(parsed.mode.is_request()); + assert_eq!(parsed.stratum, 0); + assert_eq!(parsed.poll, 5); + assert_eq!(parsed.precision, 0); + assert_eq!(parsed.timescale, NtpTimescale::Ut1); + assert_eq!(parsed.era, NtpEra(0)); + assert!(parsed.flags.interleaved_mode()); + assert!(!parsed.flags.unknown_leap()); + assert!(parsed.flags.interleaved_mode()); + assert_eq!(parsed.root_delay, NtpDuration::from_seconds(0.0)); + assert_eq!(parsed.root_dispersion, NtpDuration::from_seconds(0.0)); + assert_eq!(parsed.server_cookie, NtpServerCookie([0x0; 8])); + assert_eq!(parsed.client_cookie, NtpClientCookie([0x08, 0x07, 0x06, 0x05, 0x04, 0x03, 0x02, 0x01])); + assert_eq!(parsed.receive_timestamp, NtpTimestamp::from_fixed_int(0x0)); + assert_eq!(parsed.transmit_timestamp, NtpTimestamp::from_fixed_int(0x0)); + } + + #[test] + fn parse_resonse() { + #[rustfmt::skip] + let data = [ + // LI VN Mode + 0b_00_101_100, + // Stratum + 0x04, + // Poll + 0x05, + // Precision + 0x06, + // Timescale (0: UTC, 1: TAI, 2: UT1, 3: Leap-smeared UTC) + 0x01, + // Era + 0x07, + // Flags + 0x00, + 0b0000_00_1_0, + // Root Delay + 0x00, 0x00, 0x02, 0x3f, + // Root Dispersion + 0x00, 0x00, 0x00, 0x42, + // Server Cookie + 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, + // Client Cookie + 0x08, 0x07, 0x06, 0x05, 0x04, 0x03, 0x02, 0x01, + // Receive Timestamp + 0x11, 0x11, 0x11, 0x11, 0x11, 0x11, 0x11, 0x11, + // Transmit Timestamp + 0x22, 0x22, 0x22, 0x22, 0x22, 0x22, 0x22, 0x22, + ]; + + let (parsed, len) = NtpHeaderV5::deserialize(&data).unwrap(); + + assert_eq!(len, 48); + assert_eq!(parsed.leap, NtpLeapIndicator::NoWarning); + assert!(parsed.mode.is_response()); + assert_eq!(parsed.stratum, 4); + assert_eq!(parsed.poll, 5); + assert_eq!(parsed.precision, 6); + assert_eq!(parsed.timescale, NtpTimescale::Tai); + assert_eq!(parsed.era, NtpEra(7)); + assert!(parsed.flags.interleaved_mode()); + assert!(!parsed.flags.unknown_leap()); + assert!(parsed.flags.interleaved_mode()); + assert_eq!(parsed.root_delay, NtpDuration::from_seconds(0.00877380371298031)); + assert_eq!(parsed.root_dispersion, NtpDuration::from_seconds(0.001007080078359479)); + assert_eq!(parsed.server_cookie, NtpServerCookie([0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08])); + assert_eq!(parsed.client_cookie, NtpClientCookie([0x08, 0x07, 0x06, 0x05, 0x04, 0x03, 0x02, 0x01])); + assert_eq!(parsed.receive_timestamp, NtpTimestamp::from_fixed_int(0x1111111111111111)); + assert_eq!(parsed.transmit_timestamp, NtpTimestamp::from_fixed_int(0x2222222222222222)); } } From 4a16d5fee8ed63f3d0f3603c590dc80d8df2be48 Mon Sep 17 00:00:00 2001 From: Tamme Dittrich Date: Mon, 9 Oct 2023 16:11:42 +0200 Subject: [PATCH 03/28] Implement parsing and serialization for draft identification extension --- ntp-proto/src/packet/error.rs | 8 ++ ntp-proto/src/packet/extensionfields.rs | 99 ++++++++++++++++++++++++- 2 files changed, 104 insertions(+), 3 deletions(-) diff --git a/ntp-proto/src/packet/error.rs b/ntp-proto/src/packet/error.rs index 094813832..cbe4b03bc 100644 --- a/ntp-proto/src/packet/error.rs +++ b/ntp-proto/src/packet/error.rs @@ -10,6 +10,8 @@ pub enum ParsingError { MalformedNonce, MalformedCookiePlaceholder, DecryptError(T), + #[cfg(feature = "ntpv5")] + InvalidDraftIdentification, } impl ParsingError { @@ -22,6 +24,8 @@ impl ParsingError { MalformedNtsExtensionFields => Err(MalformedNtsExtensionFields), MalformedNonce => Err(MalformedNonce), MalformedCookiePlaceholder => Err(MalformedCookiePlaceholder), + #[cfg(feature = "ntpv5")] + InvalidDraftIdentification => Err(InvalidDraftIdentification), DecryptError(decrypt_error) => Ok(decrypt_error), } } @@ -37,6 +41,8 @@ impl ParsingError { MalformedNtsExtensionFields => MalformedNtsExtensionFields, MalformedNonce => MalformedNonce, MalformedCookiePlaceholder => MalformedCookiePlaceholder, + #[cfg(feature = "ntpv5")] + InvalidDraftIdentification => InvalidDraftIdentification, DecryptError(decrypt_error) => match decrypt_error {}, } } @@ -52,6 +58,8 @@ impl Display for ParsingError { Self::MalformedNtsExtensionFields => f.write_str("Malformed nts extension fields"), Self::MalformedNonce => f.write_str("Malformed nonce (likely invalid length)"), Self::MalformedCookiePlaceholder => f.write_str("Malformed cookie placeholder"), + #[cfg(feature = "ntpv5")] + Self::InvalidDraftIdentification => f.write_str("Draft Identification invalid"), Self::DecryptError(_) => f.write_str("Failed to decrypt NTS extension fields"), } } diff --git a/ntp-proto/src/packet/extensionfields.rs b/ntp-proto/src/packet/extensionfields.rs index 3bdbd7ef6..cc0961c6c 100644 --- a/ntp-proto/src/packet/extensionfields.rs +++ b/ntp-proto/src/packet/extensionfields.rs @@ -13,7 +13,11 @@ enum ExtensionFieldTypeId { NtsCookie, NtsCookiePlaceholder, NtsEncryptedField, - Unknown { type_id: u16 }, + Unknown { + type_id: u16, + }, + #[cfg(feature = "ntpv5")] + DraftIdentification, } impl ExtensionFieldTypeId { @@ -23,6 +27,8 @@ impl ExtensionFieldTypeId { 0x204 => Self::NtsCookie, 0x304 => Self::NtsCookiePlaceholder, 0x404 => Self::NtsEncryptedField, + #[cfg(feature = "ntpv5")] + 0xF5FF => Self::DraftIdentification, _ => Self::Unknown { type_id }, } } @@ -33,6 +39,8 @@ impl ExtensionFieldTypeId { ExtensionFieldTypeId::NtsCookie => 0x204, ExtensionFieldTypeId::NtsCookiePlaceholder => 0x304, ExtensionFieldTypeId::NtsEncryptedField => 0x404, + #[cfg(feature = "ntpv5")] + ExtensionFieldTypeId::DraftIdentification => 0xF5FF, ExtensionFieldTypeId::Unknown { type_id } => type_id, } } @@ -42,9 +50,16 @@ impl ExtensionFieldTypeId { pub enum ExtensionField<'a> { UniqueIdentifier(Cow<'a, [u8]>), NtsCookie(Cow<'a, [u8]>), - NtsCookiePlaceholder { cookie_length: u16 }, + NtsCookiePlaceholder { + cookie_length: u16, + }, InvalidNtsEncryptedField, - Unknown { type_id: u16, data: Cow<'a, [u8]> }, + #[cfg(feature = "ntpv5")] + DraftIdentification(Cow<'a, str>), + Unknown { + type_id: u16, + data: Cow<'a, [u8]>, + }, } impl<'a> std::fmt::Debug for ExtensionField<'a> { @@ -59,6 +74,10 @@ impl<'a> std::fmt::Debug for ExtensionField<'a> { .field("body_length", body_length) .finish(), Self::InvalidNtsEncryptedField => f.debug_struct("InvalidNtsEncryptedField").finish(), + #[cfg(feature = "ntpv5")] + Self::DraftIdentification(arg0) => { + f.debug_tuple("DraftIdentification").field(arg0).finish() + } Self::Unknown { type_id: typeid, data, @@ -92,6 +111,8 @@ impl<'a> ExtensionField<'a> { cookie_length: body_length, }, InvalidNtsEncryptedField => InvalidNtsEncryptedField, + #[cfg(feature = "ntpv5")] + DraftIdentification(data) => DraftIdentification(Cow::Owned(data.into_owned())), } } @@ -108,6 +129,8 @@ impl<'a> ExtensionField<'a> { cookie_length: body_length, } => Self::encode_nts_cookie_placeholder(w, *body_length, minimum_size), InvalidNtsEncryptedField => Err(std::io::ErrorKind::Other.into()), + #[cfg(feature = "ntpv5")] + DraftIdentification(data) => Self::encode_draft_identification(w, data, minimum_size), } } @@ -335,6 +358,26 @@ impl<'a> ExtensionField<'a> { Ok(()) } + #[cfg(feature = "ntpv5")] + fn encode_draft_identification( + w: &mut impl Write, + data: &str, + minimum_size: u16, + ) -> std::io::Result<()> { + Self::encode_framing( + w, + ExtensionFieldTypeId::DraftIdentification, + data.len(), + minimum_size, + )?; + + w.write_all(data.as_bytes())?; + + Self::encode_padding(w, data.len(), minimum_size)?; + + Ok(()) + } + fn decode_unique_identifier( message: &'a [u8], ) -> Result> { @@ -375,6 +418,28 @@ impl<'a> ExtensionField<'a> { }) } + #[cfg(feature = "ntpv5")] + fn decode_draft_identification( + message: &'a [u8], + ) -> Result> { + let msg = message[..].into(); + + let new_msg = match msg { + Cow::Borrowed(slice) => std::str::from_utf8(slice).ok().map(Cow::Borrowed), + Cow::Owned(vec) => String::from_utf8(vec).ok().map(Cow::Owned), + }; + + let Some(new_msg) = new_msg else { + return Err(ParsingError::InvalidDraftIdentification); + }; + + if !new_msg.is_ascii() { + return Err(ParsingError::InvalidDraftIdentification); + } + + Ok(ExtensionField::DraftIdentification(new_msg)) + } + fn decode(raw: RawExtensionField<'a>) -> Result> { type EF<'a> = ExtensionField<'a>; type TypeId = ExtensionFieldTypeId; @@ -385,6 +450,8 @@ impl<'a> ExtensionField<'a> { TypeId::UniqueIdentifier => EF::decode_unique_identifier(message), TypeId::NtsCookie => EF::decode_nts_cookie(message), TypeId::NtsCookiePlaceholder => EF::decode_nts_cookie_placeholder(message), + #[cfg(feature = "ntpv5")] + TypeId::DraftIdentification => EF::decode_draft_identification(message), type_id => EF::decode_unknown(type_id.to_type_id(), message), } } @@ -806,6 +873,32 @@ mod tests { ); } + #[cfg(feature = "ntpv5")] + #[test] + fn draft_identification() { + let test_id = "draft-ietf-ntp-ntpv5-00\0"; + let len = u16::try_from(4 + test_id.len()).unwrap(); + let mut data = vec![]; + data.extend_from_slice(&[0xF5, 0xFF]); + data.extend_from_slice(&len.to_be_bytes()); + data.extend_from_slice(test_id.as_bytes()); + + let raw = RawExtensionField::deserialize(&data, 0).unwrap(); + let ef = ExtensionField::decode(raw).unwrap(); + + match ef { + ExtensionField::DraftIdentification(ref data) => { + assert_eq!(data, test_id); + } + _ => panic!("Unexpected extensionfield {ef:?}"), + } + + let mut out = vec![]; + ef.serialize(&mut out, len).unwrap(); + + assert_eq!(&out, &data); + } + #[test] fn extension_field_minimum_size() { let minimum_size = 32; From b6bea9d65dd1c91d0e397e1ff55099509edbaa6c Mon Sep 17 00:00:00 2001 From: Tamme Dittrich Date: Mon, 9 Oct 2023 17:11:03 +0200 Subject: [PATCH 04/28] Add NTPv5 header to general NtpHeader to prepare for usage in server --- ntp-proto/src/packet/mod.rs | 102 ++++++++++++++++++++++++++++++++++++ ntp-proto/src/packet/v5.rs | 99 ++++++++++++++++++++++------------ 2 files changed, 168 insertions(+), 33 deletions(-) diff --git a/ntp-proto/src/packet/mod.rs b/ntp-proto/src/packet/mod.rs index fa63cc95f..9e8620094 100644 --- a/ntp-proto/src/packet/mod.rs +++ b/ntp-proto/src/packet/mod.rs @@ -121,6 +121,8 @@ pub struct NtpPacket<'a> { enum NtpHeader { V3(NtpHeaderV3V4), V4(NtpHeaderV3V4), + #[cfg(feature = "ntpv5")] + V5(v5::NtpHeaderV5), } #[derive(Debug, Copy, Clone, PartialEq, Eq)] @@ -357,6 +359,46 @@ impl<'a> NtpPacket<'a> { } } } + #[cfg(feature = "ntpv5")] + 5 => { + let (header, header_size) = + v5::NtpHeaderV5::deserialize(data).map_err(|e| e.generalize())?; + + let contruct_packet = |remaining_bytes: &'a [u8], efdata| { + let mac = if !remaining_bytes.is_empty() { + Some(Mac::deserialize(remaining_bytes)?) + } else { + None + }; + + let packet = NtpPacket { + header: NtpHeader::V5(header), + efdata, + mac, + }; + + Ok::<_, ParsingError>(packet) + }; + + // TODO: Check extension field handling in V5 + match ExtensionFieldData::deserialize(data, header_size, cipher) { + Ok(decoded) => { + let packet = contruct_packet(decoded.remaining_bytes, decoded.efdata) + .map_err(|e| e.generalize())?; + + Ok((packet, decoded.cookie)) + } + Err(e) => { + // return early if it is anything but a decrypt error + let invalid = e.get_decrypt_error()?; + + let packet = contruct_packet(invalid.remaining_bytes, invalid.efdata) + .map_err(|e| e.generalize())?; + + Err(ParsingError::DecryptError(packet)) + } + } + } _ => Err(PacketParsingError::InvalidVersion(version)), } } @@ -382,11 +424,15 @@ impl<'a> NtpPacket<'a> { match self.header { NtpHeader::V3(header) => header.serialize(w, 3)?, NtpHeader::V4(header) => header.serialize(w, 4)?, + #[cfg(feature = "ntpv5")] + NtpHeader::V5(_header) => todo!(), }; match self.header { NtpHeader::V3(_) => { /* No extension fields in V3 */ } NtpHeader::V4(_) => self.efdata.serialize(w, cipher)?, + #[cfg(feature = "ntpv5")] + NtpHeader::V5(_header) => todo!(), } if let Some(ref mac) = self.mac { @@ -483,6 +529,8 @@ impl<'a> NtpPacket<'a> { }, mac: None, }, + #[cfg(feature = "ntpv5")] + NtpHeader::V5(_header) => todo!(), } } @@ -540,6 +588,8 @@ impl<'a> NtpPacket<'a> { }, mac: None, }, + #[cfg(feature = "ntpv5")] + NtpHeader::V5(_header) => todo!(), } } @@ -566,6 +616,8 @@ impl<'a> NtpPacket<'a> { }, mac: None, }, + #[cfg(feature = "ntpv5")] + NtpHeader::V5(_header) => todo!(), } } @@ -586,6 +638,8 @@ impl<'a> NtpPacket<'a> { }, mac: None, }, + #[cfg(feature = "ntpv5")] + NtpHeader::V5(_header) => todo!(), } } @@ -612,6 +666,8 @@ impl<'a> NtpPacket<'a> { }, mac: None, }, + #[cfg(feature = "ntpv5")] + NtpHeader::V5(_header) => todo!(), } } @@ -632,6 +688,8 @@ impl<'a> NtpPacket<'a> { }, mac: None, }, + #[cfg(feature = "ntpv5")] + NtpHeader::V5(_header) => todo!(), } } @@ -653,6 +711,8 @@ impl<'a> NtpPacket<'a> { }, mac: None, }, + #[cfg(feature = "ntpv5")] + NtpHeader::V5(_header) => todo!(), } } } @@ -669,6 +729,8 @@ impl<'a> NtpPacket<'a> { match self.header { NtpHeader::V3(header) => header.leap, NtpHeader::V4(header) => header.leap, + #[cfg(feature = "ntpv5")] + NtpHeader::V5(header) => header.leap, } } @@ -676,6 +738,8 @@ impl<'a> NtpPacket<'a> { match self.header { NtpHeader::V3(header) => header.mode, NtpHeader::V4(header) => header.mode, + #[cfg(feature = "ntpv5")] + NtpHeader::V5(_header) => todo!(), } } @@ -683,6 +747,8 @@ impl<'a> NtpPacket<'a> { match self.header { NtpHeader::V3(header) => header.stratum, NtpHeader::V4(header) => header.stratum, + #[cfg(feature = "ntpv5")] + NtpHeader::V5(header) => header.stratum, } } @@ -690,6 +756,8 @@ impl<'a> NtpPacket<'a> { match self.header { NtpHeader::V3(header) => header.precision, NtpHeader::V4(header) => header.precision, + #[cfg(feature = "ntpv5")] + NtpHeader::V5(header) => header.precision, } } @@ -697,6 +765,8 @@ impl<'a> NtpPacket<'a> { match self.header { NtpHeader::V3(header) => header.root_delay, NtpHeader::V4(header) => header.root_delay, + #[cfg(feature = "ntpv5")] + NtpHeader::V5(header) => header.root_delay, } } @@ -704,6 +774,8 @@ impl<'a> NtpPacket<'a> { match self.header { NtpHeader::V3(header) => header.root_dispersion, NtpHeader::V4(header) => header.root_dispersion, + #[cfg(feature = "ntpv5")] + NtpHeader::V5(header) => header.root_dispersion, } } @@ -711,6 +783,8 @@ impl<'a> NtpPacket<'a> { match self.header { NtpHeader::V3(header) => header.receive_timestamp, NtpHeader::V4(header) => header.receive_timestamp, + #[cfg(feature = "ntpv5")] + NtpHeader::V5(header) => header.receive_timestamp, } } @@ -718,6 +792,8 @@ impl<'a> NtpPacket<'a> { match self.header { NtpHeader::V3(header) => header.transmit_timestamp, NtpHeader::V4(header) => header.transmit_timestamp, + #[cfg(feature = "ntpv5")] + NtpHeader::V5(header) => header.transmit_timestamp, } } @@ -725,6 +801,8 @@ impl<'a> NtpPacket<'a> { match self.header { NtpHeader::V3(header) => header.reference_id, NtpHeader::V4(header) => header.reference_id, + #[cfg(feature = "ntpv5")] + NtpHeader::V5(_header) => todo!(), } } @@ -732,6 +810,8 @@ impl<'a> NtpPacket<'a> { match self.header { NtpHeader::V3(header) => header.stratum == 0, NtpHeader::V4(header) => header.stratum == 0, + #[cfg(feature = "ntpv5")] + NtpHeader::V5(_header) => todo!(), } } @@ -777,6 +857,8 @@ impl<'a> NtpPacket<'a> { NtpHeader::V4(header) => { header.origin_timestamp == identifier.expected_origin_timestamp } + #[cfg(feature = "ntpv5")] + NtpHeader::V5(_header) => todo!(), } } } @@ -813,6 +895,8 @@ impl<'a> NtpPacket<'a> { match &mut self.header { NtpHeader::V3(ref mut header) => header.mode = mode, NtpHeader::V4(ref mut header) => header.mode = mode, + #[cfg(feature = "ntpv5")] + NtpHeader::V5(_header) => todo!(), } } @@ -820,6 +904,8 @@ impl<'a> NtpPacket<'a> { match &mut self.header { NtpHeader::V3(ref mut header) => header.origin_timestamp = timestamp, NtpHeader::V4(ref mut header) => header.origin_timestamp = timestamp, + #[cfg(feature = "ntpv5")] + NtpHeader::V5(_header) => todo!(), } } @@ -827,6 +913,8 @@ impl<'a> NtpPacket<'a> { match &mut self.header { NtpHeader::V3(ref mut header) => header.transmit_timestamp = timestamp, NtpHeader::V4(ref mut header) => header.transmit_timestamp = timestamp, + #[cfg(feature = "ntpv5")] + NtpHeader::V5(_header) => todo!(), } } @@ -834,6 +922,8 @@ impl<'a> NtpPacket<'a> { match &mut self.header { NtpHeader::V3(ref mut header) => header.receive_timestamp = timestamp, NtpHeader::V4(ref mut header) => header.receive_timestamp = timestamp, + #[cfg(feature = "ntpv5")] + NtpHeader::V5(_header) => todo!(), } } @@ -841,6 +931,8 @@ impl<'a> NtpPacket<'a> { match &mut self.header { NtpHeader::V3(ref mut header) => header.precision = precision, NtpHeader::V4(ref mut header) => header.precision = precision, + #[cfg(feature = "ntpv5")] + NtpHeader::V5(_header) => todo!(), } } @@ -848,6 +940,8 @@ impl<'a> NtpPacket<'a> { match &mut self.header { NtpHeader::V3(ref mut header) => header.leap = leap, NtpHeader::V4(ref mut header) => header.leap = leap, + #[cfg(feature = "ntpv5")] + NtpHeader::V5(_header) => todo!(), } } @@ -855,6 +949,8 @@ impl<'a> NtpPacket<'a> { match &mut self.header { NtpHeader::V3(ref mut header) => header.stratum = stratum, NtpHeader::V4(ref mut header) => header.stratum = stratum, + #[cfg(feature = "ntpv5")] + NtpHeader::V5(_header) => todo!(), } } @@ -862,6 +958,8 @@ impl<'a> NtpPacket<'a> { match &mut self.header { NtpHeader::V3(ref mut header) => header.reference_id = reference_id, NtpHeader::V4(ref mut header) => header.reference_id = reference_id, + #[cfg(feature = "ntpv5")] + NtpHeader::V5(_header) => todo!(), } } @@ -869,6 +967,8 @@ impl<'a> NtpPacket<'a> { match &mut self.header { NtpHeader::V3(ref mut header) => header.root_delay = root_delay, NtpHeader::V4(ref mut header) => header.root_delay = root_delay, + #[cfg(feature = "ntpv5")] + NtpHeader::V5(_header) => todo!(), } } @@ -876,6 +976,8 @@ impl<'a> NtpPacket<'a> { match &mut self.header { NtpHeader::V3(ref mut header) => header.root_dispersion = root_dispersion, NtpHeader::V4(ref mut header) => header.root_dispersion = root_dispersion, + #[cfg(feature = "ntpv5")] + NtpHeader::V5(_header) => todo!(), } } } diff --git a/ntp-proto/src/packet/v5.rs b/ntp-proto/src/packet/v5.rs index 5993b9552..3f0b41477 100644 --- a/ntp-proto/src/packet/v5.rs +++ b/ntp-proto/src/packet/v5.rs @@ -2,7 +2,7 @@ use crate::packet::error::ParsingError; use crate::{NtpDuration, NtpLeapIndicator, NtpTimestamp}; #[repr(u8)] -#[derive(Debug, PartialEq, Eq)] +#[derive(Debug, PartialEq, Eq, Copy, Clone)] pub enum NtpMode { Request = 3, Response = 4, @@ -27,8 +27,8 @@ impl NtpMode { } #[repr(u8)] -#[derive(Debug, PartialEq, Eq)] -enum NtpTimescale { +#[derive(Debug, PartialEq, Eq, Copy, Clone)] +pub enum NtpTimescale { Utc = 0, Tai = 1, Ut1 = 2, @@ -48,10 +48,10 @@ impl NtpTimescale { } #[derive(Debug, Copy, Clone, Eq, PartialEq)] -struct NtpEra(pub u8); +pub struct NtpEra(pub u8); #[derive(Debug, Copy, Clone, Eq, PartialEq)] -struct NtpFlags(u16); +pub struct NtpFlags(u16); impl NtpFlags { fn from_bits(bits: [u8; 2]) -> Self { @@ -68,41 +68,43 @@ impl NtpFlags { } #[derive(Debug, Copy, Clone, Eq, PartialEq)] -struct NtpServerCookie([u8; 8]); +pub struct NtpServerCookie([u8; 8]); #[derive(Debug, Copy, Clone, Eq, PartialEq)] -struct NtpClientCookie([u8; 8]); - -#[derive(Debug)] -struct NtpHeaderV5 { - leap: NtpLeapIndicator, - mode: NtpMode, - stratum: u8, - poll: i8, - precision: i8, - timescale: NtpTimescale, - era: NtpEra, - flags: NtpFlags, - root_delay: NtpDuration, - root_dispersion: NtpDuration, - server_cookie: NtpServerCookie, - client_cookie: NtpClientCookie, +pub struct NtpClientCookie([u8; 8]); + +#[derive(Debug, Copy, Clone, Eq, PartialEq)] +pub struct NtpHeaderV5 { + pub leap: NtpLeapIndicator, + pub mode: NtpMode, + pub stratum: u8, + pub poll: i8, + pub precision: i8, + pub timescale: NtpTimescale, + pub era: NtpEra, + pub flags: NtpFlags, + pub root_delay: NtpDuration, + pub root_dispersion: NtpDuration, + pub server_cookie: NtpServerCookie, + pub client_cookie: NtpClientCookie, /// Time at the server when the request arrived from the client - receive_timestamp: NtpTimestamp, + pub receive_timestamp: NtpTimestamp, /// Time at the server when the response left for the client - transmit_timestamp: NtpTimestamp, + pub transmit_timestamp: NtpTimestamp, } impl NtpHeaderV5 { const LENGTH: usize = 48; - fn deserialize(data: &[u8]) -> Result<(Self, usize), ParsingError> { + pub(crate) fn deserialize( + data: &[u8], + ) -> Result<(Self, usize), ParsingError> { if data.len() < Self::LENGTH { return Err(ParsingError::IncorrectLength); } let version = (data[0] >> 3) & 0b111; - if version != 5 { + if version != 5 { return Err(ParsingError::InvalidVersion(version)); } @@ -131,6 +133,7 @@ impl NtpHeaderV5 { #[cfg(test)] mod tests { use super::*; + use crate::{NoCipher, NtpPacket}; #[test] fn round_trip_timescale() { @@ -210,7 +213,10 @@ mod tests { assert_eq!(parsed.root_delay, NtpDuration::from_seconds(0.0)); assert_eq!(parsed.root_dispersion, NtpDuration::from_seconds(0.0)); assert_eq!(parsed.server_cookie, NtpServerCookie([0x0; 8])); - assert_eq!(parsed.client_cookie, NtpClientCookie([0x08, 0x07, 0x06, 0x05, 0x04, 0x03, 0x02, 0x01])); + assert_eq!( + parsed.client_cookie, + NtpClientCookie([0x08, 0x07, 0x06, 0x05, 0x04, 0x03, 0x02, 0x01]) + ); assert_eq!(parsed.receive_timestamp, NtpTimestamp::from_fixed_int(0x0)); assert_eq!(parsed.transmit_timestamp, NtpTimestamp::from_fixed_int(0x0)); } @@ -261,11 +267,38 @@ mod tests { assert!(parsed.flags.interleaved_mode()); assert!(!parsed.flags.unknown_leap()); assert!(parsed.flags.interleaved_mode()); - assert_eq!(parsed.root_delay, NtpDuration::from_seconds(0.00877380371298031)); - assert_eq!(parsed.root_dispersion, NtpDuration::from_seconds(0.001007080078359479)); - assert_eq!(parsed.server_cookie, NtpServerCookie([0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08])); - assert_eq!(parsed.client_cookie, NtpClientCookie([0x08, 0x07, 0x06, 0x05, 0x04, 0x03, 0x02, 0x01])); - assert_eq!(parsed.receive_timestamp, NtpTimestamp::from_fixed_int(0x1111111111111111)); - assert_eq!(parsed.transmit_timestamp, NtpTimestamp::from_fixed_int(0x2222222222222222)); + assert_eq!( + parsed.root_delay, + NtpDuration::from_seconds(0.00877380371298031) + ); + assert_eq!( + parsed.root_dispersion, + NtpDuration::from_seconds(0.001007080078359479) + ); + assert_eq!( + parsed.server_cookie, + NtpServerCookie([0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08]) + ); + assert_eq!( + parsed.client_cookie, + NtpClientCookie([0x08, 0x07, 0x06, 0x05, 0x04, 0x03, 0x02, 0x01]) + ); + assert_eq!( + parsed.receive_timestamp, + NtpTimestamp::from_fixed_int(0x1111111111111111) + ); + assert_eq!( + parsed.transmit_timestamp, + NtpTimestamp::from_fixed_int(0x2222222222222222) + ); + } + + #[test] + fn deserialize_v5() { + let mut packet = [0u8; 48]; + // Alter LI VN Mode + packet[0] = 0b_00_101_011; + + NtpPacket::deserialize(&packet, &NoCipher).unwrap(); } } From 0a92d7960d6e096e8da91ca5fbd2095f16662c3c Mon Sep 17 00:00:00 2001 From: Tamme Dittrich Date: Tue, 10 Oct 2023 09:30:27 +0200 Subject: [PATCH 05/28] Add NTPv5 feature to CI and fuzzing This ensures that all tests are run with and without the new feature, and clippy detects any warnings also in the new code. --- .github/workflows/build.yaml | 11 +++++++---- fuzz/Cargo.toml | 3 +++ 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index ed9e7b07b..c6201f60a 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -29,6 +29,7 @@ jobs: - "" features: - "" + - "--features ntpv5" steps: - name: Checkout sources uses: actions/checkout@8ade135a41bc03ea155e62e844d188df1ea18608 @@ -67,6 +68,7 @@ jobs: os: [ubuntu-latest] features: - "" + - "--features ntpv5" steps: - name: Checkout sources uses: actions/checkout@8ade135a41bc03ea155e62e844d188df1ea18608 @@ -96,6 +98,7 @@ jobs: - "" features: - "" + - "--features ntpv5" steps: - name: Checkout sources uses: actions/checkout@8ade135a41bc03ea155e62e844d188df1ea18608 @@ -243,7 +246,7 @@ jobs: uses: actions-rs/cargo@844f36862e911db73fe0815f00a4a2602c279505 with: command: clippy - args: --workspace --all-targets -- -D warnings + args: --workspace --all-targets --all-features -- -D warnings - name: Run clippy (fuzzers) uses: actions-rs/cargo@844f36862e911db73fe0815f00a4a2602c279505 with: @@ -287,7 +290,7 @@ jobs: TARGET_CC: "/home/runner/.cargo/bin/cargo-zigbuild zig cc -- -target arm-linux-gnueabihf -mcpu=generic+v7a+vfp3-d32+thumb2-neon -g" with: command: clippy - args: --target armv7-unknown-linux-gnueabihf --workspace --all-targets -- -D warnings + args: --target armv7-unknown-linux-gnueabihf --workspace --all-targets --all-features -- -D warnings clippy-macos: name: ClippyMacOS @@ -321,7 +324,7 @@ jobs: TARGET_CC: "/home/runner/.cargo/bin/cargo-zigbuild zig cc -- -target x86_64-macos-gnu -g" with: command: clippy - args: --target x86_64-apple-darwin --workspace --all-targets -- -D warnings + args: --target x86_64-apple-darwin --workspace --all-targets --all-features -- -D warnings clippy-musl: name: ClippyMusl @@ -355,7 +358,7 @@ jobs: TARGET_CC: "/home/runner/.cargo/bin/cargo-zigbuild zig cc -- -target x86_64-linux-musl" with: command: clippy - args: --target x86_64-unknown-linux-musl --workspace --all-targets -- -D warnings + args: --target x86_64-unknown-linux-musl --workspace --all-targets --all-features -- -D warnings fuzz: name: Smoke-test fuzzing targets diff --git a/fuzz/Cargo.toml b/fuzz/Cargo.toml index 0fe015e28..c5382c115 100644 --- a/fuzz/Cargo.toml +++ b/fuzz/Cargo.toml @@ -8,6 +8,9 @@ publish = false [package.metadata] cargo-fuzz = true +[features] +ntpv5 = ["ntp-proto/ntpv5"] + [dependencies] rand = "0.8.5" From 36a8201c4645b1cd5b81874cdcac150b24bc6593 Mon Sep 17 00:00:00 2001 From: Tamme Dittrich Date: Tue, 10 Oct 2023 09:42:26 +0200 Subject: [PATCH 06/28] Address clippy warnings --- ntp-proto/src/packet/mod.rs | 7 ++++++- ntp-proto/src/packet/v5.rs | 26 ++++++++++++++++---------- 2 files changed, 22 insertions(+), 11 deletions(-) diff --git a/ntp-proto/src/packet/mod.rs b/ntp-proto/src/packet/mod.rs index 9e8620094..a7df1a07a 100644 --- a/ntp-proto/src/packet/mod.rs +++ b/ntp-proto/src/packet/mod.rs @@ -738,8 +738,13 @@ impl<'a> NtpPacket<'a> { match self.header { NtpHeader::V3(header) => header.mode, NtpHeader::V4(header) => header.mode, + + // FIXME long term the return type should change to capture both mode types #[cfg(feature = "ntpv5")] - NtpHeader::V5(_header) => todo!(), + NtpHeader::V5(header) => match header.mode { + v5::NtpMode::Request => NtpAssociationMode::Client, + v5::NtpMode::Response => NtpAssociationMode::Server, + }, } } diff --git a/ntp-proto/src/packet/v5.rs b/ntp-proto/src/packet/v5.rs index 3f0b41477..b0f777992 100644 --- a/ntp-proto/src/packet/v5.rs +++ b/ntp-proto/src/packet/v5.rs @@ -148,24 +148,25 @@ mod tests { #[test] fn flags() { let flags = NtpFlags(0x00); - assert_eq!(flags.unknown_leap(), false); - assert_eq!(flags.interleaved_mode(), false); + assert!(!flags.unknown_leap()); + assert!(!flags.interleaved_mode()); let flags = NtpFlags(0x01); - assert_eq!(flags.unknown_leap(), true); - assert_eq!(flags.interleaved_mode(), false); + assert!(flags.unknown_leap()); + assert!(!flags.interleaved_mode()); let flags = NtpFlags(0x02); - assert_eq!(flags.unknown_leap(), false); - assert_eq!(flags.interleaved_mode(), true); + assert!(!flags.unknown_leap()); + assert!(flags.interleaved_mode()); let flags = NtpFlags(0x03); - assert_eq!(flags.unknown_leap(), true); - assert_eq!(flags.interleaved_mode(), true); + assert!(flags.unknown_leap()); + assert!(flags.interleaved_mode()); } #[test] fn parse_request() { + #[allow(clippy::unusual_byte_groupings)] // Bits are grouped by fields #[rustfmt::skip] let data = [ // LI VN Mode @@ -223,6 +224,7 @@ mod tests { #[test] fn parse_resonse() { + #[allow(clippy::unusual_byte_groupings)] // Bits are grouped by fields #[rustfmt::skip] let data = [ // LI VN Mode @@ -296,8 +298,12 @@ mod tests { #[test] fn deserialize_v5() { let mut packet = [0u8; 48]; - // Alter LI VN Mode - packet[0] = 0b_00_101_011; + + #[allow(clippy::unusual_byte_groupings)] // Bits are grouped by fields + { + // Alter LI VN Mode + packet[0] = 0b_00_101_011; + } NtpPacket::deserialize(&packet, &NoCipher).unwrap(); } From b5a5591912e8ee637f2db19e33eec7957092b434 Mon Sep 17 00:00:00 2001 From: Marlon Baeten Date: Tue, 10 Oct 2023 11:05:57 +0200 Subject: [PATCH 07/28] Implement v5 header serialization --- ntp-proto/src/packet/v5.rs | 169 +++++++++++++++++++++++++++++-------- 1 file changed, 133 insertions(+), 36 deletions(-) diff --git a/ntp-proto/src/packet/v5.rs b/ntp-proto/src/packet/v5.rs index b0f777992..77cd23ce2 100644 --- a/ntp-proto/src/packet/v5.rs +++ b/ntp-proto/src/packet/v5.rs @@ -17,11 +17,18 @@ impl NtpMode { }) } - pub fn is_request(&self) -> bool { + fn to_bits(self) -> u8 { + match self { + Self::Request => 3, + Self::Response => 4, + } + } + + pub(crate) fn is_request(&self) -> bool { self == &Self::Request } - pub fn is_response(&self) -> bool { + pub(crate) fn is_response(&self) -> bool { self == &Self::Response } } @@ -45,25 +52,46 @@ impl NtpTimescale { _ => return None, }) } + + fn to_bits(self) -> u8 { + match self { + Self::Utc => 0, + Self::Tai => 1, + Self::Ut1 => 2, + Self::LeadSmearedUtc => 3, + } + } } #[derive(Debug, Copy, Clone, Eq, PartialEq)] pub struct NtpEra(pub u8); #[derive(Debug, Copy, Clone, Eq, PartialEq)] -pub struct NtpFlags(u16); +pub struct NtpFlags { + unknown_leap: bool, + interleaved_mode: bool, +} impl NtpFlags { fn from_bits(bits: [u8; 2]) -> Self { - Self(u16::from_be_bytes(bits)) + Self { + unknown_leap: bits[1] & 0x01 != 0, + interleaved_mode: bits[1] & 0x02 != 0, + } } - pub fn unknown_leap(&self) -> bool { - self.0 & 0x01 != 0 - } + fn to_bits(&self) -> [u8; 2] { + let mut flags: u16 = 0; + + if self.unknown_leap { + flags = flags | 0x01; + } - pub fn interleaved_mode(&self) -> bool { - self.0 & 0x02 != 0 + if self.interleaved_mode { + flags = flags | 0x02; + } + + flags.to_be_bytes() } } @@ -128,12 +156,28 @@ impl NtpHeaderV5 { Self::LENGTH, )) } + + pub(crate) fn serialize(&self, w: &mut W) -> std::io::Result<()> { + w.write_all(&[(self.leap.to_bits() << 6) | (5 << 3) | self.mode.to_bits()])?; + w.write_all(&[self.stratum, self.poll as u8, self.precision as u8])?; + w.write_all(&[self.timescale.to_bits()])?; + w.write_all(&[self.era.0])?; + w.write_all(&self.flags.to_bits())?; + w.write_all(&self.root_delay.to_bits_short())?; + w.write_all(&self.root_dispersion.to_bits_short())?; + w.write_all(&self.server_cookie.0)?; + w.write_all(&self.client_cookie.0)?; + w.write_all(&self.receive_timestamp.to_bits())?; + w.write_all(&self.transmit_timestamp.to_bits())?; + Ok(()) + } } #[cfg(test)] mod tests { + use std::io::Cursor; + use super::*; - use crate::{NoCipher, NtpPacket}; #[test] fn round_trip_timescale() { @@ -147,21 +191,21 @@ mod tests { #[test] fn flags() { - let flags = NtpFlags(0x00); - assert!(!flags.unknown_leap()); - assert!(!flags.interleaved_mode()); + let flags = NtpFlags::from_bits([0x00, 0x00]); + assert_eq!(flags.unknown_leap, false); + assert_eq!(flags.interleaved_mode, false); - let flags = NtpFlags(0x01); - assert!(flags.unknown_leap()); - assert!(!flags.interleaved_mode()); + let flags = NtpFlags::from_bits([0x00, 0x01]); + assert_eq!(flags.unknown_leap, true); + assert_eq!(flags.interleaved_mode, false); - let flags = NtpFlags(0x02); - assert!(!flags.unknown_leap()); - assert!(flags.interleaved_mode()); + let flags = NtpFlags::from_bits([0x00, 0x02]); + assert_eq!(flags.unknown_leap, false); + assert_eq!(flags.interleaved_mode, true); - let flags = NtpFlags(0x03); - assert!(flags.unknown_leap()); - assert!(flags.interleaved_mode()); + let flags = NtpFlags::from_bits([0x00, 0x03]); + assert_eq!(flags.unknown_leap, true); + assert_eq!(flags.interleaved_mode, true); } #[test] @@ -208,9 +252,9 @@ mod tests { assert_eq!(parsed.precision, 0); assert_eq!(parsed.timescale, NtpTimescale::Ut1); assert_eq!(parsed.era, NtpEra(0)); - assert!(parsed.flags.interleaved_mode()); - assert!(!parsed.flags.unknown_leap()); - assert!(parsed.flags.interleaved_mode()); + assert!(parsed.flags.interleaved_mode); + assert!(!parsed.flags.unknown_leap); + assert!(parsed.flags.interleaved_mode); assert_eq!(parsed.root_delay, NtpDuration::from_seconds(0.0)); assert_eq!(parsed.root_dispersion, NtpDuration::from_seconds(0.0)); assert_eq!(parsed.server_cookie, NtpServerCookie([0x0; 8])); @@ -220,6 +264,12 @@ mod tests { ); assert_eq!(parsed.receive_timestamp, NtpTimestamp::from_fixed_int(0x0)); assert_eq!(parsed.transmit_timestamp, NtpTimestamp::from_fixed_int(0x0)); + + let mut buffer: [u8; 48] = [0u8; 48]; + let mut cursor = Cursor::new(buffer.as_mut_slice()); + parsed.serialize(&mut cursor).unwrap(); + + assert_eq!(data, buffer); } #[test] @@ -266,9 +316,9 @@ mod tests { assert_eq!(parsed.precision, 6); assert_eq!(parsed.timescale, NtpTimescale::Tai); assert_eq!(parsed.era, NtpEra(7)); - assert!(parsed.flags.interleaved_mode()); - assert!(!parsed.flags.unknown_leap()); - assert!(parsed.flags.interleaved_mode()); + assert!(parsed.flags.interleaved_mode); + assert!(!parsed.flags.unknown_leap); + assert!(parsed.flags.interleaved_mode); assert_eq!( parsed.root_delay, NtpDuration::from_seconds(0.00877380371298031) @@ -293,18 +343,65 @@ mod tests { parsed.transmit_timestamp, NtpTimestamp::from_fixed_int(0x2222222222222222) ); + + let mut buffer: [u8; 48] = [0u8; 48]; + let mut cursor = Cursor::new(buffer.as_mut_slice()); + parsed.serialize(&mut cursor).unwrap(); + + assert_eq!(data, buffer); } #[test] - fn deserialize_v5() { - let mut packet = [0u8; 48]; - - #[allow(clippy::unusual_byte_groupings)] // Bits are grouped by fields - { - // Alter LI VN Mode - packet[0] = 0b_00_101_011; + fn test_encode_decode_roundtrip() { + for i in 0..=u8::MAX { + let header = NtpHeaderV5 { + leap: NtpLeapIndicator::from_bits(i % 4), + mode: NtpMode::from_bits(3 + (i % 2)).unwrap(), + stratum: i.wrapping_add(1), + poll: i.wrapping_add(3) as i8, + precision: i.wrapping_add(4) as i8, + timescale: NtpTimescale::from_bits(i % 4).unwrap(), + era: NtpEra(i.wrapping_add(6)), + flags: NtpFlags { + unknown_leap: i % 3 == 0, + interleaved_mode: i % 4 == 0, + }, + root_delay: NtpDuration::from_bits_short([i; 4]), + root_dispersion: NtpDuration::from_bits_short([i.wrapping_add(1); 4]), + server_cookie: NtpServerCookie([i.wrapping_add(2); 8]), + client_cookie: NtpClientCookie([i.wrapping_add(3); 8]), + receive_timestamp: NtpTimestamp::from_bits([i.wrapping_add(4); 8]), + transmit_timestamp: NtpTimestamp::from_bits([i.wrapping_add(5); 8]), + }; + + let mut buffer: [u8; 48] = [0u8; 48]; + let mut cursor = Cursor::new(buffer.as_mut_slice()); + header.serialize(&mut cursor).unwrap(); + + let (parsed, _) = NtpHeaderV5::deserialize(&buffer).unwrap(); + + assert_eq!(header, parsed); } + } + + #[test] + fn fail_on_incorrect_length() { + let data: [u8; 47] = [0u8; 47]; - NtpPacket::deserialize(&packet, &NoCipher).unwrap(); + assert!(matches!( + NtpHeaderV5::deserialize(&data), + Err(ParsingError::IncorrectLength) + )); + } + + #[test] + fn fail_on_incorrect_version() { + let mut data: [u8; 48] = [0u8; 48]; + data[0] = 0b_00_111_100; + + assert!(matches!( + NtpHeaderV5::deserialize(&data), + Err(ParsingError::InvalidVersion(7)) + )); } } From c98c43f79006b4ddc404c88b3427a3e5e324b543 Mon Sep 17 00:00:00 2001 From: Marlon Baeten Date: Tue, 10 Oct 2023 11:50:09 +0200 Subject: [PATCH 08/28] Remove panic from v5 header parsing --- ntp-proto/src/packet/error.rs | 30 +++++++++++++++++-- ntp-proto/src/packet/mod.rs | 12 +++++--- ntp-proto/src/packet/v5.rs | 55 +++++++++++++++++++---------------- 3 files changed, 65 insertions(+), 32 deletions(-) diff --git a/ntp-proto/src/packet/error.rs b/ntp-proto/src/packet/error.rs index cbe4b03bc..2be9cfeff 100644 --- a/ntp-proto/src/packet/error.rs +++ b/ntp-proto/src/packet/error.rs @@ -12,6 +12,12 @@ pub enum ParsingError { DecryptError(T), #[cfg(feature = "ntpv5")] InvalidDraftIdentification, + #[cfg(feature = "ntpv5")] + MalformedTimescale, + #[cfg(feature = "ntpv5")] + MalformedMode, + #[cfg(feature = "ntpv5")] + InvalidFlags, } impl ParsingError { @@ -24,9 +30,15 @@ impl ParsingError { MalformedNtsExtensionFields => Err(MalformedNtsExtensionFields), MalformedNonce => Err(MalformedNonce), MalformedCookiePlaceholder => Err(MalformedCookiePlaceholder), + DecryptError(decrypt_error) => Ok(decrypt_error), #[cfg(feature = "ntpv5")] InvalidDraftIdentification => Err(InvalidDraftIdentification), - DecryptError(decrypt_error) => Ok(decrypt_error), + #[cfg(feature = "ntpv5")] + MalformedTimescale => Err(MalformedTimescale), + #[cfg(feature = "ntpv5")] + MalformedMode => Err(MalformedMode), + #[cfg(feature = "ntpv5")] + InvalidFlags => Err(InvalidFlags), } } } @@ -41,9 +53,15 @@ impl ParsingError { MalformedNtsExtensionFields => MalformedNtsExtensionFields, MalformedNonce => MalformedNonce, MalformedCookiePlaceholder => MalformedCookiePlaceholder, + DecryptError(decrypt_error) => match decrypt_error {}, #[cfg(feature = "ntpv5")] InvalidDraftIdentification => InvalidDraftIdentification, - DecryptError(decrypt_error) => match decrypt_error {}, + #[cfg(feature = "ntpv5")] + MalformedTimescale => MalformedTimescale, + #[cfg(feature = "ntpv5")] + MalformedMode => MalformedMode, + #[cfg(feature = "ntpv5")] + InvalidFlags => InvalidFlags, } } } @@ -58,9 +76,15 @@ impl Display for ParsingError { Self::MalformedNtsExtensionFields => f.write_str("Malformed nts extension fields"), Self::MalformedNonce => f.write_str("Malformed nonce (likely invalid length)"), Self::MalformedCookiePlaceholder => f.write_str("Malformed cookie placeholder"), + Self::DecryptError(_) => f.write_str("Failed to decrypt NTS extension fields"), #[cfg(feature = "ntpv5")] Self::InvalidDraftIdentification => f.write_str("Draft Identification invalid"), - Self::DecryptError(_) => f.write_str("Failed to decrypt NTS extension fields"), + #[cfg(feature = "ntpv5")] + Self::MalformedTimescale => f.write_str("Malformed timescale"), + #[cfg(feature = "ntpv5")] + Self::MalformedMode => f.write_str("Malformed mode"), + #[cfg(feature = "ntpv5")] + Self::InvalidFlags => f.write_str("Invalid flags specified"), } } } diff --git a/ntp-proto/src/packet/mod.rs b/ntp-proto/src/packet/mod.rs index a7df1a07a..746d95cd9 100644 --- a/ntp-proto/src/packet/mod.rs +++ b/ntp-proto/src/packet/mod.rs @@ -425,14 +425,14 @@ impl<'a> NtpPacket<'a> { NtpHeader::V3(header) => header.serialize(w, 3)?, NtpHeader::V4(header) => header.serialize(w, 4)?, #[cfg(feature = "ntpv5")] - NtpHeader::V5(_header) => todo!(), + NtpHeader::V5(header) => header.serialize(w)?, }; match self.header { NtpHeader::V3(_) => { /* No extension fields in V3 */ } NtpHeader::V4(_) => self.efdata.serialize(w, cipher)?, #[cfg(feature = "ntpv5")] - NtpHeader::V5(_header) => todo!(), + NtpHeader::V5(_) => self.efdata.serialize(w, cipher)?, } if let Some(ref mac) = self.mac { @@ -1180,12 +1180,16 @@ mod tests { assert!(NtpPacket::deserialize(packet, &NoCipher).is_err()); let packet = b"\x14\x02\x06\xe9\x00\x00\x02\x36\x00\x00\x03\xb7\xc0\x35\x67\x6c\xe5\xf6\x61\xfd\x6f\x16\x5f\x03\xe5\xf6\x63\xa8\x76\x19\xef\x40\xe5\xf6\x63\xa8\x79\x8c\x65\x81\xe5\xf6\x63\xa8\x79\x8e\xae\x2b"; assert!(NtpPacket::deserialize(packet, &NoCipher).is_err()); - let packet = b"\x2B\x02\x06\xe9\x00\x00\x02\x36\x00\x00\x03\xb7\xc0\x35\x67\x6c\xe5\xf6\x61\xfd\x6f\x16\x5f\x03\xe5\xf6\x63\xa8\x76\x19\xef\x40\xe5\xf6\x63\xa8\x79\x8c\x65\x81\xe5\xf6\x63\xa8\x79\x8e\xae\x2b"; - assert!(NtpPacket::deserialize(packet, &NoCipher).is_err()); let packet = b"\x34\x02\x06\xe9\x00\x00\x02\x36\x00\x00\x03\xb7\xc0\x35\x67\x6c\xe5\xf6\x61\xfd\x6f\x16\x5f\x03\xe5\xf6\x63\xa8\x76\x19\xef\x40\xe5\xf6\x63\xa8\x79\x8c\x65\x81\xe5\xf6\x63\xa8\x79\x8e\xae\x2b"; assert!(NtpPacket::deserialize(packet, &NoCipher).is_err()); let packet = b"\x3B\x02\x06\xe9\x00\x00\x02\x36\x00\x00\x03\xb7\xc0\x35\x67\x6c\xe5\xf6\x61\xfd\x6f\x16\x5f\x03\xe5\xf6\x63\xa8\x76\x19\xef\x40\xe5\xf6\x63\xa8\x79\x8c\x65\x81\xe5\xf6\x63\xa8\x79\x8e\xae\x2b"; assert!(NtpPacket::deserialize(packet, &NoCipher).is_err()); + + #[cfg(not(feature = "ntpv5"))] + { + let packet = b"\x2C\x02\x06\xe9\x00\x00\x02\x36\x00\x00\x03\xb7\xc0\x35\x67\x6c\xe5\xf6\x61\xfd\x6f\x16\x5f\x03\xe5\xf6\x63\xa8\x76\x19\xef\x40\xe5\xf6\x63\xa8\x79\x8c\x65\x81\xe5\xf6\x63\xa8\x79\x8e\xae\x2b"; + assert!(NtpPacket::deserialize(packet, &NoCipher).is_err()); + } } #[test] diff --git a/ntp-proto/src/packet/v5.rs b/ntp-proto/src/packet/v5.rs index 77cd23ce2..bb071f234 100644 --- a/ntp-proto/src/packet/v5.rs +++ b/ntp-proto/src/packet/v5.rs @@ -9,11 +9,11 @@ pub enum NtpMode { } impl NtpMode { - fn from_bits(bits: u8) -> Option { - Some(match bits { + fn from_bits(bits: u8) -> Result> { + Ok(match bits { 3 => Self::Request, 4 => Self::Response, - _ => return None, + _ => return Err(ParsingError::MalformedMode), }) } @@ -24,10 +24,12 @@ impl NtpMode { } } + #[allow(dead_code)] pub(crate) fn is_request(&self) -> bool { self == &Self::Request } + #[allow(dead_code)] pub(crate) fn is_response(&self) -> bool { self == &Self::Response } @@ -43,13 +45,13 @@ pub enum NtpTimescale { } impl NtpTimescale { - fn from_bits(bits: u8) -> Option { - Some(match bits { + fn from_bits(bits: u8) -> Result> { + Ok(match bits { 0 => Self::Utc, 1 => Self::Tai, 2 => Self::Ut1, 3 => Self::LeadSmearedUtc, - _ => return None, + _ => return Err(ParsingError::MalformedTimescale), }) } @@ -73,22 +75,26 @@ pub struct NtpFlags { } impl NtpFlags { - fn from_bits(bits: [u8; 2]) -> Self { - Self { + fn from_bits(bits: [u8; 2]) -> Result> { + if bits[0] != 0x00 || bits[1] & 0xFC != 0 { + return Err(ParsingError::InvalidFlags); + } + + Ok(Self { unknown_leap: bits[1] & 0x01 != 0, interleaved_mode: bits[1] & 0x02 != 0, - } + }) } - fn to_bits(&self) -> [u8; 2] { + fn as_bits(&self) -> [u8; 2] { let mut flags: u16 = 0; if self.unknown_leap { - flags = flags | 0x01; + flags |= 0x01; } if self.interleaved_mode { - flags = flags | 0x02; + flags |= 0x02; } flags.to_be_bytes() @@ -139,13 +145,13 @@ impl NtpHeaderV5 { Ok(( Self { leap: NtpLeapIndicator::from_bits((data[0] & 0xC0) >> 6), - mode: NtpMode::from_bits(data[0] & 0x07).unwrap(), + mode: NtpMode::from_bits(data[0] & 0x07)?, stratum: data[1], poll: data[2] as i8, precision: data[3] as i8, - timescale: NtpTimescale::from_bits(data[4]).unwrap(), + timescale: NtpTimescale::from_bits(data[4])?, era: NtpEra(data[5]), - flags: NtpFlags::from_bits(data[6..8].try_into().unwrap()), + flags: NtpFlags::from_bits(data[6..8].try_into().unwrap())?, root_delay: NtpDuration::from_bits_short(data[8..12].try_into().unwrap()), root_dispersion: NtpDuration::from_bits_short(data[12..16].try_into().unwrap()), server_cookie: NtpServerCookie(data[16..24].try_into().unwrap()), @@ -157,12 +163,13 @@ impl NtpHeaderV5 { )) } + #[allow(dead_code)] pub(crate) fn serialize(&self, w: &mut W) -> std::io::Result<()> { w.write_all(&[(self.leap.to_bits() << 6) | (5 << 3) | self.mode.to_bits()])?; w.write_all(&[self.stratum, self.poll as u8, self.precision as u8])?; w.write_all(&[self.timescale.to_bits()])?; w.write_all(&[self.era.0])?; - w.write_all(&self.flags.to_bits())?; + w.write_all(&self.flags.as_bits())?; w.write_all(&self.root_delay.to_bits_short())?; w.write_all(&self.root_dispersion.to_bits_short())?; w.write_all(&self.server_cookie.0)?; @@ -175,35 +182,33 @@ impl NtpHeaderV5 { #[cfg(test)] mod tests { - use std::io::Cursor; - use super::*; + use std::io::Cursor; #[test] fn round_trip_timescale() { for i in 0..=u8::MAX { - match NtpTimescale::from_bits(i) { - None => {} - Some(ts) => assert_eq!(ts as u8, i), + if let Ok(ts) = NtpTimescale::from_bits(i) { + assert_eq!(ts as u8, i); } } } #[test] fn flags() { - let flags = NtpFlags::from_bits([0x00, 0x00]); + let flags = NtpFlags::from_bits([0x00, 0x00]).unwrap(); assert_eq!(flags.unknown_leap, false); assert_eq!(flags.interleaved_mode, false); - let flags = NtpFlags::from_bits([0x00, 0x01]); + let flags = NtpFlags::from_bits([0x00, 0x01]).unwrap(); assert_eq!(flags.unknown_leap, true); assert_eq!(flags.interleaved_mode, false); - let flags = NtpFlags::from_bits([0x00, 0x02]); + let flags = NtpFlags::from_bits([0x00, 0x02]).unwrap(); assert_eq!(flags.unknown_leap, false); assert_eq!(flags.interleaved_mode, true); - let flags = NtpFlags::from_bits([0x00, 0x03]); + let flags = NtpFlags::from_bits([0x00, 0x03]).unwrap(); assert_eq!(flags.unknown_leap, true); assert_eq!(flags.interleaved_mode, true); } From 906584f01879a828f8aedd6351a41391cb8c2e58 Mon Sep 17 00:00:00 2001 From: Marlon Baeten Date: Tue, 10 Oct 2023 12:05:52 +0200 Subject: [PATCH 09/28] Fix clippy warnings in v5.rs --- ntp-proto/src/packet/v5.rs | 33 +++++++++++++++------------------ 1 file changed, 15 insertions(+), 18 deletions(-) diff --git a/ntp-proto/src/packet/v5.rs b/ntp-proto/src/packet/v5.rs index bb071f234..e684c9149 100644 --- a/ntp-proto/src/packet/v5.rs +++ b/ntp-proto/src/packet/v5.rs @@ -87,17 +87,10 @@ impl NtpFlags { } fn as_bits(&self) -> [u8; 2] { - let mut flags: u16 = 0; + let mut flags = self.unknown_leap as u8; + flags |= 0x02 * self.interleaved_mode as u8; - if self.unknown_leap { - flags |= 0x01; - } - - if self.interleaved_mode { - flags |= 0x02; - } - - flags.to_be_bytes() + [0x00, flags] } } @@ -197,20 +190,23 @@ mod tests { #[test] fn flags() { let flags = NtpFlags::from_bits([0x00, 0x00]).unwrap(); - assert_eq!(flags.unknown_leap, false); - assert_eq!(flags.interleaved_mode, false); + assert!(!flags.unknown_leap); + assert!(!flags.interleaved_mode); let flags = NtpFlags::from_bits([0x00, 0x01]).unwrap(); - assert_eq!(flags.unknown_leap, true); - assert_eq!(flags.interleaved_mode, false); + assert!(flags.unknown_leap); + assert!(!flags.interleaved_mode); let flags = NtpFlags::from_bits([0x00, 0x02]).unwrap(); - assert_eq!(flags.unknown_leap, false); - assert_eq!(flags.interleaved_mode, true); + assert!(!flags.unknown_leap); + assert!(flags.interleaved_mode); let flags = NtpFlags::from_bits([0x00, 0x03]).unwrap(); - assert_eq!(flags.unknown_leap, true); - assert_eq!(flags.interleaved_mode, true); + assert!(flags.unknown_leap); + assert!(flags.interleaved_mode); + + let result = NtpFlags::from_bits([0xFF, 0xFF]); + assert!(matches!(result, Err(ParsingError::InvalidFlags))); } #[test] @@ -400,6 +396,7 @@ mod tests { } #[test] + #[allow(clippy::unusual_byte_groupings)] // Bits are grouped by fields fn fail_on_incorrect_version() { let mut data: [u8; 48] = [0u8; 48]; data[0] = 0b_00_111_100; From c0c9b7d69fb493d0a08b3f46068ab8567a3f5479 Mon Sep 17 00:00:00 2001 From: Tamme Dittrich Date: Tue, 17 Oct 2023 09:22:19 +0200 Subject: [PATCH 10/28] Add extension field ids for all draft extensions --- ntp-proto/src/packet/v5.rs | 3 + ntp-proto/src/packet/v5/extension_fields.rs | 88 +++++++++++++++++++++ 2 files changed, 91 insertions(+) create mode 100644 ntp-proto/src/packet/v5/extension_fields.rs diff --git a/ntp-proto/src/packet/v5.rs b/ntp-proto/src/packet/v5.rs index e684c9149..27a5aea8b 100644 --- a/ntp-proto/src/packet/v5.rs +++ b/ntp-proto/src/packet/v5.rs @@ -1,6 +1,9 @@ use crate::packet::error::ParsingError; use crate::{NtpDuration, NtpLeapIndicator, NtpTimestamp}; +#[allow(dead_code)] +pub mod extension_fields; + #[repr(u8)] #[derive(Debug, PartialEq, Eq, Copy, Clone)] pub enum NtpMode { diff --git a/ntp-proto/src/packet/v5/extension_fields.rs b/ntp-proto/src/packet/v5/extension_fields.rs new file mode 100644 index 000000000..f930b9cdd --- /dev/null +++ b/ntp-proto/src/packet/v5/extension_fields.rs @@ -0,0 +1,88 @@ +#[derive(Debug, Copy, Clone, Eq, PartialEq)] +pub enum Type { + DraftIdentification, + Padding, + Mac, + ReferenceIdRequest, + ReferenceIdResponse, + ServerInformation, + Correction, + ReferenceTimestamp, + MonotonicReceiveTimestamp, + SecondaryReceiveTimestamp, + Unknown(u16), +} + +impl Type { + pub fn from_bits(bits: u16) -> Self { + match bits { + 0xF5FF => Self::DraftIdentification, + 0xF501 => Self::Padding, + 0xF502 => Self::Mac, + 0xF503 => Self::ReferenceIdRequest, + 0xF504 => Self::ReferenceIdResponse, + 0xF505 => Self::ServerInformation, + 0xF506 => Self::Correction, + 0xF507 => Self::ReferenceTimestamp, + 0xF508 => Self::MonotonicReceiveTimestamp, + 0xF509 => Self::SecondaryReceiveTimestamp, + other => Self::Unknown(other), + } + } + + pub fn to_bits(self) -> u16 { + match self { + Self::DraftIdentification => 0xF5FF, + Self::Padding => 0xF501, + Self::Mac => 0xF502, + Self::ReferenceIdRequest => 0xF503, + Self::ReferenceIdResponse => 0xF504, + Self::ServerInformation => 0xF505, + Self::Correction => 0xF506, + Self::ReferenceTimestamp => 0xF507, + Self::MonotonicReceiveTimestamp => 0xF508, + Self::SecondaryReceiveTimestamp => 0xF509, + Self::Unknown(other) => other, + } + } + + #[cfg(test)] + fn all_known() -> impl Iterator { + [ + Self::DraftIdentification, + Self::Padding, + Self::Mac, + Self::ReferenceIdRequest, + Self::ReferenceIdResponse, + Self::ServerInformation, + Self::Correction, + Self::ReferenceTimestamp, + Self::MonotonicReceiveTimestamp, + Self::SecondaryReceiveTimestamp, + ] + .iter() + .copied() + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn type_round_trip() { + for i in 0..=u16::MAX { + let ty = Type::from_bits(i); + assert_eq!(i, ty.to_bits()); + } + + for ty in Type::all_known() { + let bits = ty.to_bits(); + let ty2 = Type::from_bits(bits); + assert_eq!(ty, ty2); + + let bits2 = ty2.to_bits(); + assert_eq!(bits, bits2); + } + } +} From 19e557760ffe140fd7d0222f2eee07e66772e8ac Mon Sep 17 00:00:00 2001 From: Tamme Dittrich Date: Tue, 17 Oct 2023 09:30:26 +0200 Subject: [PATCH 11/28] Address PR comment and simplify draft id deserialization --- ntp-proto/src/packet/extensionfields.rs | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-) diff --git a/ntp-proto/src/packet/extensionfields.rs b/ntp-proto/src/packet/extensionfields.rs index cc0961c6c..6c7d5d695 100644 --- a/ntp-proto/src/packet/extensionfields.rs +++ b/ntp-proto/src/packet/extensionfields.rs @@ -422,22 +422,13 @@ impl<'a> ExtensionField<'a> { fn decode_draft_identification( message: &'a [u8], ) -> Result> { - let msg = message[..].into(); - - let new_msg = match msg { - Cow::Borrowed(slice) => std::str::from_utf8(slice).ok().map(Cow::Borrowed), - Cow::Owned(vec) => String::from_utf8(vec).ok().map(Cow::Owned), - }; - - let Some(new_msg) = new_msg else { - return Err(ParsingError::InvalidDraftIdentification); + let di = match core::str::from_utf8(message) { + Err(_) => return Err(ParsingError::InvalidDraftIdentification), + Ok(di) if !di.is_ascii() => return Err(ParsingError::InvalidDraftIdentification), + Ok(di) => di, }; - if !new_msg.is_ascii() { - return Err(ParsingError::InvalidDraftIdentification); - } - - Ok(ExtensionField::DraftIdentification(new_msg)) + Ok(ExtensionField::DraftIdentification(Cow::Borrowed(di))) } fn decode(raw: RawExtensionField<'a>) -> Result> { From e7d2c66173c981d0b650efaec52835e784657b7c Mon Sep 17 00:00:00 2001 From: Tamme Dittrich Date: Tue, 17 Oct 2023 09:37:10 +0200 Subject: [PATCH 12/28] Address PR comment, fix typo, add comments --- ntp-proto/src/packet/extensionfields.rs | 6 +++--- ntp-proto/src/packet/mod.rs | 13 +++++++------ 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/ntp-proto/src/packet/extensionfields.rs b/ntp-proto/src/packet/extensionfields.rs index 6c7d5d695..a953f9cdb 100644 --- a/ntp-proto/src/packet/extensionfields.rs +++ b/ntp-proto/src/packet/extensionfields.rs @@ -870,9 +870,9 @@ mod tests { let test_id = "draft-ietf-ntp-ntpv5-00\0"; let len = u16::try_from(4 + test_id.len()).unwrap(); let mut data = vec![]; - data.extend_from_slice(&[0xF5, 0xFF]); - data.extend_from_slice(&len.to_be_bytes()); - data.extend_from_slice(test_id.as_bytes()); + data.extend(&[0xF5, 0xFF]); + data.extend(&len.to_be_bytes()); + data.extend(test_id.as_bytes()); let raw = RawExtensionField::deserialize(&data, 0).unwrap(); let ef = ExtensionField::decode(raw).unwrap(); diff --git a/ntp-proto/src/packet/mod.rs b/ntp-proto/src/packet/mod.rs index 746d95cd9..5361c4375 100644 --- a/ntp-proto/src/packet/mod.rs +++ b/ntp-proto/src/packet/mod.rs @@ -325,7 +325,7 @@ impl<'a> NtpPacket<'a> { let (header, header_size) = NtpHeaderV3V4::deserialize(data).map_err(|e| e.generalize())?; - let contruct_packet = |remaining_bytes: &'a [u8], efdata| { + let construct_packet = |remaining_bytes: &'a [u8], efdata| { let mac = if !remaining_bytes.is_empty() { Some(Mac::deserialize(remaining_bytes)?) } else { @@ -343,7 +343,7 @@ impl<'a> NtpPacket<'a> { match ExtensionFieldData::deserialize(data, header_size, cipher) { Ok(decoded) => { - let packet = contruct_packet(decoded.remaining_bytes, decoded.efdata) + let packet = construct_packet(decoded.remaining_bytes, decoded.efdata) .map_err(|e| e.generalize())?; Ok((packet, decoded.cookie)) @@ -352,7 +352,7 @@ impl<'a> NtpPacket<'a> { // return early if it is anything but a decrypt error let invalid = e.get_decrypt_error()?; - let packet = contruct_packet(invalid.remaining_bytes, invalid.efdata) + let packet = construct_packet(invalid.remaining_bytes, invalid.efdata) .map_err(|e| e.generalize())?; Err(ParsingError::DecryptError(packet)) @@ -364,7 +364,7 @@ impl<'a> NtpPacket<'a> { let (header, header_size) = v5::NtpHeaderV5::deserialize(data).map_err(|e| e.generalize())?; - let contruct_packet = |remaining_bytes: &'a [u8], efdata| { + let construct_packet = |remaining_bytes: &'a [u8], efdata| { let mac = if !remaining_bytes.is_empty() { Some(Mac::deserialize(remaining_bytes)?) } else { @@ -383,7 +383,7 @@ impl<'a> NtpPacket<'a> { // TODO: Check extension field handling in V5 match ExtensionFieldData::deserialize(data, header_size, cipher) { Ok(decoded) => { - let packet = contruct_packet(decoded.remaining_bytes, decoded.efdata) + let packet = construct_packet(decoded.remaining_bytes, decoded.efdata) .map_err(|e| e.generalize())?; Ok((packet, decoded.cookie)) @@ -392,7 +392,7 @@ impl<'a> NtpPacket<'a> { // return early if it is anything but a decrypt error let invalid = e.get_decrypt_error()?; - let packet = contruct_packet(invalid.remaining_bytes, invalid.efdata) + let packet = construct_packet(invalid.remaining_bytes, invalid.efdata) .map_err(|e| e.generalize())?; Err(ParsingError::DecryptError(packet)) @@ -1187,6 +1187,7 @@ mod tests { #[cfg(not(feature = "ntpv5"))] { + // Version 5 packet should not parse without the ntpv5 feature let packet = b"\x2C\x02\x06\xe9\x00\x00\x02\x36\x00\x00\x03\xb7\xc0\x35\x67\x6c\xe5\xf6\x61\xfd\x6f\x16\x5f\x03\xe5\xf6\x63\xa8\x76\x19\xef\x40\xe5\xf6\x63\xa8\x79\x8c\x65\x81\xe5\xf6\x63\xa8\x79\x8e\xae\x2b"; assert!(NtpPacket::deserialize(packet, &NoCipher).is_err()); } From f9266f2d68f2337b3f0d49c2e94cd6a748a86c73 Mon Sep 17 00:00:00 2001 From: Marlon Baeten Date: Tue, 17 Oct 2023 10:09:04 +0200 Subject: [PATCH 13/28] Move v5 module to subdirectory --- ntp-proto/src/packet/crypto.rs | 2 +- .../{extensionfields.rs => extension_fields.rs} | 2 +- ntp-proto/src/packet/mod.rs | 6 +++--- ntp-proto/src/packet/{v5.rs => v5/mod.rs} | 11 +++++++++-- 4 files changed, 14 insertions(+), 7 deletions(-) rename ntp-proto/src/packet/{extensionfields.rs => extension_fields.rs} (99%) rename ntp-proto/src/packet/{v5.rs => v5/mod.rs} (98%) diff --git a/ntp-proto/src/packet/crypto.rs b/ntp-proto/src/packet/crypto.rs index fbb1abe21..3d2b29bd6 100644 --- a/ntp-proto/src/packet/crypto.rs +++ b/ntp-proto/src/packet/crypto.rs @@ -5,7 +5,7 @@ use zeroize::{Zeroize, ZeroizeOnDrop}; use crate::keyset::DecodedServerCookie; -use super::extensionfields::ExtensionField; +use super::extension_fields::ExtensionField; #[derive(Debug, thiserror::Error)] #[error("Could not decrypt ciphertext")] diff --git a/ntp-proto/src/packet/extensionfields.rs b/ntp-proto/src/packet/extension_fields.rs similarity index 99% rename from ntp-proto/src/packet/extensionfields.rs rename to ntp-proto/src/packet/extension_fields.rs index a953f9cdb..5bb8751a3 100644 --- a/ntp-proto/src/packet/extensionfields.rs +++ b/ntp-proto/src/packet/extension_fields.rs @@ -782,7 +782,7 @@ const fn next_multiple_of_usize(lhs: usize, rhs: usize) -> usize { mod tests { use crate::{ keyset::KeySet, - packet::{extensionfields::ExtensionFieldTypeId, AesSivCmac256}, + packet::{extension_fields::ExtensionFieldTypeId, AesSivCmac256}, }; use super::*; diff --git a/ntp-proto/src/packet/mod.rs b/ntp-proto/src/packet/mod.rs index 5361c4375..55254a00b 100644 --- a/ntp-proto/src/packet/mod.rs +++ b/ntp-proto/src/packet/mod.rs @@ -11,11 +11,11 @@ use crate::{ time_types::{NtpDuration, NtpTimestamp, PollInterval}, }; -use self::{error::ParsingError, extensionfields::ExtensionFieldData, mac::Mac}; +use self::{error::ParsingError, extension_fields::ExtensionFieldData, mac::Mac}; mod crypto; mod error; -mod extensionfields; +mod extension_fields; mod mac; #[cfg(feature = "ntpv5")] @@ -26,7 +26,7 @@ pub use crypto::{ EncryptResult, NoCipher, }; pub use error::PacketParsingError; -pub use extensionfields::ExtensionField; +pub use extension_fields::ExtensionField; #[derive(Debug, Copy, Clone, PartialEq, Eq, Serialize, Deserialize)] pub enum NtpLeapIndicator { diff --git a/ntp-proto/src/packet/v5.rs b/ntp-proto/src/packet/v5/mod.rs similarity index 98% rename from ntp-proto/src/packet/v5.rs rename to ntp-proto/src/packet/v5/mod.rs index 27a5aea8b..d7799a3f4 100644 --- a/ntp-proto/src/packet/v5.rs +++ b/ntp-proto/src/packet/v5/mod.rs @@ -90,8 +90,15 @@ impl NtpFlags { } fn as_bits(&self) -> [u8; 2] { - let mut flags = self.unknown_leap as u8; - flags |= 0x02 * self.interleaved_mode as u8; + let mut flags: u8 = 0; + + if self.unknown_leap { + flags |= 0x01; + } + + if self.interleaved_mode { + flags |= 0x02; + } [0x00, flags] } From fa9693923f76f6e0a3a0e5c11112cf0df3e80d9f Mon Sep 17 00:00:00 2001 From: Tamme Dittrich Date: Tue, 17 Oct 2023 10:54:29 +0200 Subject: [PATCH 14/28] Handle V5 extension fields --- ntp-proto/src/packet/extension_fields.rs | 125 +++++++++++++++-------- ntp-proto/src/packet/mod.rs | 15 ++- 2 files changed, 95 insertions(+), 45 deletions(-) diff --git a/ntp-proto/src/packet/extension_fields.rs b/ntp-proto/src/packet/extension_fields.rs index 5bb8751a3..ee0bca164 100644 --- a/ntp-proto/src/packet/extension_fields.rs +++ b/ntp-proto/src/packet/extension_fields.rs @@ -521,6 +521,7 @@ impl<'a> ExtensionFieldData<'a> { data: &'a [u8], header_size: usize, cipher: &impl CipherProvider, + version: ExtensionHeaderVersion, ) -> Result, ParsingError>> { use ExtensionField::InvalidNtsEncryptedField; @@ -533,9 +534,10 @@ impl<'a> ExtensionFieldData<'a> { &data[header_size..], Mac::MAXIMUM_SIZE, RawExtensionField::V4_UNENCRYPTED_MINIMUM_SIZE, + version, ) { let (offset, field) = field.map_err(|e| e.generalize())?; - size = offset + field.wire_length(); + size = offset + field.wire_length(version); match field.type_id { ExtensionFieldTypeId::NtsEncryptedField => { let encrypted = RawEncryptedField::from_message_bytes(field.message_bytes) @@ -550,18 +552,21 @@ impl<'a> ExtensionFieldData<'a> { } }; - let encrypted_fields = - match encrypted.decrypt(cipher.as_ref(), &data[..header_size + offset]) { - Ok(encrypted_fields) => encrypted_fields, - Err(e) => { - // early return if it's anything but a decrypt error - e.get_decrypt_error()?; + let encrypted_fields = match encrypted.decrypt( + cipher.as_ref(), + &data[..header_size + offset], + version, + ) { + Ok(encrypted_fields) => encrypted_fields, + Err(e) => { + // early return if it's anything but a decrypt error + e.get_decrypt_error()?; - efdata.untrusted.push(InvalidNtsEncryptedField); - is_valid_nts = false; - continue; - } - }; + efdata.untrusted.push(InvalidNtsEncryptedField); + is_valid_nts = false; + continue; + } + }; // for the current ciphers we allow in non-test code, // the nonce should always be 16 bytes @@ -638,6 +643,7 @@ impl<'a> RawEncryptedField<'a> { &self, cipher: &dyn Cipher, aad: &[u8], + version: ExtensionHeaderVersion, ) -> Result>, ParsingError>> { let plaintext = match cipher.decrypt(self.nonce, self.ciphertext, aad) { Ok(plain) => plain, @@ -648,22 +654,34 @@ impl<'a> RawEncryptedField<'a> { } }; - RawExtensionField::deserialize_sequence(&plaintext, 0, RawExtensionField::BARE_MINIMUM_SIZE) - .map(|encrypted_field| { - let encrypted_field = encrypted_field.map_err(|e| e.generalize())?.1; - if encrypted_field.type_id == ExtensionFieldTypeId::NtsEncryptedField { - // TODO: Discuss whether we want this check - Err(ParsingError::MalformedNtsExtensionFields) - } else { - Ok(ExtensionField::decode(encrypted_field) - .map_err(|e| e.generalize())? - .into_owned()) - } - }) - .collect() + RawExtensionField::deserialize_sequence( + &plaintext, + 0, + RawExtensionField::BARE_MINIMUM_SIZE, + version, + ) + .map(|encrypted_field| { + let encrypted_field = encrypted_field.map_err(|e| e.generalize())?.1; + if encrypted_field.type_id == ExtensionFieldTypeId::NtsEncryptedField { + // TODO: Discuss whether we want this check + Err(ParsingError::MalformedNtsExtensionFields) + } else { + Ok(ExtensionField::decode(encrypted_field) + .map_err(|e| e.generalize())? + .into_owned()) + } + }) + .collect() } } +#[derive(Debug, Copy, Clone, PartialEq, Eq)] +pub(super) enum ExtensionHeaderVersion { + V4, + #[cfg(feature = "ntpv5")] + V5, +} + #[derive(Debug)] struct RawExtensionField<'a> { type_id: ExtensionFieldTypeId, @@ -676,21 +694,24 @@ impl<'a> RawExtensionField<'a> { const BARE_MINIMUM_SIZE: usize = 4; const V4_UNENCRYPTED_MINIMUM_SIZE: usize = 4; - fn wire_length(&self) -> usize { + fn wire_length(&self, version: ExtensionHeaderVersion) -> usize { // field type + length + value + padding let length = 2 + 2 + self.message_bytes.len(); - // All extension fields are zero-padded to a word (four octets) boundary. - // - // message_bytes should include this padding, so this should already be true - debug_assert_eq!(length % 4, 0); + if version == ExtensionHeaderVersion::V4 { + // All extension fields are zero-padded to a word (four octets) boundary. + // + // message_bytes should include this padding, so this should already be true + debug_assert_eq!(length % 4, 0); + } - next_multiple_of_usize(length, 4) + length.next_multiple_of(4) } fn deserialize( data: &'a [u8], minimum_size: usize, + version: ExtensionHeaderVersion, ) -> Result> { use ParsingError::IncorrectLength; @@ -704,9 +725,13 @@ impl<'a> RawExtensionField<'a> { // the entire extension field in octets, including the Padding field. let field_length = u16::from_be_bytes([b2, b3]) as usize; - // padding is up to a multiple of 4 bytes, so a valid field length is divisible by 4 - if field_length < minimum_size || field_length % 4 != 0 { - return Err(ParsingError::IncorrectLength); + if field_length < minimum_size { + return Err(IncorrectLength); + } + + // In NTPv4: padding is up to a multiple of 4 bytes, so a valid field length is divisible by 4 + if version == ExtensionHeaderVersion::V4 && field_length % 4 != 0 { + return Err(IncorrectLength); } // because the field length includes padding, the message bytes may not exactly match the input @@ -722,6 +747,7 @@ impl<'a> RawExtensionField<'a> { buffer: &'a [u8], cutoff: usize, minimum_size: usize, + version: ExtensionHeaderVersion, ) -> impl Iterator< Item = Result<(usize, RawExtensionField<'a>), ParsingError>, > + 'a { @@ -730,6 +756,7 @@ impl<'a> RawExtensionField<'a> { cutoff, minimum_size, offset: 0, + version, } } } @@ -738,6 +765,7 @@ struct ExtensionFieldStreamer<'a> { cutoff: usize, minimum_size: usize, offset: usize, + version: ExtensionHeaderVersion, } impl<'a> Iterator for ExtensionFieldStreamer<'a> { @@ -750,10 +778,10 @@ impl<'a> Iterator for ExtensionFieldStreamer<'a> { return None; } - match RawExtensionField::deserialize(remaining, self.minimum_size) { + match RawExtensionField::deserialize(remaining, self.minimum_size, self.version) { Ok(field) => { let offset = self.offset; - self.offset += field.wire_length(); + self.offset += field.wire_length(self.version); Some(Ok((offset, field))) } Err(error) => { @@ -874,7 +902,7 @@ mod tests { data.extend(&len.to_be_bytes()); data.extend(test_id.as_bytes()); - let raw = RawExtensionField::deserialize(&data, 0).unwrap(); + let raw = RawExtensionField::deserialize(&data, 0, ExtensionHeaderVersion::V5).unwrap(); let ef = ExtensionField::decode(raw).unwrap(); match ef { @@ -976,7 +1004,12 @@ mod tests { let message_bytes = &w.as_ref()[..expected_length]; - let mut it = RawExtensionField::deserialize_sequence(message_bytes, 0, 0); + let mut it = RawExtensionField::deserialize_sequence( + message_bytes, + 0, + 0, + ExtensionHeaderVersion::V4, + ); let field = it.next().unwrap().unwrap(); assert!(it.next().is_none()); @@ -989,7 +1022,9 @@ mod tests { }, ) => { let raw = RawEncryptedField::from_message_bytes(message_bytes).unwrap(); - let decrypted_fields = raw.decrypt(&cipher, &[]).unwrap(); + let decrypted_fields = raw + .decrypt(&cipher, &[], ExtensionHeaderVersion::V4) + .unwrap(); assert_eq!(decrypted_fields, fields_to_encrypt); } _ => panic!("invalid"), @@ -1101,7 +1136,8 @@ mod tests { let cipher = crate::packet::crypto::NoCipher; - let result = ExtensionFieldData::deserialize(slice, 0, &cipher).unwrap(); + let result = + ExtensionFieldData::deserialize(slice, 0, &cipher, ExtensionHeaderVersion::V4).unwrap(); let DeserializedExtensionField { efdata, @@ -1140,7 +1176,8 @@ mod tests { let cipher = crate::packet::crypto::NoCipher; - let result = ExtensionFieldData::deserialize(slice, 0, &cipher).unwrap_err(); + let result = ExtensionFieldData::deserialize(slice, 0, &cipher, ExtensionHeaderVersion::V4) + .unwrap_err(); let ParsingError::DecryptError(InvalidNtsExtensionField { efdata, @@ -1182,7 +1219,8 @@ mod tests { let c2s = [0; 32]; let cipher = AesSivCmac256::new(c2s.into()); - let result = ExtensionFieldData::deserialize(slice, 0, &cipher).unwrap_err(); + let result = ExtensionFieldData::deserialize(slice, 0, &cipher, ExtensionHeaderVersion::V4) + .unwrap_err(); let ParsingError::DecryptError(InvalidNtsExtensionField { efdata, @@ -1222,7 +1260,8 @@ mod tests { let n = cursor.position() as usize; let slice = &w.as_slice()[..n]; - let result = ExtensionFieldData::deserialize(slice, 0, &keyset).unwrap(); + let result = + ExtensionFieldData::deserialize(slice, 0, &keyset, ExtensionHeaderVersion::V4).unwrap(); let DeserializedExtensionField { efdata, diff --git a/ntp-proto/src/packet/mod.rs b/ntp-proto/src/packet/mod.rs index 55254a00b..c6305e3fe 100644 --- a/ntp-proto/src/packet/mod.rs +++ b/ntp-proto/src/packet/mod.rs @@ -10,6 +10,7 @@ use crate::{ system::SystemSnapshot, time_types::{NtpDuration, NtpTimestamp, PollInterval}, }; +use extension_fields::ExtensionHeaderVersion; use self::{error::ParsingError, extension_fields::ExtensionFieldData, mac::Mac}; @@ -341,7 +342,12 @@ impl<'a> NtpPacket<'a> { Ok::<_, ParsingError>(packet) }; - match ExtensionFieldData::deserialize(data, header_size, cipher) { + match ExtensionFieldData::deserialize( + data, + header_size, + cipher, + ExtensionHeaderVersion::V4, + ) { Ok(decoded) => { let packet = construct_packet(decoded.remaining_bytes, decoded.efdata) .map_err(|e| e.generalize())?; @@ -381,7 +387,12 @@ impl<'a> NtpPacket<'a> { }; // TODO: Check extension field handling in V5 - match ExtensionFieldData::deserialize(data, header_size, cipher) { + match ExtensionFieldData::deserialize( + data, + header_size, + cipher, + ExtensionHeaderVersion::V5, + ) { Ok(decoded) => { let packet = construct_packet(decoded.remaining_bytes, decoded.efdata) .map_err(|e| e.generalize())?; From ca4ccc2e3b260d3a9e83c08bc09a936b7a96ce2d Mon Sep 17 00:00:00 2001 From: Tamme Dittrich Date: Tue, 17 Oct 2023 13:04:43 +0200 Subject: [PATCH 15/28] Undo change that violated MSRV --- ntp-proto/src/packet/extension_fields.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ntp-proto/src/packet/extension_fields.rs b/ntp-proto/src/packet/extension_fields.rs index ee0bca164..e643ebdb4 100644 --- a/ntp-proto/src/packet/extension_fields.rs +++ b/ntp-proto/src/packet/extension_fields.rs @@ -705,7 +705,7 @@ impl<'a> RawExtensionField<'a> { debug_assert_eq!(length % 4, 0); } - length.next_multiple_of(4) + next_multiple_of_usize(length, 4) } fn deserialize( From ac6682c7022f1e093a2d6c1e13029b4d25800376 Mon Sep 17 00:00:00 2001 From: Tamme Dittrich Date: Tue, 17 Oct 2023 13:19:43 +0200 Subject: [PATCH 16/28] Move NTPv5 errors in seperate enum This reduced clutter of `cfg(feature)` in NTPv4 modules. --- ntp-proto/src/packet/error.rs | 32 +++--------------------- ntp-proto/src/packet/extension_fields.rs | 5 ++-- ntp-proto/src/packet/v5/error.rs | 26 +++++++++++++++++++ ntp-proto/src/packet/v5/mod.rs | 16 ++++++++---- 4 files changed, 44 insertions(+), 35 deletions(-) create mode 100644 ntp-proto/src/packet/v5/error.rs diff --git a/ntp-proto/src/packet/error.rs b/ntp-proto/src/packet/error.rs index 2be9cfeff..b50b34b99 100644 --- a/ntp-proto/src/packet/error.rs +++ b/ntp-proto/src/packet/error.rs @@ -11,13 +11,7 @@ pub enum ParsingError { MalformedCookiePlaceholder, DecryptError(T), #[cfg(feature = "ntpv5")] - InvalidDraftIdentification, - #[cfg(feature = "ntpv5")] - MalformedTimescale, - #[cfg(feature = "ntpv5")] - MalformedMode, - #[cfg(feature = "ntpv5")] - InvalidFlags, + V5(super::v5::V5Error), } impl ParsingError { @@ -32,13 +26,7 @@ impl ParsingError { MalformedCookiePlaceholder => Err(MalformedCookiePlaceholder), DecryptError(decrypt_error) => Ok(decrypt_error), #[cfg(feature = "ntpv5")] - InvalidDraftIdentification => Err(InvalidDraftIdentification), - #[cfg(feature = "ntpv5")] - MalformedTimescale => Err(MalformedTimescale), - #[cfg(feature = "ntpv5")] - MalformedMode => Err(MalformedMode), - #[cfg(feature = "ntpv5")] - InvalidFlags => Err(InvalidFlags), + V5(e) => Err(V5(e)), } } } @@ -55,13 +43,7 @@ impl ParsingError { MalformedCookiePlaceholder => MalformedCookiePlaceholder, DecryptError(decrypt_error) => match decrypt_error {}, #[cfg(feature = "ntpv5")] - InvalidDraftIdentification => InvalidDraftIdentification, - #[cfg(feature = "ntpv5")] - MalformedTimescale => MalformedTimescale, - #[cfg(feature = "ntpv5")] - MalformedMode => MalformedMode, - #[cfg(feature = "ntpv5")] - InvalidFlags => InvalidFlags, + V5(e) => V5(e), } } } @@ -78,13 +60,7 @@ impl Display for ParsingError { Self::MalformedCookiePlaceholder => f.write_str("Malformed cookie placeholder"), Self::DecryptError(_) => f.write_str("Failed to decrypt NTS extension fields"), #[cfg(feature = "ntpv5")] - Self::InvalidDraftIdentification => f.write_str("Draft Identification invalid"), - #[cfg(feature = "ntpv5")] - Self::MalformedTimescale => f.write_str("Malformed timescale"), - #[cfg(feature = "ntpv5")] - Self::MalformedMode => f.write_str("Malformed mode"), - #[cfg(feature = "ntpv5")] - Self::InvalidFlags => f.write_str("Invalid flags specified"), + Self::V5(e) => Display::fmt(e, f), } } } diff --git a/ntp-proto/src/packet/extension_fields.rs b/ntp-proto/src/packet/extension_fields.rs index e643ebdb4..fb276cdfd 100644 --- a/ntp-proto/src/packet/extension_fields.rs +++ b/ntp-proto/src/packet/extension_fields.rs @@ -422,9 +422,10 @@ impl<'a> ExtensionField<'a> { fn decode_draft_identification( message: &'a [u8], ) -> Result> { + let err = Err(super::v5::V5Error::InvalidDraftIdentification.into()); let di = match core::str::from_utf8(message) { - Err(_) => return Err(ParsingError::InvalidDraftIdentification), - Ok(di) if !di.is_ascii() => return Err(ParsingError::InvalidDraftIdentification), + Err(_) => return err, + Ok(di) if !di.is_ascii() => return err, Ok(di) => di, }; diff --git a/ntp-proto/src/packet/v5/error.rs b/ntp-proto/src/packet/v5/error.rs new file mode 100644 index 000000000..e8c6ebd72 --- /dev/null +++ b/ntp-proto/src/packet/v5/error.rs @@ -0,0 +1,26 @@ +use std::fmt::{Display, Formatter}; + +#[derive(Debug)] +pub enum V5Error { + InvalidDraftIdentification, + MalformedTimescale, + MalformedMode, + InvalidFlags, +} + +impl Display for V5Error { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + match self { + Self::InvalidDraftIdentification => f.write_str("Draft Identification invalid"), + Self::MalformedTimescale => f.write_str("Malformed timescale"), + Self::MalformedMode => f.write_str("Malformed mode"), + Self::InvalidFlags => f.write_str("Invalid flags specified"), + } + } +} + +impl From for crate::packet::error::ParsingError { + fn from(value: V5Error) -> Self { + Self::V5(value) + } +} diff --git a/ntp-proto/src/packet/v5/mod.rs b/ntp-proto/src/packet/v5/mod.rs index d7799a3f4..731d702fb 100644 --- a/ntp-proto/src/packet/v5/mod.rs +++ b/ntp-proto/src/packet/v5/mod.rs @@ -1,9 +1,12 @@ -use crate::packet::error::ParsingError; use crate::{NtpDuration, NtpLeapIndicator, NtpTimestamp}; +mod error; #[allow(dead_code)] pub mod extension_fields; +use crate::packet::error::ParsingError; +pub use error::V5Error; + #[repr(u8)] #[derive(Debug, PartialEq, Eq, Copy, Clone)] pub enum NtpMode { @@ -16,7 +19,7 @@ impl NtpMode { Ok(match bits { 3 => Self::Request, 4 => Self::Response, - _ => return Err(ParsingError::MalformedMode), + _ => return Err(V5Error::MalformedMode.into()), }) } @@ -54,7 +57,7 @@ impl NtpTimescale { 1 => Self::Tai, 2 => Self::Ut1, 3 => Self::LeadSmearedUtc, - _ => return Err(ParsingError::MalformedTimescale), + _ => return Err(V5Error::MalformedTimescale.into()), }) } @@ -80,7 +83,7 @@ pub struct NtpFlags { impl NtpFlags { fn from_bits(bits: [u8; 2]) -> Result> { if bits[0] != 0x00 || bits[1] & 0xFC != 0 { - return Err(ParsingError::InvalidFlags); + return Err(V5Error::InvalidFlags.into()); } Ok(Self { @@ -216,7 +219,10 @@ mod tests { assert!(flags.interleaved_mode); let result = NtpFlags::from_bits([0xFF, 0xFF]); - assert!(matches!(result, Err(ParsingError::InvalidFlags))); + assert!(matches!( + result, + Err(ParsingError::V5(V5Error::InvalidFlags)) + )); } #[test] From c7fe2280981b447ca85aa1a12ba0835efd096518 Mon Sep 17 00:00:00 2001 From: Tamme Dittrich Date: Tue, 17 Oct 2023 13:21:56 +0200 Subject: [PATCH 17/28] Switch flags from using hex to binary for better readability --- ntp-proto/src/packet/v5/mod.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/ntp-proto/src/packet/v5/mod.rs b/ntp-proto/src/packet/v5/mod.rs index 731d702fb..913ee7d0d 100644 --- a/ntp-proto/src/packet/v5/mod.rs +++ b/ntp-proto/src/packet/v5/mod.rs @@ -87,8 +87,8 @@ impl NtpFlags { } Ok(Self { - unknown_leap: bits[1] & 0x01 != 0, - interleaved_mode: bits[1] & 0x02 != 0, + unknown_leap: bits[1] & 0b01 != 0, + interleaved_mode: bits[1] & 0b10 != 0, }) } @@ -96,11 +96,11 @@ impl NtpFlags { let mut flags: u8 = 0; if self.unknown_leap { - flags |= 0x01; + flags |= 0b01; } if self.interleaved_mode { - flags |= 0x02; + flags |= 0b10; } [0x00, flags] From d92043402a7ad3e66b321652a3c96d82a8cdd37b Mon Sep 17 00:00:00 2001 From: Marlon Baeten Date: Tue, 17 Oct 2023 13:47:18 +0200 Subject: [PATCH 18/28] Encode correct length in v5 extension fields --- ntp-proto/src/packet/extension_fields.rs | 187 ++++++++++++++++++----- ntp-proto/src/packet/mod.rs | 8 +- 2 files changed, 156 insertions(+), 39 deletions(-) diff --git a/ntp-proto/src/packet/extension_fields.rs b/ntp-proto/src/packet/extension_fields.rs index fb276cdfd..c756f446f 100644 --- a/ntp-proto/src/packet/extension_fields.rs +++ b/ntp-proto/src/packet/extension_fields.rs @@ -116,21 +116,30 @@ impl<'a> ExtensionField<'a> { } } - fn serialize(&self, w: &mut W, minimum_size: u16) -> std::io::Result<()> { + fn serialize( + &self, + w: &mut W, + minimum_size: u16, + version: ExtensionHeaderVersion, + ) -> std::io::Result<()> { use ExtensionField::*; match self { - Unknown { type_id, data } => Self::encode_unknown(w, *type_id, data, minimum_size), + Unknown { type_id, data } => { + Self::encode_unknown(w, *type_id, data, minimum_size, version) + } UniqueIdentifier(identifier) => { - Self::encode_unique_identifier(w, identifier, minimum_size) + Self::encode_unique_identifier(w, identifier, minimum_size, version) } - NtsCookie(cookie) => Self::encode_nts_cookie(w, cookie, minimum_size), + NtsCookie(cookie) => Self::encode_nts_cookie(w, cookie, minimum_size, version), NtsCookiePlaceholder { cookie_length: body_length, - } => Self::encode_nts_cookie_placeholder(w, *body_length, minimum_size), + } => Self::encode_nts_cookie_placeholder(w, *body_length, minimum_size, version), InvalidNtsEncryptedField => Err(std::io::ErrorKind::Other.into()), #[cfg(feature = "ntpv5")] - DraftIdentification(data) => Self::encode_draft_identification(w, data, minimum_size), + DraftIdentification(data) => { + Self::encode_draft_identification(w, data, minimum_size, version) + } } } @@ -148,6 +157,7 @@ impl<'a> ExtensionField<'a> { ef_id: ExtensionFieldTypeId, data_length: usize, minimum_size: u16, + version: ExtensionHeaderVersion, ) -> std::io::Result<()> { if data_length > u16::MAX as usize - 4 { return Err(std::io::Error::new( @@ -158,9 +168,12 @@ impl<'a> ExtensionField<'a> { // u16 for the type_id, u16 for the length let header_width = 4; + let mut actual_length = (data_length as u16 + header_width).max(minimum_size); + + if version == ExtensionHeaderVersion::V4 { + actual_length = next_multiple_of_u16(actual_length, 4) + } - let actual_length = - next_multiple_of_u16((data_length as u16 + header_width).max(minimum_size), 4); w.write_all(&ef_id.to_type_id().to_be_bytes())?; w.write_all(&actual_length.to_be_bytes()) } @@ -203,12 +216,14 @@ impl<'a> ExtensionField<'a> { w: &mut W, identifier: &[u8], minimum_size: u16, + version: ExtensionHeaderVersion, ) -> std::io::Result<()> { Self::encode_framing( w, ExtensionFieldTypeId::UniqueIdentifier, identifier.len(), minimum_size, + version, )?; w.write_all(identifier)?; Self::encode_padding(w, identifier.len(), minimum_size) @@ -218,12 +233,14 @@ impl<'a> ExtensionField<'a> { w: &mut W, cookie: &[u8], minimum_size: u16, + version: ExtensionHeaderVersion, ) -> std::io::Result<()> { Self::encode_framing( w, ExtensionFieldTypeId::NtsCookie, cookie.len(), minimum_size, + version, )?; w.write_all(cookie)?; @@ -237,12 +254,14 @@ impl<'a> ExtensionField<'a> { w: &mut W, cookie_length: u16, minimum_size: u16, + version: ExtensionHeaderVersion, ) -> std::io::Result<()> { Self::encode_framing( w, ExtensionFieldTypeId::NtsCookiePlaceholder, cookie_length as usize, minimum_size, + version, )?; Self::write_zeros(w, cookie_length)?; @@ -257,12 +276,14 @@ impl<'a> ExtensionField<'a> { type_id: u16, data: &[u8], minimum_size: u16, + version: ExtensionHeaderVersion, ) -> std::io::Result<()> { Self::encode_framing( w, ExtensionFieldTypeId::Unknown { type_id }, data.len(), minimum_size, + version, )?; w.write_all(data)?; @@ -276,6 +297,7 @@ impl<'a> ExtensionField<'a> { w: &mut Cursor<&mut [u8]>, fields_to_encrypt: &[ExtensionField], cipher: &dyn Cipher, + version: ExtensionHeaderVersion, ) -> std::io::Result<()> { let padding = [0; 4]; @@ -294,7 +316,7 @@ impl<'a> ExtensionField<'a> { // RFC 8915, section 5.5: contrary to the RFC 7822 requirement that fields have a minimum length of 16 or 28 octets, // encrypted extension fields MAY be arbitrarily short (but still MUST be a multiple of 4 octets in length) let minimum_size = 0; - field.serialize(w, minimum_size)?; + field.serialize(w, minimum_size, version)?; } let plaintext_length = w.position() - plaintext_start; @@ -363,12 +385,14 @@ impl<'a> ExtensionField<'a> { w: &mut impl Write, data: &str, minimum_size: u16, + version: ExtensionHeaderVersion, ) -> std::io::Result<()> { Self::encode_framing( w, ExtensionFieldTypeId::DraftIdentification, data.len(), minimum_size, + version, )?; w.write_all(data.as_bytes())?; @@ -485,6 +509,7 @@ impl<'a> ExtensionFieldData<'a> { &self, w: &mut Cursor<&mut [u8]>, cipher: &(impl CipherProvider + ?Sized), + version: ExtensionHeaderVersion, ) -> std::io::Result<()> { if !self.authenticated.is_empty() || !self.encrypted.is_empty() { let cipher = match cipher.get(&self.authenticated) { @@ -497,13 +522,13 @@ impl<'a> ExtensionFieldData<'a> { let minimum_size = 16; for field in &self.authenticated { - field.serialize(w, minimum_size)?; + field.serialize(w, minimum_size, version)?; } // RFC 8915, section 5.5: contrary to the RFC 7822 requirement that fields have a minimum length of 16 or 28 octets, // encrypted extension fields MAY be arbitrarily short (but still MUST be a multiple of 4 octets in length) // hence we don't provide a minimum size here - ExtensionField::encode_encrypted(w, &self.encrypted, cipher.as_ref())?; + ExtensionField::encode_encrypted(w, &self.encrypted, cipher.as_ref(), version)?; } // per RFC 7822, section 7.5.1.4. @@ -511,7 +536,7 @@ impl<'a> ExtensionFieldData<'a> { while let Some(field) = it.next() { let is_last = it.peek().is_none(); let minimum_size = if is_last { 28 } else { 16 }; - field.serialize(w, minimum_size)?; + field.serialize(w, minimum_size, version)?; } Ok(()) @@ -828,7 +853,13 @@ mod tests { fn test_unique_identifier() { let identifier: Vec<_> = (0..16).collect(); let mut w = vec![]; - ExtensionField::encode_unique_identifier(&mut w, &identifier, 0).unwrap(); + ExtensionField::encode_unique_identifier( + &mut w, + &identifier, + 0, + ExtensionHeaderVersion::V4, + ) + .unwrap(); assert_eq!( w, @@ -840,7 +871,7 @@ mod tests { fn test_nts_cookie() { let cookie: Vec<_> = (0..16).collect(); let mut w = vec![]; - ExtensionField::encode_nts_cookie(&mut w, &cookie, 0).unwrap(); + ExtensionField::encode_nts_cookie(&mut w, &cookie, 0, ExtensionHeaderVersion::V4).unwrap(); assert_eq!( w, @@ -853,7 +884,13 @@ mod tests { const COOKIE_LENGTH: usize = 16; let mut w = vec![]; - ExtensionField::encode_nts_cookie_placeholder(&mut w, COOKIE_LENGTH as u16, 0).unwrap(); + ExtensionField::encode_nts_cookie_placeholder( + &mut w, + COOKIE_LENGTH as u16, + 0, + ExtensionHeaderVersion::V4, + ) + .unwrap(); assert_eq!( w, @@ -885,7 +922,7 @@ mod tests { fn test_unknown() { let data: Vec<_> = (0..16).collect(); let mut w = vec![]; - ExtensionField::encode_unknown(&mut w, 42, &data, 0).unwrap(); + ExtensionField::encode_unknown(&mut w, 42, &data, 0, ExtensionHeaderVersion::V4).unwrap(); assert_eq!( w, @@ -914,11 +951,39 @@ mod tests { } let mut out = vec![]; - ef.serialize(&mut out, len).unwrap(); + ef.serialize(&mut out, len, ExtensionHeaderVersion::V4) + .unwrap(); assert_eq!(&out, &data); } + #[cfg(feature = "ntpv5")] + #[test] + fn extension_field_length() { + let data: Vec<_> = (0..21).collect(); + let mut w = vec![]; + ExtensionField::encode_unknown(&mut w, 42, &data, 16, ExtensionHeaderVersion::V4).unwrap(); + let raw: RawExtensionField<'_> = + RawExtensionField::deserialize(&w, 16, ExtensionHeaderVersion::V4).unwrap(); + + // v4 extension field header length includes padding bytes + assert_eq!(w[3], 28); + assert_eq!(w.len(), 28); + assert_eq!(raw.message_bytes.len(), 24); + assert_eq!(raw.wire_length(ExtensionHeaderVersion::V4), 28); + + let mut w = vec![]; + ExtensionField::encode_unknown(&mut w, 42, &data, 16, ExtensionHeaderVersion::V5).unwrap(); + let raw: RawExtensionField<'_> = + RawExtensionField::deserialize(&w, 16, ExtensionHeaderVersion::V5).unwrap(); + + // v5 extension field header length does not include padding bytes + assert_eq!(w[3], 25); + assert_eq!(w.len(), 28); + assert_eq!(raw.message_bytes.len(), 21); + assert_eq!(raw.wire_length(ExtensionHeaderVersion::V5), 28); + } + #[test] fn extension_field_minimum_size() { let minimum_size = 32; @@ -926,20 +991,33 @@ mod tests { let data: Vec<_> = (0..16).collect(); let mut w = vec![]; - ExtensionField::encode_unique_identifier(&mut w, &data, minimum_size).unwrap(); + ExtensionField::encode_unique_identifier( + &mut w, + &data, + minimum_size, + ExtensionHeaderVersion::V4, + ) + .unwrap(); assert_eq!(w.len(), expected_size); let mut w = vec![]; - ExtensionField::encode_nts_cookie(&mut w, &data, minimum_size).unwrap(); + ExtensionField::encode_nts_cookie(&mut w, &data, minimum_size, ExtensionHeaderVersion::V4) + .unwrap(); assert_eq!(w.len(), expected_size); let mut w = vec![]; - ExtensionField::encode_nts_cookie_placeholder(&mut w, data.len() as u16, minimum_size) - .unwrap(); + ExtensionField::encode_nts_cookie_placeholder( + &mut w, + data.len() as u16, + minimum_size, + ExtensionHeaderVersion::V4, + ) + .unwrap(); assert_eq!(w.len(), expected_size); let mut w = vec![]; - ExtensionField::encode_unknown(&mut w, 42, &data, minimum_size).unwrap(); + ExtensionField::encode_unknown(&mut w, 42, &data, minimum_size, ExtensionHeaderVersion::V4) + .unwrap(); assert_eq!(w.len(), expected_size); // NOTE: encryped fields do not have a minimum_size @@ -952,20 +1030,33 @@ mod tests { let data: Vec<_> = (0..15).collect(); // 15 bytes, so padding is needed let mut w = vec![]; - ExtensionField::encode_unique_identifier(&mut w, &data, minimum_size).unwrap(); + ExtensionField::encode_unique_identifier( + &mut w, + &data, + minimum_size, + ExtensionHeaderVersion::V4, + ) + .unwrap(); assert_eq!(w.len(), expected_size); let mut w = vec![]; - ExtensionField::encode_nts_cookie(&mut w, &data, minimum_size).unwrap(); + ExtensionField::encode_nts_cookie(&mut w, &data, minimum_size, ExtensionHeaderVersion::V4) + .unwrap(); assert_eq!(w.len(), expected_size); let mut w = vec![]; - ExtensionField::encode_nts_cookie_placeholder(&mut w, data.len() as u16, minimum_size) - .unwrap(); + ExtensionField::encode_nts_cookie_placeholder( + &mut w, + data.len() as u16, + minimum_size, + ExtensionHeaderVersion::V4, + ) + .unwrap(); assert_eq!(w.len(), expected_size); let mut w = vec![]; - ExtensionField::encode_unknown(&mut w, 42, &data, minimum_size).unwrap(); + ExtensionField::encode_unknown(&mut w, 42, &data, minimum_size, ExtensionHeaderVersion::V4) + .unwrap(); assert_eq!(w.len(), expected_size); let mut w = [0u8; 128]; @@ -975,7 +1066,13 @@ mod tests { let fields_to_encrypt = [ExtensionField::UniqueIdentifier(Cow::Borrowed( data.as_slice(), ))]; - ExtensionField::encode_encrypted(&mut cursor, &fields_to_encrypt, &cipher).unwrap(); + ExtensionField::encode_encrypted( + &mut cursor, + &fields_to_encrypt, + &cipher, + ExtensionHeaderVersion::V4, + ) + .unwrap(); assert_eq!( cursor.position() as usize, 2 + 6 + c2s.len() + expected_size @@ -998,7 +1095,13 @@ mod tests { let mut w = [0u8; 128]; let mut cursor = Cursor::new(w.as_mut_slice()); - ExtensionField::encode_encrypted(&mut cursor, &fields_to_encrypt, &cipher).unwrap(); + ExtensionField::encode_encrypted( + &mut cursor, + &fields_to_encrypt, + &cipher, + ExtensionHeaderVersion::V4, + ) + .unwrap(); let expected_length = 2 + 6 + next_multiple_of_usize(nonce_length, 4) + plaintext_length; assert_eq!(cursor.position() as usize, expected_length,); @@ -1047,7 +1150,9 @@ mod tests { let mut w = [0u8; 128]; let mut cursor = Cursor::new(w.as_mut_slice()); - assert!(data.serialize(&mut cursor, &cipher).is_err()); + assert!(data + .serialize(&mut cursor, &cipher, ExtensionHeaderVersion::V4) + .is_err()); } // but succeed when the cipher is not needed @@ -1060,7 +1165,9 @@ mod tests { let mut w = [0u8; 128]; let mut cursor = Cursor::new(w.as_mut_slice()); - assert!(data.serialize(&mut cursor, &cipher).is_ok()); + assert!(data + .serialize(&mut cursor, &cipher, ExtensionHeaderVersion::V4) + .is_ok()); } } @@ -1079,7 +1186,8 @@ mod tests { let mut w = [0u8; 128]; let mut cursor = Cursor::new(w.as_mut_slice()); - data.serialize(&mut cursor, &cipher).unwrap(); + data.serialize(&mut cursor, &cipher, ExtensionHeaderVersion::V4) + .unwrap(); let n = cursor.position() as usize; let slice = &w.as_slice()[..n]; @@ -1104,7 +1212,8 @@ mod tests { let mut w = [0u8; 128]; let mut cursor = Cursor::new(w.as_mut_slice()); - data.serialize(&mut cursor, &cipher).unwrap(); + data.serialize(&mut cursor, &cipher, ExtensionHeaderVersion::V4) + .unwrap(); let n = cursor.position() as usize; let slice = &w.as_slice()[..n]; @@ -1130,7 +1239,8 @@ mod tests { let mut w = [0u8; 128]; let mut cursor = Cursor::new(w.as_mut_slice()); - data.serialize(&mut cursor, &cipher).unwrap(); + data.serialize(&mut cursor, &cipher, ExtensionHeaderVersion::V4) + .unwrap(); let n = cursor.position() as usize; let slice = &w.as_slice()[..n]; @@ -1170,7 +1280,8 @@ mod tests { let mut w = [0u8; 128]; let mut cursor = Cursor::new(w.as_mut_slice()); - data.serialize(&mut cursor, &cipher).unwrap(); + data.serialize(&mut cursor, &cipher, ExtensionHeaderVersion::V4) + .unwrap(); let n = cursor.position() as usize; let slice = &w.as_slice()[..n]; @@ -1211,7 +1322,8 @@ mod tests { let mut w = [0u8; 128]; let mut cursor = Cursor::new(w.as_mut_slice()); - data.serialize(&mut cursor, &cipher).unwrap(); + data.serialize(&mut cursor, &cipher, ExtensionHeaderVersion::V4) + .unwrap(); let n = cursor.position() as usize; let slice = &w.as_slice()[..n]; @@ -1256,7 +1368,8 @@ mod tests { let mut w = [0u8; 256]; let mut cursor = Cursor::new(w.as_mut_slice()); - data.serialize(&mut cursor, &keyset).unwrap(); + data.serialize(&mut cursor, &keyset, ExtensionHeaderVersion::V4) + .unwrap(); let n = cursor.position() as usize; let slice = &w.as_slice()[..n]; diff --git a/ntp-proto/src/packet/mod.rs b/ntp-proto/src/packet/mod.rs index c6305e3fe..258cb1eac 100644 --- a/ntp-proto/src/packet/mod.rs +++ b/ntp-proto/src/packet/mod.rs @@ -441,9 +441,13 @@ impl<'a> NtpPacket<'a> { match self.header { NtpHeader::V3(_) => { /* No extension fields in V3 */ } - NtpHeader::V4(_) => self.efdata.serialize(w, cipher)?, + NtpHeader::V4(_) => self + .efdata + .serialize(w, cipher, ExtensionHeaderVersion::V4)?, #[cfg(feature = "ntpv5")] - NtpHeader::V5(_) => self.efdata.serialize(w, cipher)?, + NtpHeader::V5(_) => self + .efdata + .serialize(w, cipher, ExtensionHeaderVersion::V5)?, } if let Some(ref mac) = self.mac { From 9f24396d321224b9d5fd310dbe1d8fff3ec1fddd Mon Sep 17 00:00:00 2001 From: Marlon Baeten Date: Tue, 17 Oct 2023 13:59:50 +0200 Subject: [PATCH 19/28] Include ExtensionHeaderVersion in fuzzer call --- ntp-proto/src/packet/extension_fields.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/ntp-proto/src/packet/extension_fields.rs b/ntp-proto/src/packet/extension_fields.rs index c756f446f..63218d3ed 100644 --- a/ntp-proto/src/packet/extension_fields.rs +++ b/ntp-proto/src/packet/extension_fields.rs @@ -148,8 +148,9 @@ impl<'a> ExtensionField<'a> { &self, w: &mut W, minimum_size: u16, + version: ExtensionHeaderVersion, ) -> std::io::Result<()> { - self.serialize(w, minimum_size) + self.serialize(w, minimum_size, version) } fn encode_framing( @@ -702,7 +703,7 @@ impl<'a> RawEncryptedField<'a> { } #[derive(Debug, Copy, Clone, PartialEq, Eq)] -pub(super) enum ExtensionHeaderVersion { +pub enum ExtensionHeaderVersion { V4, #[cfg(feature = "ntpv5")] V5, From a2cd7772365ce4f734ea203e0e87619cc0a808ec Mon Sep 17 00:00:00 2001 From: Tamme Dittrich Date: Tue, 17 Oct 2023 14:38:16 +0200 Subject: [PATCH 20/28] Implement missing setters and getters for v5 header --- ntp-proto/src/packet/mod.rs | 74 +++++++++++++++++++++++++--------- ntp-proto/src/packet/v5/mod.rs | 53 ++++++++++++++++++++++-- 2 files changed, 104 insertions(+), 23 deletions(-) diff --git a/ntp-proto/src/packet/mod.rs b/ntp-proto/src/packet/mod.rs index 258cb1eac..620181a2e 100644 --- a/ntp-proto/src/packet/mod.rs +++ b/ntp-proto/src/packet/mod.rs @@ -545,7 +545,28 @@ impl<'a> NtpPacket<'a> { mac: None, }, #[cfg(feature = "ntpv5")] - NtpHeader::V5(_header) => todo!(), + NtpHeader::V5(header) => NtpPacket { + // TODO deduplicate extension handling with V4 + header: NtpHeader::V5(v5::NtpHeaderV5::timestamp_response( + system, + header, + recv_timestamp, + clock, + )), + efdata: ExtensionFieldData { + authenticated: vec![], + encrypted: vec![], + // Ignore encrypted so as not to accidentaly leak anything + untrusted: input + .efdata + .untrusted + .into_iter() + .chain(input.efdata.authenticated) + .filter(|ef| matches!(ef, ExtensionField::UniqueIdentifier(_))) + .collect(), + }, + mac: None, + }, } } @@ -604,7 +625,7 @@ impl<'a> NtpPacket<'a> { mac: None, }, #[cfg(feature = "ntpv5")] - NtpHeader::V5(_header) => todo!(), + NtpHeader::V5(_header) => todo!("No NTS support for V5 yet"), } } @@ -632,7 +653,7 @@ impl<'a> NtpPacket<'a> { mac: None, }, #[cfg(feature = "ntpv5")] - NtpHeader::V5(_header) => todo!(), + NtpHeader::V5(_header) => todo!("NTPv5 does not have KISS codes yet"), } } @@ -654,7 +675,7 @@ impl<'a> NtpPacket<'a> { mac: None, }, #[cfg(feature = "ntpv5")] - NtpHeader::V5(_header) => todo!(), + NtpHeader::V5(_header) => todo!("No NTS support yet"), } } @@ -682,7 +703,7 @@ impl<'a> NtpPacket<'a> { mac: None, }, #[cfg(feature = "ntpv5")] - NtpHeader::V5(_header) => todo!(), + NtpHeader::V5(_header) => todo!("NTPv5 does not have KISS codes yet"), } } @@ -704,7 +725,7 @@ impl<'a> NtpPacket<'a> { mac: None, }, #[cfg(feature = "ntpv5")] - NtpHeader::V5(_header) => todo!(), + NtpHeader::V5(_header) => todo!("No NTS support for NTPv5 yet"), } } @@ -727,7 +748,7 @@ impl<'a> NtpPacket<'a> { mac: None, }, #[cfg(feature = "ntpv5")] - NtpHeader::V5(_header) => todo!(), + NtpHeader::V5(_header) => todo!("No NTS support for NTPv5 yet"), } } } @@ -822,7 +843,7 @@ impl<'a> NtpPacket<'a> { NtpHeader::V3(header) => header.reference_id, NtpHeader::V4(header) => header.reference_id, #[cfg(feature = "ntpv5")] - NtpHeader::V5(_header) => todo!(), + NtpHeader::V5(_header) => todo!("NTPv5 does not have reference IDs"), } } @@ -831,7 +852,8 @@ impl<'a> NtpPacket<'a> { NtpHeader::V3(header) => header.stratum == 0, NtpHeader::V4(header) => header.stratum == 0, #[cfg(feature = "ntpv5")] - NtpHeader::V5(_header) => todo!(), + // TODO NTPv5 does not have kiss codes yet + NtpHeader::V5(_header) => false, } } @@ -878,7 +900,10 @@ impl<'a> NtpPacket<'a> { header.origin_timestamp == identifier.expected_origin_timestamp } #[cfg(feature = "ntpv5")] - NtpHeader::V5(_header) => todo!(), + NtpHeader::V5(header) => { + header.client_cookie + == v5::NtpClientCookie::from_ntp_timestamp(identifier.expected_origin_timestamp) + } } } } @@ -916,7 +941,13 @@ impl<'a> NtpPacket<'a> { NtpHeader::V3(ref mut header) => header.mode = mode, NtpHeader::V4(ref mut header) => header.mode = mode, #[cfg(feature = "ntpv5")] - NtpHeader::V5(_header) => todo!(), + NtpHeader::V5(ref mut header) => { + header.mode = match mode { + NtpAssociationMode::Client => v5::NtpMode::Request, + NtpAssociationMode::Server => v5::NtpMode::Response, + _ => todo!("NTPv5 can only handle client-server"), + } + } } } @@ -925,7 +956,10 @@ impl<'a> NtpPacket<'a> { NtpHeader::V3(ref mut header) => header.origin_timestamp = timestamp, NtpHeader::V4(ref mut header) => header.origin_timestamp = timestamp, #[cfg(feature = "ntpv5")] - NtpHeader::V5(_header) => todo!(), + // TODO can we just reuse the cookie as the origin timestamp? + NtpHeader::V5(ref mut header) => { + header.client_cookie = v5::NtpClientCookie::from_ntp_timestamp(timestamp) + } } } @@ -934,7 +968,7 @@ impl<'a> NtpPacket<'a> { NtpHeader::V3(ref mut header) => header.transmit_timestamp = timestamp, NtpHeader::V4(ref mut header) => header.transmit_timestamp = timestamp, #[cfg(feature = "ntpv5")] - NtpHeader::V5(_header) => todo!(), + NtpHeader::V5(ref mut header) => header.transmit_timestamp = timestamp, } } @@ -943,7 +977,7 @@ impl<'a> NtpPacket<'a> { NtpHeader::V3(ref mut header) => header.receive_timestamp = timestamp, NtpHeader::V4(ref mut header) => header.receive_timestamp = timestamp, #[cfg(feature = "ntpv5")] - NtpHeader::V5(_header) => todo!(), + NtpHeader::V5(ref mut header) => header.receive_timestamp = timestamp, } } @@ -952,7 +986,7 @@ impl<'a> NtpPacket<'a> { NtpHeader::V3(ref mut header) => header.precision = precision, NtpHeader::V4(ref mut header) => header.precision = precision, #[cfg(feature = "ntpv5")] - NtpHeader::V5(_header) => todo!(), + NtpHeader::V5(ref mut header) => header.precision = precision, } } @@ -961,7 +995,7 @@ impl<'a> NtpPacket<'a> { NtpHeader::V3(ref mut header) => header.leap = leap, NtpHeader::V4(ref mut header) => header.leap = leap, #[cfg(feature = "ntpv5")] - NtpHeader::V5(_header) => todo!(), + NtpHeader::V5(ref mut header) => header.leap = leap, } } @@ -970,7 +1004,7 @@ impl<'a> NtpPacket<'a> { NtpHeader::V3(ref mut header) => header.stratum = stratum, NtpHeader::V4(ref mut header) => header.stratum = stratum, #[cfg(feature = "ntpv5")] - NtpHeader::V5(_header) => todo!(), + NtpHeader::V5(ref mut header) => header.stratum = stratum, } } @@ -979,7 +1013,7 @@ impl<'a> NtpPacket<'a> { NtpHeader::V3(ref mut header) => header.reference_id = reference_id, NtpHeader::V4(ref mut header) => header.reference_id = reference_id, #[cfg(feature = "ntpv5")] - NtpHeader::V5(_header) => todo!(), + NtpHeader::V5(_header) => todo!("NTPv5 does not have refernce IDs"), } } @@ -988,7 +1022,7 @@ impl<'a> NtpPacket<'a> { NtpHeader::V3(ref mut header) => header.root_delay = root_delay, NtpHeader::V4(ref mut header) => header.root_delay = root_delay, #[cfg(feature = "ntpv5")] - NtpHeader::V5(_header) => todo!(), + NtpHeader::V5(ref mut header) => header.root_delay = root_delay, } } @@ -997,7 +1031,7 @@ impl<'a> NtpPacket<'a> { NtpHeader::V3(ref mut header) => header.root_dispersion = root_dispersion, NtpHeader::V4(ref mut header) => header.root_dispersion = root_dispersion, #[cfg(feature = "ntpv5")] - NtpHeader::V5(_header) => todo!(), + NtpHeader::V5(ref mut header) => header.root_dispersion = root_dispersion, } } } diff --git a/ntp-proto/src/packet/v5/mod.rs b/ntp-proto/src/packet/v5/mod.rs index 913ee7d0d..6c6f6d301 100644 --- a/ntp-proto/src/packet/v5/mod.rs +++ b/ntp-proto/src/packet/v5/mod.rs @@ -1,4 +1,5 @@ -use crate::{NtpDuration, NtpLeapIndicator, NtpTimestamp}; +use crate::{NtpClock, NtpDuration, NtpLeapIndicator, NtpTimestamp, SystemSnapshot}; +use rand::random; mod error; #[allow(dead_code)] @@ -108,10 +109,23 @@ impl NtpFlags { } #[derive(Debug, Copy, Clone, Eq, PartialEq)] -pub struct NtpServerCookie([u8; 8]); +pub struct NtpServerCookie(pub [u8; 8]); + +impl NtpServerCookie { + fn new_random() -> NtpServerCookie { + // TODO does this match entropy handling of the rest of the system? + Self(random()) + } +} #[derive(Debug, Copy, Clone, Eq, PartialEq)] -pub struct NtpClientCookie([u8; 8]); +pub struct NtpClientCookie(pub [u8; 8]); + +impl NtpClientCookie { + pub fn from_ntp_timestamp(ts: NtpTimestamp) -> Self { + Self(ts.to_bits()) + } +} #[derive(Debug, Copy, Clone, Eq, PartialEq)] pub struct NtpHeaderV5 { @@ -133,6 +147,39 @@ pub struct NtpHeaderV5 { pub transmit_timestamp: NtpTimestamp, } +impl NtpHeaderV5 { + pub(crate) fn timestamp_response( + system: &SystemSnapshot, + input: Self, + recv_timestamp: NtpTimestamp, + clock: &C, + ) -> Self { + Self { + leap: system.time_snapshot.leap_indicator, + mode: NtpMode::Response, + stratum: system.stratum, + // TODO this changed in NTPv5 + poll: input.poll, + precision: system.time_snapshot.precision.log2(), + // TODO this is new in NTPv5 + timescale: NtpTimescale::Utc, + // TODO this is new in NTPv5 + era: NtpEra(0), + // TODO this is new in NTPv5 + flags: NtpFlags { + unknown_leap: false, + interleaved_mode: false, + }, + root_delay: system.time_snapshot.root_delay, + root_dispersion: system.time_snapshot.root_dispersion, + server_cookie: NtpServerCookie::new_random(), + client_cookie: input.client_cookie, + receive_timestamp: recv_timestamp, + transmit_timestamp: clock.now().expect("Failed to read time"), + } + } +} + impl NtpHeaderV5 { const LENGTH: usize = 48; From aa5fa411ada3de694b772b5568e27a19c4e95a65 Mon Sep 17 00:00:00 2001 From: Marlon Baeten Date: Tue, 17 Oct 2023 14:40:22 +0200 Subject: [PATCH 21/28] Fix fuzzer for versioned extension headers --- ntp-proto/Cargo.toml | 2 +- ntp-proto/src/lib.rs | 4 ++-- ntp-proto/src/packet/extension_fields.rs | 1 + ntp-proto/src/packet/mod.rs | 3 +-- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/ntp-proto/Cargo.toml b/ntp-proto/Cargo.toml index 3fc13d1ab..0a9b029e4 100644 --- a/ntp-proto/Cargo.toml +++ b/ntp-proto/Cargo.toml @@ -24,7 +24,7 @@ md-5.workspace = true rand.workspace = true tracing.workspace = true serde.workspace = true -arbitrary = { workspace = true, optional = true } +arbitrary = { workspace = true, optional = true, features = ["derive"] } rustls.workspace = true thiserror.workspace = true aead.workspace = true diff --git a/ntp-proto/src/lib.rs b/ntp-proto/src/lib.rs index 0add3f99b..f18ffc209 100644 --- a/ntp-proto/src/lib.rs +++ b/ntp-proto/src/lib.rs @@ -43,8 +43,8 @@ mod exports { #[cfg(feature = "__internal-fuzz")] pub use super::packet::ExtensionField; pub use super::packet::{ - Cipher, CipherProvider, EncryptResult, NoCipher, NtpAssociationMode, NtpLeapIndicator, - NtpPacket, PacketParsingError, + Cipher, CipherProvider, EncryptResult, ExtensionHeaderVersion, NoCipher, + NtpAssociationMode, NtpLeapIndicator, NtpPacket, PacketParsingError, }; #[cfg(feature = "__internal-fuzz")] pub use super::peer::fuzz_measurement_from_packet; diff --git a/ntp-proto/src/packet/extension_fields.rs b/ntp-proto/src/packet/extension_fields.rs index 63218d3ed..b94c88dd3 100644 --- a/ntp-proto/src/packet/extension_fields.rs +++ b/ntp-proto/src/packet/extension_fields.rs @@ -703,6 +703,7 @@ impl<'a> RawEncryptedField<'a> { } #[derive(Debug, Copy, Clone, PartialEq, Eq)] +#[cfg_attr(feature = "__internal-fuzz", derive(arbitrary::Arbitrary))] pub enum ExtensionHeaderVersion { V4, #[cfg(feature = "ntpv5")] diff --git a/ntp-proto/src/packet/mod.rs b/ntp-proto/src/packet/mod.rs index 620181a2e..15324cc8a 100644 --- a/ntp-proto/src/packet/mod.rs +++ b/ntp-proto/src/packet/mod.rs @@ -10,7 +10,6 @@ use crate::{ system::SystemSnapshot, time_types::{NtpDuration, NtpTimestamp, PollInterval}, }; -use extension_fields::ExtensionHeaderVersion; use self::{error::ParsingError, extension_fields::ExtensionFieldData, mac::Mac}; @@ -27,7 +26,7 @@ pub use crypto::{ EncryptResult, NoCipher, }; pub use error::PacketParsingError; -pub use extension_fields::ExtensionField; +pub use extension_fields::{ExtensionField, ExtensionHeaderVersion}; #[derive(Debug, Copy, Clone, PartialEq, Eq, Serialize, Deserialize)] pub enum NtpLeapIndicator { From a6800938d7c0d11cc4fc4f0fbd448282e9f61e1b Mon Sep 17 00:00:00 2001 From: Marlon Baeten Date: Tue, 17 Oct 2023 14:47:27 +0200 Subject: [PATCH 22/28] Fix lock file --- Cargo.lock | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e8dbe9746..ac9a0aee7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -62,9 +62,9 @@ checksum = "a2e1373abdaa212b704512ec2bd8b26bd0b7d5c3f70117411a5d9a451383c859" [[package]] name = "async-trait" -version = "0.1.73" +version = "0.1.74" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "bc00ceb34980c03614e35a3a4e218276a0a824e911d07651cd0d858a51e8c0f0" +checksum = "a66537f1bb974b254c98ed142ff995236e81b9d0fe4db0575f46612cb15eb0f9" dependencies = [ "proc-macro2", "quote", @@ -473,9 +473,9 @@ checksum = "5b40af805b3121feab8a3c29f04d8ad262fa8e0561883e7653e024ae4479e6de" [[package]] name = "proc-macro2" -version = "1.0.67" +version = "1.0.69" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3d433d9f1a3e8c1263d9456598b16fec66f4acc9a74dacffd35c7bb09b3a1328" +checksum = "134c189feb4956b20f6f547d2cf727d4c0fe06722b20a0eec87ed445a97f92da" dependencies = [ "unicode-ident", ] @@ -673,9 +673,9 @@ dependencies = [ [[package]] name = "sharded-slab" -version = "0.1.6" +version = "0.1.7" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c1b21f559e07218024e7e9f90f96f601825397de0e25420135f7f952453fed0b" +checksum = "f40ca3c46823713e0d4209592e8d6e826aa57e928f09752619fc696c499637f6" dependencies = [ "lazy_static", ] @@ -704,9 +704,9 @@ checksum = "81cdd64d312baedb58e21336b31bc043b77e01cc99033ce76ef539f78e965ebc" [[package]] name = "syn" -version = "2.0.37" +version = "2.0.38" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7303ef2c05cd654186cb250d29049a24840ca25d2747c25c0381c8d9e2f582e8" +checksum = "e96b79aaa137db8f61e26363a0c9b47d8b4ec75da28b7d1d614c2303e232408b" dependencies = [ "proc-macro2", "quote", @@ -807,9 +807,9 @@ dependencies = [ [[package]] name = "tracing" -version = "0.1.37" +version = "0.1.39" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8ce8c33a8d48bd45d624a6e523445fd21ec13d3653cd51f681abf67418f54eb8" +checksum = "ee2ef2af84856a50c1d430afce2fdded0a4ec7eda868db86409b4543df0797f9" dependencies = [ "cfg-if", "pin-project-lite", @@ -819,9 +819,9 @@ dependencies = [ [[package]] name = "tracing-attributes" -version = "0.1.26" +version = "0.1.27" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5f4f31f56159e98206da9efd823404b79b6ef3143b4a7ab76e67b1751b25a4ab" +checksum = "34704c8d6ebcbc939824180af020566b01a7c01f80641264eba0999f6c2b6be7" dependencies = [ "proc-macro2", "quote", @@ -830,9 +830,9 @@ dependencies = [ [[package]] name = "tracing-core" -version = "0.1.31" +version = "0.1.32" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "0955b8137a1df6f1a2e9a37d8a6656291ff0297c1a97c24e0d8425fe2312f79a" +checksum = "c06d3da6113f116aaee68e4d601191614c9053067f9ab7f6edbcb161237daa54" dependencies = [ "once_cell", ] @@ -1033,9 +1033,9 @@ checksum = "ed94fce61571a4006852b7389a063ab983c02eb1bb37b47f8272ce92d06d9538" [[package]] name = "winnow" -version = "0.5.15" +version = "0.5.17" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7c2e3184b9c4e92ad5167ca73039d0c42476302ab603e2fec4487511f38ccefc" +checksum = "a3b801d0e0a6726477cc207f60162da452f3a95adb368399bef20a946e06f65c" dependencies = [ "memchr", ] From f18e752b39068b5c816d7660d874b09a3065601d Mon Sep 17 00:00:00 2001 From: Marlon Baeten Date: Tue, 17 Oct 2023 14:53:01 +0200 Subject: [PATCH 23/28] Update versions in fuzz/Cargo.lock --- fuzz/Cargo.lock | 98 +------------------ fuzz/fuzz_targets/encrypted_server_parsing.rs | 6 +- 2 files changed, 7 insertions(+), 97 deletions(-) diff --git a/fuzz/Cargo.lock b/fuzz/Cargo.lock index 60b068037..42e799595 100644 --- a/fuzz/Cargo.lock +++ b/fuzz/Cargo.lock @@ -74,12 +74,6 @@ dependencies = [ "syn", ] -[[package]] -name = "autocfg" -version = "1.1.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d468802bab17cbc0cc575e9b053f41e72aa36bfa6b7f55e3529ffa43161b97fa" - [[package]] name = "backtrace" version = "0.3.69" @@ -241,12 +235,6 @@ dependencies = [ "subtle", ] -[[package]] -name = "dtoa" -version = "1.0.9" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "dcbb2bf8e87535c23f7a8a321e364ce21462d0ff10cb6407820e8e96dfff6653" - [[package]] name = "equivalent" version = "1.0.1" @@ -358,16 +346,6 @@ dependencies = [ "once_cell", ] -[[package]] -name = "lock_api" -version = "0.4.10" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c1cc9717a20b1bb222f333e6a92fd32f7d8a18ddc5a3191a11af45dcbf4dcd16" -dependencies = [ - "autocfg", - "scopeguard", -] - [[package]] name = "log" version = "0.4.20" @@ -411,7 +389,7 @@ dependencies = [ [[package]] name = "ntp-os-clock" -version = "0.3.6" +version = "1.0.0" dependencies = [ "libc", "ntp-proto", @@ -420,7 +398,7 @@ dependencies = [ [[package]] name = "ntp-proto" -version = "0.3.6" +version = "1.0.0" dependencies = [ "aead", "aes-siv", @@ -446,7 +424,7 @@ dependencies = [ [[package]] name = "ntp-udp" -version = "0.3.6" +version = "1.0.0" dependencies = [ "libc", "ntp-proto", @@ -457,14 +435,13 @@ dependencies = [ [[package]] name = "ntpd" -version = "0.3.6" +version = "1.0.0" dependencies = [ "async-trait", "libc", "ntp-os-clock", "ntp-proto", "ntp-udp", - "prometheus-client", "rand 0.8.5", "rustls", "rustls-native-certs", @@ -525,29 +502,6 @@ version = "0.1.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b15813163c1d831bf4a13c3610c05c0d03b39feb07f7e09fa234dac9b15aaf39" -[[package]] -name = "parking_lot" -version = "0.12.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3742b2c103b9f06bc9fff0a37ff4912935851bee6d36f3c02bcc755bcfec228f" -dependencies = [ - "lock_api", - "parking_lot_core", -] - -[[package]] -name = "parking_lot_core" -version = "0.9.8" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "93f00c865fe7cabf650081affecd3871070f26767e7b2070a3ffae14c654b447" -dependencies = [ - "cfg-if", - "libc", - "redox_syscall", - "smallvec", - "windows-targets", -] - [[package]] name = "pin-project-lite" version = "0.2.12" @@ -569,29 +523,6 @@ dependencies = [ "unicode-ident", ] -[[package]] -name = "prometheus-client" -version = "0.21.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3c99afa9a01501019ac3a14d71d9f94050346f55ca471ce90c799a15c58f61e2" -dependencies = [ - "dtoa", - "itoa", - "parking_lot", - "prometheus-client-derive-encode", -] - -[[package]] -name = "prometheus-client-derive-encode" -version = "0.4.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "440f724eba9f6996b75d63681b0a92b06947f1457076d503a4d2e2c8f56442b8" -dependencies = [ - "proc-macro2", - "quote", - "syn", -] - [[package]] name = "quote" version = "1.0.33" @@ -635,15 +566,6 @@ dependencies = [ "getrandom", ] -[[package]] -name = "redox_syscall" -version = "0.3.5" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "567664f262709473930a4bf9e51bf2ebf3348f2e748ccc50dea20646858f8f29" -dependencies = [ - "bitflags", -] - [[package]] name = "ring" version = "0.16.20" @@ -723,12 +645,6 @@ dependencies = [ "windows-sys", ] -[[package]] -name = "scopeguard" -version = "1.2.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "94143f37725109f92c262ed2cf5e59bce7498c01bcc1502d7b9afe439a4e9f49" - [[package]] name = "sct" version = "0.7.0" @@ -811,12 +727,6 @@ dependencies = [ "lazy_static", ] -[[package]] -name = "smallvec" -version = "1.11.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "62bb4feee49fdd9f707ef802e22365a35de4b7b299de4763d44bfea899442ff9" - [[package]] name = "socket2" version = "0.5.3" diff --git a/fuzz/fuzz_targets/encrypted_server_parsing.rs b/fuzz/fuzz_targets/encrypted_server_parsing.rs index a3fe60966..440b9e912 100644 --- a/fuzz/fuzz_targets/encrypted_server_parsing.rs +++ b/fuzz/fuzz_targets/encrypted_server_parsing.rs @@ -6,7 +6,7 @@ use std::{ }; use libfuzzer_sys::fuzz_target; -use ntp_proto::{test_cookie, EncryptResult, ExtensionField, KeySetProvider, NtpPacket}; +use ntp_proto::{test_cookie, EncryptResult, ExtensionField, ExtensionHeaderVersion, KeySetProvider, NtpPacket}; use rand::{rngs::StdRng, set_thread_rng, SeedableRng}; const fn next_multiple_of(lhs: u16, rhs: u16) -> u16 { @@ -16,7 +16,7 @@ const fn next_multiple_of(lhs: u16, rhs: u16) -> u16 { } } -fuzz_target!(|parts: (Vec, Vec, Vec, Vec, u64)| { +fuzz_target!(|parts: (Vec, Vec, Vec, Vec, u64, ExtensionHeaderVersion)| { set_thread_rng(StdRng::seed_from_u64(parts.4)); // Can't test reencoding because of the keyset @@ -29,7 +29,7 @@ fuzz_target!(|parts: (Vec, Vec, Vec, Vec, u64)| { let _ = cursor.write_all(&parts.0); let cookie = test_cookie(); let enc_cookie = keyset.encode_cookie_pub(&cookie); - let _ = ExtensionField::NtsCookie(Cow::Borrowed(&enc_cookie)).serialize_pub(&mut cursor, 4); + let _ = ExtensionField::NtsCookie(Cow::Borrowed(&enc_cookie)).serialize_pub(&mut cursor, 4, parts.5); let _ = cursor.write_all(&parts.1); let mut ciphertext = parts.2.clone(); From 95fbaf52afbdd8f8356946f3369fd25cabbd3c54 Mon Sep 17 00:00:00 2001 From: Marlon Baeten Date: Tue, 17 Oct 2023 14:55:32 +0200 Subject: [PATCH 24/28] Format fuzzer code --- fuzz/fuzz_targets/encrypted_server_parsing.rs | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/fuzz/fuzz_targets/encrypted_server_parsing.rs b/fuzz/fuzz_targets/encrypted_server_parsing.rs index 440b9e912..526a8ccd9 100644 --- a/fuzz/fuzz_targets/encrypted_server_parsing.rs +++ b/fuzz/fuzz_targets/encrypted_server_parsing.rs @@ -6,7 +6,9 @@ use std::{ }; use libfuzzer_sys::fuzz_target; -use ntp_proto::{test_cookie, EncryptResult, ExtensionField, ExtensionHeaderVersion, KeySetProvider, NtpPacket}; +use ntp_proto::{ + test_cookie, EncryptResult, ExtensionField, ExtensionHeaderVersion, KeySetProvider, NtpPacket, +}; use rand::{rngs::StdRng, set_thread_rng, SeedableRng}; const fn next_multiple_of(lhs: u16, rhs: u16) -> u16 { @@ -16,7 +18,14 @@ const fn next_multiple_of(lhs: u16, rhs: u16) -> u16 { } } -fuzz_target!(|parts: (Vec, Vec, Vec, Vec, u64, ExtensionHeaderVersion)| { +fuzz_target!(|parts: ( + Vec, + Vec, + Vec, + Vec, + u64, + ExtensionHeaderVersion +)| { set_thread_rng(StdRng::seed_from_u64(parts.4)); // Can't test reencoding because of the keyset @@ -29,7 +38,11 @@ fuzz_target!(|parts: (Vec, Vec, Vec, Vec, u64, ExtensionHeaderVe let _ = cursor.write_all(&parts.0); let cookie = test_cookie(); let enc_cookie = keyset.encode_cookie_pub(&cookie); - let _ = ExtensionField::NtsCookie(Cow::Borrowed(&enc_cookie)).serialize_pub(&mut cursor, 4, parts.5); + let _ = ExtensionField::NtsCookie(Cow::Borrowed(&enc_cookie)).serialize_pub( + &mut cursor, + 4, + parts.5, + ); let _ = cursor.write_all(&parts.1); let mut ciphertext = parts.2.clone(); From a38e6c65408edd13b037634d8da12084259ab6e4 Mon Sep 17 00:00:00 2001 From: Tamme Dittrich Date: Tue, 17 Oct 2023 15:00:07 +0200 Subject: [PATCH 25/28] Also run fuzzer smoke tests for ntpv5 --- .github/workflows/build.yaml | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index c6201f60a..ed868dc74 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -363,6 +363,11 @@ jobs: fuzz: name: Smoke-test fuzzing targets runs-on: ubuntu-20.04 + strategy: + matrix: + features: + - "" + - "--features ntpv5" steps: - name: Checkout sources uses: actions/checkout@8ade135a41bc03ea155e62e844d188df1ea18608 @@ -380,9 +385,9 @@ jobs: tool: cargo-fuzz - name: Smoke-test fuzz targets run: | - cargo fuzz build - for target in $(cargo fuzz list) ; do - cargo fuzz run $target -- -max_total_time=10 + cargo fuzz build ${{ matrix.features }} + for target in $(cargo fuzz list ${{ matrix.features }}) ; do + cargo fuzz run ${{ matrix.features }} $target -- -max_total_time=10 done audit-dependencies: From a433e6a0567c97f1a554a9bdc0bce58f60964062 Mon Sep 17 00:00:00 2001 From: Marlon Baeten Date: Tue, 17 Oct 2023 15:45:22 +0200 Subject: [PATCH 26/28] We have derive_arbitrary at home --- ntp-proto/Cargo.toml | 2 +- ntp-proto/src/packet/extension_fields.rs | 18 +++++++++++++++++- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/ntp-proto/Cargo.toml b/ntp-proto/Cargo.toml index 0a9b029e4..3fc13d1ab 100644 --- a/ntp-proto/Cargo.toml +++ b/ntp-proto/Cargo.toml @@ -24,7 +24,7 @@ md-5.workspace = true rand.workspace = true tracing.workspace = true serde.workspace = true -arbitrary = { workspace = true, optional = true, features = ["derive"] } +arbitrary = { workspace = true, optional = true } rustls.workspace = true thiserror.workspace = true aead.workspace = true diff --git a/ntp-proto/src/packet/extension_fields.rs b/ntp-proto/src/packet/extension_fields.rs index b94c88dd3..33fdcc599 100644 --- a/ntp-proto/src/packet/extension_fields.rs +++ b/ntp-proto/src/packet/extension_fields.rs @@ -703,13 +703,29 @@ impl<'a> RawEncryptedField<'a> { } #[derive(Debug, Copy, Clone, PartialEq, Eq)] -#[cfg_attr(feature = "__internal-fuzz", derive(arbitrary::Arbitrary))] pub enum ExtensionHeaderVersion { V4, #[cfg(feature = "ntpv5")] V5, } +#[cfg(feature = "__internal-fuzz")] +impl<'a> arbitrary::Arbitrary<'a> for ExtensionHeaderVersion { + #[cfg(not(feature = "ntpv5"))] + fn arbitrary(u: &mut arbitrary::Unstructured<'a>) -> arbitrary::Result { + Ok(Self::V4) + } + + #[cfg(feature = "ntpv5")] + fn arbitrary(u: &mut arbitrary::Unstructured<'a>) -> arbitrary::Result { + Ok(if bool::arbitrary(u)? { + Self::V4 + } else { + Self::V5 + }) + } +} + #[derive(Debug)] struct RawExtensionField<'a> { type_id: ExtensionFieldTypeId, From 665faccca01a599c9697368d5a59b247c249ab0f Mon Sep 17 00:00:00 2001 From: Tamme Dittrich Date: Wed, 18 Oct 2023 08:44:10 +0200 Subject: [PATCH 27/28] Address review comments --- ntp-proto/src/packet/extension_fields.rs | 30 +++++------ ntp-proto/src/packet/mod.rs | 11 ++-- ntp-proto/src/packet/v5/error.rs | 8 +++ ntp-proto/src/packet/v5/extension_fields.rs | 7 ++- ntp-proto/src/packet/v5/mod.rs | 59 ++++++++++----------- 5 files changed, 58 insertions(+), 57 deletions(-) diff --git a/ntp-proto/src/packet/extension_fields.rs b/ntp-proto/src/packet/extension_fields.rs index 33fdcc599..a57941f54 100644 --- a/ntp-proto/src/packet/extension_fields.rs +++ b/ntp-proto/src/packet/extension_fields.rs @@ -447,11 +447,9 @@ impl<'a> ExtensionField<'a> { fn decode_draft_identification( message: &'a [u8], ) -> Result> { - let err = Err(super::v5::V5Error::InvalidDraftIdentification.into()); let di = match core::str::from_utf8(message) { - Err(_) => return err, - Ok(di) if !di.is_ascii() => return err, - Ok(di) => di, + Ok(di) if di.is_ascii() => di, + _ => return Err(super::v5::V5Error::InvalidDraftIdentification.into()), }; Ok(ExtensionField::DraftIdentification(Cow::Borrowed(di))) @@ -951,25 +949,25 @@ mod tests { #[cfg(feature = "ntpv5")] #[test] fn draft_identification() { - let test_id = "draft-ietf-ntp-ntpv5-00\0"; + let test_id = crate::packet::v5::DRAFT_VERSION; let len = u16::try_from(4 + test_id.len()).unwrap(); let mut data = vec![]; - data.extend(&[0xF5, 0xFF]); - data.extend(&len.to_be_bytes()); - data.extend(test_id.as_bytes()); + data.extend(&[0xF5, 0xFF]); // Type + data.extend(&len.to_be_bytes()); // Length + data.extend(test_id.as_bytes()); // Payload + data.extend(&[0]); // Padding - let raw = RawExtensionField::deserialize(&data, 0, ExtensionHeaderVersion::V5).unwrap(); + let raw = RawExtensionField::deserialize(&data, 4, ExtensionHeaderVersion::V5).unwrap(); let ef = ExtensionField::decode(raw).unwrap(); - match ef { - ExtensionField::DraftIdentification(ref data) => { - assert_eq!(data, test_id); - } - _ => panic!("Unexpected extensionfield {ef:?}"), - } + let ExtensionField::DraftIdentification(ref parsed) = ef else { + panic!("Unexpected extensionfield {ef:?}... expected DraftIdentification"); + }; + + assert_eq!(parsed, test_id); let mut out = vec![]; - ef.serialize(&mut out, len, ExtensionHeaderVersion::V4) + ef.serialize(&mut out, 4, ExtensionHeaderVersion::V5) .unwrap(); assert_eq!(&out, &data); diff --git a/ntp-proto/src/packet/mod.rs b/ntp-proto/src/packet/mod.rs index 15324cc8a..76101b9bb 100644 --- a/ntp-proto/src/packet/mod.rs +++ b/ntp-proto/src/packet/mod.rs @@ -151,7 +151,7 @@ pub struct RequestIdentifier { } impl NtpHeaderV3V4 { - const LENGTH: usize = 48; + const WIRE_LENGTH: usize = 48; /// A new, empty NtpHeader fn new() -> Self { @@ -172,7 +172,7 @@ impl NtpHeaderV3V4 { } fn deserialize(data: &[u8]) -> Result<(Self, usize), ParsingError> { - if data.len() < Self::LENGTH { + if data.len() < Self::WIRE_LENGTH { return Err(ParsingError::IncorrectLength); } @@ -191,7 +191,7 @@ impl NtpHeaderV3V4 { receive_timestamp: NtpTimestamp::from_bits(data[32..40].try_into().unwrap()), transmit_timestamp: NtpTimestamp::from_bits(data[40..48].try_into().unwrap()), }, - Self::LENGTH, + Self::WIRE_LENGTH, )) } @@ -851,8 +851,7 @@ impl<'a> NtpPacket<'a> { NtpHeader::V3(header) => header.stratum == 0, NtpHeader::V4(header) => header.stratum == 0, #[cfg(feature = "ntpv5")] - // TODO NTPv5 does not have kiss codes yet - NtpHeader::V5(_header) => false, + NtpHeader::V5(_header) => todo!("NTPv5 does not have kiss codes yet"), } } @@ -1012,7 +1011,7 @@ impl<'a> NtpPacket<'a> { NtpHeader::V3(ref mut header) => header.reference_id = reference_id, NtpHeader::V4(ref mut header) => header.reference_id = reference_id, #[cfg(feature = "ntpv5")] - NtpHeader::V5(_header) => todo!("NTPv5 does not have refernce IDs"), + NtpHeader::V5(_header) => todo!("NTPv5 does not have reference IDs"), } } diff --git a/ntp-proto/src/packet/v5/error.rs b/ntp-proto/src/packet/v5/error.rs index e8c6ebd72..03d7d5100 100644 --- a/ntp-proto/src/packet/v5/error.rs +++ b/ntp-proto/src/packet/v5/error.rs @@ -1,3 +1,4 @@ +use crate::packet::error::ParsingError; use std::fmt::{Display, Formatter}; #[derive(Debug)] @@ -8,6 +9,13 @@ pub enum V5Error { InvalidFlags, } +impl V5Error { + /// `const` alternative to `.into()` + pub const fn into_parse_err(self) -> ParsingError { + ParsingError::V5(self) + } +} + impl Display for V5Error { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { match self { diff --git a/ntp-proto/src/packet/v5/extension_fields.rs b/ntp-proto/src/packet/v5/extension_fields.rs index f930b9cdd..45cf8d6f2 100644 --- a/ntp-proto/src/packet/v5/extension_fields.rs +++ b/ntp-proto/src/packet/v5/extension_fields.rs @@ -14,7 +14,7 @@ pub enum Type { } impl Type { - pub fn from_bits(bits: u16) -> Self { + pub const fn from_bits(bits: u16) -> Self { match bits { 0xF5FF => Self::DraftIdentification, 0xF501 => Self::Padding, @@ -30,7 +30,7 @@ impl Type { } } - pub fn to_bits(self) -> u16 { + pub const fn to_bits(self) -> u16 { match self { Self::DraftIdentification => 0xF5FF, Self::Padding => 0xF501, @@ -60,8 +60,7 @@ impl Type { Self::MonotonicReceiveTimestamp, Self::SecondaryReceiveTimestamp, ] - .iter() - .copied() + .into_iter() } } diff --git a/ntp-proto/src/packet/v5/mod.rs b/ntp-proto/src/packet/v5/mod.rs index 6c6f6d301..c596fd4e9 100644 --- a/ntp-proto/src/packet/v5/mod.rs +++ b/ntp-proto/src/packet/v5/mod.rs @@ -1,3 +1,4 @@ +#![warn(clippy::missing_const_for_fn)] use crate::{NtpClock, NtpDuration, NtpLeapIndicator, NtpTimestamp, SystemSnapshot}; use rand::random; @@ -8,6 +9,9 @@ pub mod extension_fields; use crate::packet::error::ParsingError; pub use error::V5Error; +#[allow(dead_code)] +pub(crate) const DRAFT_VERSION: &str = "draft-ietf-ntp-ntpv5-00"; + #[repr(u8)] #[derive(Debug, PartialEq, Eq, Copy, Clone)] pub enum NtpMode { @@ -16,29 +20,26 @@ pub enum NtpMode { } impl NtpMode { - fn from_bits(bits: u8) -> Result> { + const fn from_bits(bits: u8) -> Result> { Ok(match bits { 3 => Self::Request, 4 => Self::Response, - _ => return Err(V5Error::MalformedMode.into()), + _ => return Err(V5Error::MalformedMode.into_parse_err()), }) } - fn to_bits(self) -> u8 { - match self { - Self::Request => 3, - Self::Response => 4, - } + const fn to_bits(self) -> u8 { + self as u8 } #[allow(dead_code)] - pub(crate) fn is_request(&self) -> bool { - self == &Self::Request + pub(crate) const fn is_request(self) -> bool { + matches!(self, Self::Request) } #[allow(dead_code)] - pub(crate) fn is_response(&self) -> bool { - self == &Self::Response + pub(crate) const fn is_response(self) -> bool { + matches!(self, Self::Response) } } @@ -48,27 +49,22 @@ pub enum NtpTimescale { Utc = 0, Tai = 1, Ut1 = 2, - LeadSmearedUtc = 3, + LeapSmearedUtc = 3, } impl NtpTimescale { - fn from_bits(bits: u8) -> Result> { + const fn from_bits(bits: u8) -> Result> { Ok(match bits { 0 => Self::Utc, 1 => Self::Tai, 2 => Self::Ut1, - 3 => Self::LeadSmearedUtc, - _ => return Err(V5Error::MalformedTimescale.into()), + 3 => Self::LeapSmearedUtc, + _ => return Err(V5Error::MalformedTimescale.into_parse_err()), }) } - fn to_bits(self) -> u8 { - match self { - Self::Utc => 0, - Self::Tai => 1, - Self::Ut1 => 2, - Self::LeadSmearedUtc => 3, - } + const fn to_bits(self) -> u8 { + self as u8 } } @@ -82,9 +78,9 @@ pub struct NtpFlags { } impl NtpFlags { - fn from_bits(bits: [u8; 2]) -> Result> { - if bits[0] != 0x00 || bits[1] & 0xFC != 0 { - return Err(V5Error::InvalidFlags.into()); + const fn from_bits(bits: [u8; 2]) -> Result> { + if bits[0] != 0x00 || bits[1] & 0b1111_1100 != 0 { + return Err(V5Error::InvalidFlags.into_parse_err()); } Ok(Self { @@ -93,7 +89,7 @@ impl NtpFlags { }) } - fn as_bits(&self) -> [u8; 2] { + const fn as_bits(self) -> [u8; 2] { let mut flags: u8 = 0; if self.unknown_leap { @@ -122,7 +118,7 @@ impl NtpServerCookie { pub struct NtpClientCookie(pub [u8; 8]); impl NtpClientCookie { - pub fn from_ntp_timestamp(ts: NtpTimestamp) -> Self { + pub const fn from_ntp_timestamp(ts: NtpTimestamp) -> Self { Self(ts.to_bits()) } } @@ -181,12 +177,13 @@ impl NtpHeaderV5 { } impl NtpHeaderV5 { - const LENGTH: usize = 48; + const WIRE_LENGTH: usize = 48; + const VERSION: u8 = 5; pub(crate) fn deserialize( data: &[u8], ) -> Result<(Self, usize), ParsingError> { - if data.len() < Self::LENGTH { + if data.len() < Self::WIRE_LENGTH { return Err(ParsingError::IncorrectLength); } @@ -212,13 +209,13 @@ impl NtpHeaderV5 { receive_timestamp: NtpTimestamp::from_bits(data[32..40].try_into().unwrap()), transmit_timestamp: NtpTimestamp::from_bits(data[40..48].try_into().unwrap()), }, - Self::LENGTH, + Self::WIRE_LENGTH, )) } #[allow(dead_code)] pub(crate) fn serialize(&self, w: &mut W) -> std::io::Result<()> { - w.write_all(&[(self.leap.to_bits() << 6) | (5 << 3) | self.mode.to_bits()])?; + w.write_all(&[(self.leap.to_bits() << 6) | (Self::VERSION << 3) | self.mode.to_bits()])?; w.write_all(&[self.stratum, self.poll as u8, self.precision as u8])?; w.write_all(&[self.timescale.to_bits()])?; w.write_all(&[self.era.0])?; From 01399e16b7cd90f68b5a2475063500d61f133264 Mon Sep 17 00:00:00 2001 From: Tamme Dittrich Date: Wed, 18 Oct 2023 09:03:18 +0200 Subject: [PATCH 28/28] Address clippy warning for fuzzing NTPv4 --- Cargo.lock | 1 - ntp-proto/src/packet/extension_fields.rs | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ac9a0aee7..09125c734 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -811,7 +811,6 @@ version = "0.1.39" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ee2ef2af84856a50c1d430afce2fdded0a4ec7eda868db86409b4543df0797f9" dependencies = [ - "cfg-if", "pin-project-lite", "tracing-attributes", "tracing-core", diff --git a/ntp-proto/src/packet/extension_fields.rs b/ntp-proto/src/packet/extension_fields.rs index a57941f54..4c9ae10f7 100644 --- a/ntp-proto/src/packet/extension_fields.rs +++ b/ntp-proto/src/packet/extension_fields.rs @@ -710,7 +710,7 @@ pub enum ExtensionHeaderVersion { #[cfg(feature = "__internal-fuzz")] impl<'a> arbitrary::Arbitrary<'a> for ExtensionHeaderVersion { #[cfg(not(feature = "ntpv5"))] - fn arbitrary(u: &mut arbitrary::Unstructured<'a>) -> arbitrary::Result { + fn arbitrary(_u: &mut arbitrary::Unstructured<'a>) -> arbitrary::Result { Ok(Self::V4) }