Repository: alex/rust-asn1 (language: rust)

Issue:
Decoding of ObjectIdentifiers fails when encoding of individual OID components exceeds 4 octets
I believe this is a relatively rare occurrence, but I have encountered this limitation when attempting to parse "real life" certificates (not contrived test cases) with the `cryptography` library. My currently installed version of cryptography is using rust-asn1 0.15.2.

It appears that the `base128` module does not support decoding individual OID components whose encoding exceeds 4 octets: https://github.com/alex/rust-asn1/blob/main/src/base128.rs#L22. I don't believe any of the ASN.1 specifications (X.680, X.690, etc.) state that the encoding of individual OID components must not be greater than 4 octets. Additionally, and at least one other tool (in particular, pyasn1) appear to handle in the expected manner, so this appears to be a limitation specific to this library. As mentioned above, OIDs with these (abnormal) components are rare, but I want to report this behavior nonetheless.

Example OID that exhibits this issue: `2.16.840.1.113733.1.16.1.2.2.7.1.750304883`. The DER encoding of this OID is: `06 13 60 86 48  01 86 F8 45 01 10 01 02 02 07 01 82 E5 E2 FC 73`. Note that the encoding of the final component uses 5 octets.

Result when running `x509.ObjectIdentifier('2.16.840.1.113733.1.16.1.2.2.7.1.750304883')` in a Python console:

```python
x509.ObjectIdentifier('2.16.840.1.113733.1.16.1.2.2.7.1.750304883')
thread '<unnamed>' panicked at 'called `Result::unwrap()` on an `Err` value: ParseError { kind: InvalidValue }', C:\Users\runneradmin\.cargo\registry\src\index.crates.io-6f17d22bba15001f\asn1-0.15.2\src\object_identifier.rs:106:58
Traceback (most recent call last):
  File "C:\Program Files\JetBrains\PyCharm 2023.1.4\plugins\python\helpers\pydev\pydevconsole.py", line 364, in runcode
    coro = func()
  File "<input>", line 1, in <module>
pyo3_runtime.PanicException: called `Result::unwrap()` on an `Err` value: ParseError { kind: InvalidValue }
thread '<unnamed>' panicked at 'called `Result::unwrap()` on an `Err` value: ParseError { kind: InvalidValue }', C:\Users\runneradmin\.cargo\registry\src\index.crates.io-6f17d22bba15001f\asn1-0.15.2\src\object_identifier.rs:106:58
thread '<unnamed>' panicked at 'called `Result::unwrap()` on an `Err` value: ParseError { kind: InvalidValue }', C:\Users\runneradmin\.cargo\registry\src\index.crates.io-6f17d22bba15001f\asn1-0.15.2\src\object_identifier.rs:106:58
thread '<unnamed>' panicked at 'called `Result::unwrap()` on an `Err` value: ParseError { kind: InvalidValue }', C:\Users\runneradmin\.cargo\registry\src\index.crates.io-6f17d22bba15001f\asn1-0.15.2\src\object_identifier.rs:106:58

```



Discussion / hints:
That's correct that `ObjectIdentifier` only supports OIDs whose components fit in 32-bits. I think we'd be willing to consider going up to 64-bit components if there are relevant known OIDs that require it.

That said, it's a bug that this ever panics, it should always be returning a clean error on too-large inputs.

Fix patch:
diff --git a/src/base128.rs b/src/base128.rs
index ecace47..eb9c781 100644
--- a/src/base128.rs
+++ b/src/base128.rs
@@ -2,12 +2,15 @@ use crate::parser::{ParseError, ParseErrorKind, ParseResult};
 
 pub(crate) fn read_base128_int(mut data: &[u8]) -> ParseResult<(u32, &[u8])> {
     let mut ret = 0u32;
-    for i in 0..4 {
+    for i in 0..5 {
         let b = match data.first() {
             Some(b) => *b,
             None => return Err(ParseError::new(ParseErrorKind::InvalidValue)),
         };
         data = &data[1..];
+        if ret > u32::MAX >> 7 {
+            return Err(ParseError::new(ParseErrorKind::InvalidValue));
+        }
         ret <<= 7;
         ret |= u32::from(b & 0x7f);
         // Integers must be minimally encoded. `i == 0 && 0x80` would mean
@@ -59,3 +62,26 @@ pub(crate) fn write_base128_int(mut data: &mut [u8], n: u32) -> Option<usize> {
 
     Some(length)
 }
+
+#[cfg(test)]
+mod tests {
+    use super::{read_base128_int, write_base128_int};
+
+    #[test]
+    fn test_read_overflow() {
+        let buf = [0x90, 0x80, 0x80, 0x80, 0x0];
+        let result = read_base128_int(&buf);
+        assert!(result.is_err());
+    }
+
+    #[test]
+    fn test_roundtrip() {
+        for i in [0, 10, u32::MAX] {
+            let mut buf = [0; 16];
+            let length = write_base128_int(&mut buf, i).unwrap();
+            let (val, remainder) = read_base128_int(&buf[..length]).unwrap();
+            assert_eq!(i, val);
+            assert!(remainder.is_empty());
+        }
+    }
+}
diff --git a/src/object_identifier.rs b/src/object_identifier.rs
index e796d92..e81cfc1 100644
--- a/src/object_identifier.rs
+++ b/src/object_identifier.rs
@@ -197,6 +197,7 @@ mod tests {
             "1.2.3.4",
             "1.2.840.133549.1.1.5",
             "2.100.3",
+            "2.1.750304883",
         ] {
             assert_eq!(
                 &ObjectIdentifier::from_string(val).unwrap().to_string(),
diff --git a/src/parser.rs b/src/parser.rs
index 7a6ebea..4cec4cc 100644
--- a/src/parser.rs
+++ b/src/parser.rs
@@ -539,7 +539,7 @@ mod tests {
             // Overflow u32 for the tag number.
             (
                 Err(ParseError::new(ParseErrorKind::InvalidTag)),
-                b"\x1f\x88\x80\x80\x80\x00\x00",
+                b"\x1f\x88\x80\x80\x80\x80\x00",
             ),
             // Long form tag for value that fits in a short form
             (


Test patch: