der, x509-cert: fix handling of negative integers (#831)

Author: woodruffw

der/src/asn1/integer.rs

@@ -43,7 +43,7 @@ macro_rules! impl_int_encoding {
             impl EncodeValue for $int {
                 fn value_len(&self) -> Result<Length> {
                     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())
                     }

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<Length> {
-        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<Length> {
-            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<Uint> for Int {
+        fn from(value: Uint) -> Self {
+            let mut inner: Vec<u8> = 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;
     }

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<Length> {
+pub(super) fn negative_encoded_len(bytes: &[u8]) -> Result<Length> {
     Length::try_from(strip_leading_ones(bytes).len())
 }
 

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<Self> {
-        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<R: Reader<'a>>(reader: &mut R, header: Header) -> Result<Self> {
         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")
+        }
     }
 }