From 130f52c09e176d2289aa29fc68f3327ee563e1fa Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Mon, 2 Jan 2023 01:12:48 -0500 Subject: [PATCH 1/5] der, x509-cert: fix handling of negative integers --- der/src/asn1.rs | 5 +- der/src/asn1/integer/bigint.rs | 388 +++++++++++++++++++++++++++------ der/src/asn1/integer/int.rs | 32 ++- der/src/error.rs | 2 +- der/src/lib.rs | 2 + x509-cert/src/serial_number.rs | 10 +- 6 files changed, 359 insertions(+), 80 deletions(-) diff --git a/der/src/asn1.rs b/der/src/asn1.rs index 2d24ee404..ba643e827 100644 --- a/der/src/asn1.rs +++ b/der/src/asn1.rs @@ -36,8 +36,8 @@ pub use const_oid::ObjectIdentifier; #[cfg_attr(docsrs, doc(cfg(feature = "alloc")))] pub use self::{ any::Any, bit_string::BitString, ia5_string::Ia5String, integer::bigint::Int, - integer::bigint::Uint, octet_string::OctetString, printable_string::PrintableString, - set_of::SetOfVec, teletex_string::TeletexString, + integer::bigint::Sint, integer::bigint::Uint, octet_string::OctetString, + printable_string::PrintableString, set_of::SetOfVec, teletex_string::TeletexString, }; pub use self::{ any::AnyRef, @@ -47,6 +47,7 @@ pub use self::{ generalized_time::GeneralizedTime, ia5_string::Ia5StringRef, integer::bigint::IntRef, + integer::bigint::SintRef, integer::bigint::UintRef, null::Null, octet_string::OctetStringRef, diff --git a/der/src/asn1/integer/bigint.rs b/der/src/asn1/integer/bigint.rs index 3178c4ed4..bb2e38a76 100644 --- a/der/src/asn1/integer/bigint.rs +++ b/der/src/asn1/integer/bigint.rs @@ -6,21 +6,131 @@ use crate::{ FixedTag, Header, Length, Reader, Result, Tag, Writer, }; -/// "Big" signed ASN.1 `INTEGER` type. +/// A "big" ASN.1 `INTEGER` type that can hold both positive and negative +/// values. +#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] +pub enum IntRef<'a> { + /// Unsigned integers. + Unsigned(UintRef<'a>), + + /// Signed integers. These are functionally always negative, + /// since we represent positive signed integers using the unsigned variant. + Signed(SintRef<'a>), +} + +impl<'a> From> for IntRef<'a> { + fn from(value: UintRef<'a>) -> Self { + Self::Unsigned(value) + } +} + +impl<'a> From> for IntRef<'a> { + fn from(value: SintRef<'a>) -> Self { + Self::Signed(value) + } +} + +impl<'a> IntRef<'a> { + /// Borrow the inner byte slice which contains the least significant bytes + /// of a big endian integer value with all leading ones stripped. + pub fn as_bytes(&self) -> &'a [u8] { + match self { + IntRef::Unsigned(i) => i.inner.as_slice(), + IntRef::Signed(i) => i.inner.as_slice(), + } + } + + /// Get the length of this [`IntRef`] in bytes. + pub fn len(&self) -> Length { + match self { + IntRef::Unsigned(i) => i.len(), + IntRef::Signed(i) => i.len(), + } + } + + /// Is the inner byte slice empty? + pub fn is_empty(self) -> bool { + match self { + IntRef::Unsigned(i) => i.is_empty(), + IntRef::Signed(i) => i.is_empty(), + } + } +} + +impl<'a> DecodeValue<'a> for IntRef<'a> { + fn decode_value>(reader: &mut R, header: Header) -> Result { + let bytes = BytesRef::decode_value(reader, header)?.as_slice(); + + // Try to decode as unsigned first, then signed. + let result: Self = match uint::decode_to_slice(bytes) { + Ok(result) => Ok(UintRef::new(result)?.into()), + Err(Error { + kind: ErrorKind::Value { .. }, + .. + }) => Ok(SintRef::new(int::decode_to_slice(bytes)?)?.into()), + Err(e) => Err(e), + }?; + + // Ensure we compute the same encoded length as the original any value. + if result.value_len()? != header.length { + return Err(Self::TAG.non_canonical_error()); + } + + Ok(result) + } +} + +impl<'a> EncodeValue for IntRef<'a> { + fn value_len(&self) -> Result { + match self { + IntRef::Unsigned(i) => i.value_len(), + IntRef::Signed(i) => i.value_len(), + } + } + + fn encode_value(&self, writer: &mut impl Writer) -> Result<()> { + match self { + IntRef::Unsigned(i) => i.encode_value(writer), + IntRef::Signed(i) => i.encode_value(writer), + } + } +} + +impl<'a> From<&IntRef<'a>> for IntRef<'a> { + fn from(value: &IntRef<'a>) -> IntRef<'a> { + *value + } +} + +impl<'a> TryFrom> for IntRef<'a> { + type Error = Error; + + fn try_from(any: AnyRef<'a>) -> Result> { + any.decode_as() + } +} + +impl<'a> FixedTag for IntRef<'a> { + const TAG: Tag = Tag::Integer; +} + +impl<'a> OrdIsValueOrd for IntRef<'a> {} + +/// "Big" negative ASN.1 `INTEGER` type. /// /// Provides direct access to the underlying big endian bytes which comprise -/// an signed integer value. +/// an negative integer value. /// /// Intended for use cases like very large integers that are used in /// cryptographic applications (e.g. keys, signatures). #[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] -pub struct IntRef<'a> { +pub struct SintRef<'a> { /// Inner value inner: BytesRef<'a>, } -impl<'a> IntRef<'a> { - /// Create a new [`IntRef`] from a byte slice. +impl<'a> SintRef<'a> { + /// Create a new [`SintRef`] from a byte slice. pub fn new(bytes: &'a [u8]) -> Result { let inner = BytesRef::new(int::strip_leading_ones(bytes)) .map_err(|_| ErrorKind::Length { tag: Self::TAG })?; @@ -34,7 +144,7 @@ impl<'a> IntRef<'a> { self.inner.as_slice() } - /// Get the length of this [`IntRef`] in bytes. + /// Get the length of this [`SintRef`] in bytes. pub fn len(&self) -> Length { self.inner.len() } @@ -45,7 +155,7 @@ impl<'a> IntRef<'a> { } } -impl<'a> DecodeValue<'a> for IntRef<'a> { +impl<'a> DecodeValue<'a> for SintRef<'a> { fn decode_value>(reader: &mut R, header: Header) -> Result { let bytes = BytesRef::decode_value(reader, header)?.as_slice(); let result = Self::new(int::decode_to_slice(bytes)?)?; @@ -59,35 +169,40 @@ impl<'a> DecodeValue<'a> for IntRef<'a> { } } -impl<'a> EncodeValue for IntRef<'a> { +impl<'a> EncodeValue for SintRef<'a> { fn value_len(&self) -> Result { int::encoded_len(self.inner.as_slice()) } fn encode_value(&self, writer: &mut impl Writer) -> Result<()> { + // Add leading `0xFF` byte if required + if self.value_len()? > self.len() { + writer.write_byte(0xFF)?; + } + writer.write(self.as_bytes()) } } -impl<'a> From<&IntRef<'a>> for IntRef<'a> { - fn from(value: &IntRef<'a>) -> IntRef<'a> { +impl<'a> From<&SintRef<'a>> for SintRef<'a> { + fn from(value: &SintRef<'a>) -> SintRef<'a> { *value } } -impl<'a> TryFrom> for IntRef<'a> { +impl<'a> TryFrom> for SintRef<'a> { type Error = Error; - fn try_from(any: AnyRef<'a>) -> Result> { + fn try_from(any: AnyRef<'a>) -> Result> { any.decode_as() } } -impl<'a> FixedTag for IntRef<'a> { +impl<'a> FixedTag for SintRef<'a> { const TAG: Tag = Tag::Integer; } -impl<'a> OrdIsValueOrd for IntRef<'a> {} +impl<'a> OrdIsValueOrd for SintRef<'a> {} /// "Big" unsigned ASN.1 `INTEGER` type. /// @@ -178,11 +293,11 @@ impl<'a> FixedTag for UintRef<'a> { impl<'a> OrdIsValueOrd for UintRef<'a> {} #[cfg(feature = "alloc")] -pub use self::allocating::{Int, Uint}; +pub use self::allocating::{Int, Sint, Uint}; #[cfg(feature = "alloc")] mod allocating { - use super::{super::int, super::uint, IntRef, UintRef}; + use super::{super::int, super::uint, IntRef, SintRef, UintRef}; use crate::{ asn1::AnyRef, ord::OrdIsValueOrd, @@ -191,20 +306,133 @@ mod allocating { Result, Tag, Writer, }; - /// "Big" signed ASN.1 `INTEGER` type. + /// "Big" ASN.1 `INTEGER` type that can hold both positive and negative + /// values. + #[derive(Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] + pub enum Int { + /// Unsigned integers. + Unsigned(Uint), + + /// Signed integers. These are functionally always negative, + /// since we represent positive signed integers using the unsigned variant. + Signed(Sint), + } + + impl From for Int { + fn from(value: Sint) -> Self { + Int::Signed(value) + } + } + + impl From for Int { + fn from(value: Uint) -> Self { + Int::Unsigned(value) + } + } + + impl Int { + /// Borrow the inner byte slice which contains the least significant bytes + /// of a big endian integer value with all leading ones stripped. + pub fn as_bytes(&self) -> &[u8] { + match self { + Int::Unsigned(i) => i.inner.as_slice(), + Int::Signed(i) => i.inner.as_slice(), + } + } + + /// Get the length of this [`Int`] in bytes. + pub fn len(&self) -> Length { + match self { + Int::Unsigned(i) => i.inner.len(), + Int::Signed(i) => i.inner.len(), + } + } + + /// Is the inner byte slice empty? + pub fn is_empty(&self) -> bool { + match self { + Int::Unsigned(i) => i.inner.is_empty(), + Int::Signed(i) => i.inner.is_empty(), + } + } + } + + impl<'a> DecodeValue<'a> for Int { + fn decode_value>(reader: &mut R, header: Header) -> Result { + let bytes = BytesOwned::decode_value(reader, header)?; + + // Try to decode as unsigned first, then signed. + let result: Self = match uint::decode_to_slice(bytes.as_slice()) { + Ok(result) => Ok(Uint::new(result)?.into()), + Err(Error { + kind: ErrorKind::Value { .. }, + .. + }) => Ok(Sint::new(int::decode_to_slice(bytes.as_slice())?)?.into()), + Err(e) => Err(e), + }?; + + // Ensure we compute the same encoded length as the original any value. + if result.value_len()? != header.length { + return Err(Self::TAG.non_canonical_error()); + } + + Ok(result) + } + } + + impl EncodeValue for Int { + fn value_len(&self) -> Result { + match self { + Int::Unsigned(i) => i.value_len(), + Int::Signed(i) => i.value_len(), + } + } + + fn encode_value(&self, writer: &mut impl Writer) -> Result<()> { + match self { + Int::Unsigned(i) => i.encode_value(writer), + Int::Signed(i) => i.encode_value(writer), + } + } + } + + impl<'a> From<&IntRef<'a>> for Int { + fn from(value: &IntRef<'a>) -> Int { + match value { + IntRef::Unsigned(i) => Int::Unsigned(i.into()), + IntRef::Signed(i) => Int::Signed(i.into()), + } + } + } + + impl<'a> TryFrom> for Int { + type Error = Error; + + fn try_from(any: AnyRef<'a>) -> Result { + any.decode_as() + } + } + + impl FixedTag for Int { + const TAG: Tag = Tag::Integer; + } + + impl OrdIsValueOrd for Int {} + + /// "Big" negative ASN.1 `INTEGER` type. /// - /// Provides direct storage for the big endian bytes which comprise an - /// signed integer value. + /// Provides direct storage for the big endian bytes which comprise a + /// negative integer value. /// /// Intended for use cases like very large integers that are used in /// cryptographic applications (e.g. keys, signatures). #[derive(Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] - pub struct Int { + pub struct Sint { /// Inner value inner: BytesOwned, } - impl Int { + impl Sint { /// Create a new [`Int`] from a byte slice. pub fn new(bytes: &[u8]) -> Result { let inner = BytesOwned::new(int::strip_leading_ones(bytes)) @@ -230,7 +458,7 @@ mod allocating { } } - impl<'a> DecodeValue<'a> for Int { + impl<'a> DecodeValue<'a> for Sint { fn decode_value>(reader: &mut R, header: Header) -> Result { let bytes = BytesOwned::decode_value(reader, header)?; let result = Self::new(int::decode_to_slice(bytes.as_slice())?)?; @@ -244,52 +472,57 @@ mod allocating { } } - impl EncodeValue for Int { + impl EncodeValue for Sint { fn value_len(&self) -> Result { int::encoded_len(self.inner.as_slice()) } fn encode_value(&self, writer: &mut impl Writer) -> Result<()> { + // Add leading `0xFF` byte if required + if self.value_len()? > self.len() { + writer.write_byte(0xFF)?; + } + writer.write(self.as_bytes()) } } - impl<'a> From<&IntRef<'a>> for Int { - fn from(value: &IntRef<'a>) -> Int { + impl<'a> From<&SintRef<'a>> for Sint { + fn from(value: &SintRef<'a>) -> Sint { let inner = BytesOwned::new(value.as_bytes()).expect("Invalid Int"); - Int { inner } + Sint { inner } } } - impl<'a> TryFrom> for Int { + impl<'a> TryFrom> for Sint { type Error = Error; - fn try_from(any: AnyRef<'a>) -> Result { + fn try_from(any: AnyRef<'a>) -> Result { any.decode_as() } } - impl FixedTag for Int { + impl FixedTag for Sint { const TAG: Tag = Tag::Integer; } - impl OrdIsValueOrd for Int {} + impl OrdIsValueOrd for Sint {} - impl<'a> RefToOwned<'a> for IntRef<'a> { - type Owned = Int; + impl<'a> RefToOwned<'a> for SintRef<'a> { + type Owned = Sint; fn to_owned(&self) -> Self::Owned { let inner = self.inner.to_owned(); - Int { inner } + Sint { inner } } } - impl OwnedToRef for Int { - type Borrowed<'a> = IntRef<'a>; + impl OwnedToRef for Sint { + type Borrowed<'a> = SintRef<'a>; fn to_ref(&self) -> Self::Borrowed<'_> { let inner = self.inner.to_ref(); - IntRef { inner } + SintRef { inner } } } @@ -403,41 +636,38 @@ mod allocating { #[cfg(test)] mod tests { - use super::UintRef; + use super::{IntRef, UintRef}; use crate::{ - asn1::{integer::tests::*, AnyRef, IntRef}, + asn1::{integer::tests::*, AnyRef, SintRef}, Decode, Encode, ErrorKind, SliceWriter, Tag, }; #[test] fn decode_int_bytes() { - // Positive numbers decode, but have zero extensions as necessary - // (to distinguish them from negative representations). - assert_eq!(&[0], IntRef::from_der(I0_BYTES).unwrap().as_bytes()); - assert_eq!(&[127], IntRef::from_der(I127_BYTES).unwrap().as_bytes()); - assert_eq!(&[0, 128], IntRef::from_der(I128_BYTES).unwrap().as_bytes()); - assert_eq!(&[0, 255], IntRef::from_der(I255_BYTES).unwrap().as_bytes()); - - assert_eq!( - &[0x01, 0x00], - IntRef::from_der(I256_BYTES).unwrap().as_bytes() - ); - - assert_eq!( - &[0x7F, 0xFF], - IntRef::from_der(I32767_BYTES).unwrap().as_bytes() - ); + for &example in &[ + I0_BYTES, + I127_BYTES, + I128_BYTES, + I255_BYTES, + I256_BYTES, + I32767_BYTES, + ] { + let int = IntRef::from_der(example).unwrap(); + assert!(matches!(int, IntRef::Unsigned(_))); + assert_eq!( + UintRef::from_der(example).unwrap().as_bytes(), + int.as_bytes() + ); + } - // Negative integers decode. - assert_eq!(&[128], IntRef::from_der(INEG128_BYTES).unwrap().as_bytes()); - assert_eq!( - &[255, 127], - IntRef::from_der(INEG129_BYTES).unwrap().as_bytes() - ); - assert_eq!( - &[128, 0], - IntRef::from_der(INEG32768_BYTES).unwrap().as_bytes() - ); + for &example in &[INEG128_BYTES, INEG129_BYTES, INEG32768_BYTES] { + let int = IntRef::from_der(example).unwrap(); + assert!(matches!(int, IntRef::Signed(_))); + assert_eq!( + SintRef::from_der(example).unwrap().as_bytes(), + int.as_bytes() + ); + } } #[test] @@ -449,6 +679,10 @@ mod tests { I255_BYTES, I256_BYTES, I32767_BYTES, + I65535_BYTES, + INEG128_BYTES, + INEG129_BYTES, + INEG32768_BYTES, ] { let uint = IntRef::from_der(example).unwrap(); @@ -459,9 +693,33 @@ mod tests { let result = encoder.finish().unwrap(); assert_eq!(example, result); } + } + + #[test] + fn decode_sint_bytes() { + // Positive numbers do not decode. + assert!(SintRef::from_der(I127_BYTES).is_err()); + assert!(SintRef::from_der(I128_BYTES).is_err()); + assert!(SintRef::from_der(I255_BYTES).is_err()); + + assert!(SintRef::from_der(I256_BYTES).is_err()); + + assert!(SintRef::from_der(I32767_BYTES).is_err()); + + // Negative integers (and zero) decode. + assert_eq!(&[0], SintRef::from_der(I0_BYTES).unwrap().as_bytes()); + assert_eq!(&[128], SintRef::from_der(INEG128_BYTES).unwrap().as_bytes()); + assert_eq!(&[127], SintRef::from_der(INEG129_BYTES).unwrap().as_bytes()); + assert_eq!( + &[128, 0], + SintRef::from_der(INEG32768_BYTES).unwrap().as_bytes() + ); + } + #[test] + fn encode_sint_bytes() { for &example in &[INEG128_BYTES, INEG129_BYTES, INEG32768_BYTES] { - let uint = IntRef::from_der(example).unwrap(); + let uint = SintRef::from_der(example).unwrap(); let mut buf = [0u8; 128]; let mut encoder = SliceWriter::new(&mut buf); diff --git a/der/src/asn1/integer/int.rs b/der/src/asn1/integer/int.rs index 48782e125..dba7f233f 100644 --- a/der/src/asn1/integer/int.rs +++ b/der/src/asn1/integer/int.rs @@ -4,13 +4,18 @@ use super::is_highest_bit_set; use crate::{ErrorKind, Length, Result, Tag, Writer}; pub(super) fn decode_to_slice(bytes: &[u8]) -> Result<&[u8]> { - // The `INTEGER` type always encodes a signed value and we're decoding - // as signed here, so we allow a zero extension or sign extension byte, - // but only as permitted under DER canonicalization. + // We only support decoding canonicalized negative integers here. match bytes { [] => Err(Tag::Integer.non_canonical_error()), + + // Non-canonical forms are not allowed. [0x00, byte, ..] if *byte < 0x80 => Err(Tag::Integer.non_canonical_error()), [0xFF, byte, ..] if *byte >= 0x80 => Err(Tag::Integer.non_canonical_error()), + + // Positive integers are not allowed. + [byte, ..] if *byte < 0x80 && *byte > 0 => Err(Tag::Integer.value_error()), + [0x00, _, ..] => Err(Tag::Integer.value_error()), + [0xFF, rest @ ..] => Ok(rest), _ => Ok(bytes), } } @@ -49,7 +54,8 @@ where /// Get the encoded length for the given signed integer serialized as bytes. #[inline] pub(super) fn encoded_len(bytes: &[u8]) -> Result { - Length::try_from(strip_leading_ones(bytes).len()) + let bytes = strip_leading_ones(bytes); + Length::try_from(bytes.len())? + u8::from(needs_leading_one(bytes)) } /// Strip the leading all-ones bytes from the given byte slice. @@ -66,6 +72,11 @@ pub(crate) fn strip_leading_ones(mut bytes: &[u8]) -> &[u8] { bytes } +/// Does the given integer need a leading one? +fn needs_leading_one(bytes: &[u8]) -> bool { + matches!(bytes.first(), Some(byte) if *byte < 0x80 && *byte > 0) +} + #[cfg(test)] mod tests { use super::decode_to_slice; @@ -74,17 +85,22 @@ mod tests { fn decode_to_slice_non_canonical() { // Empty integers are always non-canonical. assert!(decode_to_slice(&[]).is_err()); + // Positives with excessive zero extension are non-canonical. assert!(decode_to_slice(&[0x00, 0x00]).is_err()); + // Negatives with excessive sign extension are non-canonical. assert!(decode_to_slice(&[0xFF, 0x80]).is_err()); + + // Any positives are non-canonical. + assert!(decode_to_slice(&[0x01]).is_err()); + assert!(decode_to_slice(&[0x01, 0x02]).is_err()); + assert!(decode_to_slice(&[0x01, 0x00]).is_err()); } #[test] fn decode_to_slice_canonical() { - assert_eq!(decode_to_slice(&[0x00]).unwrap(), &[0x00]); - assert_eq!(decode_to_slice(&[0x01]).unwrap(), &[0x01]); - assert_eq!(decode_to_slice(&[0x00, 0x80]).unwrap(), &[0x00, 0x80]); - assert_eq!(decode_to_slice(&[0xFF, 0x00]).unwrap(), &[0xFF, 0x00]); + assert_eq!(decode_to_slice(&[0xFF, 0x01]).unwrap(), &[0x01]); + assert_eq!(decode_to_slice(&[0x00]).unwrap(), &[0x00]) } } diff --git a/der/src/error.rs b/der/src/error.rs index 5e492a48d..7e35d188e 100644 --- a/der/src/error.rs +++ b/der/src/error.rs @@ -18,7 +18,7 @@ pub type Result = core::result::Result; #[derive(Copy, Clone, Debug, Eq, PartialEq)] pub struct Error { /// Kind of error. - kind: ErrorKind, + pub(crate) kind: ErrorKind, /// Position inside of message where error occurred. position: Option, diff --git a/der/src/lib.rs b/der/src/lib.rs index 9068feb2c..45a5d4ede 100644 --- a/der/src/lib.rs +++ b/der/src/lib.rs @@ -58,6 +58,7 @@ //! - [`SequenceOf`]: ASN.1 `SEQUENCE OF`. //! - [`SetOf`], [`SetOfVec`]: ASN.1 `SET OF`. //! - [`UintRef`]: ASN.1 unsigned `INTEGER` with raw access to encoded bytes. +//! - [`IntRef`]: ASN.1 positive or negative `INTEGER` with raw access to encoded bytes. //! - [`UtcTime`]: ASN.1 `UTCTime`. //! - [`Utf8StringRef`]: ASN.1 `UTF8String`. //! @@ -306,6 +307,7 @@ //! [`BitStringRef`]: asn1::BitStringRef //! [`GeneralizedTime`]: asn1::GeneralizedTime //! [`Ia5StringRef`]: asn1::Ia5StringRef +//! [`IntRef`]: asn1::IntRef //! [`Null`]: asn1::Null //! [`ObjectIdentifier`]: asn1::ObjectIdentifier //! [`OctetString`]: asn1::OctetString diff --git a/x509-cert/src/serial_number.rs b/x509-cert/src/serial_number.rs index dd8b13c54..d7cfdfb2e 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 @@ -34,13 +34,15 @@ impl SerialNumber { /// Create a new [`SerialNumber`] from a byte slice. pub fn new(bytes: &[u8]) -> Result { - let inner = Int::new(bytes)?; + let inner = Uint::new(bytes)?; if inner.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 From b9ee5e495f72ea8a24e4119f888aea90b9e2e797 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Mon, 2 Jan 2023 23:41:35 -0500 Subject: [PATCH 2/5] Revert "der, x509-cert: fix handling of negative integers" This reverts commit 130f52c09e176d2289aa29fc68f3327ee563e1fa. --- der/src/asn1.rs | 5 +- der/src/asn1/integer/bigint.rs | 388 ++++++--------------------------- der/src/asn1/integer/int.rs | 32 +-- der/src/error.rs | 2 +- der/src/lib.rs | 2 - x509-cert/src/serial_number.rs | 10 +- 6 files changed, 80 insertions(+), 359 deletions(-) diff --git a/der/src/asn1.rs b/der/src/asn1.rs index ba643e827..2d24ee404 100644 --- a/der/src/asn1.rs +++ b/der/src/asn1.rs @@ -36,8 +36,8 @@ pub use const_oid::ObjectIdentifier; #[cfg_attr(docsrs, doc(cfg(feature = "alloc")))] pub use self::{ any::Any, bit_string::BitString, ia5_string::Ia5String, integer::bigint::Int, - integer::bigint::Sint, integer::bigint::Uint, octet_string::OctetString, - printable_string::PrintableString, set_of::SetOfVec, teletex_string::TeletexString, + integer::bigint::Uint, octet_string::OctetString, printable_string::PrintableString, + set_of::SetOfVec, teletex_string::TeletexString, }; pub use self::{ any::AnyRef, @@ -47,7 +47,6 @@ pub use self::{ generalized_time::GeneralizedTime, ia5_string::Ia5StringRef, integer::bigint::IntRef, - integer::bigint::SintRef, integer::bigint::UintRef, null::Null, octet_string::OctetStringRef, diff --git a/der/src/asn1/integer/bigint.rs b/der/src/asn1/integer/bigint.rs index bb2e38a76..3178c4ed4 100644 --- a/der/src/asn1/integer/bigint.rs +++ b/der/src/asn1/integer/bigint.rs @@ -6,131 +6,21 @@ use crate::{ FixedTag, Header, Length, Reader, Result, Tag, Writer, }; -/// A "big" ASN.1 `INTEGER` type that can hold both positive and negative -/// values. -#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] -pub enum IntRef<'a> { - /// Unsigned integers. - Unsigned(UintRef<'a>), - - /// Signed integers. These are functionally always negative, - /// since we represent positive signed integers using the unsigned variant. - Signed(SintRef<'a>), -} - -impl<'a> From> for IntRef<'a> { - fn from(value: UintRef<'a>) -> Self { - Self::Unsigned(value) - } -} - -impl<'a> From> for IntRef<'a> { - fn from(value: SintRef<'a>) -> Self { - Self::Signed(value) - } -} - -impl<'a> IntRef<'a> { - /// Borrow the inner byte slice which contains the least significant bytes - /// of a big endian integer value with all leading ones stripped. - pub fn as_bytes(&self) -> &'a [u8] { - match self { - IntRef::Unsigned(i) => i.inner.as_slice(), - IntRef::Signed(i) => i.inner.as_slice(), - } - } - - /// Get the length of this [`IntRef`] in bytes. - pub fn len(&self) -> Length { - match self { - IntRef::Unsigned(i) => i.len(), - IntRef::Signed(i) => i.len(), - } - } - - /// Is the inner byte slice empty? - pub fn is_empty(self) -> bool { - match self { - IntRef::Unsigned(i) => i.is_empty(), - IntRef::Signed(i) => i.is_empty(), - } - } -} - -impl<'a> DecodeValue<'a> for IntRef<'a> { - fn decode_value>(reader: &mut R, header: Header) -> Result { - let bytes = BytesRef::decode_value(reader, header)?.as_slice(); - - // Try to decode as unsigned first, then signed. - let result: Self = match uint::decode_to_slice(bytes) { - Ok(result) => Ok(UintRef::new(result)?.into()), - Err(Error { - kind: ErrorKind::Value { .. }, - .. - }) => Ok(SintRef::new(int::decode_to_slice(bytes)?)?.into()), - Err(e) => Err(e), - }?; - - // Ensure we compute the same encoded length as the original any value. - if result.value_len()? != header.length { - return Err(Self::TAG.non_canonical_error()); - } - - Ok(result) - } -} - -impl<'a> EncodeValue for IntRef<'a> { - fn value_len(&self) -> Result { - match self { - IntRef::Unsigned(i) => i.value_len(), - IntRef::Signed(i) => i.value_len(), - } - } - - fn encode_value(&self, writer: &mut impl Writer) -> Result<()> { - match self { - IntRef::Unsigned(i) => i.encode_value(writer), - IntRef::Signed(i) => i.encode_value(writer), - } - } -} - -impl<'a> From<&IntRef<'a>> for IntRef<'a> { - fn from(value: &IntRef<'a>) -> IntRef<'a> { - *value - } -} - -impl<'a> TryFrom> for IntRef<'a> { - type Error = Error; - - fn try_from(any: AnyRef<'a>) -> Result> { - any.decode_as() - } -} - -impl<'a> FixedTag for IntRef<'a> { - const TAG: Tag = Tag::Integer; -} - -impl<'a> OrdIsValueOrd for IntRef<'a> {} - -/// "Big" negative ASN.1 `INTEGER` type. +/// "Big" signed ASN.1 `INTEGER` type. /// /// Provides direct access to the underlying big endian bytes which comprise -/// an negative integer value. +/// an signed integer value. /// /// Intended for use cases like very large integers that are used in /// cryptographic applications (e.g. keys, signatures). #[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] -pub struct SintRef<'a> { +pub struct IntRef<'a> { /// Inner value inner: BytesRef<'a>, } -impl<'a> SintRef<'a> { - /// Create a new [`SintRef`] from a byte slice. +impl<'a> IntRef<'a> { + /// Create a new [`IntRef`] from a byte slice. pub fn new(bytes: &'a [u8]) -> Result { let inner = BytesRef::new(int::strip_leading_ones(bytes)) .map_err(|_| ErrorKind::Length { tag: Self::TAG })?; @@ -144,7 +34,7 @@ impl<'a> SintRef<'a> { self.inner.as_slice() } - /// Get the length of this [`SintRef`] in bytes. + /// Get the length of this [`IntRef`] in bytes. pub fn len(&self) -> Length { self.inner.len() } @@ -155,7 +45,7 @@ impl<'a> SintRef<'a> { } } -impl<'a> DecodeValue<'a> for SintRef<'a> { +impl<'a> DecodeValue<'a> for IntRef<'a> { fn decode_value>(reader: &mut R, header: Header) -> Result { let bytes = BytesRef::decode_value(reader, header)?.as_slice(); let result = Self::new(int::decode_to_slice(bytes)?)?; @@ -169,40 +59,35 @@ impl<'a> DecodeValue<'a> for SintRef<'a> { } } -impl<'a> EncodeValue for SintRef<'a> { +impl<'a> EncodeValue for IntRef<'a> { fn value_len(&self) -> Result { int::encoded_len(self.inner.as_slice()) } fn encode_value(&self, writer: &mut impl Writer) -> Result<()> { - // Add leading `0xFF` byte if required - if self.value_len()? > self.len() { - writer.write_byte(0xFF)?; - } - writer.write(self.as_bytes()) } } -impl<'a> From<&SintRef<'a>> for SintRef<'a> { - fn from(value: &SintRef<'a>) -> SintRef<'a> { +impl<'a> From<&IntRef<'a>> for IntRef<'a> { + fn from(value: &IntRef<'a>) -> IntRef<'a> { *value } } -impl<'a> TryFrom> for SintRef<'a> { +impl<'a> TryFrom> for IntRef<'a> { type Error = Error; - fn try_from(any: AnyRef<'a>) -> Result> { + fn try_from(any: AnyRef<'a>) -> Result> { any.decode_as() } } -impl<'a> FixedTag for SintRef<'a> { +impl<'a> FixedTag for IntRef<'a> { const TAG: Tag = Tag::Integer; } -impl<'a> OrdIsValueOrd for SintRef<'a> {} +impl<'a> OrdIsValueOrd for IntRef<'a> {} /// "Big" unsigned ASN.1 `INTEGER` type. /// @@ -293,11 +178,11 @@ impl<'a> FixedTag for UintRef<'a> { impl<'a> OrdIsValueOrd for UintRef<'a> {} #[cfg(feature = "alloc")] -pub use self::allocating::{Int, Sint, Uint}; +pub use self::allocating::{Int, Uint}; #[cfg(feature = "alloc")] mod allocating { - use super::{super::int, super::uint, IntRef, SintRef, UintRef}; + use super::{super::int, super::uint, IntRef, UintRef}; use crate::{ asn1::AnyRef, ord::OrdIsValueOrd, @@ -306,133 +191,20 @@ mod allocating { Result, Tag, Writer, }; - /// "Big" ASN.1 `INTEGER` type that can hold both positive and negative - /// values. - #[derive(Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] - pub enum Int { - /// Unsigned integers. - Unsigned(Uint), - - /// Signed integers. These are functionally always negative, - /// since we represent positive signed integers using the unsigned variant. - Signed(Sint), - } - - impl From for Int { - fn from(value: Sint) -> Self { - Int::Signed(value) - } - } - - impl From for Int { - fn from(value: Uint) -> Self { - Int::Unsigned(value) - } - } - - impl Int { - /// Borrow the inner byte slice which contains the least significant bytes - /// of a big endian integer value with all leading ones stripped. - pub fn as_bytes(&self) -> &[u8] { - match self { - Int::Unsigned(i) => i.inner.as_slice(), - Int::Signed(i) => i.inner.as_slice(), - } - } - - /// Get the length of this [`Int`] in bytes. - pub fn len(&self) -> Length { - match self { - Int::Unsigned(i) => i.inner.len(), - Int::Signed(i) => i.inner.len(), - } - } - - /// Is the inner byte slice empty? - pub fn is_empty(&self) -> bool { - match self { - Int::Unsigned(i) => i.inner.is_empty(), - Int::Signed(i) => i.inner.is_empty(), - } - } - } - - impl<'a> DecodeValue<'a> for Int { - fn decode_value>(reader: &mut R, header: Header) -> Result { - let bytes = BytesOwned::decode_value(reader, header)?; - - // Try to decode as unsigned first, then signed. - let result: Self = match uint::decode_to_slice(bytes.as_slice()) { - Ok(result) => Ok(Uint::new(result)?.into()), - Err(Error { - kind: ErrorKind::Value { .. }, - .. - }) => Ok(Sint::new(int::decode_to_slice(bytes.as_slice())?)?.into()), - Err(e) => Err(e), - }?; - - // Ensure we compute the same encoded length as the original any value. - if result.value_len()? != header.length { - return Err(Self::TAG.non_canonical_error()); - } - - Ok(result) - } - } - - impl EncodeValue for Int { - fn value_len(&self) -> Result { - match self { - Int::Unsigned(i) => i.value_len(), - Int::Signed(i) => i.value_len(), - } - } - - fn encode_value(&self, writer: &mut impl Writer) -> Result<()> { - match self { - Int::Unsigned(i) => i.encode_value(writer), - Int::Signed(i) => i.encode_value(writer), - } - } - } - - impl<'a> From<&IntRef<'a>> for Int { - fn from(value: &IntRef<'a>) -> Int { - match value { - IntRef::Unsigned(i) => Int::Unsigned(i.into()), - IntRef::Signed(i) => Int::Signed(i.into()), - } - } - } - - impl<'a> TryFrom> for Int { - type Error = Error; - - fn try_from(any: AnyRef<'a>) -> Result { - any.decode_as() - } - } - - impl FixedTag for Int { - const TAG: Tag = Tag::Integer; - } - - impl OrdIsValueOrd for Int {} - - /// "Big" negative ASN.1 `INTEGER` type. + /// "Big" signed ASN.1 `INTEGER` type. /// - /// Provides direct storage for the big endian bytes which comprise a - /// negative integer value. + /// Provides direct storage for the big endian bytes which comprise an + /// signed integer value. /// /// Intended for use cases like very large integers that are used in /// cryptographic applications (e.g. keys, signatures). #[derive(Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] - pub struct Sint { + pub struct Int { /// Inner value inner: BytesOwned, } - impl Sint { + impl Int { /// Create a new [`Int`] from a byte slice. pub fn new(bytes: &[u8]) -> Result { let inner = BytesOwned::new(int::strip_leading_ones(bytes)) @@ -458,7 +230,7 @@ mod allocating { } } - impl<'a> DecodeValue<'a> for Sint { + impl<'a> DecodeValue<'a> for Int { fn decode_value>(reader: &mut R, header: Header) -> Result { let bytes = BytesOwned::decode_value(reader, header)?; let result = Self::new(int::decode_to_slice(bytes.as_slice())?)?; @@ -472,57 +244,52 @@ mod allocating { } } - impl EncodeValue for Sint { + impl EncodeValue for Int { fn value_len(&self) -> Result { int::encoded_len(self.inner.as_slice()) } fn encode_value(&self, writer: &mut impl Writer) -> Result<()> { - // Add leading `0xFF` byte if required - if self.value_len()? > self.len() { - writer.write_byte(0xFF)?; - } - writer.write(self.as_bytes()) } } - impl<'a> From<&SintRef<'a>> for Sint { - fn from(value: &SintRef<'a>) -> Sint { + impl<'a> From<&IntRef<'a>> for Int { + fn from(value: &IntRef<'a>) -> Int { let inner = BytesOwned::new(value.as_bytes()).expect("Invalid Int"); - Sint { inner } + Int { inner } } } - impl<'a> TryFrom> for Sint { + impl<'a> TryFrom> for Int { type Error = Error; - fn try_from(any: AnyRef<'a>) -> Result { + fn try_from(any: AnyRef<'a>) -> Result { any.decode_as() } } - impl FixedTag for Sint { + impl FixedTag for Int { const TAG: Tag = Tag::Integer; } - impl OrdIsValueOrd for Sint {} + impl OrdIsValueOrd for Int {} - impl<'a> RefToOwned<'a> for SintRef<'a> { - type Owned = Sint; + impl<'a> RefToOwned<'a> for IntRef<'a> { + type Owned = Int; fn to_owned(&self) -> Self::Owned { let inner = self.inner.to_owned(); - Sint { inner } + Int { inner } } } - impl OwnedToRef for Sint { - type Borrowed<'a> = SintRef<'a>; + impl OwnedToRef for Int { + type Borrowed<'a> = IntRef<'a>; fn to_ref(&self) -> Self::Borrowed<'_> { let inner = self.inner.to_ref(); - SintRef { inner } + IntRef { inner } } } @@ -636,38 +403,41 @@ mod allocating { #[cfg(test)] mod tests { - use super::{IntRef, UintRef}; + use super::UintRef; use crate::{ - asn1::{integer::tests::*, AnyRef, SintRef}, + asn1::{integer::tests::*, AnyRef, IntRef}, Decode, Encode, ErrorKind, SliceWriter, Tag, }; #[test] fn decode_int_bytes() { - for &example in &[ - I0_BYTES, - I127_BYTES, - I128_BYTES, - I255_BYTES, - I256_BYTES, - I32767_BYTES, - ] { - let int = IntRef::from_der(example).unwrap(); - assert!(matches!(int, IntRef::Unsigned(_))); - assert_eq!( - UintRef::from_der(example).unwrap().as_bytes(), - int.as_bytes() - ); - } + // Positive numbers decode, but have zero extensions as necessary + // (to distinguish them from negative representations). + assert_eq!(&[0], IntRef::from_der(I0_BYTES).unwrap().as_bytes()); + assert_eq!(&[127], IntRef::from_der(I127_BYTES).unwrap().as_bytes()); + assert_eq!(&[0, 128], IntRef::from_der(I128_BYTES).unwrap().as_bytes()); + assert_eq!(&[0, 255], IntRef::from_der(I255_BYTES).unwrap().as_bytes()); - for &example in &[INEG128_BYTES, INEG129_BYTES, INEG32768_BYTES] { - let int = IntRef::from_der(example).unwrap(); - assert!(matches!(int, IntRef::Signed(_))); - assert_eq!( - SintRef::from_der(example).unwrap().as_bytes(), - int.as_bytes() - ); - } + assert_eq!( + &[0x01, 0x00], + IntRef::from_der(I256_BYTES).unwrap().as_bytes() + ); + + assert_eq!( + &[0x7F, 0xFF], + IntRef::from_der(I32767_BYTES).unwrap().as_bytes() + ); + + // Negative integers decode. + assert_eq!(&[128], IntRef::from_der(INEG128_BYTES).unwrap().as_bytes()); + assert_eq!( + &[255, 127], + IntRef::from_der(INEG129_BYTES).unwrap().as_bytes() + ); + assert_eq!( + &[128, 0], + IntRef::from_der(INEG32768_BYTES).unwrap().as_bytes() + ); } #[test] @@ -679,10 +449,6 @@ mod tests { I255_BYTES, I256_BYTES, I32767_BYTES, - I65535_BYTES, - INEG128_BYTES, - INEG129_BYTES, - INEG32768_BYTES, ] { let uint = IntRef::from_der(example).unwrap(); @@ -693,33 +459,9 @@ mod tests { let result = encoder.finish().unwrap(); assert_eq!(example, result); } - } - - #[test] - fn decode_sint_bytes() { - // Positive numbers do not decode. - assert!(SintRef::from_der(I127_BYTES).is_err()); - assert!(SintRef::from_der(I128_BYTES).is_err()); - assert!(SintRef::from_der(I255_BYTES).is_err()); - - assert!(SintRef::from_der(I256_BYTES).is_err()); - - assert!(SintRef::from_der(I32767_BYTES).is_err()); - - // Negative integers (and zero) decode. - assert_eq!(&[0], SintRef::from_der(I0_BYTES).unwrap().as_bytes()); - assert_eq!(&[128], SintRef::from_der(INEG128_BYTES).unwrap().as_bytes()); - assert_eq!(&[127], SintRef::from_der(INEG129_BYTES).unwrap().as_bytes()); - assert_eq!( - &[128, 0], - SintRef::from_der(INEG32768_BYTES).unwrap().as_bytes() - ); - } - #[test] - fn encode_sint_bytes() { for &example in &[INEG128_BYTES, INEG129_BYTES, INEG32768_BYTES] { - let uint = SintRef::from_der(example).unwrap(); + let uint = IntRef::from_der(example).unwrap(); let mut buf = [0u8; 128]; let mut encoder = SliceWriter::new(&mut buf); diff --git a/der/src/asn1/integer/int.rs b/der/src/asn1/integer/int.rs index dba7f233f..48782e125 100644 --- a/der/src/asn1/integer/int.rs +++ b/der/src/asn1/integer/int.rs @@ -4,18 +4,13 @@ use super::is_highest_bit_set; use crate::{ErrorKind, Length, Result, Tag, Writer}; pub(super) fn decode_to_slice(bytes: &[u8]) -> Result<&[u8]> { - // We only support decoding canonicalized negative integers here. + // The `INTEGER` type always encodes a signed value and we're decoding + // as signed here, so we allow a zero extension or sign extension byte, + // but only as permitted under DER canonicalization. match bytes { [] => Err(Tag::Integer.non_canonical_error()), - - // Non-canonical forms are not allowed. [0x00, byte, ..] if *byte < 0x80 => Err(Tag::Integer.non_canonical_error()), [0xFF, byte, ..] if *byte >= 0x80 => Err(Tag::Integer.non_canonical_error()), - - // Positive integers are not allowed. - [byte, ..] if *byte < 0x80 && *byte > 0 => Err(Tag::Integer.value_error()), - [0x00, _, ..] => Err(Tag::Integer.value_error()), - [0xFF, rest @ ..] => Ok(rest), _ => Ok(bytes), } } @@ -54,8 +49,7 @@ where /// Get the encoded length for the given signed integer serialized as bytes. #[inline] pub(super) fn encoded_len(bytes: &[u8]) -> Result { - let bytes = strip_leading_ones(bytes); - Length::try_from(bytes.len())? + u8::from(needs_leading_one(bytes)) + Length::try_from(strip_leading_ones(bytes).len()) } /// Strip the leading all-ones bytes from the given byte slice. @@ -72,11 +66,6 @@ pub(crate) fn strip_leading_ones(mut bytes: &[u8]) -> &[u8] { bytes } -/// Does the given integer need a leading one? -fn needs_leading_one(bytes: &[u8]) -> bool { - matches!(bytes.first(), Some(byte) if *byte < 0x80 && *byte > 0) -} - #[cfg(test)] mod tests { use super::decode_to_slice; @@ -85,22 +74,17 @@ mod tests { fn decode_to_slice_non_canonical() { // Empty integers are always non-canonical. assert!(decode_to_slice(&[]).is_err()); - // Positives with excessive zero extension are non-canonical. assert!(decode_to_slice(&[0x00, 0x00]).is_err()); - // Negatives with excessive sign extension are non-canonical. assert!(decode_to_slice(&[0xFF, 0x80]).is_err()); - - // Any positives are non-canonical. - assert!(decode_to_slice(&[0x01]).is_err()); - assert!(decode_to_slice(&[0x01, 0x02]).is_err()); - assert!(decode_to_slice(&[0x01, 0x00]).is_err()); } #[test] fn decode_to_slice_canonical() { - assert_eq!(decode_to_slice(&[0xFF, 0x01]).unwrap(), &[0x01]); - assert_eq!(decode_to_slice(&[0x00]).unwrap(), &[0x00]) + assert_eq!(decode_to_slice(&[0x00]).unwrap(), &[0x00]); + assert_eq!(decode_to_slice(&[0x01]).unwrap(), &[0x01]); + assert_eq!(decode_to_slice(&[0x00, 0x80]).unwrap(), &[0x00, 0x80]); + assert_eq!(decode_to_slice(&[0xFF, 0x00]).unwrap(), &[0xFF, 0x00]); } } diff --git a/der/src/error.rs b/der/src/error.rs index 7e35d188e..5e492a48d 100644 --- a/der/src/error.rs +++ b/der/src/error.rs @@ -18,7 +18,7 @@ pub type Result = core::result::Result; #[derive(Copy, Clone, Debug, Eq, PartialEq)] pub struct Error { /// Kind of error. - pub(crate) kind: ErrorKind, + kind: ErrorKind, /// Position inside of message where error occurred. position: Option, diff --git a/der/src/lib.rs b/der/src/lib.rs index 45a5d4ede..9068feb2c 100644 --- a/der/src/lib.rs +++ b/der/src/lib.rs @@ -58,7 +58,6 @@ //! - [`SequenceOf`]: ASN.1 `SEQUENCE OF`. //! - [`SetOf`], [`SetOfVec`]: ASN.1 `SET OF`. //! - [`UintRef`]: ASN.1 unsigned `INTEGER` with raw access to encoded bytes. -//! - [`IntRef`]: ASN.1 positive or negative `INTEGER` with raw access to encoded bytes. //! - [`UtcTime`]: ASN.1 `UTCTime`. //! - [`Utf8StringRef`]: ASN.1 `UTF8String`. //! @@ -307,7 +306,6 @@ //! [`BitStringRef`]: asn1::BitStringRef //! [`GeneralizedTime`]: asn1::GeneralizedTime //! [`Ia5StringRef`]: asn1::Ia5StringRef -//! [`IntRef`]: asn1::IntRef //! [`Null`]: asn1::Null //! [`ObjectIdentifier`]: asn1::ObjectIdentifier //! [`OctetString`]: asn1::OctetString diff --git a/x509-cert/src/serial_number.rs b/x509-cert/src/serial_number.rs index d7cfdfb2e..dd8b13c54 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, asn1::Uint, DecodeValue, EncodeValue, ErrorKind, FixedTag, Header, Length, Reader, - Result, Tag, ValueOrd, Writer, + asn1::Int, DecodeValue, EncodeValue, ErrorKind, FixedTag, Header, Length, Reader, Result, Tag, + ValueOrd, Writer, }; /// [RFC 5280 Section 4.1.2.2.] Serial Number @@ -34,15 +34,13 @@ impl SerialNumber { /// Create a new [`SerialNumber`] from a byte slice. pub fn new(bytes: &[u8]) -> Result { - let inner = Uint::new(bytes)?; + let inner = Int::new(bytes)?; if inner.len() > SerialNumber::MAX_LEN { return Err(ErrorKind::Overlength.into()); } - Ok(Self { - inner: inner.into(), - }) + Ok(Self { inner }) } /// Borrow the inner byte slice which contains the least significant bytes From 054d0ca8bb79efea958f263d87707883d92a1d34 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Tue, 3 Jan 2023 00:48:57 -0500 Subject: [PATCH 3/5] der, x509-cert: clean up negative int handling Take two. --- der/src/asn1/integer.rs | 2 +- der/src/asn1/integer/bigint.rs | 24 +++++++++++- der/src/asn1/integer/int.rs | 6 +-- x509-cert/src/serial_number.rs | 67 +++++++++++++++++++++++++++++----- 4 files changed, 84 insertions(+), 15 deletions(-) diff --git a/der/src/asn1/integer.rs b/der/src/asn1/integer.rs index 52dbd8468..7ebfac2f2 100644 --- a/der/src/asn1/integer.rs +++ b/der/src/asn1/integer.rs @@ -36,7 +36,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 3178c4ed4..888de192d 100644 --- a/der/src/asn1/integer/bigint.rs +++ b/der/src/asn1/integer/bigint.rs @@ -61,7 +61,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<()> { @@ -182,6 +183,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::{ asn1::AnyRef, @@ -246,7 +249,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<()> { @@ -261,6 +265,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<'a> TryFrom> for Int { type Error = Error; 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 dd8b13c54..f426e837f 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* tp 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,10 +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]).unwrap(); + { + 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") + } } } From 234f8585e014e8bd1e4a69616ff1bf887ecc4e1f Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Tue, 3 Jan 2023 00:58:25 -0500 Subject: [PATCH 4/5] serial_number: typo --- x509-cert/src/serial_number.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x509-cert/src/serial_number.rs b/x509-cert/src/serial_number.rs index f426e837f..ae9242b21 100644 --- a/x509-cert/src/serial_number.rs +++ b/x509-cert/src/serial_number.rs @@ -44,7 +44,7 @@ impl SerialNumber { // 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* tp 20 bytes or fewer while permitting + // `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()); From 20f0dd8e81682706c9c5b496d123ba31615f107a Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Fri, 20 Jan 2023 00:25:29 -0500 Subject: [PATCH 5/5] x509-cert: fixup merge --- x509-cert/src/serial_number.rs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/x509-cert/src/serial_number.rs b/x509-cert/src/serial_number.rs index 406b26dbf..ae9242b21 100644 --- a/x509-cert/src/serial_number.rs +++ b/x509-cert/src/serial_number.rs @@ -134,11 +134,6 @@ mod tests { let too_big = [0x80; 20]; assert!(SerialNumber::new(&too_big).is_err()); } - } - - fn serial_number_display() { - let sn = SerialNumber::new(&[0xAA, 0xBB, 0xCC, 0x01, 0x10, 0x00, 0x11]) - .expect("unexpected error"); // Creating a new serial with the maximum encoding succeeds. {