Skip to content

Commit 4cb99df

Browse files
tarcieritarcieri
authored
der: error position tracking improvements (#1889)
- 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()`
1 parent 5277fcf commit 4cb99df

36 files changed

+127
-114
lines changed

‎Cargo.lock‎

Lines changed: 2 additions & 4 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎Cargo.toml‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,3 +58,11 @@ tls_codec_derive = { path = "./tls_codec/derive" }
5858
x509-tsp = { path = "./x509-tsp" }
5959
x509-cert = { path = "./x509-cert" }
6060
x509-ocsp = { path = "./x509-ocsp" }
61+
62+
[patch.crates-io.elliptic-curve]
63+
git = "https://github.com/RustCrypto/traits.git"
64+
branch = "elliptic-curve/der-error-fixups"
65+
66+
[patch.crates-io.ecdsa]
67+
git = "https://github.com/RustCrypto/signatures.git"
68+
branch = "der-error-fixups"

‎der/src/asn1/any.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ impl<'a> AnyRef<'a> {
6363
T: Choice<'a> + DecodeValue<'a>,
6464
{
6565
if !T::can_decode(self.tag) {
66-
return Err(self.tag.unexpected_error(None).into());
66+
return Err(self.tag.unexpected_error(None).to_error().into());
6767
}
6868

6969
let header = Header {

‎der/src/asn1/bit_string.rs‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ impl<'a> BitStringRef<'a> {
3939
/// from the final octet. This number is 0 if the value is octet-aligned.
4040
pub fn new(unused_bits: u8, bytes: &'a [u8]) -> Result<Self> {
4141
if (unused_bits > Self::MAX_UNUSED_BITS) || (unused_bits != 0 && bytes.is_empty()) {
42-
return Err(Self::TAG.value_error());
42+
return Err(Self::TAG.value_error().into());
4343
}
4444

4545
let inner = BytesRef::new(bytes).map_err(|_| Self::TAG.length_error())?;
@@ -206,8 +206,9 @@ impl<'a, const N: usize> TryFrom<BitStringRef<'a>> for [u8; N] {
206206

207207
fn try_from(bit_string: BitStringRef<'a>) -> Result<Self> {
208208
let bytes: &[u8] = TryFrom::try_from(bit_string)?;
209-
210-
bytes.try_into().map_err(|_| Tag::BitString.length_error())
209+
bytes
210+
.try_into()
211+
.map_err(|_| Tag::BitString.length_error().into())
211212
}
212213
}
213214

@@ -217,7 +218,7 @@ impl<'a> TryFrom<BitStringRef<'a>> for &'a [u8] {
217218
fn try_from(bit_string: BitStringRef<'a>) -> Result<&'a [u8]> {
218219
bit_string
219220
.as_bytes()
220-
.ok_or_else(|| Tag::BitString.value_error())
221+
.ok_or_else(|| Tag::BitString.value_error().into())
221222
}
222223
}
223224

@@ -400,7 +401,7 @@ mod allocating {
400401
bit_string
401402
.as_bytes()
402403
.map(|bytes| bytes.to_vec())
403-
.ok_or_else(|| Tag::BitString.value_error())
404+
.ok_or_else(|| Tag::BitString.value_error().into())
404405
}
405406
}
406407

@@ -620,7 +621,7 @@ mod tests {
620621
fn reject_unused_bits_in_empty_string() {
621622
assert_eq!(
622623
parse_bitstring(&[0x03]).err().unwrap().kind(),
623-
Tag::BitString.value_error().kind()
624+
Tag::BitString.value_error()
624625
)
625626
}
626627

‎der/src/asn1/bmp_string.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ impl BmpString {
2222
let bytes = bytes.into();
2323

2424
if bytes.len() % 2 != 0 {
25-
return Err(Tag::BmpString.length_error());
25+
return Err(Tag::BmpString.length_error().into());
2626
}
2727

2828
let ret = Self {
@@ -34,7 +34,7 @@ impl BmpString {
3434
// Character is in the Basic Multilingual Plane
3535
Ok(c) if (c as u64) < u64::from(u16::MAX) => (),
3636
// Characters outside Basic Multilingual Plane or unpaired surrogates
37-
_ => return Err(Tag::BmpString.value_error()),
37+
_ => return Err(Tag::BmpString.value_error().into()),
3838
}
3939
}
4040

‎der/src/asn1/boolean.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ impl<'a> DecodeValue<'a> for bool {
2525
match reader.read_byte()? {
2626
FALSE_OCTET => Ok(false),
2727
TRUE_OCTET => Ok(true),
28-
_ => Err(Self::TAG.non_canonical_error()),
28+
_ => Err(reader.error(Self::TAG.non_canonical_error())),
2929
}
3030
}
3131
}

‎der/src/asn1/generalized_time.rs‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ impl GeneralizedTime {
4949
pub fn from_unix_duration(unix_duration: Duration) -> Result<Self> {
5050
DateTime::from_unix_duration(unix_duration)
5151
.map(Into::into)
52-
.map_err(|_| Self::TAG.value_error())
52+
.map_err(|_| Self::TAG.value_error().into())
5353
}
5454

5555
/// Get the duration of this timestamp since `UNIX_EPOCH`.
@@ -62,7 +62,7 @@ impl GeneralizedTime {
6262
pub fn from_system_time(time: SystemTime) -> Result<Self> {
6363
DateTime::try_from(time)
6464
.map(Into::into)
65-
.map_err(|_| Self::TAG.value_error())
65+
.map_err(|_| Self::TAG.value_error().into())
6666
}
6767

6868
/// Convert to [`SystemTime`].
@@ -79,7 +79,7 @@ impl<'a> DecodeValue<'a> for GeneralizedTime {
7979

8080
fn decode_value<R: Reader<'a>>(reader: &mut R, header: Header) -> Result<Self> {
8181
if Self::LENGTH != usize::try_from(header.length)? {
82-
return Err(Self::TAG.value_error());
82+
return Err(reader.error(Self::TAG.value_error()));
8383
}
8484

8585
let mut bytes = [0u8; Self::LENGTH];
@@ -117,10 +117,10 @@ impl<'a> DecodeValue<'a> for GeneralizedTime {
117117
let second = datetime::decode_decimal(Self::TAG, sec1, sec2)?;
118118

119119
DateTime::new(year, month, day, hour, minute, second)
120-
.map_err(|_| Self::TAG.value_error())
120+
.map_err(|_| reader.error(Self::TAG.value_error()))
121121
.and_then(|dt| Self::from_unix_duration(dt.unix_duration()))
122122
}
123-
_ => Err(Self::TAG.value_error()),
123+
_ => Err(reader.error(Self::TAG.value_error())),
124124
}
125125
}
126126
}

‎der/src/asn1/ia5_string.rs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -50,12 +50,12 @@ impl<'a> Ia5StringRef<'a> {
5050

5151
// Validate all characters are within IA5String's allowed set
5252
if input.iter().any(|&c| c > 0x7F) {
53-
return Err(Self::TAG.value_error());
53+
return Err(Self::TAG.value_error().into());
5454
}
5555

5656
StrRef::from_bytes(input)
5757
.map(|inner| Self { inner })
58-
.map_err(|_| Self::TAG.value_error())
58+
.map_err(|_| Self::TAG.value_error().into())
5959
}
6060
}
6161

@@ -122,7 +122,7 @@ mod allocation {
122122

123123
StrOwned::from_bytes(input)
124124
.map(|inner| Self { inner })
125-
.map_err(|_| Self::TAG.value_error())
125+
.map_err(|_| Self::TAG.value_error().into())
126126
}
127127
}
128128

@@ -181,7 +181,7 @@ mod allocation {
181181

182182
StrOwned::new(input)
183183
.map(|inner| Self { inner })
184-
.map_err(|_| Self::TAG.value_error())
184+
.map_err(|_| Self::TAG.value_error().into())
185185
}
186186
}
187187
}

‎der/src/asn1/integer/int.rs‎

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -21,11 +21,11 @@ macro_rules! impl_encoding_traits {
2121
let max_length = u32::from(header.length) as usize;
2222

2323
if max_length == 0 {
24-
return Err(Tag::Integer.length_error());
24+
return Err(reader.error(Tag::Integer.length_error()));
2525
}
2626

2727
if max_length > buf.len() {
28-
return Err(Self::TAG.non_canonical_error());
28+
return Err(reader.error(Self::TAG.non_canonical_error()));
2929
}
3030

3131
let bytes = reader.read_into(&mut buf[..max_length])?;
@@ -40,7 +40,7 @@ macro_rules! impl_encoding_traits {
4040

4141
// Ensure we compute the same encoded length as the original any value
4242
if header.length != result.value_len()? {
43-
return Err(Self::TAG.non_canonical_error());
43+
return Err(reader.error(Self::TAG.non_canonical_error()));
4444
}
4545

4646
Ok(result)
@@ -144,7 +144,7 @@ impl<'a> DecodeValue<'a> for IntRef<'a> {
144144

145145
// Ensure we compute the same encoded length as the original any value.
146146
if result.value_len()? != header.length {
147-
return Err(Self::TAG.non_canonical_error());
147+
return Err(reader.error(Self::TAG.non_canonical_error()));
148148
}
149149

150150
Ok(result)
@@ -238,7 +238,7 @@ mod allocating {
238238

239239
// Ensure we compute the same encoded length as the original any value.
240240
if result.value_len()? != header.length {
241-
return Err(Self::TAG.non_canonical_error());
241+
return Err(reader.error(Self::TAG.non_canonical_error()));
242242
}
243243

244244
Ok(result)
@@ -375,13 +375,15 @@ mod allocating {
375375

376376
/// Ensure `INTEGER` is canonically encoded.
377377
fn validate_canonical(bytes: &[u8]) -> Result<()> {
378+
let non_canonical_error = Tag::Integer.non_canonical_error().into();
379+
378380
// The `INTEGER` type always encodes a signed value and we're decoding
379381
// as signed here, so we allow a zero extension or sign extension byte,
380382
// but only as permitted under DER canonicalization.
381383
match bytes {
382-
[] => Err(Tag::Integer.non_canonical_error()),
383-
[0x00, byte, ..] if *byte < 0x80 => Err(Tag::Integer.non_canonical_error()),
384-
[0xFF, byte, ..] if *byte >= 0x80 => Err(Tag::Integer.non_canonical_error()),
384+
[] => Err(non_canonical_error),
385+
[0x00, byte, ..] if *byte < 0x80 => Err(non_canonical_error),
386+
[0xFF, byte, ..] if *byte >= 0x80 => Err(non_canonical_error),
385387
_ => Ok(()),
386388
}
387389
}

‎der/src/asn1/integer/uint.rs‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -25,19 +25,19 @@ macro_rules! impl_encoding_traits {
2525
let max_length = u32::from(header.length) as usize;
2626

2727
if max_length == 0 {
28-
return Err(Tag::Integer.length_error());
28+
return Err(reader.error(Tag::Integer.length_error()));
2929
}
3030

3131
if max_length > buf.len() {
32-
return Err(Self::TAG.non_canonical_error());
32+
return Err(reader.error(Self::TAG.non_canonical_error()));
3333
}
3434

3535
let bytes = reader.read_into(&mut buf[..max_length])?;
3636
let result = Self::from_be_bytes(decode_to_array(bytes)?);
3737

3838
// Ensure we compute the same encoded length as the original any value
3939
if header.length != result.value_len()? {
40-
return Err(Self::TAG.non_canonical_error());
40+
return Err(reader.error(Self::TAG.non_canonical_error()));
4141
}
4242

4343
Ok(result)
@@ -127,7 +127,7 @@ impl<'a> DecodeValue<'a> for UintRef<'a> {
127127

128128
// Ensure we compute the same encoded length as the original any value.
129129
if result.value_len()? != header.length {
130-
return Err(Self::TAG.non_canonical_error());
130+
return Err(reader.error(Self::TAG.non_canonical_error()));
131131
}
132132

133133
Ok(result)
@@ -221,7 +221,7 @@ mod allocating {
221221

222222
// Ensure we compute the same encoded length as the original any value.
223223
if result.value_len()? != header.length {
224-
return Err(Self::TAG.non_canonical_error());
224+
return Err(reader.error(Self::TAG.non_canonical_error()));
225225
}
226226

227227
Ok(result)
@@ -328,11 +328,11 @@ pub(crate) fn decode_to_slice(bytes: &[u8]) -> Result<&[u8]> {
328328
// integer (since we're decoding an unsigned integer).
329329
// We expect all such cases to have a leading `0x00` byte.
330330
match bytes {
331-
[] => Err(Tag::Integer.non_canonical_error()),
331+
[] => Err(Tag::Integer.non_canonical_error().into()),
332332
[0] => Ok(bytes),
333-
[0, byte, ..] if *byte < 0x80 => Err(Tag::Integer.non_canonical_error()),
333+
[0, byte, ..] if *byte < 0x80 => Err(Tag::Integer.non_canonical_error().into()),
334334
[0, rest @ ..] => Ok(rest),
335-
[byte, ..] if *byte >= 0x80 => Err(Tag::Integer.value_error()),
335+
[byte, ..] if *byte >= 0x80 => Err(Tag::Integer.value_error().into()),
336336
_ => Ok(bytes),
337337
}
338338
}

0 commit comments

Comments
 (0)