Skip to content

Commit

Permalink
Fix #319: CTAP2.0 bug in preflighting, which can omit credential data
Browse files Browse the repository at this point in the history
Extend the mock device to be able to skip the low-level byte-by-byte
comparison of incoming and outgoing data, and instead use CTAP requests
and responses directly, for higher-level business-logic testing.
Add tests for preflighting.
  • Loading branch information
Martin Sirringhaus authored and jschanck committed Nov 18, 2023
1 parent be6526c commit 32f8e14
Show file tree
Hide file tree
Showing 13 changed files with 492 additions and 19 deletions.
4 changes: 3 additions & 1 deletion src/ctap2/commands/bio_enrollment.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use serde_bytes::ByteBuf;
use serde_cbor::{from_slice, to_vec, Value};
use std::fmt;

use super::{Command, CommandError, PinUvAuthCommand, RequestCtap2, StatusCode};
use super::{Command, CommandError, CtapResponse, PinUvAuthCommand, RequestCtap2, StatusCode};

#[derive(Debug, Clone, Copy)]
pub enum BioEnrollmentModality {
Expand Down Expand Up @@ -518,6 +518,8 @@ pub struct BioEnrollmentResponse {
pub(crate) max_template_friendly_name: Option<u64>,
}

impl CtapResponse for BioEnrollmentResponse {}

impl<'de> Deserialize<'de> for BioEnrollmentResponse {
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where
Expand Down
3 changes: 3 additions & 0 deletions src/ctap2/commands/client_pin.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#![allow(non_upper_case_globals)]
use super::CtapResponse;
// Note: Needed for PinUvAuthTokenPermission
// The current version of `bitflags` doesn't seem to allow
// to set this for an individual bitflag-struct.
Expand Down Expand Up @@ -155,6 +156,8 @@ pub struct ClientPinResponse {
pub uv_retries: Option<u8>,
}

impl CtapResponse for ClientPinResponse {}

impl<'de> Deserialize<'de> for ClientPinResponse {
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where
Expand Down
4 changes: 3 additions & 1 deletion src/ctap2/commands/credential_management.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use super::{Command, CommandError, PinUvAuthCommand, RequestCtap2, StatusCode};
use super::{Command, CommandError, CtapResponse, PinUvAuthCommand, RequestCtap2, StatusCode};
use crate::{
crypto::{COSEKey, PinUvAuthParam, PinUvAuthToken},
ctap2::server::{
Expand Down Expand Up @@ -171,6 +171,8 @@ pub struct CredentialManagementResponse {
pub large_blob_key: Option<Vec<u8>>,
}

impl CtapResponse for CredentialManagementResponse {}

#[derive(Debug, PartialEq, Eq, Serialize)]
pub struct CredentialRpListEntry {
/// RP Information
Expand Down
9 changes: 8 additions & 1 deletion src/ctap2/commands/get_assertion.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use super::get_info::AuthenticatorInfo;
use super::{
Command, CommandError, PinUvAuthCommand, RequestCtap1, RequestCtap2, Retryable, StatusCode,
Command, CommandError, CtapResponse, PinUvAuthCommand, RequestCtap1, RequestCtap2, Retryable,
StatusCode,
};
use crate::consts::{
PARAMETER_SIZE, U2F_AUTHENTICATE, U2F_DONT_ENFORCE_USER_PRESENCE_AND_SIGN,
Expand Down Expand Up @@ -292,6 +293,9 @@ impl Serialize for GetAssertion {
}
}

type GetAssertionOutput = Vec<GetAssertionResult>;
impl CtapResponse for GetAssertionOutput {}

impl RequestCtap1 for GetAssertion {
type Output = Vec<GetAssertionResult>;
type AdditionalInfo = PublicKeyCredentialDescriptor;
Expand Down Expand Up @@ -508,6 +512,7 @@ impl GetAssertionResult {
}
}

#[derive(Debug, PartialEq)]
pub struct GetAssertionResponse {
pub credentials: Option<PublicKeyCredentialDescriptor>,
pub auth_data: AuthenticatorData,
Expand All @@ -516,6 +521,8 @@ pub struct GetAssertionResponse {
pub number_of_credentials: Option<usize>,
}

impl CtapResponse for GetAssertionResponse {}

impl<'de> Deserialize<'de> for GetAssertionResponse {
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where
Expand Down
4 changes: 3 additions & 1 deletion src/ctap2/commands/get_info.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use super::{Command, CommandError, RequestCtap2, StatusCode};
use super::{Command, CommandError, CtapResponse, RequestCtap2, StatusCode};
use crate::ctap2::attestation::AAGuid;
use crate::ctap2::server::PublicKeyCredentialParameters;
use crate::transport::errors::HIDError;
Expand Down Expand Up @@ -368,6 +368,8 @@ impl AuthenticatorInfo {
}
}

impl CtapResponse for AuthenticatorInfo {}

macro_rules! parse_next_optional_value {
($name:expr, $map:expr) => {
if $name.is_some() {
Expand Down
5 changes: 4 additions & 1 deletion src/ctap2/commands/get_version.rs
Original file line number Diff line number Diff line change
@@ -1,14 +1,17 @@
use super::{CommandError, RequestCtap1, Retryable};
use super::{CommandError, CtapResponse, RequestCtap1, Retryable};
use crate::consts::U2F_VERSION;
use crate::transport::errors::{ApduErrorStatus, HIDError};
use crate::transport::{FidoDevice, VirtualFidoDevice};
use crate::u2ftypes::CTAP1RequestAPDU;

#[allow(non_camel_case_types)]
#[derive(Debug, Clone, PartialEq, Eq)]
pub enum U2FInfo {
U2F_V2,
}

impl CtapResponse for U2FInfo {}

#[derive(Debug, Default)]
// TODO(baloo): if one does not issue U2F_VERSION before makecredentials or getassertion, token
// will return error (ConditionsNotSatified), test this in unit tests
Expand Down
5 changes: 4 additions & 1 deletion src/ctap2/commands/make_credentials.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use super::get_info::{AuthenticatorInfo, AuthenticatorVersion};
use super::{
Command, CommandError, PinUvAuthCommand, RequestCtap1, RequestCtap2, Retryable, StatusCode,
Command, CommandError, CtapResponse, PinUvAuthCommand, RequestCtap1, RequestCtap2, Retryable,
StatusCode,
};
use crate::consts::{PARAMETER_SIZE, U2F_REGISTER, U2F_REQUEST_USER_PRESENCE};
use crate::crypto::{
Expand Down Expand Up @@ -199,6 +200,8 @@ impl<'de> Deserialize<'de> for MakeCredentialsResult {
}
}

impl CtapResponse for MakeCredentialsResult {}

#[derive(Copy, Clone, Debug, Default, Serialize)]
#[cfg_attr(test, derive(Deserialize))]
pub struct MakeCredentialsOptions {
Expand Down
12 changes: 7 additions & 5 deletions src/ctap2/commands/mod.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
use crate::crypto::{CryptoError, PinUvAuthParam, PinUvAuthToken};
use crate::ctap2::commands::client_pin::{
GetPinRetries, GetUvRetries, PinError,
};
use crate::ctap2::commands::client_pin::{GetPinRetries, GetUvRetries, PinError};
use crate::ctap2::commands::get_info::AuthenticatorInfo;
use crate::ctap2::server::UserVerificationRequirement;
use crate::errors::AuthenticatorError;
Expand Down Expand Up @@ -50,7 +48,7 @@ impl<T> From<T> for Retryable<T> {
}

pub trait RequestCtap1: fmt::Debug {
type Output;
type Output: CtapResponse;
// E.g.: For GetAssertion, which key-handle is currently being tested
type AdditionalInfo;

Expand All @@ -75,7 +73,7 @@ pub trait RequestCtap1: fmt::Debug {
}

pub trait RequestCtap2: fmt::Debug {
type Output;
type Output: CtapResponse;

fn command(&self) -> Command;

Expand All @@ -93,6 +91,10 @@ pub trait RequestCtap2: fmt::Debug {
) -> Result<Self::Output, HIDError>;
}

// Sadly, needs to be 'static to enable us in tests to collect them in a Vec
// but all of them are 'static, so this is currently no problem.
pub trait CtapResponse: std::fmt::Debug + 'static {}

#[derive(Debug, Clone)]
pub enum PinUvAuthResult {
/// Request is CTAP1 and does not need PinUvAuth
Expand Down
Loading

0 comments on commit 32f8e14

Please sign in to comment.