From 4c84e940391149b1fb3526e148d290376787a71c Mon Sep 17 00:00:00 2001 From: Kamran Khan Date: Mon, 7 Dec 2020 21:23:55 -0800 Subject: [PATCH 01/15] Use new APDU parser in CTAP1 code --- src/ctap/apdu.rs | 33 +++++++++++++++----------- src/ctap/ctap1.rs | 59 ++++++++++++++++++++++------------------------- 2 files changed, 47 insertions(+), 45 deletions(-) diff --git a/src/ctap/apdu.rs b/src/ctap/apdu.rs index 949151e..530a85b 100644 --- a/src/ctap/apdu.rs +++ b/src/ctap/apdu.rs @@ -53,10 +53,10 @@ pub enum ApduInstructions { #[allow(dead_code)] #[derive(Default, PartialEq)] pub struct ApduHeader { - cla: u8, - ins: u8, - p1: u8, - p2: u8, + pub cla: u8, + pub ins: u8, + pub p1: u8, + pub p2: u8, } impl From<&[u8; APDU_HEADER_LEN]> for ApduHeader { @@ -96,11 +96,11 @@ pub enum ApduType { #[allow(dead_code)] #[derive(PartialEq)] pub struct APDU { - header: ApduHeader, - lc: u16, - data: Vec, - le: u32, - case_type: ApduType, + pub header: ApduHeader, + pub lc: u16, + pub data: Vec, + pub le: u32, + pub case_type: ApduType, } impl TryFrom<&[u8]> for APDU { @@ -168,12 +168,17 @@ impl TryFrom<&[u8]> for APDU { } if payload.len() > 2 { // Lc is possibly three-bytes long - let extended_apdu_lc: usize = BigEndian::read_u16(&payload[1..]) as usize; - let extended_apdu_le_len: usize = if payload.len() > extended_apdu_lc { - payload.len() - extended_apdu_lc - 3 - } else { - 0 + let extended_apdu_lc: usize = BigEndian::read_u16(&payload[1..3]) as usize; + if payload.len() < extended_apdu_lc + 3 { + return Err(ApduStatusCode::SW_WRONG_LENGTH); + } + let extended_apdu_le_len: usize = match payload.len() - extended_apdu_lc { + // There's some possible Le bytes at the end + 2..=5 => payload.len() - extended_apdu_lc - 3, + // There are more bytes than even Le3 can consume, return an error + _ => return Err(ApduStatusCode::SW_WRONG_LENGTH), }; + if byte_0 == 0 && extended_apdu_le_len <= 3 { // If first byte is zero AND the next two bytes can be parsed as a big-endian // length that covers the rest of the block (plus few additional bytes for Le), we diff --git a/src/ctap/ctap1.rs b/src/ctap/ctap1.rs index e434c36..ea5f894 100644 --- a/src/ctap/ctap1.rs +++ b/src/ctap/ctap1.rs @@ -12,6 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. +use super::apdu::{ApduStatusCode, APDU}; use super::hid::ChannelID; use super::status_code::Ctap2StatusCode; use super::CtapState; @@ -118,11 +119,16 @@ impl TryFrom<&[u8]> for U2fCommand { type Error = Ctap1StatusCode; fn try_from(message: &[u8]) -> Result { - if message.len() < Ctap1Command::APDU_HEADER_LEN as usize { - return Err(Ctap1StatusCode::SW_WRONG_DATA); - } - - let (apdu, payload) = message.split_at(Ctap1Command::APDU_HEADER_LEN as usize); + let apdu: APDU = match APDU::try_from(message) { + Ok(apdu) => apdu, + // Todo: Better conversion between ApduStatusCode and Ctap1StatusCode + // Maybe use TryFrom? + Err(apdu_status_code) => match apdu_status_code { + ApduStatusCode::SW_WRONG_LENGTH => return Err(Ctap1StatusCode::SW_WRONG_LENGTH), + ApduStatusCode::SW_WRONG_DATA => return Err(Ctap1StatusCode::SW_WRONG_DATA), + _ => return Err(Ctap1StatusCode::SW_COMMAND_ABORTED), + }, + }; // ISO7816 APDU Header format. Each cell is 1 byte. Note that the CTAP flavor always // encodes the length on 3 bytes and doesn't use the field "Le" (Length Expected). @@ -131,30 +137,29 @@ impl TryFrom<&[u8]> for U2fCommand { // +-----+-----+----+----+-----+-----+-----+ // | CLA | INS | P1 | P2 | Lc1 | Lc2 | Lc3 | // +-----+-----+----+----+-----+-----+-----+ - if apdu[0] != Ctap1Command::CTAP1_CLA { + if apdu.header.cla != Ctap1Command::CTAP1_CLA { return Err(Ctap1StatusCode::SW_CLA_NOT_SUPPORTED); } - let lc = (((apdu[4] as u32) << 16) | ((apdu[5] as u32) << 8) | (apdu[6] as u32)) as usize; - // Since there is always request data, the expected length is either omitted or // encoded in 2 bytes. - if lc != payload.len() && lc + 2 != payload.len() { + // Todo: support extended APDUs now that the new parser can work with those + if apdu.lc as usize != apdu.data.len() && (apdu.lc as usize) + 2 != apdu.data.len() { return Err(Ctap1StatusCode::SW_WRONG_LENGTH); } - match apdu[1] { + match apdu.header.ins { // U2F raw message format specification, Section 4.1 // +-----------------+-------------------+ // + Challenge (32B) | Application (32B) | // +-----------------+-------------------+ Ctap1Command::U2F_REGISTER => { - if lc != 64 { + if apdu.lc != 64 { return Err(Ctap1StatusCode::SW_WRONG_LENGTH); } Ok(Self::Register { - challenge: *array_ref!(payload, 0, 32), - application: *array_ref!(payload, 32, 32), + challenge: *array_ref!(apdu.data, 0, 32), + application: *array_ref!(apdu.data, 32, 32), }) } @@ -163,25 +168,25 @@ impl TryFrom<&[u8]> for U2fCommand { // + Challenge (32B) | Application (32B) | key handle len (1B) | key handle | // +-----------------+-------------------+---------------------+------------+ Ctap1Command::U2F_AUTHENTICATE => { - if lc < 65 { + if apdu.lc < 65 { return Err(Ctap1StatusCode::SW_WRONG_LENGTH); } - let handle_length = payload[64] as usize; - if lc != 65 + handle_length { + let handle_length = apdu.data[64] as usize; + if apdu.lc as usize != 65 + handle_length { return Err(Ctap1StatusCode::SW_WRONG_LENGTH); } - let flag = Ctap1Flags::try_from(apdu[2])?; + let flag = Ctap1Flags::try_from(apdu.header.p1)?; Ok(Self::Authenticate { - challenge: *array_ref!(payload, 0, 32), - application: *array_ref!(payload, 32, 32), - key_handle: payload[65..lc].to_vec(), + challenge: *array_ref!(apdu.data, 0, 32), + application: *array_ref!(apdu.data, 32, 32), + key_handle: apdu.data[65..].to_vec(), flags: flag, }) } // U2F raw message format specification, Section 6.1 Ctap1Command::U2F_VERSION => { - if lc != 0 { + if apdu.lc != 0 { return Err(Ctap1StatusCode::SW_WRONG_LENGTH); } Ok(Self::Version) @@ -190,7 +195,7 @@ impl TryFrom<&[u8]> for U2fCommand { // For Vendor specific command. Ctap1Command::VENDOR_SPECIFIC_FIRST..=Ctap1Command::VENDOR_SPECIFIC_LAST => { Ok(Self::VendorSpecific { - payload: payload.to_vec(), + payload: apdu.data.to_vec(), }) } @@ -202,8 +207,6 @@ impl TryFrom<&[u8]> for U2fCommand { pub struct Ctap1Command {} impl Ctap1Command { - const APDU_HEADER_LEN: u32 = 7; // CLA + INS + P1 + P2 + LC1-3 - const CTAP1_CLA: u8 = 0; // This byte is used in Register, but only serves backwards compatibility. const LEGACY_BYTE: u8 = 0x05; @@ -571,13 +574,7 @@ mod test { let mut message = create_authenticate_message(&application, Ctap1Flags::CheckOnly, &key_handle); - message.push(0x00); - let response = Ctap1Command::process_command(&message, &mut ctap_state, START_CLOCK_VALUE); - assert_eq!(response, Err(Ctap1StatusCode::SW_WRONG_LENGTH)); - - // Two extra zeros are okay, they could encode the expected response length. - message.push(0x00); - message.push(0x00); + message.extend_from_slice(&[0x00, 0x00, 0x00, 0x00]); let response = Ctap1Command::process_command(&message, &mut ctap_state, START_CLOCK_VALUE); assert_eq!(response, Err(Ctap1StatusCode::SW_WRONG_LENGTH)); } From e4d160aaeeb4c209908a1c9a1eb073273d2792df Mon Sep 17 00:00:00 2001 From: Kamran Khan Date: Mon, 7 Dec 2020 23:32:04 -0800 Subject: [PATCH 02/15] Use TryFrom to convert between APDU and CTAP status codes --- src/ctap/ctap1.rs | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/src/ctap/ctap1.rs b/src/ctap/ctap1.rs index ea5f894..b0ed506 100644 --- a/src/ctap/ctap1.rs +++ b/src/ctap/ctap1.rs @@ -60,6 +60,24 @@ impl TryFrom for Ctap1StatusCode { } } +impl TryFrom for Ctap1StatusCode { + type Error = (); + + fn try_from(apdu_status_code: ApduStatusCode) -> Result { + match apdu_status_code { + ApduStatusCode::SW_WRONG_LENGTH => Ok(Ctap1StatusCode::SW_WRONG_LENGTH), + ApduStatusCode::SW_WRONG_DATA => Ok(Ctap1StatusCode::SW_WRONG_DATA), + ApduStatusCode::SW_CLA_INVALID => Ok(Ctap1StatusCode::SW_CLA_NOT_SUPPORTED), + ApduStatusCode::SW_INS_INVALID => Ok(Ctap1StatusCode::SW_INS_NOT_SUPPORTED), + ApduStatusCode::SW_COND_USE_NOT_SATISFIED => { + Ok(Ctap1StatusCode::SW_CONDITIONS_NOT_SATISFIED) + } + ApduStatusCode::SW_SUCCESS => Ok(Ctap1StatusCode::SW_NO_ERROR), + _ => Ok(Ctap1StatusCode::SW_COMMAND_ABORTED), + } + } +} + impl Into for Ctap1StatusCode { fn into(self) -> u16 { self as u16 @@ -121,13 +139,9 @@ impl TryFrom<&[u8]> for U2fCommand { fn try_from(message: &[u8]) -> Result { let apdu: APDU = match APDU::try_from(message) { Ok(apdu) => apdu, - // Todo: Better conversion between ApduStatusCode and Ctap1StatusCode - // Maybe use TryFrom? - Err(apdu_status_code) => match apdu_status_code { - ApduStatusCode::SW_WRONG_LENGTH => return Err(Ctap1StatusCode::SW_WRONG_LENGTH), - ApduStatusCode::SW_WRONG_DATA => return Err(Ctap1StatusCode::SW_WRONG_DATA), - _ => return Err(Ctap1StatusCode::SW_COMMAND_ABORTED), - }, + Err(apdu_status_code) => { + return Err(Ctap1StatusCode::try_from(apdu_status_code).unwrap()) + } }; // ISO7816 APDU Header format. Each cell is 1 byte. Note that the CTAP flavor always From 373464b72d0d851d10481d3371f1ea09b4958a98 Mon Sep 17 00:00:00 2001 From: Kamran Khan Date: Mon, 7 Dec 2020 23:35:47 -0800 Subject: [PATCH 03/15] Remove redundant type declaration --- src/ctap/apdu.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ctap/apdu.rs b/src/ctap/apdu.rs index 530a85b..e3dfe23 100644 --- a/src/ctap/apdu.rs +++ b/src/ctap/apdu.rs @@ -168,7 +168,7 @@ impl TryFrom<&[u8]> for APDU { } if payload.len() > 2 { // Lc is possibly three-bytes long - let extended_apdu_lc: usize = BigEndian::read_u16(&payload[1..3]) as usize; + let extended_apdu_lc = BigEndian::read_u16(&payload[1..3]) as usize; if payload.len() < extended_apdu_lc + 3 { return Err(ApduStatusCode::SW_WRONG_LENGTH); } From 2d17bb2afafe1d94abd55978c6863d2015ff219b Mon Sep 17 00:00:00 2001 From: Kamran Khan Date: Mon, 7 Dec 2020 23:38:21 -0800 Subject: [PATCH 04/15] Readability improvements --- src/ctap/apdu.rs | 4 ++-- src/ctap/ctap1.rs | 13 +++++++------ 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/ctap/apdu.rs b/src/ctap/apdu.rs index e3dfe23..70b17eb 100644 --- a/src/ctap/apdu.rs +++ b/src/ctap/apdu.rs @@ -172,9 +172,9 @@ impl TryFrom<&[u8]> for APDU { if payload.len() < extended_apdu_lc + 3 { return Err(ApduStatusCode::SW_WRONG_LENGTH); } - let extended_apdu_le_len: usize = match payload.len() - extended_apdu_lc { + let extended_apdu_le_len: usize = match payload.len() - extended_apdu_lc - 3 { // There's some possible Le bytes at the end - 2..=5 => payload.len() - extended_apdu_lc - 3, + 0..=3 => payload.len() - extended_apdu_lc - 3, // There are more bytes than even Le3 can consume, return an error _ => return Err(ApduStatusCode::SW_WRONG_LENGTH), }; diff --git a/src/ctap/ctap1.rs b/src/ctap/ctap1.rs index b0ed506..2677201 100644 --- a/src/ctap/ctap1.rs +++ b/src/ctap/ctap1.rs @@ -144,6 +144,8 @@ impl TryFrom<&[u8]> for U2fCommand { } }; + let lc = apdu.lc as usize; + // ISO7816 APDU Header format. Each cell is 1 byte. Note that the CTAP flavor always // encodes the length on 3 bytes and doesn't use the field "Le" (Length Expected). // We keep the 2 byte of "Le" for the packet length in mind, but always ignore its value. @@ -157,8 +159,7 @@ impl TryFrom<&[u8]> for U2fCommand { // Since there is always request data, the expected length is either omitted or // encoded in 2 bytes. - // Todo: support extended APDUs now that the new parser can work with those - if apdu.lc as usize != apdu.data.len() && (apdu.lc as usize) + 2 != apdu.data.len() { + if lc != apdu.data.len() && (lc as usize) + 2 != apdu.data.len() { return Err(Ctap1StatusCode::SW_WRONG_LENGTH); } @@ -168,7 +169,7 @@ impl TryFrom<&[u8]> for U2fCommand { // + Challenge (32B) | Application (32B) | // +-----------------+-------------------+ Ctap1Command::U2F_REGISTER => { - if apdu.lc != 64 { + if lc != 64 { return Err(Ctap1StatusCode::SW_WRONG_LENGTH); } Ok(Self::Register { @@ -182,11 +183,11 @@ impl TryFrom<&[u8]> for U2fCommand { // + Challenge (32B) | Application (32B) | key handle len (1B) | key handle | // +-----------------+-------------------+---------------------+------------+ Ctap1Command::U2F_AUTHENTICATE => { - if apdu.lc < 65 { + if lc < 65 { return Err(Ctap1StatusCode::SW_WRONG_LENGTH); } let handle_length = apdu.data[64] as usize; - if apdu.lc as usize != 65 + handle_length { + if lc as usize != 65 + handle_length { return Err(Ctap1StatusCode::SW_WRONG_LENGTH); } let flag = Ctap1Flags::try_from(apdu.header.p1)?; @@ -200,7 +201,7 @@ impl TryFrom<&[u8]> for U2fCommand { // U2F raw message format specification, Section 6.1 Ctap1Command::U2F_VERSION => { - if apdu.lc != 0 { + if lc != 0 { return Err(Ctap1StatusCode::SW_WRONG_LENGTH); } Ok(Self::Version) From 56bc86c5d01146e4c7c52f58a17e7386e87f147c Mon Sep 17 00:00:00 2001 From: Kamran Khan Date: Mon, 7 Dec 2020 23:40:06 -0800 Subject: [PATCH 05/15] No need to cast again --- src/ctap/ctap1.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/ctap/ctap1.rs b/src/ctap/ctap1.rs index 2677201..4d18846 100644 --- a/src/ctap/ctap1.rs +++ b/src/ctap/ctap1.rs @@ -159,7 +159,7 @@ impl TryFrom<&[u8]> for U2fCommand { // Since there is always request data, the expected length is either omitted or // encoded in 2 bytes. - if lc != apdu.data.len() && (lc as usize) + 2 != apdu.data.len() { + if lc != apdu.data.len() && lc + 2 != apdu.data.len() { return Err(Ctap1StatusCode::SW_WRONG_LENGTH); } @@ -187,7 +187,7 @@ impl TryFrom<&[u8]> for U2fCommand { return Err(Ctap1StatusCode::SW_WRONG_LENGTH); } let handle_length = apdu.data[64] as usize; - if lc as usize != 65 + handle_length { + if lc != 65 + handle_length { return Err(Ctap1StatusCode::SW_WRONG_LENGTH); } let flag = Ctap1Flags::try_from(apdu.header.p1)?; From 0da13cd61fc4485496227645c26bee34f8ad39ac Mon Sep 17 00:00:00 2001 From: Kamran Khan Date: Wed, 9 Dec 2020 20:43:06 -0800 Subject: [PATCH 06/15] De-deuplicate le length calculation --- src/ctap/apdu.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/ctap/apdu.rs b/src/ctap/apdu.rs index 70b17eb..7136084 100644 --- a/src/ctap/apdu.rs +++ b/src/ctap/apdu.rs @@ -172,9 +172,12 @@ impl TryFrom<&[u8]> for APDU { if payload.len() < extended_apdu_lc + 3 { return Err(ApduStatusCode::SW_WRONG_LENGTH); } - let extended_apdu_le_len: usize = match payload.len() - extended_apdu_lc - 3 { + + let possible_le_len = payload.len() as i32 - extended_apdu_lc as i32 - 3; + + let extended_apdu_le_len: usize = match possible_le_len { // There's some possible Le bytes at the end - 0..=3 => payload.len() - extended_apdu_lc - 3, + 0..=3 => possible_le_len as usize, // There are more bytes than even Le3 can consume, return an error _ => return Err(ApduStatusCode::SW_WRONG_LENGTH), }; From 6f1c63e9b8b451993e7f2ca3498849b4f2c1aab6 Mon Sep 17 00:00:00 2001 From: Kamran Khan Date: Wed, 9 Dec 2020 21:06:49 -0800 Subject: [PATCH 07/15] Add test cases to cover different length scenarios --- src/ctap/ctap1.rs | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/src/ctap/ctap1.rs b/src/ctap/ctap1.rs index 4d18846..ff07c0a 100644 --- a/src/ctap/ctap1.rs +++ b/src/ctap/ctap1.rs @@ -586,10 +586,25 @@ mod test { let key_handle = ctap_state .encrypt_key_handle(sk, &application, None) .unwrap(); - let mut message = - create_authenticate_message(&application, Ctap1Flags::CheckOnly, &key_handle); + let mut message = create_authenticate_message( + &application, + Ctap1Flags::DontEnforceUpAndSign, + &key_handle, + ); - message.extend_from_slice(&[0x00, 0x00, 0x00, 0x00]); + message.push(0x00); + let response = Ctap1Command::process_command(&message, &mut ctap_state, START_CLOCK_VALUE); + assert!(response.is_ok()); + + message.push(0x00); + let response = Ctap1Command::process_command(&message, &mut ctap_state, START_CLOCK_VALUE); + assert!(response.is_ok()); + + message.push(0x00); + let response = Ctap1Command::process_command(&message, &mut ctap_state, START_CLOCK_VALUE); + assert!(response.is_ok()); + + message.push(0x00); let response = Ctap1Command::process_command(&message, &mut ctap_state, START_CLOCK_VALUE); assert_eq!(response, Err(Ctap1StatusCode::SW_WRONG_LENGTH)); } From 162c00a0d12e5c92bd08ff180d365c57568476b3 Mon Sep 17 00:00:00 2001 From: Kamran Khan Date: Thu, 10 Dec 2020 19:54:25 -0800 Subject: [PATCH 08/15] Simplify Le length calculation --- src/ctap/apdu.rs | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/ctap/apdu.rs b/src/ctap/apdu.rs index 7136084..3bf83da 100644 --- a/src/ctap/apdu.rs +++ b/src/ctap/apdu.rs @@ -173,14 +173,10 @@ impl TryFrom<&[u8]> for APDU { return Err(ApduStatusCode::SW_WRONG_LENGTH); } - let possible_le_len = payload.len() as i32 - extended_apdu_lc as i32 - 3; - - let extended_apdu_le_len: usize = match possible_le_len { - // There's some possible Le bytes at the end - 0..=3 => possible_le_len as usize, - // There are more bytes than even Le3 can consume, return an error - _ => return Err(ApduStatusCode::SW_WRONG_LENGTH), - }; + let extended_apdu_le_len: usize = payload.len() - extended_apdu_lc - 3; + if extended_apdu_le_len > 3 { + return Err(ApduStatusCode::SW_WRONG_LENGTH); + } if byte_0 == 0 && extended_apdu_le_len <= 3 { // If first byte is zero AND the next two bytes can be parsed as a big-endian From 21bdbd8114236795430326c31fb33b6d7a856d2b Mon Sep 17 00:00:00 2001 From: Kamran Khan Date: Thu, 10 Dec 2020 20:01:06 -0800 Subject: [PATCH 09/15] Use integers instead of ByteArray for the ApduStatusCode enum --- src/ctap/apdu.rs | 41 +++++++++++------------------------------ 1 file changed, 11 insertions(+), 30 deletions(-) diff --git a/src/ctap/apdu.rs b/src/ctap/apdu.rs index 3bf83da..b1d662f 100644 --- a/src/ctap/apdu.rs +++ b/src/ctap/apdu.rs @@ -2,44 +2,25 @@ use alloc::vec::Vec; use byteorder::{BigEndian, ByteOrder}; use core::convert::TryFrom; -type ByteArray = &'static [u8]; - const APDU_HEADER_LEN: usize = 4; #[cfg_attr(test, derive(Clone, Debug))] -#[allow(non_camel_case_types)] +#[allow(non_camel_case_types, dead_code)] #[derive(PartialEq)] pub enum ApduStatusCode { - SW_SUCCESS, + SW_SUCCESS = 0x90_00, /// Command successfully executed; 'XX' bytes of data are /// available and can be requested using GET RESPONSE. - SW_GET_RESPONSE, - SW_WRONG_DATA, - SW_WRONG_LENGTH, - SW_COND_USE_NOT_SATISFIED, - SW_FILE_NOT_FOUND, - SW_INCORRECT_P1P2, + SW_GET_RESPONSE = 0x61_00, + SW_WRONG_DATA = 0x6a_80, + SW_WRONG_LENGTH = 0x67_00, + SW_COND_USE_NOT_SATISFIED = 0x69_85, + SW_FILE_NOT_FOUND = 0x6a_82, + SW_INCORRECT_P1P2 = 0x6a_86, /// Instruction code not supported or invalid - SW_INS_INVALID, - SW_CLA_INVALID, - SW_INTERNAL_EXCEPTION, -} - -impl From for ByteArray { - fn from(status_code: ApduStatusCode) -> ByteArray { - match status_code { - ApduStatusCode::SW_SUCCESS => b"\x90\x00", - ApduStatusCode::SW_GET_RESPONSE => b"\x61\x00", - ApduStatusCode::SW_WRONG_DATA => b"\x6A\x80", - ApduStatusCode::SW_WRONG_LENGTH => b"\x67\x00", - ApduStatusCode::SW_COND_USE_NOT_SATISFIED => b"\x69\x85", - ApduStatusCode::SW_FILE_NOT_FOUND => b"\x6a\x82", - ApduStatusCode::SW_INCORRECT_P1P2 => b"\x6a\x86", - ApduStatusCode::SW_INS_INVALID => b"\x6d\x00", - ApduStatusCode::SW_CLA_INVALID => b"\x6e\x00", - ApduStatusCode::SW_INTERNAL_EXCEPTION => b"\x6f\x00", - } - } + SW_INS_INVALID = 0x6d_00, + SW_CLA_INVALID = 0x6e_00, + SW_INTERNAL_EXCEPTION = 0x6f_00, } #[allow(dead_code)] From 29dbff7a40a1207e19fc6f2e0249eebd6b6d5db8 Mon Sep 17 00:00:00 2001 From: Kamran Khan Date: Thu, 10 Dec 2020 20:15:05 -0800 Subject: [PATCH 10/15] The great ApduStatusCode encroachment --- src/ctap/apdu.rs | 6 +++ src/ctap/ctap1.rs | 101 ++++++++++---------------------------------- src/ctap/hid/mod.rs | 2 +- 3 files changed, 30 insertions(+), 79 deletions(-) diff --git a/src/ctap/apdu.rs b/src/ctap/apdu.rs index b1d662f..6338d15 100644 --- a/src/ctap/apdu.rs +++ b/src/ctap/apdu.rs @@ -23,6 +23,12 @@ pub enum ApduStatusCode { SW_INTERNAL_EXCEPTION = 0x6f_00, } +impl From for u16 { + fn from(_: ApduStatusCode) -> Self { + 0 + } +} + #[allow(dead_code)] pub enum ApduInstructions { Select = 0xA4, diff --git a/src/ctap/ctap1.rs b/src/ctap/ctap1.rs index ff07c0a..1156c6d 100644 --- a/src/ctap/ctap1.rs +++ b/src/ctap/ctap1.rs @@ -23,67 +23,12 @@ use core::convert::TryFrom; use crypto::rng256::Rng256; use libtock_drivers::timer::ClockValue; +// For now, they're the same thing with apdu.rs containing the authoritative definition +pub type Ctap1StatusCode = ApduStatusCode; + // The specification referenced in this file is at: // https://fidoalliance.org/specs/fido-u2f-v1.2-ps-20170411/fido-u2f-raw-message-formats-v1.2-ps-20170411.pdf -// status codes specification (version 20170411) section 3.3 -#[allow(non_camel_case_types)] -#[cfg_attr(any(test, feature = "debug_ctap"), derive(Debug, PartialEq))] -pub enum Ctap1StatusCode { - SW_NO_ERROR = 0x9000, - SW_CONDITIONS_NOT_SATISFIED = 0x6985, - SW_WRONG_DATA = 0x6A80, - SW_WRONG_LENGTH = 0x6700, - SW_CLA_NOT_SUPPORTED = 0x6E00, - SW_INS_NOT_SUPPORTED = 0x6D00, - SW_MEMERR = 0x6501, - SW_COMMAND_ABORTED = 0x6F00, - SW_VENDOR_KEY_HANDLE_TOO_LONG = 0xF000, -} - -impl TryFrom for Ctap1StatusCode { - type Error = (); - - fn try_from(value: u16) -> Result { - match value { - 0x9000 => Ok(Ctap1StatusCode::SW_NO_ERROR), - 0x6985 => Ok(Ctap1StatusCode::SW_CONDITIONS_NOT_SATISFIED), - 0x6A80 => Ok(Ctap1StatusCode::SW_WRONG_DATA), - 0x6700 => Ok(Ctap1StatusCode::SW_WRONG_LENGTH), - 0x6E00 => Ok(Ctap1StatusCode::SW_CLA_NOT_SUPPORTED), - 0x6D00 => Ok(Ctap1StatusCode::SW_INS_NOT_SUPPORTED), - 0x6501 => Ok(Ctap1StatusCode::SW_MEMERR), - 0x6F00 => Ok(Ctap1StatusCode::SW_COMMAND_ABORTED), - 0xF000 => Ok(Ctap1StatusCode::SW_VENDOR_KEY_HANDLE_TOO_LONG), - _ => Err(()), - } - } -} - -impl TryFrom for Ctap1StatusCode { - type Error = (); - - fn try_from(apdu_status_code: ApduStatusCode) -> Result { - match apdu_status_code { - ApduStatusCode::SW_WRONG_LENGTH => Ok(Ctap1StatusCode::SW_WRONG_LENGTH), - ApduStatusCode::SW_WRONG_DATA => Ok(Ctap1StatusCode::SW_WRONG_DATA), - ApduStatusCode::SW_CLA_INVALID => Ok(Ctap1StatusCode::SW_CLA_NOT_SUPPORTED), - ApduStatusCode::SW_INS_INVALID => Ok(Ctap1StatusCode::SW_INS_NOT_SUPPORTED), - ApduStatusCode::SW_COND_USE_NOT_SATISFIED => { - Ok(Ctap1StatusCode::SW_CONDITIONS_NOT_SATISFIED) - } - ApduStatusCode::SW_SUCCESS => Ok(Ctap1StatusCode::SW_NO_ERROR), - _ => Ok(Ctap1StatusCode::SW_COMMAND_ABORTED), - } - } -} - -impl Into for Ctap1StatusCode { - fn into(self) -> u16 { - self as u16 - } -} - #[cfg_attr(any(test, feature = "debug_ctap"), derive(Clone, Debug))] #[derive(PartialEq)] pub enum Ctap1Flags { @@ -154,7 +99,7 @@ impl TryFrom<&[u8]> for U2fCommand { // | CLA | INS | P1 | P2 | Lc1 | Lc2 | Lc3 | // +-----+-----+----+----+-----+-----+-----+ if apdu.header.cla != Ctap1Command::CTAP1_CLA { - return Err(Ctap1StatusCode::SW_CLA_NOT_SUPPORTED); + return Err(Ctap1StatusCode::SW_CLA_INVALID); } // Since there is always request data, the expected length is either omitted or @@ -214,7 +159,7 @@ impl TryFrom<&[u8]> for U2fCommand { }) } - _ => Err(Ctap1StatusCode::SW_INS_NOT_SUPPORTED), + _ => Err(Ctap1StatusCode::SW_INS_INVALID), } } } @@ -252,7 +197,7 @@ impl Ctap1Command { application, } => { if !ctap_state.u2f_up_state.consume_up(clock_value) { - return Err(Ctap1StatusCode::SW_CONDITIONS_NOT_SATISFIED); + return Err(Ctap1StatusCode::SW_COND_USE_NOT_SATISFIED); } Ctap1Command::process_register(challenge, application, ctap_state) } @@ -267,7 +212,7 @@ impl Ctap1Command { if flags == Ctap1Flags::EnforceUpAndSign && !ctap_state.u2f_up_state.consume_up(clock_value) { - return Err(Ctap1StatusCode::SW_CONDITIONS_NOT_SATISFIED); + return Err(Ctap1StatusCode::SW_COND_USE_NOT_SATISFIED); } Ctap1Command::process_authenticate( challenge, @@ -282,7 +227,7 @@ impl Ctap1Command { U2fCommand::Version => Ok(Vec::::from(super::U2F_VERSION_STRING)), // TODO: should we return an error instead such as SW_INS_NOT_SUPPORTED? - U2fCommand::VendorSpecific { .. } => Err(Ctap1StatusCode::SW_NO_ERROR), + U2fCommand::VendorSpecific { .. } => Err(Ctap1StatusCode::SW_INTERNAL_EXCEPTION), } } @@ -310,22 +255,22 @@ impl Ctap1Command { let pk = sk.genpk(); let key_handle = ctap_state .encrypt_key_handle(sk, &application, None) - .map_err(|_| Ctap1StatusCode::SW_COMMAND_ABORTED)?; + .map_err(|_| Ctap1StatusCode::SW_INTERNAL_EXCEPTION)?; if key_handle.len() > 0xFF { // This is just being defensive with unreachable code. - return Err(Ctap1StatusCode::SW_VENDOR_KEY_HANDLE_TOO_LONG); + return Err(Ctap1StatusCode::SW_WRONG_LENGTH); } let certificate = ctap_state .persistent_store .attestation_certificate() - .map_err(|_| Ctap1StatusCode::SW_MEMERR)? - .ok_or(Ctap1StatusCode::SW_COMMAND_ABORTED)?; + .map_err(|_| Ctap1StatusCode::SW_INTERNAL_EXCEPTION)? + .ok_or(Ctap1StatusCode::SW_INTERNAL_EXCEPTION)?; let private_key = ctap_state .persistent_store .attestation_private_key() - .map_err(|_| Ctap1StatusCode::SW_MEMERR)? - .ok_or(Ctap1StatusCode::SW_COMMAND_ABORTED)?; + .map_err(|_| Ctap1StatusCode::SW_INTERNAL_EXCEPTION)? + .ok_or(Ctap1StatusCode::SW_INTERNAL_EXCEPTION)?; let mut response = Vec::with_capacity(105 + key_handle.len() + certificate.len()); response.push(Ctap1Command::LEGACY_BYTE); @@ -380,14 +325,14 @@ impl Ctap1Command { .map_err(|_| Ctap1StatusCode::SW_WRONG_DATA)?; if let Some(credential_source) = credential_source { if flags == Ctap1Flags::CheckOnly { - return Err(Ctap1StatusCode::SW_CONDITIONS_NOT_SATISFIED); + return Err(Ctap1StatusCode::SW_COND_USE_NOT_SATISFIED); } ctap_state .increment_global_signature_counter() .map_err(|_| Ctap1StatusCode::SW_WRONG_DATA)?; let mut signature_data = ctap_state .generate_auth_data(&application, Ctap1Command::USER_PRESENCE_INDICATOR_BYTE) - .map_err(|_| Ctap1StatusCode::SW_WRONG_DATA)?; + .map_err(|_| Ctap1StatusCode::SW_COND_USE_NOT_SATISFIED)?; signature_data.extend(&challenge); let signature = credential_source .private_key @@ -466,7 +411,7 @@ mod test { ctap_state.u2f_up_state.grant_up(START_CLOCK_VALUE); let response = Ctap1Command::process_command(&message, &mut ctap_state, START_CLOCK_VALUE); // Certificate and private key are missing - assert_eq!(response, Err(Ctap1StatusCode::SW_COMMAND_ABORTED)); + assert_eq!(response, Err(Ctap1StatusCode::SW_INTERNAL_EXCEPTION)); let fake_key = [0x41u8; key_material::ATTESTATION_PRIVATE_KEY_LENGTH]; assert!(ctap_state @@ -477,7 +422,7 @@ mod test { ctap_state.u2f_up_state.grant_up(START_CLOCK_VALUE); let response = Ctap1Command::process_command(&message, &mut ctap_state, START_CLOCK_VALUE); // Certificate is still missing - assert_eq!(response, Err(Ctap1StatusCode::SW_COMMAND_ABORTED)); + assert_eq!(response, Err(Ctap1StatusCode::SW_INTERNAL_EXCEPTION)); let fake_cert = [0x99u8; 100]; // Arbitrary length assert!(ctap_state @@ -534,7 +479,7 @@ mod test { ctap_state.u2f_up_state.grant_up(START_CLOCK_VALUE); let response = Ctap1Command::process_command(&message, &mut ctap_state, TIMEOUT_CLOCK_VALUE); - assert_eq!(response, Err(Ctap1StatusCode::SW_CONDITIONS_NOT_SATISFIED)); + assert_eq!(response, Err(Ctap1StatusCode::SW_COND_USE_NOT_SATISFIED)); } #[test] @@ -552,7 +497,7 @@ mod test { let message = create_authenticate_message(&application, Ctap1Flags::CheckOnly, &key_handle); let response = Ctap1Command::process_command(&message, &mut ctap_state, START_CLOCK_VALUE); - assert_eq!(response, Err(Ctap1StatusCode::SW_CONDITIONS_NOT_SATISFIED)); + assert_eq!(response, Err(Ctap1StatusCode::SW_COND_USE_NOT_SATISFIED)); } #[test] @@ -626,7 +571,7 @@ mod test { message[0] = 0xEE; let response = Ctap1Command::process_command(&message, &mut ctap_state, START_CLOCK_VALUE); - assert_eq!(response, Err(Ctap1StatusCode::SW_CLA_NOT_SUPPORTED)); + assert_eq!(response, Err(Ctap1StatusCode::SW_CLA_INVALID)); } #[test] @@ -646,7 +591,7 @@ mod test { message[1] = 0xEE; let response = Ctap1Command::process_command(&message, &mut ctap_state, START_CLOCK_VALUE); - assert_eq!(response, Err(Ctap1StatusCode::SW_INS_NOT_SUPPORTED)); + assert_eq!(response, Err(Ctap1StatusCode::SW_INS_INVALID)); } #[test] @@ -768,6 +713,6 @@ mod test { ctap_state.u2f_up_state.grant_up(START_CLOCK_VALUE); let response = Ctap1Command::process_command(&message, &mut ctap_state, TIMEOUT_CLOCK_VALUE); - assert_eq!(response, Err(Ctap1StatusCode::SW_CONDITIONS_NOT_SATISFIED)); + assert_eq!(response, Err(Ctap1StatusCode::SW_COND_USE_NOT_SATISFIED)); } } diff --git a/src/ctap/hid/mod.rs b/src/ctap/hid/mod.rs index 3874792..7315449 100644 --- a/src/ctap/hid/mod.rs +++ b/src/ctap/hid/mod.rs @@ -417,7 +417,7 @@ impl CtapHid { #[cfg(feature = "with_ctap1")] fn ctap1_success_message(cid: ChannelID, payload: &[u8]) -> HidPacketIterator { let mut response = payload.to_vec(); - let code: u16 = ctap1::Ctap1StatusCode::SW_NO_ERROR.into(); + let code: u16 = ctap1::Ctap1StatusCode::SW_INTERNAL_EXCEPTION.into(); response.extend_from_slice(&code.to_be_bytes()); CtapHid::split_message(Message { cid, From a7eb38aac8b6b3400cebba9b09fefca1c04cfbb3 Mon Sep 17 00:00:00 2001 From: Kamran Khan Date: Thu, 10 Dec 2020 21:26:44 -0800 Subject: [PATCH 11/15] Use checked sub --- src/ctap/apdu.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/ctap/apdu.rs b/src/ctap/apdu.rs index 7a65fd2..66dde63 100644 --- a/src/ctap/apdu.rs +++ b/src/ctap/apdu.rs @@ -174,7 +174,8 @@ impl TryFrom<&[u8]> for APDU { return Err(ApduStatusCode::SW_WRONG_LENGTH); } - let extended_apdu_le_len: usize = payload.len() - extended_apdu_lc - 3; + let extended_apdu_le_len: usize = + payload.len().checked_sub(extended_apdu_lc + 3).unwrap_or(0); if extended_apdu_le_len > 3 { return Err(ApduStatusCode::SW_WRONG_LENGTH); } From f74d1b9ffd96d54315066058fa3a4656968c17df Mon Sep 17 00:00:00 2001 From: Kamran Khan Date: Thu, 10 Dec 2020 21:27:52 -0800 Subject: [PATCH 12/15] Return error when Le calculation overflows --- src/ctap/apdu.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/ctap/apdu.rs b/src/ctap/apdu.rs index 66dde63..d9344ec 100644 --- a/src/ctap/apdu.rs +++ b/src/ctap/apdu.rs @@ -174,8 +174,10 @@ impl TryFrom<&[u8]> for APDU { return Err(ApduStatusCode::SW_WRONG_LENGTH); } - let extended_apdu_le_len: usize = - payload.len().checked_sub(extended_apdu_lc + 3).unwrap_or(0); + let extended_apdu_le_len: usize = payload + .len() + .checked_sub(extended_apdu_lc + 3) + .unwrap_or(0xff); if extended_apdu_le_len > 3 { return Err(ApduStatusCode::SW_WRONG_LENGTH); } From 5882a6a3cc95348dc0b88bc0f68214e573bf1dd3 Mon Sep 17 00:00:00 2001 From: Kamran Khan Date: Thu, 10 Dec 2020 23:40:47 -0800 Subject: [PATCH 13/15] Fix ApduStatusCode->u16 implementation --- src/ctap/apdu.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/ctap/apdu.rs b/src/ctap/apdu.rs index d9344ec..ea59ac5 100644 --- a/src/ctap/apdu.rs +++ b/src/ctap/apdu.rs @@ -38,8 +38,8 @@ pub enum ApduStatusCode { } impl From for u16 { - fn from(_: ApduStatusCode) -> Self { - 0 + fn from(code: ApduStatusCode) -> Self { + code as u16 } } From dbbdddd58b83188dfca433c471c867a2458e0cf9 Mon Sep 17 00:00:00 2001 From: Kamran Khan Date: Mon, 14 Dec 2020 03:45:13 -0800 Subject: [PATCH 14/15] Fix error codes --- src/ctap/apdu.rs | 6 ++---- src/ctap/ctap1.rs | 10 +++++----- src/ctap/hid/mod.rs | 2 +- 3 files changed, 8 insertions(+), 10 deletions(-) diff --git a/src/ctap/apdu.rs b/src/ctap/apdu.rs index ea59ac5..14926ba 100644 --- a/src/ctap/apdu.rs +++ b/src/ctap/apdu.rs @@ -26,6 +26,7 @@ pub enum ApduStatusCode { /// Command successfully executed; 'XX' bytes of data are /// available and can be requested using GET RESPONSE. SW_GET_RESPONSE = 0x61_00, + SW_MEMERR = 0x65_01, SW_WRONG_DATA = 0x6a_80, SW_WRONG_LENGTH = 0x67_00, SW_COND_USE_NOT_SATISFIED = 0x69_85, @@ -177,10 +178,7 @@ impl TryFrom<&[u8]> for APDU { let extended_apdu_le_len: usize = payload .len() .checked_sub(extended_apdu_lc + 3) - .unwrap_or(0xff); - if extended_apdu_le_len > 3 { - return Err(ApduStatusCode::SW_WRONG_LENGTH); - } + .ok_or(ApduStatusCode::SW_WRONG_LENGTH)?; if byte_0 == 0 && extended_apdu_le_len <= 3 { // If first byte is zero AND the next two bytes can be parsed as a big-endian diff --git a/src/ctap/ctap1.rs b/src/ctap/ctap1.rs index 13f4819..cc15fa0 100644 --- a/src/ctap/ctap1.rs +++ b/src/ctap/ctap1.rs @@ -227,7 +227,7 @@ impl Ctap1Command { U2fCommand::Version => Ok(Vec::::from(super::U2F_VERSION_STRING)), // TODO: should we return an error instead such as SW_INS_NOT_SUPPORTED? - U2fCommand::VendorSpecific { .. } => Err(Ctap1StatusCode::SW_INTERNAL_EXCEPTION), + U2fCommand::VendorSpecific { .. } => Err(Ctap1StatusCode::SW_SUCCESS), } } @@ -258,13 +258,13 @@ impl Ctap1Command { .map_err(|_| Ctap1StatusCode::SW_INTERNAL_EXCEPTION)?; if key_handle.len() > 0xFF { // This is just being defensive with unreachable code. - return Err(Ctap1StatusCode::SW_WRONG_LENGTH); + return Err(Ctap1StatusCode::SW_INTERNAL_EXCEPTION); } let certificate = ctap_state .persistent_store .attestation_certificate() - .map_err(|_| Ctap1StatusCode::SW_INTERNAL_EXCEPTION)? + .map_err(|_| Ctap1StatusCode::SW_MEMERR)? .ok_or(Ctap1StatusCode::SW_INTERNAL_EXCEPTION)?; let private_key = ctap_state .persistent_store @@ -332,7 +332,7 @@ impl Ctap1Command { .map_err(|_| Ctap1StatusCode::SW_WRONG_DATA)?; let mut signature_data = ctap_state .generate_auth_data(&application, Ctap1Command::USER_PRESENCE_INDICATOR_BYTE) - .map_err(|_| Ctap1StatusCode::SW_COND_USE_NOT_SATISFIED)?; + .map_err(|_| Ctap1StatusCode::SW_WRONG_DATA)?; signature_data.extend(&challenge); let signature = credential_source .private_key @@ -542,7 +542,7 @@ mod test { message.push(0x00); let response = Ctap1Command::process_command(&message, &mut ctap_state, START_CLOCK_VALUE); - assert_eq!(response, Err(Ctap1StatusCode::SW_WRONG_LENGTH)); + assert_eq!(response, Err(Ctap1StatusCode::SW_INTERNAL_EXCEPTION)); } #[test] diff --git a/src/ctap/hid/mod.rs b/src/ctap/hid/mod.rs index 7315449..a36063b 100644 --- a/src/ctap/hid/mod.rs +++ b/src/ctap/hid/mod.rs @@ -417,7 +417,7 @@ impl CtapHid { #[cfg(feature = "with_ctap1")] fn ctap1_success_message(cid: ChannelID, payload: &[u8]) -> HidPacketIterator { let mut response = payload.to_vec(); - let code: u16 = ctap1::Ctap1StatusCode::SW_INTERNAL_EXCEPTION.into(); + let code: u16 = ctap1::Ctap1StatusCode::SW_SUCCESS.into(); response.extend_from_slice(&code.to_be_bytes()); CtapHid::split_message(Message { cid, From 35bdfe90ed52a5fac37d84b1ea6702b7d8c78001 Mon Sep 17 00:00:00 2001 From: Kamran Khan Date: Mon, 14 Dec 2020 04:54:25 -0800 Subject: [PATCH 15/15] Re-instate the length check for Le bytes --- src/ctap/apdu.rs | 3 +++ src/ctap/ctap1.rs | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/ctap/apdu.rs b/src/ctap/apdu.rs index 14926ba..c99bf84 100644 --- a/src/ctap/apdu.rs +++ b/src/ctap/apdu.rs @@ -179,6 +179,9 @@ impl TryFrom<&[u8]> for APDU { .len() .checked_sub(extended_apdu_lc + 3) .ok_or(ApduStatusCode::SW_WRONG_LENGTH)?; + if extended_apdu_le_len > 3 { + return Err(ApduStatusCode::SW_WRONG_LENGTH); + } if byte_0 == 0 && extended_apdu_le_len <= 3 { // If first byte is zero AND the next two bytes can be parsed as a big-endian diff --git a/src/ctap/ctap1.rs b/src/ctap/ctap1.rs index cc15fa0..0932e2c 100644 --- a/src/ctap/ctap1.rs +++ b/src/ctap/ctap1.rs @@ -542,7 +542,7 @@ mod test { message.push(0x00); let response = Ctap1Command::process_command(&message, &mut ctap_state, START_CLOCK_VALUE); - assert_eq!(response, Err(Ctap1StatusCode::SW_INTERNAL_EXCEPTION)); + assert_eq!(response, Err(Ctap1StatusCode::SW_WRONG_LENGTH)); } #[test]