Merge branch '8-fuzzing'

Initial fuzzing support Stability fixes for the flaws found during the fuzzing Add missed algorithm descriptor for the authentication Use own Flexiber fork Fixes #8
Nitrokey · Dec 1, 2022 · 9428bdc · 9428bdc
2 parents e46f000 + 6d66c31
commit 9428bdc
Show file tree

Hide file tree

Showing 8 changed files with 162 additions and 33 deletions.
diff --git a/Cargo.toml b/Cargo.toml
@@ -39,3 +39,6 @@ log-debug = []
 log-warn = []
 log-error = []
 
+
+[patch.crates-io]
+flexiber = { git = "https://github.com/Nitrokey/flexiber", rev = "0.1.1.nitrokey" }
diff --git a/fuzz/Cargo.toml b/fuzz/Cargo.toml
@@ -0,0 +1,47 @@
+[package]
+name = "oath-authenticator-fuzz"
+version = "0.0.0"
+publish = false
+edition = "2021"
+
+[package.metadata]
+cargo-fuzz = true
+
+[dependencies]
+libfuzzer-sys = "0.4"
+apdu-dispatch = { version = "0.1",  optional = true }
+flexiber = { version = "0.1.0", features = ["derive", "heapless"] }
+heapless = "0.7"
+heapless-bytes = "0.3"
+hex-literal = "0.3"
+interchange = "0.2"
+iso7816 = "0.1"
+serde = { version = "1", default-features = false }
+trussed = { version = "0.1.0", features = ["virt", "verbose-tests"] }
+ctaphid-dispatch = { version = "0.1", optional = true }
+usbd-ctaphid = { git = "https://github.com/Nitrokey/nitrokey-3-firmware", optional = true }
+
+[dependencies.oath-authenticator]
+path = ".."
+
+[features]
+default = ["ctaphid-dispatch", "usbd-ctaphid", "apdu-dispatch"]
+
+
+# Prevent this from interfering with workspaces
+[workspace]
+members = ["."]
+
+[profile.release]
+debug = 1
+
+[[bin]]
+name = "fuzz_target_1"
+path = "fuzz_targets/fuzz_target_1.rs"
+test = false
+doc = false
+
+
+[patch.crates-io]
+trussed = { git = "https://github.com/trussed-dev/trussed", branch = "main" }
+flexiber = { git = "https://github.com/Nitrokey/flexiber", branch = "oath-authenticator" }
diff --git a/fuzz/Makefile b/fuzz/Makefile
@@ -0,0 +1,37 @@
+# Copyright (C) 2022 Nitrokey GmbH
+# SPDX-License-Identifier: CC0-1.0
+
+FUZZ_DURATION?="0"
+FUZZ_JOBS?=$(shell nproc)
+.NOTPARALLEL:
+
+.PHONY: check
+check:
+	reuse lint
+
+.PHONY: fuzz
+fuzz:
+	nice cargo +nightly fuzz run --jobs ${FUZZ_JOBS} fuzz_target_1 corpus -- -max_total_time=${FUZZ_DURATION}
+
+.PHONY: fuzz-cov
+fuzz-cov:
+	cargo +nightly fuzz coverage fuzz_target_1 corpus
+	$(MAKE) fuzz-cov-show
+
+LLVMCOV=~/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/bin/llvm-cov
+.PHONY: fuzz-cov-show
+fuzz-cov-show:
+	$(LLVMCOV) show --format=html \
+		--instr-profile=coverage/fuzz_target_1/coverage.profdata \
+		 ${CARGO_TARGET_DIR}/x86_64-unknown-linux-gnu/release/fuzz_target_1 \
+		> fuzz_coverage.html
+
+.PHONY: ci
+ci: check
+
+.PHONY: setup
+setup:
+	rustup component add clippy rustfmt && rustup toolchain install nightly
+	rustup component add llvm-tools-preview
+	cargo install cargo-tarpaulin cargo-fuzz --profile release
+	python3 -m pip install reuse
diff --git a/fuzz/fuzz_targets/fuzz_target_1.rs b/fuzz/fuzz_targets/fuzz_target_1.rs
@@ -0,0 +1,14 @@
+#![no_main]
+
+use libfuzzer_sys::fuzz_target;
+
+fuzz_target!(|data: &[u8]| {
+    trussed::virt::with_ram_client("oath", move |client| {
+        let mut oath = oath_authenticator::Authenticator::<_>::new(client);
+        let mut response = heapless::Vec::<u8, { 3 * 1024 }>::new();
+
+        if let Ok(command) = iso7816::Command::<{ 10 * 255 }>::try_from(&data) {
+            oath.respond(&command, &mut response).ok();
+        }
+    })
+});
diff --git a/src/authenticator.rs b/src/authenticator.rs
@@ -15,7 +15,7 @@ use trussed::{
     types::{KeyId, Location, PathBuf},
 };
 
-use crate::{command, Command, oath, state::{CommandState, State}};
+use crate::{ensure, command, Command, oath, state::{CommandState, State}};
 use crate::command::VerifyCode;
 use crate::oath::Kind;
 
@@ -93,6 +93,10 @@ pub struct ChallengingAnswerToSelect {
     // instead of '74 00', as with the tagged/Option derivation.
     #[tlv(simple = "0x74")] // Tag::Challenge
     challenge: [u8; 8],
+
+    #[tlv(simple = "0x7b")] // Tag::Algorithm
+    // algorithm: oath::Algorithm,
+    algorithm: [u8; 1],
 }
 
 impl AnswerToSelect {
@@ -114,6 +118,8 @@ impl AnswerToSelect {
             version: self.version,
             salt: self.salt,
             challenge: challenge,
+            // algorithm: oath::Algorithm::Sha1  // TODO set proper algo
+            algorithm: [0x01]  // TODO set proper algo
         }
     }
 }
@@ -174,9 +180,9 @@ where
     fn inner_respond<const C: usize, const R: usize>(&mut self, command: &iso7816::Command<C>, reply: &mut Data<R>) -> Result
     {
         let class = command.class();
-        assert!(class.chain().last_or_only());
-        assert!(class.secure_messaging().none());
-        assert!(class.channel() == Some(0));
+        ensure(class.chain().last_or_only(), Status::CommandChainingNotSupported)?;
+        ensure(class.secure_messaging().none(), Status::SecureMessagingNotSupported)?;
+        ensure(class.channel() == Some(0), Status::ClassNotSupported)?;
 
         // parse Iso7816Command as PivCommand
         let command: Command = command.try_into()?;
@@ -326,7 +332,8 @@ where
             self.state.runtime.previously = Some(CommandState::ListCredentials(file_index));
 
             // deserialize
-            let credential: Credential = postcard_deserialize(&serialized_credential).unwrap();
+            let credential: Credential = postcard_deserialize(&serialized_credential)
+                .map_err(|_| Status::IncorrectDataParameter)?;
 
             // append data in form:
             // 72
@@ -452,7 +459,8 @@ where
             // info_now!("serialized credential: {}", hex_str!(&serialized_credential));
 
             // deserialize
-            let credential: Credential = postcard_deserialize(&serialized_credential).unwrap();
+            let credential: Credential = postcard_deserialize(&serialized_credential)
+                .map_err(|_| Status::UnspecifiedPersistentExecutionError)?;
 
             // add to response
             reply.push(0x71).unwrap();
@@ -746,13 +754,17 @@ where
             }
         }
 
-        if found.is_none() {
-            // Failed verification
-            self.wink_bad();
-            return Err(Status::VerificationFailed);
-        }
+        let found = match found {
+            None => {
+                // Failed verification
+                self.wink_bad();
+                self.delay_on_failure();
+                return Err(Status::VerificationFailed);
+            }
+            Some(val) => val
+        };
 
-        self.bump_counter_for_cred(credential, found.unwrap())?;
+        self.bump_counter_for_cred(credential, found)?;
         self.wink_good();
 
         // Verification passed
@@ -787,13 +799,12 @@ where
         );
         // load-bump counter
         let filename = self.filename_for_label(credential.label);
-        // TODO: use try_syscall
-        syscall!(self.trussed.write_file(
+        try_syscall!(self.trussed.write_file(
                         Location::Internal,
                         filename,
                         postcard_serialize_bytes(&credential).unwrap(),
                         None
-                    ));
+                    )).map_err(|_| Status::UnspecifiedPersistentExecutionError)?;
         Ok(())
     }
 
@@ -827,6 +838,12 @@ where
         // TODO blink green LED for 10 seconds, highest priority
         syscall!(self.trussed.wink(Duration::from_secs(10)));
     }
+
+    fn delay_on_failure(&mut self){
+        use crate::FAILURE_FORCED_DELAY_MILLISECONDS;
+        // TODO block for the time defined in the constant
+        // DESIGN allow only a couple of failures per power cycle? Similarly to the FIDO2 PIN
+    }
 }
 
 #[derive(Clone, Copy, Debug, Deserialize, Eq, PartialEq, Serialize)]

diff --git a/src/command.rs b/src/command.rs
@@ -2,7 +2,7 @@ use core::convert::{TryFrom, TryInto};
 
 use iso7816::{Data, Status};
 
-use crate::oath;
+use crate::{ensure, oath};
 
 const FAILED_PARSING_ERROR: Status = iso7816::Status::IncorrectDataParameter;
 
@@ -72,7 +72,8 @@ impl<'l, const C: usize> TryFrom<&'l Data<C>> for SetPassword<'l> {
         use flexiber::TaggedSlice;
         let mut decoder = flexiber::Decoder::new(data);
         let slice: TaggedSlice = decoder.decode().map_err(|_| FAILED_PARSING_ERROR)?;
-        assert!(slice.tag() == (oath::Tag::Key as u8).try_into().unwrap());
+        ensure(slice.tag() == (oath::Tag::Key as u8).try_into().unwrap(), FAILED_PARSING_ERROR)?;
+        if slice.as_bytes().len() < 2 { return Err(FAILED_PARSING_ERROR) };
         let (key_header, key) = slice.as_bytes().split_at(1);
 
         let kind: oath::Kind = key_header[0].try_into()?;
@@ -81,12 +82,12 @@ impl<'l, const C: usize> TryFrom<&'l Data<C>> for SetPassword<'l> {
         // assert!(algorithm == oath::Algorithm::Sha1);
 
         let slice: TaggedSlice = decoder.decode().map_err(|_| FAILED_PARSING_ERROR)?;
-        assert!(slice.tag() == (oath::Tag::Challenge as u8).try_into().unwrap());
+        ensure(slice.tag() == (oath::Tag::Challenge as u8).try_into().unwrap(), FAILED_PARSING_ERROR)?;
         let challenge = slice.as_bytes();
         // assert_eq!(challenge.len(), 8);
 
         let slice: TaggedSlice = decoder.decode().map_err(|_| FAILED_PARSING_ERROR)?;
-        assert!(slice.tag() == (oath::Tag::Response as u8).try_into().unwrap());
+        ensure(slice.tag() == (oath::Tag::Response as u8).try_into().unwrap(), FAILED_PARSING_ERROR)?;
         let response = slice.as_bytes();
         // assert_eq!(response.len(), 20);
 
@@ -113,11 +114,11 @@ impl<'l, const C: usize> TryFrom<&'l Data<C>> for Validate<'l> {
         let mut decoder = flexiber::Decoder::new(data);
 
         let slice: TaggedSlice = decoder.decode().map_err(|_| FAILED_PARSING_ERROR)?;
-        assert!(slice.tag() == (oath::Tag::Response as u8).try_into().unwrap());
+        ensure(slice.tag() == (oath::Tag::Response as u8).try_into().unwrap(), FAILED_PARSING_ERROR)?;
         let response = slice.as_bytes();
 
         let slice: TaggedSlice = decoder.decode().map_err(|_| FAILED_PARSING_ERROR)?;
-        assert!(slice.tag() == (oath::Tag::Challenge as u8).try_into().unwrap());
+        ensure(slice.tag() == (oath::Tag::Challenge as u8).try_into().unwrap(), FAILED_PARSING_ERROR)?;
         let challenge = slice.as_bytes();
 
         Ok(Validate { challenge, response })
@@ -137,11 +138,11 @@ impl<'l, const C: usize> TryFrom<&'l Data<C>> for VerifyCode<'l> {
         let mut decoder = flexiber::Decoder::new(data);
 
         let first: TaggedSlice = decoder.decode().map_err(|_| FAILED_PARSING_ERROR)?;
-        assert!(first.tag() == (oath::Tag::Name as u8).try_into().unwrap());
+        ensure(first.tag() == (oath::Tag::Name as u8).try_into().unwrap(), FAILED_PARSING_ERROR)?;
         let label = first.as_bytes();
 
         let slice: TaggedSlice = decoder.decode().map_err(|_| FAILED_PARSING_ERROR)?;
-        assert!(slice.tag() == (oath::Tag::Response as u8).try_into().unwrap());
+        ensure(slice.tag() == (oath::Tag::Response as u8).try_into().unwrap(), FAILED_PARSING_ERROR)?;
         let response = u32::from_be_bytes(slice.as_bytes().try_into().map_err(|_| FAILED_PARSING_ERROR )?);
 
         Ok(VerifyCode { label, response })
@@ -161,11 +162,11 @@ impl<'l, const C: usize> TryFrom<&'l Data<C>> for Calculate<'l> {
         let mut decoder = flexiber::Decoder::new(data);
 
         let first: TaggedSlice = decoder.decode().map_err(|_| FAILED_PARSING_ERROR)?;
-        assert!(first.tag() == (oath::Tag::Name as u8).try_into().unwrap());
+        ensure(first.tag() == (oath::Tag::Name as u8).try_into().unwrap(), FAILED_PARSING_ERROR)?;
         let label = first.as_bytes();
 
         let second: TaggedSlice = decoder.decode().map_err(|_| FAILED_PARSING_ERROR)?;
-        assert!(second.tag() == (oath::Tag::Challenge as u8).try_into().unwrap());
+        ensure(second.tag() == (oath::Tag::Challenge as u8).try_into().unwrap(), FAILED_PARSING_ERROR)?;
         let challenge = second.as_bytes();
 
         Ok(Calculate { label, challenge })
@@ -184,7 +185,7 @@ impl<'l, const C: usize> TryFrom<&'l Data<C>> for CalculateAll<'l> {
         let mut decoder = flexiber::Decoder::new(data);
 
         let first: TaggedSlice = decoder.decode().map_err(|_| FAILED_PARSING_ERROR)?;
-        assert!(first.tag() == (oath::Tag::Challenge as u8).try_into().unwrap());
+        ensure(first.tag() == (oath::Tag::Challenge as u8).try_into().unwrap(), FAILED_PARSING_ERROR)?;
         let challenge = first.as_bytes();
 
         Ok(CalculateAll { challenge })
@@ -213,7 +214,7 @@ impl<'l, const C: usize> TryFrom<&'l Data<C>> for Delete<'l> {
         let mut decoder = flexiber::Decoder::new(data);
 
         let first: TaggedSlice = decoder.decode().map_err(|_| FAILED_PARSING_ERROR)?;
-        assert!(first.tag() == (oath::Tag::Name as u8).try_into().unwrap());
+        ensure(first.tag() == (oath::Tag::Name as u8).try_into().unwrap(), FAILED_PARSING_ERROR)?;
         let label = first.as_bytes();
 
         Ok(Delete { label })
@@ -284,7 +285,7 @@ impl<'a> flexiber::Decodable<'a> for Properties {
         let two_bytes: [u8; 2] = decoder.decode()?;
         let [tag, properties] = two_bytes;
         use flexiber::Tagged;
-        assert_eq!(flexiber::Tag::try_from(tag).unwrap(), Self::tag());
+        ensure(flexiber::Tag::try_from(tag).unwrap() == Self::tag(), flexiber::ErrorKind::Failed)?;
         Ok(Properties(properties))
     }
 }
@@ -305,13 +306,14 @@ impl<'l, const C: usize> TryFrom<&'l Data<C>> for Register<'l> {
 
         // first comes the label of the credential, with Tag::Name
         let first: TaggedSlice = decoder.decode().map_err(|_| FAILED_PARSING_ERROR)?;
-        // TODO make asserts recoverable errors
-        assert!(first.tag() == (oath::Tag::Name as u8).try_into().unwrap());
+        ensure(first.tag() == (oath::Tag::Name as u8).try_into().unwrap(), FAILED_PARSING_ERROR)?;
         let label = first.as_bytes();
 
         // then come (kind,algorithm,digits) and the actual secret (somewhat massaged)
         let second: TaggedSlice = decoder.decode().map_err(|_| FAILED_PARSING_ERROR)?;
-        second.tag().assert_eq((oath::Tag::Key as u8).try_into().unwrap()).unwrap();
+        second.tag().assert_eq((oath::Tag::Key as u8).try_into().unwrap()).map_err(|_| FAILED_PARSING_ERROR)?;
+
+        if second.as_bytes().len() < 3 { return Err(FAILED_PARSING_ERROR) };
         let (secret_header, secret) = second.as_bytes().split_at(2);
 
         let kind: oath::Kind = secret_header[0].try_into()?;

diff --git a/src/lib.rs b/src/lib.rs
@@ -23,6 +23,7 @@ pub const YUBICO_OATH_AID: &[u8] = &hex!("A000000527 2101");// 01");
 
 /// This constant defines timeout for the regular UP confirmation
 pub const UP_TIMEOUT_MILLISECONDS: u32 = 15 * 1000;
+pub const FAILURE_FORCED_DELAY_MILLISECONDS: u32 = 1 * 1000;
 
 // class AID(bytes, Enum):
 //     OTP = b'\xa0\x00\x00\x05\x27 \x20\x01'
@@ -32,3 +33,11 @@ pub const UP_TIMEOUT_MILLISECONDS: u32 = 15 * 1000;
 //     PIV = b'\xa0\x00\x00\x03\x08'
 //     U2F = b'\xa0\x00\x00\x06\x47\x2f\x00\x01'  # Official
 //     U2F_YUBICO = b'\xa0\x00\x00\x05\x27\x10\x02'  # Yubico - No longer used
+
+
+fn ensure<T>(cond: bool, err: T) ->  core::result::Result<(), T> {
+    match cond {
+        true => Ok(()),
+        false => Err(err)
+    }
+}
diff --git a/src/state.rs b/src/state.rs
@@ -94,12 +94,12 @@ impl State {
         let result = f(trussed, &mut state);
 
         // 3. Always write it back
-        syscall!(trussed.write_file(
+        try_syscall!(trussed.write_file(
             Location::Internal,
             PathBuf::from(Self::FILENAME),
             postcard_serialize_bytes(&state).unwrap(),
             None,
-        ));
+        )).map_err(|_| Status::NotEnoughMemory)?;
 
         // 4. Return whatever
         result