diff --git a/der/src/asn1/integer.rs b/der/src/asn1/integer.rs index d205ca97d..5e892de11 100644 --- a/der/src/asn1/integer.rs +++ b/der/src/asn1/integer.rs @@ -43,7 +43,7 @@ macro_rules! impl_int_encoding { impl EncodeValue for $int { fn value_len(&self) -> Result { if *self < 0 { - int::encoded_len(&(*self as $uint).to_be_bytes()) + int::negative_encoded_len(&(*self as $uint).to_be_bytes()) } else { uint::encoded_len(&self.to_be_bytes()) } diff --git a/der/src/asn1/integer/bigint.rs b/der/src/asn1/integer/bigint.rs index 33eb06922..9d71edb79 100644 --- a/der/src/asn1/integer/bigint.rs +++ b/der/src/asn1/integer/bigint.rs @@ -66,7 +66,8 @@ impl<'a> DecodeValue<'a> for IntRef<'a> { impl<'a> EncodeValue for IntRef<'a> { fn value_len(&self) -> Result { - int::encoded_len(self.inner.as_slice()) + // Signed integers always hold their full encoded form. + Ok(self.inner.len()) } fn encode_value(&self, writer: &mut impl Writer) -> Result<()> { @@ -176,6 +177,8 @@ pub use self::allocating::{Int, Uint}; #[cfg(feature = "alloc")] mod allocating { + use alloc::vec::Vec; + use super::{super::int, super::uint, IntRef, UintRef}; use crate::{ ord::OrdIsValueOrd, @@ -244,7 +247,8 @@ mod allocating { impl EncodeValue for Int { fn value_len(&self) -> Result { - int::encoded_len(self.inner.as_slice()) + // Signed integers always hold their full encoded form. + Ok(self.inner.len()) } fn encode_value(&self, writer: &mut impl Writer) -> Result<()> { @@ -259,6 +263,22 @@ mod allocating { } } + impl From for Int { + fn from(value: Uint) -> Self { + let mut inner: Vec = Vec::new(); + + // Add leading `0x00` byte if required + if value.value_len().expect("invalid Uint") > value.len() { + inner.push(0x00); + } + + inner.extend_from_slice(value.as_bytes()); + let inner = BytesOwned::new(inner).expect("invalid Uint"); + + Int { inner } + } + } + impl FixedTag for Int { const TAG: Tag = Tag::Integer; } diff --git a/der/src/asn1/integer/int.rs b/der/src/asn1/integer/int.rs index 48782e125..6e29a4ccb 100644 --- a/der/src/asn1/integer/int.rs +++ b/der/src/asn1/integer/int.rs @@ -1,4 +1,4 @@ -//! Support for encoding negative integers +//! Support for encoding signed integers use super::is_highest_bit_set; use crate::{ErrorKind, Length, Result, Tag, Writer}; @@ -46,9 +46,9 @@ where writer.write(strip_leading_ones(bytes)) } -/// Get the encoded length for the given signed integer serialized as bytes. +/// Get the encoded length for the given **negative** integer serialized as bytes. #[inline] -pub(super) fn encoded_len(bytes: &[u8]) -> Result { +pub(super) fn negative_encoded_len(bytes: &[u8]) -> Result { Length::try_from(strip_leading_ones(bytes).len()) } diff --git a/x509-cert/src/serial_number.rs b/x509-cert/src/serial_number.rs index 8cd0a6b69..ae9242b21 100644 --- a/x509-cert/src/serial_number.rs +++ b/x509-cert/src/serial_number.rs @@ -3,8 +3,8 @@ use core::fmt::Display; use der::{ - asn1::Int, DecodeValue, EncodeValue, ErrorKind, FixedTag, Header, Length, Reader, Result, Tag, - ValueOrd, Writer, + asn1::Int, asn1::Uint, DecodeValue, EncodeValue, ErrorKind, FixedTag, Header, Length, Reader, + Result, Tag, ValueOrd, Writer, }; /// [RFC 5280 Section 4.1.2.2.] Serial Number @@ -32,15 +32,27 @@ impl SerialNumber { /// Maximum length in bytes for a [`SerialNumber`] pub const MAX_LEN: Length = Length::new(20); + /// See notes in `SerialNumber::new` and `SerialNumber::decode_value`. + const MAX_DECODE_LEN: Length = Length::new(21); + /// Create a new [`SerialNumber`] from a byte slice. + /// + /// The byte slice **must** represent a positive integer. pub fn new(bytes: &[u8]) -> Result { - let inner = Int::new(bytes)?; - - if inner.len() > SerialNumber::MAX_LEN { + let inner = Uint::new(bytes)?; + + // The user might give us a 20 byte unsigned integer with a high MSB, + // which we'd then encode with 21 octets to preserve the sign bit. + // RFC 5280 is ambiguous about whether this is valid, so we limit + // `SerialNumber` *encodings* to 20 bytes or fewer while permitting + // `SerialNumber` *decodings* to have up to 21 bytes below. + if inner.value_len()? > SerialNumber::MAX_LEN { return Err(ErrorKind::Overlength.into()); } - Ok(Self { inner }) + Ok(Self { + inner: inner.into(), + }) } /// Borrow the inner byte slice which contains the least significant bytes @@ -64,7 +76,10 @@ impl<'a> DecodeValue<'a> for SerialNumber { fn decode_value>(reader: &mut R, header: Header) -> Result { let inner = Int::decode_value(reader, header)?; - if inner.len() > SerialNumber::MAX_LEN { + // See the note in `SerialNumber::new`: we permit lengths of 21 bytes here, + // since some X.509 implementations interpret the limit of 20 bytes to refer + // to the pre-encoded value. + if inner.len() > SerialNumber::MAX_DECODE_LEN { return Err(ErrorKind::Overlength.into()); } @@ -112,11 +127,44 @@ mod tests { use super::*; + #[test] + fn serial_number_invariants() { + // Creating a new serial with an oversized encoding (due to high MSB) fails. + { + let too_big = [0x80; 20]; + assert!(SerialNumber::new(&too_big).is_err()); + } + + // Creating a new serial with the maximum encoding succeeds. + { + let just_enough = [ + 0x7F, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, + 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, + ]; + assert!(SerialNumber::new(&just_enough).is_ok()); + } + } + #[test] fn serial_number_display() { - let sn = SerialNumber::new(&[0xAA, 0xBB, 0xCC, 0x01, 0x10, 0x00, 0x11]) - .expect("unexpected error"); + { + let sn = SerialNumber::new(&[0x11, 0x22, 0x33]).unwrap(); + + assert_eq!(sn.to_string(), "11:22:33") + } + + { + let sn = SerialNumber::new(&[0xAA, 0xBB, 0xCC, 0x01, 0x10, 0x00, 0x11]).unwrap(); - assert_eq!(sn.to_string(), "AA:BB:CC:01:10:00:11") + // We force the user's serial to be positive if they give us a negative one. + assert_eq!(sn.to_string(), "00:AA:BB:CC:01:10:00:11") + } + + { + let sn = SerialNumber::new(&[0x00, 0x00, 0x01]).unwrap(); + + // Leading zeroes are ignored, due to canonicalization. + assert_eq!(sn.to_string(), "01") + } } }