From f0485cdee05274b7a48e537e43b72da954de2401 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Wed, 28 Dec 2022 00:26:15 -0500 Subject: [PATCH 1/4] der: initial support for signed bigints --- der/src/asn1.rs | 22 ++-- der/src/asn1/integer.rs | 3 +- der/src/asn1/integer/bigint.rs | 191 ++++++++++++++++++++++++++++++++- der/src/asn1/integer/int.rs | 20 +++- 4 files changed, 217 insertions(+), 19 deletions(-) diff --git a/der/src/asn1.rs b/der/src/asn1.rs index 4b1bcf52e..0516af6be 100644 --- a/der/src/asn1.rs +++ b/der/src/asn1.rs @@ -25,6 +25,16 @@ mod utc_time; mod utf8_string; mod videotex_string; +#[cfg(feature = "oid")] +#[cfg_attr(docsrs, doc(cfg(feature = "oid")))] +pub use const_oid::ObjectIdentifier; + +#[cfg(feature = "alloc")] +#[cfg_attr(docsrs, doc(cfg(feature = "alloc")))] +pub use self::{ + any::Any, bit_string::BitString, integer::bigint::Uint, octet_string::OctetString, + set_of::SetOfVec, +}; pub use self::{ any::AnyRef, bit_string::{BitStringIter, BitStringRef}, @@ -32,6 +42,7 @@ pub use self::{ context_specific::{ContextSpecific, ContextSpecificRef}, generalized_time::GeneralizedTime, ia5_string::Ia5StringRef, + integer::bigint::IntRef, integer::bigint::UintRef, null::Null, octet_string::OctetStringRef, @@ -44,14 +55,3 @@ pub use self::{ utf8_string::Utf8StringRef, videotex_string::VideotexStringRef, }; - -#[cfg(feature = "alloc")] -#[cfg_attr(docsrs, doc(cfg(feature = "alloc")))] -pub use self::{ - any::Any, bit_string::BitString, integer::bigint::Uint, octet_string::OctetString, - set_of::SetOfVec, -}; - -#[cfg(feature = "oid")] -#[cfg_attr(docsrs, doc(cfg(feature = "oid")))] -pub use const_oid::ObjectIdentifier; diff --git a/der/src/asn1/integer.rs b/der/src/asn1/integer.rs index 762eb7cdb..f594aa1e6 100644 --- a/der/src/asn1/integer.rs +++ b/der/src/asn1/integer.rs @@ -4,11 +4,12 @@ pub(super) mod bigint; pub(super) mod int; pub(super) mod uint; +use core::{cmp::Ordering, mem}; + use crate::{ asn1::AnyRef, ByteSlice, DecodeValue, EncodeValue, Error, FixedTag, Header, Length, Reader, Result, SliceWriter, Tag, ValueOrd, Writer, }; -use core::{cmp::Ordering, mem}; macro_rules! impl_int_encoding { ($($int:ty => $uint:ty),+) => { diff --git a/der/src/asn1/integer/bigint.rs b/der/src/asn1/integer/bigint.rs index 1d5eda0a1..b3ef1a84a 100644 --- a/der/src/asn1/integer/bigint.rs +++ b/der/src/asn1/integer/bigint.rs @@ -1,11 +1,94 @@ //! "Big" ASN.1 `INTEGER` types. -use super::uint; +use super::{int, uint}; use crate::{ asn1::AnyRef, ord::OrdIsValueOrd, ByteSlice, DecodeValue, EncodeValue, Error, ErrorKind, FixedTag, Header, Length, Reader, Result, Tag, Writer, }; +/// "Big" signed ASN.1 `INTEGER` type. +/// +/// Provides direct access to the underlying 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(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] +pub struct IntRef<'a> { + /// Inner value + inner: ByteSlice<'a>, +} + +impl<'a> IntRef<'a> { + /// Create a new [`IntRef`] from a byte slice. + pub fn new(bytes: &'a [u8]) -> Result { + let inner = ByteSlice::new(int::strip_leading_ones(bytes)) + .map_err(|_| ErrorKind::Length { tag: Self::TAG })?; + + Ok(Self { inner }) + } + + /// 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] { + self.inner.as_slice() + } + + /// Get the length of this [`IntRef`] in bytes. + pub fn len(&self) -> Length { + self.inner.len() + } + + /// Is the inner byte slice empty? + pub fn is_empty(&self) -> bool { + self.inner.is_empty() + } +} + +impl<'a> DecodeValue<'a> for IntRef<'a> { + fn decode_value>(reader: &mut R, header: Header) -> Result { + let bytes = ByteSlice::decode_value(reader, header)?.as_slice(); + let result = Self::new(int::decode_to_slice(bytes)?)?; + + // 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 { + int::encoded_len(self.inner.as_slice()) + } + + fn encode_value(&self, writer: &mut dyn Writer) -> Result<()> { + writer.write(self.as_bytes()) + } +} + +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_into() + } +} + +impl<'a> FixedTag for IntRef<'a> { + const TAG: Tag = Tag::Integer; +} + +impl<'a> OrdIsValueOrd for IntRef<'a> {} + /// "Big" unsigned ASN.1 `INTEGER` type. /// /// Provides direct access to the underlying big endian bytes which comprise an @@ -95,11 +178,11 @@ impl<'a> FixedTag for UintRef<'a> { impl<'a> OrdIsValueOrd for UintRef<'a> {} #[cfg(feature = "alloc")] -pub use self::allocating::Uint; +pub use self::allocating::{Int, Uint}; #[cfg(feature = "alloc")] mod allocating { - use super::{super::uint, UintRef}; + use super::{super::int, super::uint, IntRef, UintRef}; use crate::{ asn1::AnyRef, ord::OrdIsValueOrd, @@ -108,6 +191,108 @@ mod allocating { Result, Tag, Writer, }; + /// "Big" signed ASN.1 `INTEGER` type. + /// + /// 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 Int { + /// Inner value + inner: Bytes, + } + + impl Int { + /// Create a new [`Int`] from a byte slice. + pub fn new(bytes: &[u8]) -> Result { + let inner = Bytes::new(int::strip_leading_ones(bytes)) + .map_err(|_| ErrorKind::Length { tag: Self::TAG })?; + + Ok(Self { inner }) + } + + /// 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] { + self.inner.as_slice() + } + + /// Get the length of this [`Int`] in bytes. + pub fn len(&self) -> Length { + self.inner.len() + } + + /// Is the inner byte slice empty? + pub fn is_empty(&self) -> bool { + self.inner.is_empty() + } + } + + impl<'a> DecodeValue<'a> for Int { + fn decode_value>(reader: &mut R, header: Header) -> Result { + let bytes = Bytes::decode_value(reader, header)?; + let result = Self::new(int::decode_to_slice(bytes.as_slice())?)?; + + // 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 { + int::encoded_len(self.inner.as_slice()) + } + + fn encode_value(&self, writer: &mut dyn Writer) -> Result<()> { + writer.write(self.as_bytes()) + } + } + + impl<'a> From<&IntRef<'a>> for Int { + fn from(value: &IntRef<'a>) -> Int { + let inner = Bytes::new(value.as_bytes()).expect("Invalid Int"); + Int { inner } + } + } + + impl<'a> TryFrom> for Int { + type Error = Error; + + fn try_from(any: AnyRef<'a>) -> Result { + any.decode_into() + } + } + + impl FixedTag for Int { + const TAG: Tag = Tag::Integer; + } + + impl OrdIsValueOrd for Int {} + + impl<'a> RefToOwned<'a> for IntRef<'a> { + type Owned = Int; + fn to_owned(&self) -> Self::Owned { + let inner = self.inner.to_owned(); + + Int { inner } + } + } + + impl OwnedToRef for Int { + type Borrowed<'a> = IntRef<'a>; + fn to_ref(&self) -> Self::Borrowed<'_> { + let inner = self.inner.to_ref(); + + IntRef { inner } + } + } + /// "Big" unsigned ASN.1 `INTEGER` type. /// /// Provides direct storage for the big endian bytes which comprise an diff --git a/der/src/asn1/integer/int.rs b/der/src/asn1/integer/int.rs index a9fe43890..89b3b4184 100644 --- a/der/src/asn1/integer/int.rs +++ b/der/src/asn1/integer/int.rs @@ -1,9 +1,21 @@ //! Support for encoding negative integers use super::is_highest_bit_set; -use crate::{ErrorKind, Length, Result, Writer}; +use crate::{ErrorKind, Length, Result, Tag, Writer}; -/// Decode an unsigned integer of the specified size. +pub(super) fn decode_to_slice(bytes: &[u8]) -> Result<&[u8]> { + // The `INTEGER` type always encodes a signed value, so for unsigned + // values the leading `0x00` byte may need to be removed. + match bytes { + [] => Err(Tag::Integer.non_canonical_error()), + [0] => Ok(bytes), + [0, byte, ..] if *byte < 0x80 => Err(Tag::Integer.non_canonical_error()), + [0, rest @ ..] => Ok(rest), + _ => Ok(bytes), + } +} + +/// Decode an signed integer of the specified size. /// /// Returns a byte array of the requested size containing a big endian integer. pub(super) fn decode_to_array(bytes: &[u8]) -> Result<[u8; N]> { @@ -34,14 +46,14 @@ where writer.write(strip_leading_ones(bytes)) } -/// Get the encoded length for the given unsigned integer serialized as bytes. +/// 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()) } /// Strip the leading all-ones bytes from the given byte slice. -fn strip_leading_ones(mut bytes: &[u8]) -> &[u8] { +pub(crate) fn strip_leading_ones(mut bytes: &[u8]) -> &[u8] { while let Some((byte, rest)) = bytes.split_first() { if *byte == 0xFF && is_highest_bit_set(rest) { bytes = rest; From f07ddda46975e8277ece14c7083d683263337aba Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Wed, 28 Dec 2022 17:14:47 -0500 Subject: [PATCH 2/4] der/asn1/integer: fix signed decoding, add tests Signed-off-by: William Woodruff --- der/src/asn1/integer/bigint.rs | 65 +++++++++++++++++++++++++++++++++- der/src/asn1/integer/int.rs | 33 ++++++++++++++--- 2 files changed, 92 insertions(+), 6 deletions(-) diff --git a/der/src/asn1/integer/bigint.rs b/der/src/asn1/integer/bigint.rs index b3ef1a84a..f7b107df6 100644 --- a/der/src/asn1/integer/bigint.rs +++ b/der/src/asn1/integer/bigint.rs @@ -405,10 +405,73 @@ mod allocating { mod tests { use super::UintRef; use crate::{ - asn1::{integer::tests::*, AnyRef}, + asn1::{integer::tests::*, AnyRef, IntRef}, 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() + ); + + // 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] + fn encode_int_bytes() { + for &example in &[ + I0_BYTES, + I127_BYTES, + I128_BYTES, + I255_BYTES, + I256_BYTES, + I32767_BYTES, + ] { + let uint = IntRef::from_der(example).unwrap(); + + let mut buf = [0u8; 128]; + let mut encoder = SliceWriter::new(&mut buf); + uint.encode(&mut encoder).unwrap(); + + let result = encoder.finish().unwrap(); + assert_eq!(example, result); + } + + for &example in &[INEG128_BYTES, INEG129_BYTES, INEG32768_BYTES] { + let uint = IntRef::from_der(example).unwrap(); + + let mut buf = [0u8; 128]; + let mut encoder = SliceWriter::new(&mut buf); + uint.encode(&mut encoder).unwrap(); + + let result = encoder.finish().unwrap(); + assert_eq!(example, result); + } + } + #[test] fn decode_uint_bytes() { assert_eq!(&[0], UintRef::from_der(I0_BYTES).unwrap().as_bytes()); diff --git a/der/src/asn1/integer/int.rs b/der/src/asn1/integer/int.rs index 89b3b4184..48782e125 100644 --- a/der/src/asn1/integer/int.rs +++ b/der/src/asn1/integer/int.rs @@ -4,13 +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]> { - // The `INTEGER` type always encodes a signed value, so for unsigned - // values the leading `0x00` byte may need to be removed. + // 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()), - [0] => Ok(bytes), - [0, byte, ..] if *byte < 0x80 => Err(Tag::Integer.non_canonical_error()), - [0, rest @ ..] => Ok(rest), + [0x00, byte, ..] if *byte < 0x80 => Err(Tag::Integer.non_canonical_error()), + [0xFF, byte, ..] if *byte >= 0x80 => Err(Tag::Integer.non_canonical_error()), _ => Ok(bytes), } } @@ -65,3 +65,26 @@ pub(crate) fn strip_leading_ones(mut bytes: &[u8]) -> &[u8] { bytes } + +#[cfg(test)] +mod tests { + use super::decode_to_slice; + + #[test] + 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()); + } + + #[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]); + } +} From e1a7e46f950926977cf7b8ba56fe5069060a78a1 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Wed, 28 Dec 2022 17:25:52 -0500 Subject: [PATCH 3/4] x509-cert, der: switch SerialNumber to signed Signed-off-by: William Woodruff --- der/src/asn1.rs | 4 ++-- x509-cert/src/serial_number.rs | 10 +++++----- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/der/src/asn1.rs b/der/src/asn1.rs index 0516af6be..947966ec8 100644 --- a/der/src/asn1.rs +++ b/der/src/asn1.rs @@ -32,8 +32,8 @@ pub use const_oid::ObjectIdentifier; #[cfg(feature = "alloc")] #[cfg_attr(docsrs, doc(cfg(feature = "alloc")))] pub use self::{ - any::Any, bit_string::BitString, integer::bigint::Uint, octet_string::OctetString, - set_of::SetOfVec, + any::Any, bit_string::BitString, integer::bigint::Int, integer::bigint::Uint, + octet_string::OctetString, set_of::SetOfVec, }; pub use self::{ any::AnyRef, diff --git a/x509-cert/src/serial_number.rs b/x509-cert/src/serial_number.rs index c671f0ce2..bfa4f3bae 100644 --- a/x509-cert/src/serial_number.rs +++ b/x509-cert/src/serial_number.rs @@ -3,7 +3,7 @@ use core::fmt::Display; use der::{ - asn1::Uint, DecodeValue, EncodeValue, ErrorKind, FixedTag, Header, Length, Reader, Result, Tag, + asn1::Int, DecodeValue, EncodeValue, ErrorKind, FixedTag, Header, Length, Reader, Result, Tag, ValueOrd, Writer, }; @@ -25,7 +25,7 @@ use der::{ /// gracefully handle such certificates. #[derive(Clone, Debug, Eq, PartialEq, ValueOrd, PartialOrd, Ord)] pub struct SerialNumber { - inner: Uint, + inner: Int, } impl SerialNumber { @@ -34,7 +34,7 @@ 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()); @@ -62,7 +62,7 @@ impl EncodeValue for SerialNumber { impl<'a> DecodeValue<'a> for SerialNumber { fn decode_value>(reader: &mut R, header: Header) -> Result { - let inner = Uint::decode_value(reader, header)?; + let inner = Int::decode_value(reader, header)?; if inner.len() > SerialNumber::MAX_LEN { return Err(ErrorKind::Overlength.into()); @@ -73,7 +73,7 @@ impl<'a> DecodeValue<'a> for SerialNumber { } impl FixedTag for SerialNumber { - const TAG: Tag = ::TAG; + const TAG: Tag = ::TAG; } impl Display for SerialNumber { From e258392156837a95fa1e3a0b8951075d8c770a96 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Wed, 28 Dec 2022 17:43:01 -0500 Subject: [PATCH 4/4] x509-cert: add a round-trip negative SN test Signed-off-by: William Woodruff --- x509-cert/tests/certificate.rs | 15 +++++++++++++++ .../28903a635b5280fae6774c0b6da7d6baa64af2e8.der | Bin 0 -> 1370 bytes 2 files changed, 15 insertions(+) create mode 100644 x509-cert/tests/examples/28903a635b5280fae6774c0b6da7d6baa64af2e8.der diff --git a/x509-cert/tests/certificate.rs b/x509-cert/tests/certificate.rs index 8775f82bb..7ad3f0ade 100644 --- a/x509-cert/tests/certificate.rs +++ b/x509-cert/tests/certificate.rs @@ -371,3 +371,18 @@ fn decode_cert() { cert.signature.raw_bytes() ); } + +#[test] +fn decode_cert_negative_serial_number() { + let der_encoded_cert = include_bytes!("examples/28903a635b5280fae6774c0b6da7d6baa64af2e8.der"); + + let cert = Certificate::from_der(der_encoded_cert).unwrap(); + assert_eq!( + cert.tbs_certificate.serial_number.as_bytes(), + // INTEGER (125 bit) -2.370157924795571e+37 + &[238, 43, 61, 235, 212, 33, 222, 20, 168, 98, 172, 4, 243, 221, 196, 1] + ); + + let reencoded = cert.to_vec().unwrap(); + assert_eq!(der_encoded_cert, reencoded.as_slice()); +} diff --git a/x509-cert/tests/examples/28903a635b5280fae6774c0b6da7d6baa64af2e8.der b/x509-cert/tests/examples/28903a635b5280fae6774c0b6da7d6baa64af2e8.der new file mode 100644 index 0000000000000000000000000000000000000000..f1317bf3b2bdd38c9d3e97f65379e76cbcc0ef37 GIT binary patch literal 1370 zcmXqLVhuBBVzFDm%*4pVB=An#_VpFTdm<~6*06lOdxX(|myJ`a&7D}zDf zXG3lSPB!LH7B*of*I+|y14|HxOW4RUJvA>mGf}}gu_Q4kF)vXeB~`&WwWuUBEi*YW zIWu2D!_U)AAyC)A!obkb+)UR~(@?`e4WyY{SUxzls4O+JSRtS^DJL_z7+tTSsev&> zrA}CCdTOacMoCFQv6a4ld3m{Ba$-p`&>p?K)Dr!&)S|?qqSPD(jbu#&Vtgq%DWSs9p{82K3tni#p5niv@wHY*+Xe>B}`YKMEX!#7#+&&|au za)F;`bRC|woRj7x?vVTZ)Tws;+s|DdDX~o=lW!b&@_%zc(WkO+n`4~< zDlbg2%jRL&cO#q6PwS1{r87;7BvOrq%jeCCl@h*j__pnd=~eFv-Y$PubGz`o?v$ka zx!1m0?6Y6m$|bpD#SP9VX{I@;VIj^-+Z_*;kh~!Gb01z;>O1Y zjSmcDfeA`hkVVWuq){j}IX*Et*#Rlt8t{Xpg&7(Dv#=U411SSOkN`hOfCZQ_*&zD) zSj1RF798$&S!KS|b^r8+sEZ#f7R`Pc+iOq{Qm(*KVNhmJ!p5!5#>m3>wStkEk;@>} zK!=S3D8S0f&crB2Fyk1Q!c{XFXiBLQnca}n0kD_=rUOQX1s>5&jNg1cc36g*&S|I$ zo_RIu|HeY=GnSlF;;!9#zmG9G_OjfKzqS5n7dSpJnY{n>qqaHblO}}8+~DGcjoS~pF8JZ`;Mwaj+f=!^#!Y|?%tfP zlOor;e^1G@g@>zz-tb7}HS^fKC~}+s+v957^k*9v{&@1YWvvES*cw(ZlOyXNU_KJ}$|5)=16pS0Hs|8}08t833E#U+Ll+H+Fp%w3u=)hej|>&4X^d7s}!|Lm}>+mWBg{KN?WtyS)D literal 0 HcmV?d00001