Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial fuzzing support and fixes #11

Merged
merged 11 commits into from
Dec 1, 2022
3 changes: 3 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -39,3 +39,6 @@ log-debug = []
log-warn = []
log-error = []


[patch.crates-io]
flexiber = { git = "https://github.com/Nitrokey/flexiber", branch = "oath-authenticator" }
47 changes: 47 additions & 0 deletions fuzz/Cargo.toml
Original file line number Diff line number Diff line change
@@ -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" }
37 changes: 37 additions & 0 deletions fuzz/Makefile
Original file line number Diff line number Diff line change
@@ -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
14 changes: 14 additions & 0 deletions fuzz/fuzz_targets/fuzz_target_1.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
#![no_main]

use libfuzzer_sys::fuzz_target;

fuzz_target!(|data: &[u8]| {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we want to be chain multiple commands within one round of fuzzing. Otherwise many code paths can't be explored, for example for authentication, and modification/deletion. You can use Arbitrary to get a vec of commands.

Also does Oath need authentication for some command? I think it would be very hard for the fuzzer find the correct password so it should probably be in a seed corpus.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Multiple commands - yes, that's already in the list. Will check your implementation with Arbitrary.
  2. No need for authentication by default.
  3. Removed the empty lib.rs.

While fuzzing is currently shallow due to causes you have mentioned, it already found problems within the binary parser, which I expect will be the only one found here.

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();
}
})
});
Empty file added fuzz/lib.rs
Empty file.
33 changes: 19 additions & 14 deletions src/authenticator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ use trussed::{
types::{KeyId, Location, PathBuf},
};

use crate::{command, Command, oath, state::{CommandState, State}};
use crate::{assertfn, command, Command, oath, state::{CommandState, State}};
use crate::command::VerifyCode;
use crate::oath::Kind;

Expand Down Expand Up @@ -174,9 +174,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));
assertfn(class.chain().last_or_only(), Status::CommandChainingNotSupported)?;
assertfn(class.secure_messaging().none(), Status::SecureMessagingNotSupported)?;
assertfn(class.channel() == Some(0), Status::ClassNotSupported)?;

// parse Iso7816Command as PivCommand
let command: Command = command.try_into()?;
Expand Down Expand Up @@ -326,7 +326,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
Expand Down Expand Up @@ -452,7 +453,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();
Expand Down Expand Up @@ -746,13 +748,16 @@ where
}
}

if found.is_none() {
// Failed verification
self.wink_bad();
return Err(Status::VerificationFailed);
}
let found = match found {
None => {
// Failed verification
self.wink_bad();
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
Expand Down Expand Up @@ -788,12 +793,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(())
}

Expand Down
33 changes: 18 additions & 15 deletions src/command.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use core::convert::{TryFrom, TryInto};

use iso7816::{Data, Status};

use crate::oath;
use crate::{assertfn, oath};

const FAILED_PARSING_ERROR: Status = iso7816::Status::IncorrectDataParameter;

Expand Down Expand Up @@ -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());
assertfn(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()?;
Expand All @@ -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());
assertfn(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());
assertfn(slice.tag() == (oath::Tag::Response as u8).try_into().unwrap(), FAILED_PARSING_ERROR)?;
let response = slice.as_bytes();
// assert_eq!(response.len(), 20);

Expand All @@ -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());
assertfn(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());
assertfn(slice.tag() == (oath::Tag::Challenge as u8).try_into().unwrap(), FAILED_PARSING_ERROR)?;
let challenge = slice.as_bytes();

Ok(Validate { challenge, response })
Expand All @@ -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());
assertfn(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());
assertfn(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 })
Expand All @@ -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());
assertfn(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());
assertfn(second.tag() == (oath::Tag::Challenge as u8).try_into().unwrap(), FAILED_PARSING_ERROR)?;
let challenge = second.as_bytes();

Ok(Calculate { label, challenge })
Expand All @@ -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());
assertfn(first.tag() == (oath::Tag::Challenge as u8).try_into().unwrap(), FAILED_PARSING_ERROR)?;
let challenge = first.as_bytes();

Ok(CalculateAll { challenge })
Expand Down Expand Up @@ -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());
assertfn(first.tag() == (oath::Tag::Name as u8).try_into().unwrap(), FAILED_PARSING_ERROR)?;
let label = first.as_bytes();

Ok(Delete { label })
Expand Down Expand Up @@ -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());
assertfn(flexiber::Tag::try_from(tag).unwrap() == Self::tag(), flexiber::ErrorKind::Failed)?;
Ok(Properties(properties))
}
}
Expand All @@ -306,12 +307,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());
assertfn(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()?;
Expand Down
8 changes: 8 additions & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,3 +32,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 assertfn<T>(cond: bool, err: T) -> core::result::Result<(), T> {
szszszsz marked this conversation as resolved.
Show resolved Hide resolved
match cond {
true => Ok(()),
false => Err(err)
}
}
4 changes: 2 additions & 2 deletions src/state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down