From c506fdc4041086861da5245e9c9a6c0bcd9de293 Mon Sep 17 00:00:00 2001 From: Tony Arcieri Date: Sat, 21 Jun 2025 18:30:16 -0600 Subject: [PATCH] der: error position tracking improvements - Changes all `Tag::*_error` methods to return `ErrorKind` rather than `Error`, and changes call sites where these methods were used with a `Reader` in scope to call `reader.error()` to ensure error position is propagated appropriately. - Locates sites of `ErrorKind::*.into()` where a `Reader` is in scope and changes them to use `reader.error()` --- Cargo.lock | 6 ++---- Cargo.toml | 8 ++++++++ der/src/asn1/any.rs | 2 +- der/src/asn1/bit_string.rs | 13 +++++++------ der/src/asn1/bmp_string.rs | 4 ++-- der/src/asn1/boolean.rs | 2 +- der/src/asn1/generalized_time.rs | 10 +++++----- der/src/asn1/ia5_string.rs | 8 ++++---- der/src/asn1/integer/int.rs | 18 ++++++++++-------- der/src/asn1/integer/uint.rs | 16 ++++++++-------- der/src/asn1/internal_macros.rs | 8 ++++---- der/src/asn1/octet_string.rs | 4 ++-- der/src/asn1/printable_string.rs | 8 ++++---- der/src/asn1/real.rs | 10 +++++----- der/src/asn1/sequence_of.rs | 2 +- der/src/asn1/teletex_string.rs | 8 ++++---- der/src/asn1/utc_time.rs | 10 +++++----- der/src/asn1/videotex_string.rs | 4 ++-- der/src/datetime.rs | 4 ++-- der/src/error.rs | 5 +++++ der/src/header.rs | 2 +- der/src/length.rs | 16 +++++++++++----- der/src/reader/pem.rs | 2 +- der/src/reader/slice.rs | 7 +------ der/src/tag.rs | 29 ++++++++++++++--------------- der_derive/src/enumerated.rs | 2 +- pkcs1/src/params.rs | 6 +++--- pkcs1/src/version.rs | 4 ++-- pkcs5/src/lib.rs | 2 +- pkcs5/src/pbes2.rs | 2 +- pkcs5/src/pbes2/kdf.rs | 4 ++-- pkcs5/src/pbes2/kdf/salt.rs | 4 ++-- pkcs8/src/private_key_info.rs | 3 +-- pkcs8/src/version.rs | 4 ++-- sec1/src/private_key.rs | 2 +- x509-cert/src/certificate.rs | 2 +- 36 files changed, 127 insertions(+), 114 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 0f23ac1d4..dce2c5728 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -523,8 +523,7 @@ dependencies = [ [[package]] name = "ecdsa" version = "0.17.0-rc.3" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "908741ea702207ae9456173adf9aaa8fffdd3a057e55ca9c77f62a05b9ad08d1" +source = "git+https://github.com/RustCrypto/signatures.git?branch=der-error-fixups#80909c41ef37718c7f2dfb3b8cd6c828b907f1e6" dependencies = [ "der", "digest", @@ -544,8 +543,7 @@ checksum = "48c757948c5ede0e46177b7add2e67155f70e33c07fea8284df6576da70b3719" [[package]] name = "elliptic-curve" version = "0.14.0-rc.7" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "eb5fad57f7e416b8f8e81df65daa6a87e402a4469990fd1ba36a8c79515a5fbf" +source = "git+https://github.com/RustCrypto/traits.git?branch=elliptic-curve%2Fder-error-fixups#89dcefcba7896d1e6dd34871ad484c186909d2a9" dependencies = [ "base16ct", "crypto-bigint", diff --git a/Cargo.toml b/Cargo.toml index 85cc8f141..ea16a23d2 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -58,3 +58,11 @@ tls_codec_derive = { path = "./tls_codec/derive" } x509-tsp = { path = "./x509-tsp" } x509-cert = { path = "./x509-cert" } x509-ocsp = { path = "./x509-ocsp" } + +[patch.crates-io.elliptic-curve] +git = "https://github.com/RustCrypto/traits.git" +branch = "elliptic-curve/der-error-fixups" + +[patch.crates-io.ecdsa] +git = "https://github.com/RustCrypto/signatures.git" +branch = "der-error-fixups" diff --git a/der/src/asn1/any.rs b/der/src/asn1/any.rs index 1e698c406..d1bbb05e8 100644 --- a/der/src/asn1/any.rs +++ b/der/src/asn1/any.rs @@ -63,7 +63,7 @@ impl<'a> AnyRef<'a> { T: Choice<'a> + DecodeValue<'a>, { if !T::can_decode(self.tag) { - return Err(self.tag.unexpected_error(None).into()); + return Err(self.tag.unexpected_error(None).to_error().into()); } let header = Header { diff --git a/der/src/asn1/bit_string.rs b/der/src/asn1/bit_string.rs index 488229b96..29d3d07ea 100644 --- a/der/src/asn1/bit_string.rs +++ b/der/src/asn1/bit_string.rs @@ -39,7 +39,7 @@ impl<'a> BitStringRef<'a> { /// from the final octet. This number is 0 if the value is octet-aligned. pub fn new(unused_bits: u8, bytes: &'a [u8]) -> Result { if (unused_bits > Self::MAX_UNUSED_BITS) || (unused_bits != 0 && bytes.is_empty()) { - return Err(Self::TAG.value_error()); + return Err(Self::TAG.value_error().into()); } let inner = BytesRef::new(bytes).map_err(|_| Self::TAG.length_error())?; @@ -206,8 +206,9 @@ impl<'a, const N: usize> TryFrom> for [u8; N] { fn try_from(bit_string: BitStringRef<'a>) -> Result { let bytes: &[u8] = TryFrom::try_from(bit_string)?; - - bytes.try_into().map_err(|_| Tag::BitString.length_error()) + bytes + .try_into() + .map_err(|_| Tag::BitString.length_error().into()) } } @@ -217,7 +218,7 @@ impl<'a> TryFrom> for &'a [u8] { fn try_from(bit_string: BitStringRef<'a>) -> Result<&'a [u8]> { bit_string .as_bytes() - .ok_or_else(|| Tag::BitString.value_error()) + .ok_or_else(|| Tag::BitString.value_error().into()) } } @@ -400,7 +401,7 @@ mod allocating { bit_string .as_bytes() .map(|bytes| bytes.to_vec()) - .ok_or_else(|| Tag::BitString.value_error()) + .ok_or_else(|| Tag::BitString.value_error().into()) } } @@ -620,7 +621,7 @@ mod tests { fn reject_unused_bits_in_empty_string() { assert_eq!( parse_bitstring(&[0x03]).err().unwrap().kind(), - Tag::BitString.value_error().kind() + Tag::BitString.value_error() ) } diff --git a/der/src/asn1/bmp_string.rs b/der/src/asn1/bmp_string.rs index 16a02aa1c..fae633462 100644 --- a/der/src/asn1/bmp_string.rs +++ b/der/src/asn1/bmp_string.rs @@ -22,7 +22,7 @@ impl BmpString { let bytes = bytes.into(); if bytes.len() % 2 != 0 { - return Err(Tag::BmpString.length_error()); + return Err(Tag::BmpString.length_error().into()); } let ret = Self { @@ -34,7 +34,7 @@ impl BmpString { // Character is in the Basic Multilingual Plane Ok(c) if (c as u64) < u64::from(u16::MAX) => (), // Characters outside Basic Multilingual Plane or unpaired surrogates - _ => return Err(Tag::BmpString.value_error()), + _ => return Err(Tag::BmpString.value_error().into()), } } diff --git a/der/src/asn1/boolean.rs b/der/src/asn1/boolean.rs index 5fd36f5a4..66af351a9 100644 --- a/der/src/asn1/boolean.rs +++ b/der/src/asn1/boolean.rs @@ -25,7 +25,7 @@ impl<'a> DecodeValue<'a> for bool { match reader.read_byte()? { FALSE_OCTET => Ok(false), TRUE_OCTET => Ok(true), - _ => Err(Self::TAG.non_canonical_error()), + _ => Err(reader.error(Self::TAG.non_canonical_error())), } } } diff --git a/der/src/asn1/generalized_time.rs b/der/src/asn1/generalized_time.rs index 63305315b..daa6ffa86 100644 --- a/der/src/asn1/generalized_time.rs +++ b/der/src/asn1/generalized_time.rs @@ -49,7 +49,7 @@ impl GeneralizedTime { pub fn from_unix_duration(unix_duration: Duration) -> Result { DateTime::from_unix_duration(unix_duration) .map(Into::into) - .map_err(|_| Self::TAG.value_error()) + .map_err(|_| Self::TAG.value_error().into()) } /// Get the duration of this timestamp since `UNIX_EPOCH`. @@ -62,7 +62,7 @@ impl GeneralizedTime { pub fn from_system_time(time: SystemTime) -> Result { DateTime::try_from(time) .map(Into::into) - .map_err(|_| Self::TAG.value_error()) + .map_err(|_| Self::TAG.value_error().into()) } /// Convert to [`SystemTime`]. @@ -79,7 +79,7 @@ impl<'a> DecodeValue<'a> for GeneralizedTime { fn decode_value>(reader: &mut R, header: Header) -> Result { if Self::LENGTH != usize::try_from(header.length)? { - return Err(Self::TAG.value_error()); + return Err(reader.error(Self::TAG.value_error())); } let mut bytes = [0u8; Self::LENGTH]; @@ -117,10 +117,10 @@ impl<'a> DecodeValue<'a> for GeneralizedTime { let second = datetime::decode_decimal(Self::TAG, sec1, sec2)?; DateTime::new(year, month, day, hour, minute, second) - .map_err(|_| Self::TAG.value_error()) + .map_err(|_| reader.error(Self::TAG.value_error())) .and_then(|dt| Self::from_unix_duration(dt.unix_duration())) } - _ => Err(Self::TAG.value_error()), + _ => Err(reader.error(Self::TAG.value_error())), } } } diff --git a/der/src/asn1/ia5_string.rs b/der/src/asn1/ia5_string.rs index b42cbb40d..d6c4ca8f9 100644 --- a/der/src/asn1/ia5_string.rs +++ b/der/src/asn1/ia5_string.rs @@ -50,12 +50,12 @@ impl<'a> Ia5StringRef<'a> { // Validate all characters are within IA5String's allowed set if input.iter().any(|&c| c > 0x7F) { - return Err(Self::TAG.value_error()); + return Err(Self::TAG.value_error().into()); } StrRef::from_bytes(input) .map(|inner| Self { inner }) - .map_err(|_| Self::TAG.value_error()) + .map_err(|_| Self::TAG.value_error().into()) } } @@ -122,7 +122,7 @@ mod allocation { StrOwned::from_bytes(input) .map(|inner| Self { inner }) - .map_err(|_| Self::TAG.value_error()) + .map_err(|_| Self::TAG.value_error().into()) } } @@ -181,7 +181,7 @@ mod allocation { StrOwned::new(input) .map(|inner| Self { inner }) - .map_err(|_| Self::TAG.value_error()) + .map_err(|_| Self::TAG.value_error().into()) } } } diff --git a/der/src/asn1/integer/int.rs b/der/src/asn1/integer/int.rs index 2203ba14e..f95420bf3 100644 --- a/der/src/asn1/integer/int.rs +++ b/der/src/asn1/integer/int.rs @@ -21,11 +21,11 @@ macro_rules! impl_encoding_traits { let max_length = u32::from(header.length) as usize; if max_length == 0 { - return Err(Tag::Integer.length_error()); + return Err(reader.error(Tag::Integer.length_error())); } if max_length > buf.len() { - return Err(Self::TAG.non_canonical_error()); + return Err(reader.error(Self::TAG.non_canonical_error())); } let bytes = reader.read_into(&mut buf[..max_length])?; @@ -40,7 +40,7 @@ macro_rules! impl_encoding_traits { // Ensure we compute the same encoded length as the original any value if header.length != result.value_len()? { - return Err(Self::TAG.non_canonical_error()); + return Err(reader.error(Self::TAG.non_canonical_error())); } Ok(result) @@ -144,7 +144,7 @@ impl<'a> DecodeValue<'a> for IntRef<'a> { // 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()); + return Err(reader.error(Self::TAG.non_canonical_error())); } Ok(result) @@ -238,7 +238,7 @@ mod allocating { // 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()); + return Err(reader.error(Self::TAG.non_canonical_error())); } Ok(result) @@ -375,13 +375,15 @@ mod allocating { /// Ensure `INTEGER` is canonically encoded. fn validate_canonical(bytes: &[u8]) -> Result<()> { + let non_canonical_error = Tag::Integer.non_canonical_error().into(); + // 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()), - [0x00, byte, ..] if *byte < 0x80 => Err(Tag::Integer.non_canonical_error()), - [0xFF, byte, ..] if *byte >= 0x80 => Err(Tag::Integer.non_canonical_error()), + [] => Err(non_canonical_error), + [0x00, byte, ..] if *byte < 0x80 => Err(non_canonical_error), + [0xFF, byte, ..] if *byte >= 0x80 => Err(non_canonical_error), _ => Ok(()), } } diff --git a/der/src/asn1/integer/uint.rs b/der/src/asn1/integer/uint.rs index caf0e1519..289dfd1ee 100644 --- a/der/src/asn1/integer/uint.rs +++ b/der/src/asn1/integer/uint.rs @@ -25,11 +25,11 @@ macro_rules! impl_encoding_traits { let max_length = u32::from(header.length) as usize; if max_length == 0 { - return Err(Tag::Integer.length_error()); + return Err(reader.error(Tag::Integer.length_error())); } if max_length > buf.len() { - return Err(Self::TAG.non_canonical_error()); + return Err(reader.error(Self::TAG.non_canonical_error())); } let bytes = reader.read_into(&mut buf[..max_length])?; @@ -37,7 +37,7 @@ macro_rules! impl_encoding_traits { // Ensure we compute the same encoded length as the original any value if header.length != result.value_len()? { - return Err(Self::TAG.non_canonical_error()); + return Err(reader.error(Self::TAG.non_canonical_error())); } Ok(result) @@ -127,7 +127,7 @@ impl<'a> DecodeValue<'a> for UintRef<'a> { // 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()); + return Err(reader.error(Self::TAG.non_canonical_error())); } Ok(result) @@ -221,7 +221,7 @@ mod allocating { // 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()); + return Err(reader.error(Self::TAG.non_canonical_error())); } Ok(result) @@ -328,11 +328,11 @@ pub(crate) fn decode_to_slice(bytes: &[u8]) -> Result<&[u8]> { // integer (since we're decoding an unsigned integer). // We expect all such cases to have a leading `0x00` byte. match bytes { - [] => Err(Tag::Integer.non_canonical_error()), + [] => Err(Tag::Integer.non_canonical_error().into()), [0] => Ok(bytes), - [0, byte, ..] if *byte < 0x80 => Err(Tag::Integer.non_canonical_error()), + [0, byte, ..] if *byte < 0x80 => Err(Tag::Integer.non_canonical_error().into()), [0, rest @ ..] => Ok(rest), - [byte, ..] if *byte >= 0x80 => Err(Tag::Integer.value_error()), + [byte, ..] if *byte >= 0x80 => Err(Tag::Integer.value_error().into()), _ => Ok(bytes), } } diff --git a/der/src/asn1/internal_macros.rs b/der/src/asn1/internal_macros.rs index aeea1b2f6..e1667db1d 100644 --- a/der/src/asn1/internal_macros.rs +++ b/der/src/asn1/internal_macros.rs @@ -148,7 +148,7 @@ macro_rules! impl_custom_class { // the encoding shall be constructed if the base encoding is constructed if header.tag.is_constructed() != T::CONSTRUCTED { - return Err(header.tag.non_canonical_error().into()); + return Err(reader.error(header.tag.non_canonical_error()).into()); } // read_nested checks if header matches decoded length @@ -186,7 +186,7 @@ macro_rules! impl_custom_class { // encoding shall be constructed if !header.tag.is_constructed() { - return Err(header.tag.non_canonical_error().into()); + return Err(reader.error(header.tag.non_canonical_error()).into()); } match header.tag { Tag::$class_enum_name { number, .. } => Ok(Self { @@ -197,7 +197,7 @@ macro_rules! impl_custom_class { T::decode(reader) })?, }), - tag => Err(tag.unexpected_error(None).into()), + tag => Err(reader.error(tag.unexpected_error(None)).into()) } } } @@ -264,7 +264,7 @@ macro_rules! impl_custom_class { tag_mode: TagMode::default(), value: T::from_der(any.value())?, }), - tag => Err(tag.unexpected_error(None).into()), + tag => Err(tag.unexpected_error(None).to_error().into()), } } } diff --git a/der/src/asn1/octet_string.rs b/der/src/asn1/octet_string.rs index 72023bbe2..4e862eb0b 100644 --- a/der/src/asn1/octet_string.rs +++ b/der/src/asn1/octet_string.rs @@ -128,7 +128,7 @@ impl<'a, const N: usize> TryFrom> for [u8; N] { octet_string .as_bytes() .try_into() - .map_err(|_| Tag::OctetString.length_error()) + .map_err(|_| Tag::OctetString.length_error().into()) } } @@ -140,7 +140,7 @@ impl<'a, const N: usize> TryFrom> for heapless::Vec { octet_string .as_bytes() .try_into() - .map_err(|_| Tag::OctetString.length_error()) + .map_err(|_| Tag::OctetString.length_error().into()) } } diff --git a/der/src/asn1/printable_string.rs b/der/src/asn1/printable_string.rs index b271e809e..f61f9c30c 100644 --- a/der/src/asn1/printable_string.rs +++ b/der/src/asn1/printable_string.rs @@ -83,13 +83,13 @@ impl<'a> PrintableStringRef<'a> { | b':' | b'=' | b'?' => (), - _ => return Err(Self::TAG.value_error()), + _ => return Err(Self::TAG.value_error().into()), } } StrRef::from_bytes(input) .map(|inner| Self { inner }) - .map_err(|_| Self::TAG.value_error()) + .map_err(|_| Self::TAG.value_error().into()) } } @@ -173,7 +173,7 @@ mod allocation { StrOwned::from_bytes(input) .map(|inner| Self { inner }) - .map_err(|_| Self::TAG.value_error()) + .map_err(|_| Self::TAG.value_error().into()) } } @@ -236,7 +236,7 @@ mod allocation { StrOwned::new(input) .map(|inner| Self { inner }) - .map_err(|_| Self::TAG.value_error()) + .map_err(|_| Self::TAG.value_error().into()) } } } diff --git a/der/src/asn1/real.rs b/der/src/asn1/real.rs index 3c8d8900f..998dc5687 100644 --- a/der/src/asn1/real.rs +++ b/der/src/asn1/real.rs @@ -31,7 +31,7 @@ impl<'a> DecodeValue<'a> for f64 { if base != 0 { // Real related error: base is not DER compliant (base encoded in enum) - return Err(Tag::Real.value_error()); + return Err(reader.error(Tag::Real.value_error())); } // Section 8.5.7.3 @@ -52,7 +52,7 @@ impl<'a> DecodeValue<'a> for f64 { } _ => { // Real related error: encoded exponent cannot be represented on an IEEE-754 double - return Err(Tag::Real.value_error()); + return Err(reader.error(Tag::Real.value_error())); } }; // Section 8.5.7.5: Read the remaining bytes for the mantissa @@ -72,14 +72,14 @@ impl<'a> DecodeValue<'a> for f64 { 1 => Ok(f64::NEG_INFINITY), 2 => Ok(f64::NAN), 3 => Ok(-0.0_f64), - _ => Err(Tag::Real.value_error()), + _ => Err(reader.error(Tag::Real.value_error())), } } else { let astr = StrRef::from_bytes(&bytes[1..])?; match astr.inner.parse::() { Ok(val) => Ok(val), // Real related error: encoding not supported or malformed - Err(_) => Err(Tag::Real.value_error()), + Err(_) => Err(reader.error(Tag::Real.value_error())), } } } @@ -174,7 +174,7 @@ impl EncodeValue for f64 { 3 => first_byte |= 0b0000_0010, _ => { // TODO: support multi octet exponent encoding? - return Err(Tag::Real.value_error()); + return Err(Tag::Real.value_error().into()); } } diff --git a/der/src/asn1/sequence_of.rs b/der/src/asn1/sequence_of.rs index 021cbc328..5929d6e7d 100644 --- a/der/src/asn1/sequence_of.rs +++ b/der/src/asn1/sequence_of.rs @@ -162,7 +162,7 @@ where .into_array() .map(|elem| elem.expect("arrayvec length mismatch"))) } else { - Err(Self::TAG.length_error().into()) + Err(reader.error(Self::TAG.length_error()).into()) } } } diff --git a/der/src/asn1/teletex_string.rs b/der/src/asn1/teletex_string.rs index 415cea7a2..1da728c92 100644 --- a/der/src/asn1/teletex_string.rs +++ b/der/src/asn1/teletex_string.rs @@ -57,12 +57,12 @@ impl<'a> TeletexStringRef<'a> { // FIXME: support higher part of the charset if input.iter().any(|&c| c > 0x7F) { - return Err(Self::TAG.value_error()); + return Err(Self::TAG.value_error().into()); } StrRef::from_bytes(input) .map(|inner| Self { inner }) - .map_err(|_| Self::TAG.value_error()) + .map_err(|_| Self::TAG.value_error().into()) } } @@ -135,7 +135,7 @@ mod allocation { StrOwned::from_bytes(input) .map(|inner| Self { inner }) - .map_err(|_| Self::TAG.value_error()) + .map_err(|_| Self::TAG.value_error().into()) } } @@ -198,7 +198,7 @@ mod allocation { StrOwned::new(input) .map(|inner| Self { inner }) - .map_err(|_| Self::TAG.value_error()) + .map_err(|_| Self::TAG.value_error().into()) } } } diff --git a/der/src/asn1/utc_time.rs b/der/src/asn1/utc_time.rs index fa52ffed4..b43a9a874 100644 --- a/der/src/asn1/utc_time.rs +++ b/der/src/asn1/utc_time.rs @@ -44,7 +44,7 @@ impl UtcTime { if datetime.year() <= UtcTime::MAX_YEAR { Ok(Self(datetime)) } else { - Err(Self::TAG.value_error()) + Err(Self::TAG.value_error().into()) } } @@ -86,7 +86,7 @@ impl<'a> DecodeValue<'a> for UtcTime { fn decode_value>(reader: &mut R, header: Header) -> Result { if Self::LENGTH != usize::try_from(header.length)? { - return Err(Self::TAG.value_error()); + return Err(reader.error(Self::TAG.value_error())); } let mut bytes = [0u8; Self::LENGTH]; @@ -125,10 +125,10 @@ impl<'a> DecodeValue<'a> for UtcTime { .ok_or(ErrorKind::DateTime)?; DateTime::new(year, month, day, hour, minute, second) - .map_err(|_| Self::TAG.value_error()) + .map_err(|_| reader.error(Self::TAG.value_error())) .and_then(|dt| Self::from_unix_duration(dt.unix_duration())) } - _ => Err(Self::TAG.value_error()), + _ => Err(reader.error(Self::TAG.value_error())), } } } @@ -142,7 +142,7 @@ impl EncodeValue for UtcTime { let year = match self.0.year() { y @ 1950..=1999 => y.checked_sub(1900), y @ 2000..=2049 => y.checked_sub(2000), - _ => return Err(Self::TAG.value_error()), + _ => return Err(Self::TAG.value_error().into()), } .and_then(|y| u8::try_from(y).ok()) .ok_or(ErrorKind::DateTime)?; diff --git a/der/src/asn1/videotex_string.rs b/der/src/asn1/videotex_string.rs index 021d621ef..c049f2490 100644 --- a/der/src/asn1/videotex_string.rs +++ b/der/src/asn1/videotex_string.rs @@ -34,12 +34,12 @@ impl<'a> VideotexStringRef<'a> { // Validate all characters are within VideotexString's allowed set // FIXME: treat as if it were IA5String if input.iter().any(|&c| c > 0x7F) { - return Err(Self::TAG.value_error()); + return Err(Self::TAG.value_error().into()); } StrRef::from_bytes(input) .map(|inner| Self { inner }) - .map_err(|_| Self::TAG.value_error()) + .map_err(|_| Self::TAG.value_error().into()) } } diff --git a/der/src/datetime.rs b/der/src/datetime.rs index 95785bb4d..87d1ce027 100644 --- a/der/src/datetime.rs +++ b/der/src/datetime.rs @@ -426,7 +426,7 @@ pub(crate) fn decode_decimal(tag: Tag, hi: u8, lo: u8) -> Result { if hi.is_ascii_digit() && lo.is_ascii_digit() { Ok((hi - b'0') * 10 + (lo - b'0')) } else { - Err(tag.value_error()) + Err(tag.value_error().into()) } } @@ -438,7 +438,7 @@ where let hi_val = value / 10; if hi_val >= 10 { - return Err(tag.value_error()); + return Err(tag.value_error().into()); } writer.write_byte(b'0'.checked_add(hi_val).ok_or(ErrorKind::Overflow)?)?; diff --git a/der/src/error.rs b/der/src/error.rs index 71f0f7292..0044563c8 100644 --- a/der/src/error.rs +++ b/der/src/error.rs @@ -306,6 +306,11 @@ impl ErrorKind { pub fn at(self, position: Length) -> Error { Error::new(self, position) } + + /// Convert to an error, omitting position information. + pub fn to_error(self) -> Error { + Error::from_kind(self) + } } impl fmt::Display for ErrorKind { diff --git a/der/src/header.rs b/der/src/header.rs index 4176017cd..58cbe40a7 100644 --- a/der/src/header.rs +++ b/der/src/header.rs @@ -38,7 +38,7 @@ impl<'a> Decode<'a> for Header { let length = Length::decode(reader).map_err(|e| { if e.kind() == ErrorKind::Overlength { - ErrorKind::Length { tag }.into() + reader.error(tag.length_error()) } else { e } diff --git a/der/src/length.rs b/der/src/length.rs index 961e58695..a08693b42 100644 --- a/der/src/length.rs +++ b/der/src/length.rs @@ -220,16 +220,22 @@ impl<'a> Decode<'a> for Length { // Indefinite lengths are allowed when decoding BER EncodingRules::Ber => decode_indefinite_length(&mut reader.clone()), // Indefinite lengths are disallowed when decoding DER - EncodingRules::Der => Err(ErrorKind::IndefiniteLength.into()), + EncodingRules::Der => Err(reader.error(ErrorKind::IndefiniteLength)), }, // 1-4 byte variable-sized length prefix tag @ 0x81..=0x84 => { - let nbytes = tag.checked_sub(0x80).ok_or(ErrorKind::Overlength)? as usize; + let nbytes = tag + .checked_sub(0x80) + .ok_or_else(|| reader.error(ErrorKind::Overlength))? + as usize; + debug_assert!(nbytes <= 4); let mut decoded_len = 0u32; for _ in 0..nbytes { - decoded_len = decoded_len.checked_shl(8).ok_or(ErrorKind::Overflow)? + decoded_len = decoded_len + .checked_shl(8) + .ok_or_else(|| reader.error(ErrorKind::Overflow))? | u32::from(reader.read_byte()?); } @@ -240,12 +246,12 @@ impl<'a> Decode<'a> for Length { if length.initial_octet() == Some(tag) { Ok(length) } else { - Err(ErrorKind::Overlength.into()) + Err(reader.error(ErrorKind::Overlength)) } } _ => { // We specialize to a maximum 4-byte length (including initial octet) - Err(ErrorKind::Overlength.into()) + Err(reader.error(ErrorKind::Overlength)) } } } diff --git a/der/src/reader/pem.rs b/der/src/reader/pem.rs index 8ce813087..3345326d3 100644 --- a/der/src/reader/pem.rs +++ b/der/src/reader/pem.rs @@ -68,7 +68,7 @@ impl<'i> Reader<'i> for PemReader<'i> { fn read_slice(&mut self, _len: Length) -> Result<&'i [u8]> { // Can't borrow from PEM because it requires decoding - Err(ErrorKind::Reader.into()) + Err(self.error(ErrorKind::Reader)) } fn read_into<'o>(&mut self, buf: &'o mut [u8]) -> Result<&'o [u8]> { diff --git a/der/src/reader/slice.rs b/der/src/reader/slice.rs index 54bbce834..1d8d4fa25 100644 --- a/der/src/reader/slice.rs +++ b/der/src/reader/slice.rs @@ -1,6 +1,6 @@ //! Slice reader. -use crate::{BytesRef, Decode, EncodingRules, Error, ErrorKind, Length, Reader, Tag}; +use crate::{BytesRef, Decode, EncodingRules, Error, ErrorKind, Length, Reader}; /// [`Reader`] which consumes an input byte slice. #[derive(Clone, Debug)] @@ -44,11 +44,6 @@ impl<'a> SliceReader<'a> { kind.at(self.position) } - /// Return an error for an invalid value with the given tag. - pub fn value_error(&mut self, tag: Tag) -> Error { - self.error(tag.value_error().kind()) - } - /// Did the decoding operation fail due to an error? pub fn is_failed(&self) -> bool { self.failed diff --git a/der/src/tag.rs b/der/src/tag.rs index d5c9a03ef..93f5b0e51 100644 --- a/der/src/tag.rs +++ b/der/src/tag.rs @@ -190,7 +190,7 @@ impl Tag { if self == expected { Ok(self) } else { - Err(self.unexpected_error(Some(expected))) + Err(self.unexpected_error(Some(expected)).into()) } } @@ -266,30 +266,29 @@ impl Tag { } /// Create an [`Error`] for an invalid [`Length`]. - pub fn length_error(self) -> Error { - ErrorKind::Length { tag: self }.into() + pub fn length_error(self) -> ErrorKind { + ErrorKind::Length { tag: self } } /// Create an [`Error`] for an non-canonical value with the ASN.1 type /// identified by this tag. - pub fn non_canonical_error(self) -> Error { - ErrorKind::Noncanonical { tag: self }.into() + pub fn non_canonical_error(self) -> ErrorKind { + ErrorKind::Noncanonical { tag: self } } /// Create an [`Error`] because the current tag was unexpected, with an /// optional expected tag. - pub fn unexpected_error(self, expected: Option) -> Error { + pub fn unexpected_error(self, expected: Option) -> ErrorKind { ErrorKind::TagUnexpected { expected, actual: self, } - .into() } /// Create an [`Error`] for an invalid value with the ASN.1 type identified /// by this tag. - pub fn value_error(self) -> Error { - ErrorKind::Value { tag: self }.into() + pub fn value_error(self) -> ErrorKind { + ErrorKind::Value { tag: self } } } @@ -346,8 +345,8 @@ impl<'a> Decode<'a> for Tag { } } // universal tag in long form - 0x1F => return Err(ErrorKind::TagNumberInvalid.into()), - byte => return Err(ErrorKind::TagUnknown { byte }.into()), + 0x1F => return Err(reader.error(ErrorKind::TagNumberInvalid)), + byte => return Err(reader.error(ErrorKind::TagUnknown { byte })), }; Ok(tag) @@ -370,24 +369,24 @@ fn parse_parts<'a, R: Reader<'a>>(first_byte: u8, reader: &mut R) -> Result<(boo if byte & 0x80 == 0 { if multi_byte_tag_number < u32::from(TagNumber::MASK) { - return Err(ErrorKind::TagNumberInvalid.into()); + return Err(reader.error(ErrorKind::TagNumberInvalid)); } return Ok((constructed, TagNumber(multi_byte_tag_number))); } else if i == 0 && multi_byte_tag_number == 0 { // 8.1.2.4.2c says "bits 7 to 1 of the first subsequent octet shall not all be zero" - return Err(ErrorKind::TagNumberInvalid.into()); + return Err(reader.error(ErrorKind::TagNumberInvalid)); } if multi_byte_tag_number.leading_zeros() < 7 { - return Err(ErrorKind::TagNumberInvalid.into()); + return Err(reader.error(ErrorKind::TagNumberInvalid)); } multi_byte_tag_number <<= 7; } // missing terminator byte - Err(ErrorKind::TagNumberInvalid.into()) + Err(reader.error(ErrorKind::TagNumberInvalid)) } impl Encode for Tag { diff --git a/der_derive/src/enumerated.rs b/der_derive/src/enumerated.rs index d1abdc67d..bbd16ea20 100644 --- a/der_derive/src/enumerated.rs +++ b/der_derive/src/enumerated.rs @@ -160,7 +160,7 @@ impl DeriveEnumerated { fn try_from(n: #repr) -> ::core::result::Result { match n { #(#try_from_body)* - _ => Err(#tag.value_error().into()) + _ => Err(#tag.value_error().to_error().into()) } } } diff --git a/pkcs1/src/params.rs b/pkcs1/src/params.rs index 099a21893..b9d228a5b 100644 --- a/pkcs1/src/params.rs +++ b/pkcs1/src/params.rs @@ -39,10 +39,10 @@ impl Default for TrailerField { impl<'a> DecodeValue<'a> for TrailerField { type Error = der::Error; - fn decode_value>(decoder: &mut R, header: der::Header) -> der::Result { - match u8::decode_value(decoder, header)? { + fn decode_value>(reader: &mut R, header: der::Header) -> der::Result { + match u8::decode_value(reader, header)? { 1 => Ok(TrailerField::BC), - _ => Err(Self::TAG.value_error()), + _ => Err(reader.error(Self::TAG.value_error())), } } } diff --git a/pkcs1/src/version.rs b/pkcs1/src/version.rs index f7ff79801..227dcd028 100644 --- a/pkcs1/src/version.rs +++ b/pkcs1/src/version.rs @@ -53,8 +53,8 @@ impl TryFrom for Version { impl<'a> Decode<'a> for Version { type Error = der::Error; - fn decode>(decoder: &mut R) -> der::Result { - Version::try_from(u8::decode(decoder)?).map_err(|_| Self::TAG.value_error()) + fn decode>(reader: &mut R) -> der::Result { + Version::try_from(u8::decode(reader)?).map_err(|_| reader.error(Self::TAG.value_error())) } } diff --git a/pkcs5/src/lib.rs b/pkcs5/src/lib.rs index cf23e984c..5fac153b9 100644 --- a/pkcs5/src/lib.rs +++ b/pkcs5/src/lib.rs @@ -199,7 +199,7 @@ impl TryFrom> for EncryptionScheme { if alg.oid == pbes2::PBES2_OID { match alg.parameters { Some(params) => pbes2::Parameters::try_from(params).map(Into::into), - None => Err(Tag::OctetString.value_error()), + None => Err(Tag::OctetString.value_error().into()), } } else { pbes1::Algorithm::try_from(alg).map(Into::into) diff --git a/pkcs5/src/pbes2.rs b/pkcs5/src/pbes2.rs index b3453f5fd..f8832859a 100644 --- a/pkcs5/src/pbes2.rs +++ b/pkcs5/src/pbes2.rs @@ -450,7 +450,7 @@ impl TryFrom> for EncryptionScheme { // TODO(tarcieri): support for non-AES algorithms? let iv = match alg.parameters { Some(params) => params.decode_as::>()?.as_bytes(), - None => return Err(Tag::OctetString.value_error()), + None => return Err(Tag::OctetString.value_error().into()), }; match alg.oid { diff --git a/pkcs5/src/pbes2/kdf.rs b/pkcs5/src/pbes2/kdf.rs index efb40cb1c..01434c862 100644 --- a/pkcs5/src/pbes2/kdf.rs +++ b/pkcs5/src/pbes2/kdf.rs @@ -160,7 +160,7 @@ impl TryFrom> for Kdf { oid => Err(ErrorKind::OidUnknown { oid }.into()), } } else { - Err(Tag::OctetString.value_error()) + Err(Tag::OctetString.value_error().into()) } } } @@ -334,7 +334,7 @@ impl TryFrom> for Pbkdf2Prf { if let Some(params) = alg.parameters { // TODO(tarcieri): support non-NULL parameters? if !params.is_null() { - return Err(params.tag().value_error()); + return Err(params.tag().value_error().into()); } } diff --git a/pkcs5/src/pbes2/kdf/salt.rs b/pkcs5/src/pbes2/kdf/salt.rs index 59e961594..d29204e3e 100644 --- a/pkcs5/src/pbes2/kdf/salt.rs +++ b/pkcs5/src/pbes2/kdf/salt.rs @@ -20,7 +20,7 @@ impl Salt { let slice = slice.as_ref(); if slice.len() > Self::MAX_LEN { - return Err(Self::TAG.length_error()); + return Err(Self::TAG.length_error().into()); } let mut inner = [0u8; Self::MAX_LEN]; @@ -62,7 +62,7 @@ impl<'a> DecodeValue<'a> for Salt { let length = usize::try_from(header.length)?; if length > Self::MAX_LEN { - return Err(Self::TAG.length_error()); + return Err(reader.error(Self::TAG.length_error())); } let mut inner = [0u8; Self::MAX_LEN]; diff --git a/pkcs8/src/private_key_info.rs b/pkcs8/src/private_key_info.rs index d5320eb44..8132b6db6 100644 --- a/pkcs8/src/private_key_info.rs +++ b/pkcs8/src/private_key_info.rs @@ -213,8 +213,7 @@ where constructed: true, number: PUBLIC_KEY_TAG, } - .value_error() - .kind(), + .value_error(), )); } diff --git a/pkcs8/src/version.rs b/pkcs8/src/version.rs index b35730a55..d0d8745a5 100644 --- a/pkcs8/src/version.rs +++ b/pkcs8/src/version.rs @@ -28,8 +28,8 @@ impl Version { impl<'a> Decode<'a> for Version { type Error = der::Error; - fn decode>(decoder: &mut R) -> der::Result { - Version::try_from(u8::decode(decoder)?).map_err(|_| Self::TAG.value_error()) + fn decode>(reader: &mut R) -> der::Result { + Version::try_from(u8::decode(reader)?).map_err(|_| reader.error(Self::TAG.value_error())) } } diff --git a/sec1/src/private_key.rs b/sec1/src/private_key.rs index 37a7c96ed..2da371876 100644 --- a/sec1/src/private_key.rs +++ b/sec1/src/private_key.rs @@ -99,7 +99,7 @@ impl<'a> DecodeValue<'a> for EcPrivateKey<'a> { fn decode_value>(reader: &mut R, _header: Header) -> der::Result { if u8::decode(reader)? != VERSION { - return Err(Tag::Integer.value_error()); + return Err(reader.error(Tag::Integer.value_error())); } let private_key = OctetStringRef::decode(reader)?.as_bytes(); diff --git a/x509-cert/src/certificate.rs b/x509-cert/src/certificate.rs index 836477219..9b67620dd 100644 --- a/x509-cert/src/certificate.rs +++ b/x509-cert/src/certificate.rs @@ -32,7 +32,7 @@ pub trait Profile: PartialEq + Debug + Eq + Ord + Clone + Copy + Default + 'stat // since some X.509 implementations interpret the limit of 20 bytes to refer // to the pre-encoded value. if serial.inner.len() > SerialNumber::::MAX_DECODE_LEN { - Err(Tag::Integer.value_error()) + Err(Tag::Integer.value_error().into()) } else { Ok(()) }