Repository: pyfisch/cbor
Pull request #153 (2019-10-03 17:52:40)

Prevent stack overflow from nested tags
In the deserializer decrement the remaining depth
each time a tagged value is encountered to prevent
stack overflows caused by this recursion.
Small malicious input can cause this overflow,
therefore it is considered to be a security issue.

I'd like to thank Eric Rafaloff at Trail of Bits,
who discovered this issue during a code review,
for reporting it.

Add a test case.


Patch:
diff --git a/.travis.yml b/.travis.yml
index fe6a40c..e282a75 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -13,11 +13,10 @@ before_script:
   - rustup target add thumbv7em-none-eabihf     # Any target that does not have a standard library will do
 script:
   - cargo fmt --all -- --check
-  - (rustup component add clippy && cargo clippy --all -- -D clippy::all) || true
+  - (rustup component add clippy && cargo clippy --all --all-features -- -D clippy::all) || exit 0
   - cargo build
   - cargo test
-  - cargo build --no-default-features --features alloc --target thumbv7em-none-eabihf # Test we can build a platform that does not have std.
+  - cargo build --no-default-features --target thumbv7em-none-eabihf # Test we can build a platform that does not have std.
   - cargo test --no-default-features --lib --tests # Run no_std tests
-  - [[ $TRAVIS_RUST_VERSION != "1.31.0" ]] && cargo build --no-default-features --features alloc
   - cargo build --features unsealed_read_write # The crate should still build when the unsealed_read_write feature is enabled.
   - cargo build --no-default-features --features unsealed_read_write # The crate should still build when the unsealed_read_write feature is enabled and std disabled.
diff --git a/src/de.rs b/src/de.rs
index 69cb456..9e259b5 100644
--- a/src/de.rs
+++ b/src/de.rs
@@ -704,22 +704,22 @@ where
             0xbf => self.parse_indefinite_map(visitor),
 
             // Major type 6: optional semantic tagging of other major types
-            0xc0..=0xd7 => self.parse_value(visitor),
+            0xc0..=0xd7 => self.recursion_checked(|de| de.parse_value(visitor)),
             0xd8 => {
                 self.parse_u8()?;
-                self.parse_value(visitor)
+                self.recursion_checked(|de| de.parse_value(visitor))
             }
             0xd9 => {
                 self.parse_u16()?;
-                self.parse_value(visitor)
+                self.recursion_checked(|de| de.parse_value(visitor))
             }
             0xda => {
                 self.parse_u32()?;
-                self.parse_value(visitor)
+                self.recursion_checked(|de| de.parse_value(visitor))
             }
             0xdb => {
                 self.parse_u64()?;
-                self.parse_value(visitor)
+                self.recursion_checked(|de| de.parse_value(visitor))
             }
             0xdc..=0xdf => Err(self.error(ErrorCode::UnassignedCode)),
 


Test patch:
diff --git a/tests/de.rs b/tests/de.rs
index 7e3fd9c..49f6e8b 100644
--- a/tests/de.rs
+++ b/tests/de.rs
@@ -725,4 +725,15 @@ mod std_tests {
         let deserialized_ip = from_slice::<IpAddr>(&buf).unwrap();
         assert_eq!(ip, deserialized_ip);
     }
+
+    #[test]
+    fn attempt_stack_overflow() {
+        // Create a tag 17, followed by 999 more tag 17:
+        // 17(17(17(17(17(17(17(17(17(17(17(17(17(17(17(17(17(17(...
+        // This causes deep recursion in the decoder and may
+        // exhaust the stack and therfore result in a stack overflow.
+        let input = vec![0xd1; 1000];
+        let err = serde_cbor::from_slice::<serde_cbor::Value>(&input).expect_err("recursion limit");
+        assert!(err.is_syntax());
+    }
 }