From 58b5e4d8fab3d72443a58ea2aaabd624bc5df46a Mon Sep 17 00:00:00 2001 From: Mirna Date: Fri, 13 Nov 2020 09:32:59 +0200 Subject: [PATCH 01/23] Add short APDUs parser --- src/ctap/apdu.rs | 303 +++++++++++++++++++++++++++++++++++++++++++++++ src/ctap/mod.rs | 1 + 2 files changed, 304 insertions(+) create mode 100644 src/ctap/apdu.rs diff --git a/src/ctap/apdu.rs b/src/ctap/apdu.rs new file mode 100644 index 0000000..f324a03 --- /dev/null +++ b/src/ctap/apdu.rs @@ -0,0 +1,303 @@ +use alloc::vec::Vec; +use core::convert::TryFrom; + +type ByteArray = &'static [u8]; + +#[cfg_attr(test, derive(Clone, Debug))] +#[allow(non_camel_case_types)] +#[derive(PartialEq)] +pub enum ApduStatusCode { + SW_SUCCESS, + /// 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, + /// 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", + } + } +} + +#[allow(non_camel_case_types, dead_code)] +pub enum ApduIns { + SELECT = 0xA4, + READ_BINARY = 0xB0, + GET_RESPONSE = 0xC0, +} + +#[cfg_attr(test, derive(Clone, Debug))] +#[allow(dead_code)] +#[derive(Default, PartialEq)] +pub struct ApduHeader { + cla: u8, + ins: u8, + p1: u8, + p2: u8, +} + +impl From<&[u8]> for ApduHeader { + fn from(header: &[u8]) -> Self { + ApduHeader { + cla: header[0], + ins: header[1], + p1: header[2], + p2: header[3], + } + } +} + +#[cfg_attr(test, derive(Clone, Debug))] +#[allow(dead_code)] +#[derive(Default, PartialEq)] +pub struct APDU { + header: ApduHeader, + lc: u16, + data: Vec, + le: u32, + case_type: u8, +} + +const APDU_HEADER_LEN: u8 = 4; + +impl TryFrom<&[u8]> for APDU { + type Error = ApduStatusCode; + + fn try_from(frame: &[u8]) -> Result { + if frame.len() < APDU_HEADER_LEN as usize { + return Err(ApduStatusCode::SW_WRONG_DATA); + } + // +-----+-----+----+----+ + // header | CLA | INS | P1 | P2 | + // +-----+-----+----+----+ + let (header, payload) = frame.split_at(APDU_HEADER_LEN as usize); + + let mut apdu = APDU { + header: header.into(), + lc: 0x00, + data: Vec::new(), + le: 0x00, + case_type: 0x00, + }; + + // case 1 + if payload.is_empty() { + apdu.case_type = 0x01; + } else { + let byte_0 = payload[0]; + // case 2S (Le) + if payload.len() == 1 { + apdu.case_type = 0x02; + apdu.le = if byte_0 == 0x00 { + // Ne = 256 + 0x100 + } else { + byte_0.into() + } + } + // case 3S (Lc + data) + if payload.len() == (1 + byte_0) as usize && byte_0 != 0 { + apdu.case_type = 0x03; + apdu.lc = byte_0.into(); + apdu.data = payload[1..].to_vec(); + } + // case 4S (Lc + data + Le) + if payload.len() == (1 + byte_0 + 1) as usize && byte_0 != 0 { + apdu.case_type = 0x04; + apdu.lc = byte_0.into(); + apdu.data = payload[1..(payload.len() - 1)].to_vec(); + apdu.le = (*payload.last().unwrap()).into(); + if apdu.le == 0x00 { + apdu.le = 0x100; + } + } + } + // TODO: Add extended length cases + if apdu.case_type == 0x00 { + return Err(ApduStatusCode::SW_COND_USE_NOT_SATISFIED); + } + Ok(apdu) + } +} + +#[cfg(test)] +mod test { + use super::*; + + fn pass_frame(frame: &[u8]) -> Result { + APDU::try_from(frame) + } + + #[test] + fn test_case_type_1() { + let frame: [u8; 4] = [0x00, 0x12, 0x00, 0x80]; + let response = pass_frame(&frame); + assert!(response.is_ok()); + let expected = APDU { + header: ApduHeader { + cla: 0x00, + ins: 0x12, + p1: 0x00, + p2: 0x80, + }, + lc: 0x00, + data: Vec::new(), + le: 0x00, + case_type: 0x01, + }; + assert_eq!(expected, response.unwrap()); + } + + #[test] + fn test_case_type_2_short() { + let frame: [u8; 5] = [0x00, 0xb0, 0x00, 0x00, 0x0f]; + let response = pass_frame(&frame); + assert!(response.is_ok()); + let expected = APDU { + header: ApduHeader { + cla: 0x00, + ins: 0xb0, + p1: 0x00, + p2: 0x00, + }, + lc: 0x00, + data: Vec::new(), + le: 0x0f, + case_type: 0x02, + }; + assert_eq!(expected, response.unwrap()); + } + + #[test] + fn test_case_type_2_short_le() { + let frame: [u8; 5] = [0x00, 0xb0, 0x00, 0x00, 0x00]; + let response = pass_frame(&frame); + assert!(response.is_ok()); + let expected = APDU { + header: ApduHeader { + cla: 0x00, + ins: 0xb0, + p1: 0x00, + p2: 0x00, + }, + lc: 0x00, + data: Vec::new(), + le: 0x100, + case_type: 0x02, + }; + assert_eq!(expected, response.unwrap()); + } + + #[test] + fn test_case_type_3_short() { + let frame: [u8; 7] = [0x00, 0xa4, 0x00, 0x0c, 0x02, 0xe1, 0x04]; + let payload = [0xe1, 0x04]; + let response = pass_frame(&frame); + assert!(response.is_ok()); + let expected = APDU { + header: ApduHeader { + cla: 0x00, + ins: 0xa4, + p1: 0x00, + p2: 0x0c, + }, + lc: 0x02, + data: payload.to_vec(), + le: 0x00, + case_type: 0x03, + }; + assert_eq!(expected, response.unwrap()); + } + + #[test] + fn test_case_type_4_short() { + let frame: [u8; 13] = [ + 0x00, 0xa4, 0x04, 0x00, 0x07, 0xd2, 0x76, 0x00, 0x00, 0x85, 0x01, 0x01, 0xff, + ]; + let payload = [0xd2, 0x76, 0x00, 0x00, 0x85, 0x01, 0x01]; + let response = pass_frame(&frame); + assert!(response.is_ok()); + let expected = APDU { + header: ApduHeader { + cla: 0x00, + ins: 0xa4, + p1: 0x04, + p2: 0x00, + }, + lc: 0x07, + data: payload.to_vec(), + le: 0xff, + case_type: 0x04, + }; + assert_eq!(expected, response.unwrap()); + } + + #[test] + fn test_case_type_4_short_le() { + let frame: [u8; 13] = [ + 0x00, 0xa4, 0x04, 0x00, 0x07, 0xd2, 0x76, 0x00, 0x00, 0x85, 0x01, 0x01, 0x00, + ]; + let payload = [0xd2, 0x76, 0x00, 0x00, 0x85, 0x01, 0x01]; + let response = pass_frame(&frame); + assert!(response.is_ok()); + let expected = APDU { + header: ApduHeader { + cla: 0x00, + ins: 0xa4, + p1: 0x04, + p2: 0x00, + }, + lc: 0x07, + data: payload.to_vec(), + le: 0x100, + case_type: 0x04, + }; + assert_eq!(expected, response.unwrap()); + } + + #[test] + fn test_invalid_apdu_header_length() { + let frame: [u8; 3] = [0x00, 0x12, 0x00]; + let response = pass_frame(&frame); + assert!(response.is_err()); + assert_eq!(Some(ApduStatusCode::SW_WRONG_DATA), response.err()); + } + + #[test] + fn test_unsupported_case_type() { + let frame: [u8; 73] = [ + 0x00, 0x01, 0x03, 0x00, 0x00, 0x00, 0x40, 0xe3, 0x8f, 0xde, 0x51, 0x3d, 0xac, 0x9d, + 0x1c, 0x6e, 0x86, 0x76, 0x31, 0x40, 0x25, 0x96, 0x86, 0x4d, 0x29, 0xe8, 0x07, 0xb3, + 0x56, 0x19, 0xdf, 0x4a, 0x00, 0x02, 0xae, 0x2a, 0x8c, 0x9d, 0x5a, 0xab, 0xc3, 0x4b, + 0x4e, 0xb9, 0x78, 0xb9, 0x11, 0xe5, 0x52, 0x40, 0xf3, 0x45, 0x64, 0x9c, 0xd3, 0xd7, + 0xe8, 0xb5, 0x83, 0xfb, 0xe0, 0x66, 0x98, 0x4d, 0x98, 0x81, 0xf7, 0xb5, 0x49, 0x4d, + 0xcb, 0x00, 0x00, + ]; + let response = pass_frame(&frame); + assert!(response.is_err()); + assert_eq!( + Some(ApduStatusCode::SW_COND_USE_NOT_SATISFIED), + response.err() + ); + } +} diff --git a/src/ctap/mod.rs b/src/ctap/mod.rs index 1a98ce5..2551e7b 100644 --- a/src/ctap/mod.rs +++ b/src/ctap/mod.rs @@ -12,6 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. +pub mod apdu; pub mod command; #[cfg(feature = "with_ctap1")] mod ctap1; From fce91744c68abe1f41761500b563b4e10c612e80 Mon Sep 17 00:00:00 2001 From: Mirna Date: Fri, 13 Nov 2020 22:06:27 +0200 Subject: [PATCH 02/23] Addressing some of the requested changes --- src/ctap/apdu.rs | 98 +++++++++++++++++++++++++++--------------------- 1 file changed, 56 insertions(+), 42 deletions(-) diff --git a/src/ctap/apdu.rs b/src/ctap/apdu.rs index f324a03..431b18e 100644 --- a/src/ctap/apdu.rs +++ b/src/ctap/apdu.rs @@ -3,6 +3,8 @@ 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)] #[derive(PartialEq)] @@ -39,11 +41,11 @@ impl From for ByteArray { } } -#[allow(non_camel_case_types, dead_code)] -pub enum ApduIns { - SELECT = 0xA4, - READ_BINARY = 0xB0, - GET_RESPONSE = 0xC0, +#[allow(dead_code)] +pub enum ApduInstructions { + Select = 0xA4, + ReadBinary = 0xB0, + GetResponse = 0xC0, } #[cfg_attr(test, derive(Clone, Debug))] @@ -67,6 +69,33 @@ impl From<&[u8]> for ApduHeader { } } +#[cfg_attr(test, derive(Clone, Debug))] +#[derive(PartialEq)] +/// The APDU cases +pub enum Case { + Le, + LcData, + LcDataLe, + // TODO: More cases to add as extended length APDUs + // Le could be 2 or 3 Bytes +} + +#[cfg_attr(test, derive(Clone, Debug))] +#[allow(dead_code)] +#[derive(PartialEq)] +pub enum ApduType { + Instruction, + Short(Case), + Extended(Case), + Unknown, +} + +impl Default for ApduType { + fn default() -> ApduType { + ApduType::Unknown + } +} + #[cfg_attr(test, derive(Clone, Debug))] #[allow(dead_code)] #[derive(Default, PartialEq)] @@ -75,11 +104,9 @@ pub struct APDU { lc: u16, data: Vec, le: u32, - case_type: u8, + case_type: ApduType, } -const APDU_HEADER_LEN: u8 = 4; - impl TryFrom<&[u8]> for APDU { type Error = ApduStatusCode; @@ -90,24 +117,23 @@ impl TryFrom<&[u8]> for APDU { // +-----+-----+----+----+ // header | CLA | INS | P1 | P2 | // +-----+-----+----+----+ - let (header, payload) = frame.split_at(APDU_HEADER_LEN as usize); + let (header, payload) = frame.split_at(APDU_HEADER_LEN); let mut apdu = APDU { header: header.into(), lc: 0x00, data: Vec::new(), le: 0x00, - case_type: 0x00, + case_type: ApduType::default(), }; // case 1 if payload.is_empty() { - apdu.case_type = 0x01; + apdu.case_type = ApduType::Instruction; } else { let byte_0 = payload[0]; - // case 2S (Le) if payload.len() == 1 { - apdu.case_type = 0x02; + apdu.case_type = ApduType::Short(Case::Le); apdu.le = if byte_0 == 0x00 { // Ne = 256 0x100 @@ -115,15 +141,13 @@ impl TryFrom<&[u8]> for APDU { byte_0.into() } } - // case 3S (Lc + data) if payload.len() == (1 + byte_0) as usize && byte_0 != 0 { - apdu.case_type = 0x03; + apdu.case_type = ApduType::Short(Case::LcData); apdu.lc = byte_0.into(); apdu.data = payload[1..].to_vec(); } - // case 4S (Lc + data + Le) if payload.len() == (1 + byte_0 + 1) as usize && byte_0 != 0 { - apdu.case_type = 0x04; + apdu.case_type = ApduType::Short(Case::LcDataLe); apdu.lc = byte_0.into(); apdu.data = payload[1..(payload.len() - 1)].to_vec(); apdu.le = (*payload.last().unwrap()).into(); @@ -133,7 +157,7 @@ impl TryFrom<&[u8]> for APDU { } } // TODO: Add extended length cases - if apdu.case_type == 0x00 { + if apdu.case_type == ApduType::default() { return Err(ApduStatusCode::SW_COND_USE_NOT_SATISFIED); } Ok(apdu) @@ -163,16 +187,15 @@ mod test { lc: 0x00, data: Vec::new(), le: 0x00, - case_type: 0x01, + case_type: ApduType::Instruction, }; - assert_eq!(expected, response.unwrap()); + assert_eq!(Ok(expected), response); } #[test] fn test_case_type_2_short() { let frame: [u8; 5] = [0x00, 0xb0, 0x00, 0x00, 0x0f]; let response = pass_frame(&frame); - assert!(response.is_ok()); let expected = APDU { header: ApduHeader { cla: 0x00, @@ -183,16 +206,15 @@ mod test { lc: 0x00, data: Vec::new(), le: 0x0f, - case_type: 0x02, + case_type: ApduType::Short(Case::Le), }; - assert_eq!(expected, response.unwrap()); + assert_eq!(Ok(expected), response); } #[test] fn test_case_type_2_short_le() { let frame: [u8; 5] = [0x00, 0xb0, 0x00, 0x00, 0x00]; let response = pass_frame(&frame); - assert!(response.is_ok()); let expected = APDU { header: ApduHeader { cla: 0x00, @@ -203,9 +225,9 @@ mod test { lc: 0x00, data: Vec::new(), le: 0x100, - case_type: 0x02, + case_type: ApduType::Short(Case::Le), }; - assert_eq!(expected, response.unwrap()); + assert_eq!(Ok(expected), response); } #[test] @@ -213,7 +235,6 @@ mod test { let frame: [u8; 7] = [0x00, 0xa4, 0x00, 0x0c, 0x02, 0xe1, 0x04]; let payload = [0xe1, 0x04]; let response = pass_frame(&frame); - assert!(response.is_ok()); let expected = APDU { header: ApduHeader { cla: 0x00, @@ -224,9 +245,9 @@ mod test { lc: 0x02, data: payload.to_vec(), le: 0x00, - case_type: 0x03, + case_type: ApduType::Short(Case::LcData), }; - assert_eq!(expected, response.unwrap()); + assert_eq!(Ok(expected), response); } #[test] @@ -236,7 +257,6 @@ mod test { ]; let payload = [0xd2, 0x76, 0x00, 0x00, 0x85, 0x01, 0x01]; let response = pass_frame(&frame); - assert!(response.is_ok()); let expected = APDU { header: ApduHeader { cla: 0x00, @@ -247,9 +267,9 @@ mod test { lc: 0x07, data: payload.to_vec(), le: 0xff, - case_type: 0x04, + case_type: ApduType::Short(Case::LcDataLe), }; - assert_eq!(expected, response.unwrap()); + assert_eq!(Ok(expected), response); } #[test] @@ -259,7 +279,6 @@ mod test { ]; let payload = [0xd2, 0x76, 0x00, 0x00, 0x85, 0x01, 0x01]; let response = pass_frame(&frame); - assert!(response.is_ok()); let expected = APDU { header: ApduHeader { cla: 0x00, @@ -270,17 +289,16 @@ mod test { lc: 0x07, data: payload.to_vec(), le: 0x100, - case_type: 0x04, + case_type: ApduType::Short(Case::LcDataLe), }; - assert_eq!(expected, response.unwrap()); + assert_eq!(Ok(expected), response); } #[test] fn test_invalid_apdu_header_length() { let frame: [u8; 3] = [0x00, 0x12, 0x00]; let response = pass_frame(&frame); - assert!(response.is_err()); - assert_eq!(Some(ApduStatusCode::SW_WRONG_DATA), response.err()); + assert_eq!(Err(ApduStatusCode::SW_WRONG_DATA), response); } #[test] @@ -294,10 +312,6 @@ mod test { 0xcb, 0x00, 0x00, ]; let response = pass_frame(&frame); - assert!(response.is_err()); - assert_eq!( - Some(ApduStatusCode::SW_COND_USE_NOT_SATISFIED), - response.err() - ); + assert_eq!(Err(ApduStatusCode::SW_COND_USE_NOT_SATISFIED), response); } } From 5f5f72b6d13f31da3a54569f75d16e85f134451d Mon Sep 17 00:00:00 2001 From: Kamran Khan Date: Mon, 30 Nov 2020 02:04:52 -0800 Subject: [PATCH 03/23] Use arrayref for converting into ApduHeader --- src/ctap/apdu.rs | 6 +++--- src/lib.rs | 4 ++++ src/main.rs | 3 +++ 3 files changed, 10 insertions(+), 3 deletions(-) diff --git a/src/ctap/apdu.rs b/src/ctap/apdu.rs index 431b18e..fd03f2a 100644 --- a/src/ctap/apdu.rs +++ b/src/ctap/apdu.rs @@ -58,8 +58,8 @@ pub struct ApduHeader { p2: u8, } -impl From<&[u8]> for ApduHeader { - fn from(header: &[u8]) -> Self { +impl From<&[u8; 4]> for ApduHeader { + fn from(header: &[u8; 4]) -> Self { ApduHeader { cla: header[0], ins: header[1], @@ -120,7 +120,7 @@ impl TryFrom<&[u8]> for APDU { let (header, payload) = frame.split_at(APDU_HEADER_LEN); let mut apdu = APDU { - header: header.into(), + header: array_ref!(header, 0, 4).into(), lc: 0x00, data: Vec::new(), le: 0x00, diff --git a/src/lib.rs b/src/lib.rs index d6260c7..07fd02e 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -18,3 +18,7 @@ extern crate alloc; pub mod ctap; pub mod embedded_flash; + +#[macro_use] +extern crate arrayref; + diff --git a/src/main.rs b/src/main.rs index 51c8305..ca10c3a 100644 --- a/src/main.rs +++ b/src/main.rs @@ -19,6 +19,9 @@ extern crate alloc; extern crate core; extern crate lang_items; +#[macro_use] +extern crate arrayref; + mod ctap; pub mod embedded_flash; From f8a6fb35e2bf178e755106e5b4e00a72ab7d56d0 Mon Sep 17 00:00:00 2001 From: Kamran Khan Date: Mon, 30 Nov 2020 08:46:02 -0800 Subject: [PATCH 04/23] Ignore dirty submodules --- .gitmodules | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.gitmodules b/.gitmodules index b70a516..273601b 100644 --- a/.gitmodules +++ b/.gitmodules @@ -1,6 +1,8 @@ [submodule "third_party/libtock-rs"] path = third_party/libtock-rs url = https://github.com/tock/libtock-rs + ignore = dirty [submodule "third_party/tock"] path = third_party/tock url = https://github.com/tock/tock + ignore = dirty From 94f548d5c53195c08df5ec59ce26867060050861 Mon Sep 17 00:00:00 2001 From: Kamran Khan Date: Mon, 30 Nov 2020 14:35:01 -0800 Subject: [PATCH 05/23] Add extended APDU parser --- src/ctap/apdu.rs | 143 +++++++++++++++++++++++++++++++++++++++++------ src/main.rs | 2 +- 2 files changed, 127 insertions(+), 18 deletions(-) diff --git a/src/ctap/apdu.rs b/src/ctap/apdu.rs index fd03f2a..a761564 100644 --- a/src/ctap/apdu.rs +++ b/src/ctap/apdu.rs @@ -1,4 +1,5 @@ use alloc::vec::Vec; +use byteorder::{BigEndian, ByteOrder}; use core::convert::TryFrom; type ByteArray = &'static [u8]; @@ -73,11 +74,14 @@ impl From<&[u8; 4]> for ApduHeader { #[derive(PartialEq)] /// The APDU cases pub enum Case { - Le, - LcData, - LcDataLe, - // TODO: More cases to add as extended length APDUs - // Le could be 2 or 3 Bytes + Le1, + Lc1Data, + Lc1DataLe1, + Lc3Data, + Lc3DataLe1, + Lc3DataLe2, + Lc3DataLe3, + Unknown, } #[cfg_attr(test, derive(Clone, Debug))] @@ -127,13 +131,16 @@ impl TryFrom<&[u8]> for APDU { case_type: ApduType::default(), }; - // case 1 if payload.is_empty() { + // This branch is for Lc = 0 apdu.case_type = ApduType::Instruction; } else { + // Lc is not equal to zero, let's figure out how long it is let byte_0 = payload[0]; if payload.len() == 1 { - apdu.case_type = ApduType::Short(Case::Le); + // There is only one byte in the payload, that byte cannot be Lc because that would + // entail at *least* one another byte in the payload (for the command data) + apdu.case_type = ApduType::Short(Case::Le1); apdu.le = if byte_0 == 0x00 { // Ne = 256 0x100 @@ -142,12 +149,16 @@ impl TryFrom<&[u8]> for APDU { } } if payload.len() == (1 + byte_0) as usize && byte_0 != 0 { - apdu.case_type = ApduType::Short(Case::LcData); + // Lc is one-byte long and since the size specified by Lc covers the rest of the + // payload there's no Le at the end + apdu.case_type = ApduType::Short(Case::Lc1Data); apdu.lc = byte_0.into(); apdu.data = payload[1..].to_vec(); } if payload.len() == (1 + byte_0 + 1) as usize && byte_0 != 0 { - apdu.case_type = ApduType::Short(Case::LcDataLe); + // Lc is one-byte long and since the size specified by Lc covers the rest of the + // payload with ONE additional byte that byte must be Le + apdu.case_type = ApduType::Short(Case::Lc1DataLe1); apdu.lc = byte_0.into(); apdu.data = payload[1..(payload.len() - 1)].to_vec(); apdu.le = (*payload.last().unwrap()).into(); @@ -155,8 +166,38 @@ impl TryFrom<&[u8]> for APDU { apdu.le = 0x100; } } + 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 + }; + 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 + // have an extended-length APDU + apdu.case_type = ApduType::Extended(match extended_apdu_le_len { + 0 => Case::Lc3Data, + 1 => Case::Lc3DataLe1, + 2 => Case::Lc3DataLe2, + 3 => Case::Lc3DataLe3, + _ => Case::Unknown, + }); + apdu.data = payload[3..(payload.len() - extended_apdu_le_len)].to_vec(); + apdu.lc = extended_apdu_lc as u16; + apdu.le = match extended_apdu_le_len { + 0 => 0, + 1 => (*payload.last().unwrap()).into(), + 2 => BigEndian::read_u16(&payload[payload.len() - extended_apdu_le_len..]) + as u32, + 3 => BigEndian::read_u32(&payload[payload.len() - extended_apdu_le_len..]), + _ => 0, + } + } + } } - // TODO: Add extended length cases if apdu.case_type == ApduType::default() { return Err(ApduStatusCode::SW_COND_USE_NOT_SATISFIED); } @@ -206,7 +247,7 @@ mod test { lc: 0x00, data: Vec::new(), le: 0x0f, - case_type: ApduType::Short(Case::Le), + case_type: ApduType::Short(Case::Le1), }; assert_eq!(Ok(expected), response); } @@ -225,7 +266,7 @@ mod test { lc: 0x00, data: Vec::new(), le: 0x100, - case_type: ApduType::Short(Case::Le), + case_type: ApduType::Short(Case::Le1), }; assert_eq!(Ok(expected), response); } @@ -245,7 +286,7 @@ mod test { lc: 0x02, data: payload.to_vec(), le: 0x00, - case_type: ApduType::Short(Case::LcData), + case_type: ApduType::Short(Case::Lc1Data), }; assert_eq!(Ok(expected), response); } @@ -267,7 +308,7 @@ mod test { lc: 0x07, data: payload.to_vec(), le: 0xff, - case_type: ApduType::Short(Case::LcDataLe), + case_type: ApduType::Short(Case::Lc1DataLe1), }; assert_eq!(Ok(expected), response); } @@ -289,7 +330,7 @@ mod test { lc: 0x07, data: payload.to_vec(), le: 0x100, - case_type: ApduType::Short(Case::LcDataLe), + case_type: ApduType::Short(Case::Lc1DataLe1), }; assert_eq!(Ok(expected), response); } @@ -302,7 +343,56 @@ mod test { } #[test] - fn test_unsupported_case_type() { + fn test_extended_length_apdu() { + let frame: [u8; 186] = [ + 0x00, 0x02, 0x03, 0x00, 0x00, 0x00, 0xb1, 0x60, 0xc5, 0xb3, 0x42, 0x58, 0x6b, 0x49, + 0xdb, 0x3e, 0x72, 0xd8, 0x24, 0x4b, 0xa5, 0x6c, 0x8d, 0x79, 0x2b, 0x65, 0x08, 0xe8, + 0xda, 0x9b, 0x0e, 0x2b, 0xc1, 0x63, 0x0d, 0xbc, 0xf3, 0x6d, 0x66, 0xa5, 0x46, 0x72, + 0xb2, 0x22, 0xc4, 0xcf, 0x95, 0xe1, 0x51, 0xed, 0x8d, 0x4d, 0x3c, 0x76, 0x7a, 0x6c, + 0xc3, 0x49, 0x43, 0x59, 0x43, 0x79, 0x4e, 0x88, 0x4f, 0x3d, 0x02, 0x3a, 0x82, 0x29, + 0xfd, 0x70, 0x3f, 0x8b, 0xd4, 0xff, 0xe0, 0xa8, 0x93, 0xdf, 0x1a, 0x58, 0x34, 0x16, + 0xb0, 0x1b, 0x8e, 0xbc, 0xf0, 0x2d, 0xc9, 0x99, 0x8d, 0x6f, 0xe4, 0x8a, 0xb2, 0x70, + 0x9a, 0x70, 0x3a, 0x27, 0x71, 0x88, 0x3c, 0x75, 0x30, 0x16, 0xfb, 0x02, 0x11, 0x4d, + 0x30, 0x54, 0x6c, 0x4e, 0x8c, 0x76, 0xb2, 0xf0, 0xa8, 0x4e, 0xd6, 0x90, 0xe4, 0x40, + 0x25, 0x6a, 0xdd, 0x64, 0x63, 0x3e, 0x83, 0x4f, 0x8b, 0x25, 0xcf, 0x88, 0x68, 0x80, + 0x01, 0x07, 0xdb, 0xc8, 0x64, 0xf7, 0xca, 0x4f, 0xd1, 0xc7, 0x95, 0x7c, 0xe8, 0x45, + 0xbc, 0xda, 0xd4, 0xef, 0x45, 0x63, 0x5a, 0x7a, 0x65, 0x3f, 0xaa, 0x22, 0x67, 0xe7, + 0x8a, 0xf2, 0x5f, 0xe8, 0x59, 0x2e, 0x0b, 0xc6, 0x85, 0xc6, 0xf7, 0x0e, 0x9e, 0xdb, + 0xb6, 0x2b, 0x00, 0x00, + ]; + let payload = [ + 0x60, 0xc5, 0xb3, 0x42, 0x58, 0x6b, 0x49, 0xdb, 0x3e, 0x72, 0xd8, 0x24, 0x4b, 0xa5, + 0x6c, 0x8d, 0x79, 0x2b, 0x65, 0x08, 0xe8, 0xda, 0x9b, 0x0e, 0x2b, 0xc1, 0x63, 0x0d, + 0xbc, 0xf3, 0x6d, 0x66, 0xa5, 0x46, 0x72, 0xb2, 0x22, 0xc4, 0xcf, 0x95, 0xe1, 0x51, + 0xed, 0x8d, 0x4d, 0x3c, 0x76, 0x7a, 0x6c, 0xc3, 0x49, 0x43, 0x59, 0x43, 0x79, 0x4e, + 0x88, 0x4f, 0x3d, 0x02, 0x3a, 0x82, 0x29, 0xfd, 0x70, 0x3f, 0x8b, 0xd4, 0xff, 0xe0, + 0xa8, 0x93, 0xdf, 0x1a, 0x58, 0x34, 0x16, 0xb0, 0x1b, 0x8e, 0xbc, 0xf0, 0x2d, 0xc9, + 0x99, 0x8d, 0x6f, 0xe4, 0x8a, 0xb2, 0x70, 0x9a, 0x70, 0x3a, 0x27, 0x71, 0x88, 0x3c, + 0x75, 0x30, 0x16, 0xfb, 0x02, 0x11, 0x4d, 0x30, 0x54, 0x6c, 0x4e, 0x8c, 0x76, 0xb2, + 0xf0, 0xa8, 0x4e, 0xd6, 0x90, 0xe4, 0x40, 0x25, 0x6a, 0xdd, 0x64, 0x63, 0x3e, 0x83, + 0x4f, 0x8b, 0x25, 0xcf, 0x88, 0x68, 0x80, 0x01, 0x07, 0xdb, 0xc8, 0x64, 0xf7, 0xca, + 0x4f, 0xd1, 0xc7, 0x95, 0x7c, 0xe8, 0x45, 0xbc, 0xda, 0xd4, 0xef, 0x45, 0x63, 0x5a, + 0x7a, 0x65, 0x3f, 0xaa, 0x22, 0x67, 0xe7, 0x8a, 0xf2, 0x5f, 0xe8, 0x59, 0x2e, 0x0b, + 0xc6, 0x85, 0xc6, 0xf7, 0x0e, 0x9e, 0xdb, 0xb6, 0x2b, + ]; + let response = pass_frame(&frame); + let expected = APDU { + header: ApduHeader { + cla: 0x00, + ins: 0x02, + p1: 0x03, + p2: 0x00, + }, + lc: 0xb1, + data: payload.to_vec(), + le: 0x00, + case_type: ApduType::Extended(Case::Lc3DataLe2), + }; + assert_eq!(Ok(expected), response); + } + + #[test] + fn test_previously_unsupported_case_type() { let frame: [u8; 73] = [ 0x00, 0x01, 0x03, 0x00, 0x00, 0x00, 0x40, 0xe3, 0x8f, 0xde, 0x51, 0x3d, 0xac, 0x9d, 0x1c, 0x6e, 0x86, 0x76, 0x31, 0x40, 0x25, 0x96, 0x86, 0x4d, 0x29, 0xe8, 0x07, 0xb3, @@ -311,7 +401,26 @@ mod test { 0xe8, 0xb5, 0x83, 0xfb, 0xe0, 0x66, 0x98, 0x4d, 0x98, 0x81, 0xf7, 0xb5, 0x49, 0x4d, 0xcb, 0x00, 0x00, ]; + let payload = [ + 0xe3, 0x8f, 0xde, 0x51, 0x3d, 0xac, 0x9d, 0x1c, 0x6e, 0x86, 0x76, 0x31, 0x40, 0x25, + 0x96, 0x86, 0x4d, 0x29, 0xe8, 0x07, 0xb3, 0x56, 0x19, 0xdf, 0x4a, 0x00, 0x02, 0xae, + 0x2a, 0x8c, 0x9d, 0x5a, 0xab, 0xc3, 0x4b, 0x4e, 0xb9, 0x78, 0xb9, 0x11, 0xe5, 0x52, + 0x40, 0xf3, 0x45, 0x64, 0x9c, 0xd3, 0xd7, 0xe8, 0xb5, 0x83, 0xfb, 0xe0, 0x66, 0x98, + 0x4d, 0x98, 0x81, 0xf7, 0xb5, 0x49, 0x4d, 0xcb, + ]; let response = pass_frame(&frame); - assert_eq!(Err(ApduStatusCode::SW_COND_USE_NOT_SATISFIED), response); + let expected = APDU { + header: ApduHeader { + cla: 0x00, + ins: 0x01, + p1: 0x03, + p2: 0x00, + }, + lc: 0x40, + data: payload.to_vec(), + le: 0x00, + case_type: ApduType::Extended(Case::Lc3DataLe2), + }; + assert_eq!(Ok(expected), response); } } diff --git a/src/main.rs b/src/main.rs index ca10c3a..2ca12a8 100644 --- a/src/main.rs +++ b/src/main.rs @@ -18,9 +18,9 @@ extern crate alloc; #[cfg(feature = "std")] extern crate core; extern crate lang_items; - #[macro_use] extern crate arrayref; +extern crate byteorder; mod ctap; pub mod embedded_flash; From ce46af0b6b9897bfd5287f98033d3efbffc449f3 Mon Sep 17 00:00:00 2001 From: Kamran Khan Date: Mon, 30 Nov 2020 14:43:44 -0800 Subject: [PATCH 06/23] Make cargo fmt happy --- src/lib.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/lib.rs b/src/lib.rs index 07fd02e..a5c4cba 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -21,4 +21,3 @@ pub mod embedded_flash; #[macro_use] extern crate arrayref; - From dc95310fc0e58418b546c2f9def1f70a06e515fa Mon Sep 17 00:00:00 2001 From: Kamran Khan Date: Tue, 1 Dec 2020 10:13:25 -0800 Subject: [PATCH 07/23] Clarify comments --- 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 a761564..dd95caa 100644 --- a/src/ctap/apdu.rs +++ b/src/ctap/apdu.rs @@ -132,10 +132,10 @@ impl TryFrom<&[u8]> for APDU { }; if payload.is_empty() { - // This branch is for Lc = 0 + // Lc is zero-bytes in length apdu.case_type = ApduType::Instruction; } else { - // Lc is not equal to zero, let's figure out how long it is + // Lc is not zero-bytes in length, let's figure out how long it is let byte_0 = payload[0]; if payload.len() == 1 { // There is only one byte in the payload, that byte cannot be Lc because that would From b9ffe7e4ce0664a0ebe4e5088efbbaa16a75e1a0 Mon Sep 17 00:00:00 2001 From: Kamran Khan Date: Wed, 2 Dec 2020 23:02:07 -0800 Subject: [PATCH 08/23] Use constant instead of hardcoded integer --- 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 dd95caa..5783cd7 100644 --- a/src/ctap/apdu.rs +++ b/src/ctap/apdu.rs @@ -59,8 +59,8 @@ pub struct ApduHeader { p2: u8, } -impl From<&[u8; 4]> for ApduHeader { - fn from(header: &[u8; 4]) -> Self { +impl From<&[u8; APDU_HEADER_LEN]> for ApduHeader { + fn from(header: &[u8; APDU_HEADER_LEN]) -> Self { ApduHeader { cla: header[0], ins: header[1], From 2c49718fee700d349e0f5df9d1079823eca5259b Mon Sep 17 00:00:00 2001 From: Kamran Khan Date: Wed, 2 Dec 2020 23:03:35 -0800 Subject: [PATCH 09/23] Lc3DataLe3 is not a valid case --- 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 5783cd7..6c2a03a 100644 --- a/src/ctap/apdu.rs +++ b/src/ctap/apdu.rs @@ -80,7 +80,7 @@ pub enum Case { Lc3Data, Lc3DataLe1, Lc3DataLe2, - Lc3DataLe3, + Le3, Unknown, } From 0420ad8de6ebb9157202dd5333d1b49e012ed79a Mon Sep 17 00:00:00 2001 From: Kamran Khan Date: Wed, 2 Dec 2020 23:06:24 -0800 Subject: [PATCH 10/23] Use constant for consistency --- 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 6c2a03a..d5210b1 100644 --- a/src/ctap/apdu.rs +++ b/src/ctap/apdu.rs @@ -124,7 +124,7 @@ impl TryFrom<&[u8]> for APDU { let (header, payload) = frame.split_at(APDU_HEADER_LEN); let mut apdu = APDU { - header: array_ref!(header, 0, 4).into(), + header: array_ref!(header, 0, APDU_HEADER_LEN).into(), lc: 0x00, data: Vec::new(), le: 0x00, From 1d8c103d9b8180b93994e35ea9365c780e15803b Mon Sep 17 00:00:00 2001 From: Kamran Khan Date: Wed, 2 Dec 2020 23:29:11 -0800 Subject: [PATCH 11/23] Construct and return immutable instances of APDU instead of mutating one --- src/ctap/apdu.rs | 110 +++++++++++++++++++++++++++-------------------- 1 file changed, 64 insertions(+), 46 deletions(-) diff --git a/src/ctap/apdu.rs b/src/ctap/apdu.rs index d5210b1..80be816 100644 --- a/src/ctap/apdu.rs +++ b/src/ctap/apdu.rs @@ -123,48 +123,56 @@ impl TryFrom<&[u8]> for APDU { // +-----+-----+----+----+ let (header, payload) = frame.split_at(APDU_HEADER_LEN); - let mut apdu = APDU { - header: array_ref!(header, 0, APDU_HEADER_LEN).into(), - lc: 0x00, - data: Vec::new(), - le: 0x00, - case_type: ApduType::default(), - }; - if payload.is_empty() { // Lc is zero-bytes in length - apdu.case_type = ApduType::Instruction; + return Ok(APDU { + header: array_ref!(header, 0, APDU_HEADER_LEN).into(), + lc: 0x00, + data: Vec::new(), + le: 0x00, + case_type: ApduType::Instruction, + }); } else { // Lc is not zero-bytes in length, let's figure out how long it is let byte_0 = payload[0]; if payload.len() == 1 { // There is only one byte in the payload, that byte cannot be Lc because that would // entail at *least* one another byte in the payload (for the command data) - apdu.case_type = ApduType::Short(Case::Le1); - apdu.le = if byte_0 == 0x00 { - // Ne = 256 - 0x100 - } else { - byte_0.into() - } + return Ok(APDU { + header: array_ref!(header, 0, APDU_HEADER_LEN).into(), + lc: 0x00, + data: Vec::new(), + le: if byte_0 == 0x00 { + // Ne = 256 + 0x100 + } else { + byte_0.into() + }, + case_type: ApduType::Short(Case::Le1), + }); } if payload.len() == (1 + byte_0) as usize && byte_0 != 0 { // Lc is one-byte long and since the size specified by Lc covers the rest of the // payload there's no Le at the end - apdu.case_type = ApduType::Short(Case::Lc1Data); - apdu.lc = byte_0.into(); - apdu.data = payload[1..].to_vec(); + return Ok(APDU { + header: array_ref!(header, 0, APDU_HEADER_LEN).into(), + lc: byte_0.into(), + data: payload[1..].to_vec(), + case_type: ApduType::Short(Case::Lc1Data), + le: 0, + }); } if payload.len() == (1 + byte_0 + 1) as usize && byte_0 != 0 { // Lc is one-byte long and since the size specified by Lc covers the rest of the // payload with ONE additional byte that byte must be Le - apdu.case_type = ApduType::Short(Case::Lc1DataLe1); - apdu.lc = byte_0.into(); - apdu.data = payload[1..(payload.len() - 1)].to_vec(); - apdu.le = (*payload.last().unwrap()).into(); - if apdu.le == 0x00 { - apdu.le = 0x100; - } + let last_byte: u32 = (*payload.last().unwrap()).into(); + return Ok(APDU { + header: array_ref!(header, 0, APDU_HEADER_LEN).into(), + lc: byte_0.into(), + data: payload[1..(payload.len() - 1)].to_vec(), + le: if last_byte == 0x00 { 0x100 } else { last_byte }, + case_type: ApduType::Short(Case::Lc1DataLe1), + }); } if payload.len() > 2 { // Lc is possibly three-bytes long @@ -178,30 +186,40 @@ impl TryFrom<&[u8]> for APDU { // 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 // have an extended-length APDU - apdu.case_type = ApduType::Extended(match extended_apdu_le_len { - 0 => Case::Lc3Data, - 1 => Case::Lc3DataLe1, - 2 => Case::Lc3DataLe2, - 3 => Case::Lc3DataLe3, - _ => Case::Unknown, + let last_byte: u32 = (*payload.last().unwrap()).into(); + return Ok(APDU { + header: array_ref!(header, 0, APDU_HEADER_LEN).into(), + lc: extended_apdu_lc as u16, + data: payload[3..(payload.len() - extended_apdu_le_len)].to_vec(), + le: match extended_apdu_le_len { + 0 => 0, + 1 => { + if last_byte == 0x00 { + 0x100 + } else { + last_byte + } + } + 2 => BigEndian::read_u16( + &payload[payload.len() - extended_apdu_le_len..], + ) as u32, + 3 => BigEndian::read_u32( + &payload[payload.len() - extended_apdu_le_len..], + ), + _ => 0, + }, + case_type: ApduType::Extended(match extended_apdu_le_len { + 0 => Case::Lc3Data, + 1 => Case::Lc3DataLe1, + 2 => Case::Lc3DataLe2, + 3 => Case::Le3, + _ => Case::Unknown, + }), }); - apdu.data = payload[3..(payload.len() - extended_apdu_le_len)].to_vec(); - apdu.lc = extended_apdu_lc as u16; - apdu.le = match extended_apdu_le_len { - 0 => 0, - 1 => (*payload.last().unwrap()).into(), - 2 => BigEndian::read_u16(&payload[payload.len() - extended_apdu_le_len..]) - as u32, - 3 => BigEndian::read_u32(&payload[payload.len() - extended_apdu_le_len..]), - _ => 0, - } } } } - if apdu.case_type == ApduType::default() { - return Err(ApduStatusCode::SW_COND_USE_NOT_SATISFIED); - } - Ok(apdu) + return Err(ApduStatusCode::SW_COND_USE_NOT_SATISFIED); } } From 524ebe3fce0877dc019fb504d6c98aab31d751d3 Mon Sep 17 00:00:00 2001 From: Kamran Khan Date: Wed, 2 Dec 2020 23:32:25 -0800 Subject: [PATCH 12/23] Prevent int overflow by casting before addition --- 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 80be816..8e53276 100644 --- a/src/ctap/apdu.rs +++ b/src/ctap/apdu.rs @@ -151,7 +151,7 @@ impl TryFrom<&[u8]> for APDU { case_type: ApduType::Short(Case::Le1), }); } - if payload.len() == (1 + byte_0) as usize && byte_0 != 0 { + if payload.len() == 1 + (byte_0 as usize) && byte_0 != 0 { // Lc is one-byte long and since the size specified by Lc covers the rest of the // payload there's no Le at the end return Ok(APDU { @@ -162,7 +162,7 @@ impl TryFrom<&[u8]> for APDU { le: 0, }); } - if payload.len() == (1 + byte_0 + 1) as usize && byte_0 != 0 { + if payload.len() == 2 + (byte_0 as usize) && byte_0 != 0 { // Lc is one-byte long and since the size specified by Lc covers the rest of the // payload with ONE additional byte that byte must be Le let last_byte: u32 = (*payload.last().unwrap()).into(); From 9fc1ac114d99bc7cc2b57c72103e042765b09fbe Mon Sep 17 00:00:00 2001 From: Kamran Khan Date: Wed, 2 Dec 2020 23:39:48 -0800 Subject: [PATCH 13/23] Reuse frame bytes for payload --- src/ctap/apdu.rs | 24 ++---------------------- 1 file changed, 2 insertions(+), 22 deletions(-) diff --git a/src/ctap/apdu.rs b/src/ctap/apdu.rs index 8e53276..6d06dc5 100644 --- a/src/ctap/apdu.rs +++ b/src/ctap/apdu.rs @@ -378,21 +378,7 @@ mod test { 0x8a, 0xf2, 0x5f, 0xe8, 0x59, 0x2e, 0x0b, 0xc6, 0x85, 0xc6, 0xf7, 0x0e, 0x9e, 0xdb, 0xb6, 0x2b, 0x00, 0x00, ]; - let payload = [ - 0x60, 0xc5, 0xb3, 0x42, 0x58, 0x6b, 0x49, 0xdb, 0x3e, 0x72, 0xd8, 0x24, 0x4b, 0xa5, - 0x6c, 0x8d, 0x79, 0x2b, 0x65, 0x08, 0xe8, 0xda, 0x9b, 0x0e, 0x2b, 0xc1, 0x63, 0x0d, - 0xbc, 0xf3, 0x6d, 0x66, 0xa5, 0x46, 0x72, 0xb2, 0x22, 0xc4, 0xcf, 0x95, 0xe1, 0x51, - 0xed, 0x8d, 0x4d, 0x3c, 0x76, 0x7a, 0x6c, 0xc3, 0x49, 0x43, 0x59, 0x43, 0x79, 0x4e, - 0x88, 0x4f, 0x3d, 0x02, 0x3a, 0x82, 0x29, 0xfd, 0x70, 0x3f, 0x8b, 0xd4, 0xff, 0xe0, - 0xa8, 0x93, 0xdf, 0x1a, 0x58, 0x34, 0x16, 0xb0, 0x1b, 0x8e, 0xbc, 0xf0, 0x2d, 0xc9, - 0x99, 0x8d, 0x6f, 0xe4, 0x8a, 0xb2, 0x70, 0x9a, 0x70, 0x3a, 0x27, 0x71, 0x88, 0x3c, - 0x75, 0x30, 0x16, 0xfb, 0x02, 0x11, 0x4d, 0x30, 0x54, 0x6c, 0x4e, 0x8c, 0x76, 0xb2, - 0xf0, 0xa8, 0x4e, 0xd6, 0x90, 0xe4, 0x40, 0x25, 0x6a, 0xdd, 0x64, 0x63, 0x3e, 0x83, - 0x4f, 0x8b, 0x25, 0xcf, 0x88, 0x68, 0x80, 0x01, 0x07, 0xdb, 0xc8, 0x64, 0xf7, 0xca, - 0x4f, 0xd1, 0xc7, 0x95, 0x7c, 0xe8, 0x45, 0xbc, 0xda, 0xd4, 0xef, 0x45, 0x63, 0x5a, - 0x7a, 0x65, 0x3f, 0xaa, 0x22, 0x67, 0xe7, 0x8a, 0xf2, 0x5f, 0xe8, 0x59, 0x2e, 0x0b, - 0xc6, 0x85, 0xc6, 0xf7, 0x0e, 0x9e, 0xdb, 0xb6, 0x2b, - ]; + let payload = array_ref!(frame, 7, 186 - 7 - 2); let response = pass_frame(&frame); let expected = APDU { header: ApduHeader { @@ -419,13 +405,7 @@ mod test { 0xe8, 0xb5, 0x83, 0xfb, 0xe0, 0x66, 0x98, 0x4d, 0x98, 0x81, 0xf7, 0xb5, 0x49, 0x4d, 0xcb, 0x00, 0x00, ]; - let payload = [ - 0xe3, 0x8f, 0xde, 0x51, 0x3d, 0xac, 0x9d, 0x1c, 0x6e, 0x86, 0x76, 0x31, 0x40, 0x25, - 0x96, 0x86, 0x4d, 0x29, 0xe8, 0x07, 0xb3, 0x56, 0x19, 0xdf, 0x4a, 0x00, 0x02, 0xae, - 0x2a, 0x8c, 0x9d, 0x5a, 0xab, 0xc3, 0x4b, 0x4e, 0xb9, 0x78, 0xb9, 0x11, 0xe5, 0x52, - 0x40, 0xf3, 0x45, 0x64, 0x9c, 0xd3, 0xd7, 0xe8, 0xb5, 0x83, 0xfb, 0xe0, 0x66, 0x98, - 0x4d, 0x98, 0x81, 0xf7, 0xb5, 0x49, 0x4d, 0xcb, - ]; + let payload = array_ref!(frame, 7, 73 - 7 - 2); let response = pass_frame(&frame); let expected = APDU { header: ApduHeader { From 943d7af5039656acb551df7af0172344e1791802 Mon Sep 17 00:00:00 2001 From: Kamran Khan Date: Wed, 2 Dec 2020 23:43:35 -0800 Subject: [PATCH 14/23] Payload does not need to be an array --- 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 6d06dc5..8cff770 100644 --- a/src/ctap/apdu.rs +++ b/src/ctap/apdu.rs @@ -378,7 +378,7 @@ mod test { 0x8a, 0xf2, 0x5f, 0xe8, 0x59, 0x2e, 0x0b, 0xc6, 0x85, 0xc6, 0xf7, 0x0e, 0x9e, 0xdb, 0xb6, 0x2b, 0x00, 0x00, ]; - let payload = array_ref!(frame, 7, 186 - 7 - 2); + let payload: &[u8] = &frame[7..frame.len() - 2]; let response = pass_frame(&frame); let expected = APDU { header: ApduHeader { @@ -405,7 +405,7 @@ mod test { 0xe8, 0xb5, 0x83, 0xfb, 0xe0, 0x66, 0x98, 0x4d, 0x98, 0x81, 0xf7, 0xb5, 0x49, 0x4d, 0xcb, 0x00, 0x00, ]; - let payload = array_ref!(frame, 7, 73 - 7 - 2); + let payload: &[u8] = &frame[7..frame.len() - 2]; let response = pass_frame(&frame); let expected = APDU { header: ApduHeader { From 71ec2cf937260c075222960405f478108cbe0106 Mon Sep 17 00:00:00 2001 From: Kamran Khan Date: Thu, 3 Dec 2020 07:50:05 -0800 Subject: [PATCH 15/23] Return an error when the case isn't determined --- src/ctap/apdu.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/ctap/apdu.rs b/src/ctap/apdu.rs index 8cff770..07cf136 100644 --- a/src/ctap/apdu.rs +++ b/src/ctap/apdu.rs @@ -81,7 +81,6 @@ pub enum Case { Lc3DataLe1, Lc3DataLe2, Le3, - Unknown, } #[cfg_attr(test, derive(Clone, Debug))] @@ -213,7 +212,7 @@ impl TryFrom<&[u8]> for APDU { 1 => Case::Lc3DataLe1, 2 => Case::Lc3DataLe2, 3 => Case::Le3, - _ => Case::Unknown, + _ => return Err(ApduStatusCode::SW_COND_USE_NOT_SATISFIED), }), }); } From 69cdd4a0dc3c1bb6967afc89c752c966ce6b8d99 Mon Sep 17 00:00:00 2001 From: Kamran Khan Date: Thu, 3 Dec 2020 07:53:22 -0800 Subject: [PATCH 16/23] Use (relatively more) appropriate error code) --- src/ctap/apdu.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/ctap/apdu.rs b/src/ctap/apdu.rs index 07cf136..db4653f 100644 --- a/src/ctap/apdu.rs +++ b/src/ctap/apdu.rs @@ -205,20 +205,20 @@ impl TryFrom<&[u8]> for APDU { 3 => BigEndian::read_u32( &payload[payload.len() - extended_apdu_le_len..], ), - _ => 0, + _ => return Err(ApduStatusCode::SW_INTERNAL_EXCEPTION), }, case_type: ApduType::Extended(match extended_apdu_le_len { 0 => Case::Lc3Data, 1 => Case::Lc3DataLe1, 2 => Case::Lc3DataLe2, 3 => Case::Le3, - _ => return Err(ApduStatusCode::SW_COND_USE_NOT_SATISFIED), + _ => return Err(ApduStatusCode::SW_INTERNAL_EXCEPTION), }), }); } } } - return Err(ApduStatusCode::SW_COND_USE_NOT_SATISFIED); + return Err(ApduStatusCode::SW_INTERNAL_EXCEPTION); } } From cc8bdb982d327275abc73a5c80dcfa84b22220f8 Mon Sep 17 00:00:00 2001 From: Kamran Khan Date: Thu, 3 Dec 2020 07:55:34 -0800 Subject: [PATCH 17/23] Remove unknown apdu type --- src/ctap/apdu.rs | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/src/ctap/apdu.rs b/src/ctap/apdu.rs index db4653f..0afe6b7 100644 --- a/src/ctap/apdu.rs +++ b/src/ctap/apdu.rs @@ -90,18 +90,11 @@ pub enum ApduType { Instruction, Short(Case), Extended(Case), - Unknown, -} - -impl Default for ApduType { - fn default() -> ApduType { - ApduType::Unknown - } } #[cfg_attr(test, derive(Clone, Debug))] #[allow(dead_code)] -#[derive(Default, PartialEq)] +#[derive(PartialEq)] pub struct APDU { header: ApduHeader, lc: u16, From bec94f02be79c4ae520ef97acc7b5899d352cf14 Mon Sep 17 00:00:00 2001 From: Kamran Khan Date: Thu, 3 Dec 2020 08:10:44 -0800 Subject: [PATCH 18/23] Tweak Le appropriately depending on its swize --- src/ctap/apdu.rs | 31 +++++++++++++++++++++++-------- 1 file changed, 23 insertions(+), 8 deletions(-) diff --git a/src/ctap/apdu.rs b/src/ctap/apdu.rs index 0afe6b7..17677c9 100644 --- a/src/ctap/apdu.rs +++ b/src/ctap/apdu.rs @@ -192,12 +192,27 @@ impl TryFrom<&[u8]> for APDU { last_byte } } - 2 => BigEndian::read_u16( - &payload[payload.len() - extended_apdu_le_len..], - ) as u32, - 3 => BigEndian::read_u32( - &payload[payload.len() - extended_apdu_le_len..], - ), + 2 => { + let le_parsed = BigEndian::read_u16(&payload[payload.len() - 2..]); + if le_parsed == 0x00 { + 0x100 + } else { + le_parsed as u32 + } + } + 3 => { + let le_first_byte: u32 = + (*payload.get(payload.len() - 3).unwrap()).into(); + if le_first_byte != 0x00 { + return Err(ApduStatusCode::SW_INTERNAL_EXCEPTION); + } + let le_parsed = BigEndian::read_u16(&payload[payload.len() - 2..]); + if le_parsed == 0x00 { + 0x10000 + } else { + le_parsed as u32 + } + } _ => return Err(ApduStatusCode::SW_INTERNAL_EXCEPTION), }, case_type: ApduType::Extended(match extended_apdu_le_len { @@ -381,7 +396,7 @@ mod test { }, lc: 0xb1, data: payload.to_vec(), - le: 0x00, + le: 0x100, case_type: ApduType::Extended(Case::Lc3DataLe2), }; assert_eq!(Ok(expected), response); @@ -408,7 +423,7 @@ mod test { }, lc: 0x40, data: payload.to_vec(), - le: 0x00, + le: 0x100, case_type: ApduType::Extended(Case::Lc3DataLe2), }; assert_eq!(Ok(expected), response); From 4bfce88e9b056d3ba2e79c081607dbf9fde00ca6 Mon Sep 17 00:00:00 2001 From: Kamran Khan Date: Thu, 3 Dec 2020 08:14:07 -0800 Subject: [PATCH 19/23] Remove indention level made redundant by early-return --- src/ctap/apdu.rs | 192 +++++++++++++++++++++++------------------------ 1 file changed, 96 insertions(+), 96 deletions(-) diff --git a/src/ctap/apdu.rs b/src/ctap/apdu.rs index 17677c9..6a94790 100644 --- a/src/ctap/apdu.rs +++ b/src/ctap/apdu.rs @@ -124,108 +124,108 @@ impl TryFrom<&[u8]> for APDU { le: 0x00, case_type: ApduType::Instruction, }); - } else { - // Lc is not zero-bytes in length, let's figure out how long it is - let byte_0 = payload[0]; - if payload.len() == 1 { - // There is only one byte in the payload, that byte cannot be Lc because that would - // entail at *least* one another byte in the payload (for the command data) - return Ok(APDU { - header: array_ref!(header, 0, APDU_HEADER_LEN).into(), - lc: 0x00, - data: Vec::new(), - le: if byte_0 == 0x00 { - // Ne = 256 - 0x100 - } else { - byte_0.into() - }, - case_type: ApduType::Short(Case::Le1), - }); - } - if payload.len() == 1 + (byte_0 as usize) && byte_0 != 0 { - // Lc is one-byte long and since the size specified by Lc covers the rest of the - // payload there's no Le at the end - return Ok(APDU { - header: array_ref!(header, 0, APDU_HEADER_LEN).into(), - lc: byte_0.into(), - data: payload[1..].to_vec(), - case_type: ApduType::Short(Case::Lc1Data), - le: 0, - }); - } - if payload.len() == 2 + (byte_0 as usize) && byte_0 != 0 { - // Lc is one-byte long and since the size specified by Lc covers the rest of the - // payload with ONE additional byte that byte must be Le + } + // Lc is not zero-bytes in length, let's figure out how long it is + let byte_0 = payload[0]; + if payload.len() == 1 { + // There is only one byte in the payload, that byte cannot be Lc because that would + // entail at *least* one another byte in the payload (for the command data) + return Ok(APDU { + header: array_ref!(header, 0, APDU_HEADER_LEN).into(), + lc: 0x00, + data: Vec::new(), + le: if byte_0 == 0x00 { + // Ne = 256 + 0x100 + } else { + byte_0.into() + }, + case_type: ApduType::Short(Case::Le1), + }); + } + if payload.len() == 1 + (byte_0 as usize) && byte_0 != 0 { + // Lc is one-byte long and since the size specified by Lc covers the rest of the + // payload there's no Le at the end + return Ok(APDU { + header: array_ref!(header, 0, APDU_HEADER_LEN).into(), + lc: byte_0.into(), + data: payload[1..].to_vec(), + case_type: ApduType::Short(Case::Lc1Data), + le: 0, + }); + } + if payload.len() == 2 + (byte_0 as usize) && byte_0 != 0 { + // Lc is one-byte long and since the size specified by Lc covers the rest of the + // payload with ONE additional byte that byte must be Le + let last_byte: u32 = (*payload.last().unwrap()).into(); + return Ok(APDU { + header: array_ref!(header, 0, APDU_HEADER_LEN).into(), + lc: byte_0.into(), + data: payload[1..(payload.len() - 1)].to_vec(), + le: if last_byte == 0x00 { 0x100 } else { last_byte }, + case_type: ApduType::Short(Case::Lc1DataLe1), + }); + } + 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 + }; + 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 + // have an extended-length APDU let last_byte: u32 = (*payload.last().unwrap()).into(); return Ok(APDU { header: array_ref!(header, 0, APDU_HEADER_LEN).into(), - lc: byte_0.into(), - data: payload[1..(payload.len() - 1)].to_vec(), - le: if last_byte == 0x00 { 0x100 } else { last_byte }, - case_type: ApduType::Short(Case::Lc1DataLe1), + lc: extended_apdu_lc as u16, + data: payload[3..(payload.len() - extended_apdu_le_len)].to_vec(), + le: match extended_apdu_le_len { + 0 => 0, + 1 => { + if last_byte == 0x00 { + 0x100 + } else { + last_byte + } + } + 2 => { + let le_parsed = BigEndian::read_u16(&payload[payload.len() - 2..]); + if le_parsed == 0x00 { + 0x100 + } else { + le_parsed as u32 + } + } + 3 => { + let le_first_byte: u32 = + (*payload.get(payload.len() - 3).unwrap()).into(); + if le_first_byte != 0x00 { + return Err(ApduStatusCode::SW_INTERNAL_EXCEPTION); + } + let le_parsed = BigEndian::read_u16(&payload[payload.len() - 2..]); + if le_parsed == 0x00 { + 0x10000 + } else { + le_parsed as u32 + } + } + _ => return Err(ApduStatusCode::SW_INTERNAL_EXCEPTION), + }, + case_type: ApduType::Extended(match extended_apdu_le_len { + 0 => Case::Lc3Data, + 1 => Case::Lc3DataLe1, + 2 => Case::Lc3DataLe2, + 3 => Case::Le3, + _ => return Err(ApduStatusCode::SW_INTERNAL_EXCEPTION), + }), }); } - 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 - }; - 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 - // have an extended-length APDU - let last_byte: u32 = (*payload.last().unwrap()).into(); - return Ok(APDU { - header: array_ref!(header, 0, APDU_HEADER_LEN).into(), - lc: extended_apdu_lc as u16, - data: payload[3..(payload.len() - extended_apdu_le_len)].to_vec(), - le: match extended_apdu_le_len { - 0 => 0, - 1 => { - if last_byte == 0x00 { - 0x100 - } else { - last_byte - } - } - 2 => { - let le_parsed = BigEndian::read_u16(&payload[payload.len() - 2..]); - if le_parsed == 0x00 { - 0x100 - } else { - le_parsed as u32 - } - } - 3 => { - let le_first_byte: u32 = - (*payload.get(payload.len() - 3).unwrap()).into(); - if le_first_byte != 0x00 { - return Err(ApduStatusCode::SW_INTERNAL_EXCEPTION); - } - let le_parsed = BigEndian::read_u16(&payload[payload.len() - 2..]); - if le_parsed == 0x00 { - 0x10000 - } else { - le_parsed as u32 - } - } - _ => return Err(ApduStatusCode::SW_INTERNAL_EXCEPTION), - }, - case_type: ApduType::Extended(match extended_apdu_le_len { - 0 => Case::Lc3Data, - 1 => Case::Lc3DataLe1, - 2 => Case::Lc3DataLe2, - 3 => Case::Le3, - _ => return Err(ApduStatusCode::SW_INTERNAL_EXCEPTION), - }), - }); - } - } } + return Err(ApduStatusCode::SW_INTERNAL_EXCEPTION); } } From 1eaff57c885836e80e4bb76ee28649dabf0ea93c Mon Sep 17 00:00:00 2001 From: Kamran Khan Date: Thu, 3 Dec 2020 08:25:34 -0800 Subject: [PATCH 20/23] Le should be interpreted as 0x10000 even in the 2-byte case --- src/ctap/apdu.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/ctap/apdu.rs b/src/ctap/apdu.rs index 6a94790..b39d7fb 100644 --- a/src/ctap/apdu.rs +++ b/src/ctap/apdu.rs @@ -195,7 +195,7 @@ impl TryFrom<&[u8]> for APDU { 2 => { let le_parsed = BigEndian::read_u16(&payload[payload.len() - 2..]); if le_parsed == 0x00 { - 0x100 + 0x10000 } else { le_parsed as u32 } @@ -396,7 +396,7 @@ mod test { }, lc: 0xb1, data: payload.to_vec(), - le: 0x100, + le: 0x10000, case_type: ApduType::Extended(Case::Lc3DataLe2), }; assert_eq!(Ok(expected), response); @@ -423,7 +423,7 @@ mod test { }, lc: 0x40, data: payload.to_vec(), - le: 0x100, + le: 0x10000, case_type: ApduType::Extended(Case::Lc3DataLe2), }; assert_eq!(Ok(expected), response); From b032a15654711bafcc797c148ed389056f8bc92d Mon Sep 17 00:00:00 2001 From: Fabian Kaczmarczyck Date: Fri, 4 Dec 2020 13:17:33 +0100 Subject: [PATCH 21/23] makes the global signature counter more privacy friendly --- src/ctap/ctap1.rs | 35 +++++-- src/ctap/mod.rs | 246 ++++++++++++++++++++++++++++++++++++++++---- src/ctap/storage.rs | 31 +++++- 3 files changed, 276 insertions(+), 36 deletions(-) diff --git a/src/ctap/ctap1.rs b/src/ctap/ctap1.rs index 5bc759c..5919431 100644 --- a/src/ctap/ctap1.rs +++ b/src/ctap/ctap1.rs @@ -388,6 +388,7 @@ impl Ctap1Command { mod test { use super::super::{key_material, CREDENTIAL_ID_BASE_SIZE, USE_SIGNATURE_COUNTER}; use super::*; + use byteorder::{BigEndian, ByteOrder}; use crypto::rng256::ThreadRng256; use crypto::Hash256; @@ -642,6 +643,16 @@ mod test { assert_eq!(response, Err(Ctap1StatusCode::SW_WRONG_DATA)); } + fn check_signature_counter(response: &[u8; 4], signature_counter: u32) { + if USE_SIGNATURE_COUNTER { + let mut signature_counter_bytes = [0u8; 4]; + BigEndian::write_u32(&mut signature_counter_bytes, signature_counter); + assert_eq!(response, &signature_counter_bytes); + } else { + assert_eq!(response, &[0x00, 0x00, 0x00, 0x00]); + } + } + #[test] fn test_process_authenticate_enforce() { let mut rng = ThreadRng256 {}; @@ -662,11 +673,13 @@ mod test { let response = Ctap1Command::process_command(&message, &mut ctap_state, START_CLOCK_VALUE).unwrap(); assert_eq!(response[0], 0x01); - if USE_SIGNATURE_COUNTER { - assert_eq!(response[1..5], [0x00, 0x00, 0x00, 0x01]); - } else { - assert_eq!(response[1..5], [0x00, 0x00, 0x00, 0x00]); - } + check_signature_counter( + array_ref!(response, 1, 4), + ctap_state + .persistent_store + .global_signature_counter() + .unwrap(), + ); } #[test] @@ -690,11 +703,13 @@ mod test { let response = Ctap1Command::process_command(&message, &mut ctap_state, TIMEOUT_CLOCK_VALUE).unwrap(); assert_eq!(response[0], 0x01); - if USE_SIGNATURE_COUNTER { - assert_eq!(response[1..5], [0x00, 0x00, 0x00, 0x01]); - } else { - assert_eq!(response[1..5], [0x00, 0x00, 0x00, 0x00]); - } + check_signature_counter( + array_ref!(response, 1, 4), + ctap_state + .persistent_store + .global_signature_counter() + .unwrap(), + ); } #[test] diff --git a/src/ctap/mod.rs b/src/ctap/mod.rs index d67796b..56cca9a 100644 --- a/src/ctap/mod.rs +++ b/src/ctap/mod.rs @@ -81,6 +81,7 @@ const USE_BATCH_ATTESTATION: bool = false; // need a flash storage friendly way to implement this feature. The implemented // solution is a compromise to be compatible with U2F and not wasting storage. const USE_SIGNATURE_COUNTER: bool = true; +pub const INITIAL_SIGNATURE_COUNTER: u32 = 1; // Our credential ID consists of // - 16 byte initialization vector for AES-256, // - 32 byte ECDSA private key for the credential, @@ -215,7 +216,9 @@ where pub fn increment_global_signature_counter(&mut self) -> Result<(), Ctap2StatusCode> { if USE_SIGNATURE_COUNTER { - self.persistent_store.incr_global_signature_counter()?; + let increment = self.rng.gen_uniform_u32x8()[0] % 8 + 1; + self.persistent_store + .incr_global_signature_counter(increment)?; } Ok(()) } @@ -1094,9 +1097,43 @@ mod test { // The expected response is split to only assert the non-random parts. assert_eq!(fmt, "packed"); let mut expected_auth_data = vec![ - 0xA3, 0x79, 0xA6, 0xF6, 0xEE, 0xAF, 0xB9, 0xA5, 0x5E, 0x37, 0x8C, 0x11, 0x80, - 0x34, 0xE2, 0x75, 0x1E, 0x68, 0x2F, 0xAB, 0x9F, 0x2D, 0x30, 0xAB, 0x13, 0xD2, - 0x12, 0x55, 0x86, 0xCE, 0x19, 0x47, 0x41, 0x00, 0x00, 0x00, 0x00, + 0xA3, + 0x79, + 0xA6, + 0xF6, + 0xEE, + 0xAF, + 0xB9, + 0xA5, + 0x5E, + 0x37, + 0x8C, + 0x11, + 0x80, + 0x34, + 0xE2, + 0x75, + 0x1E, + 0x68, + 0x2F, + 0xAB, + 0x9F, + 0x2D, + 0x30, + 0xAB, + 0x13, + 0xD2, + 0x12, + 0x55, + 0x86, + 0xCE, + 0x19, + 0x47, + 0x41, + 0x00, + 0x00, + 0x00, + INITIAL_SIGNATURE_COUNTER as u8, ]; expected_auth_data.extend(&ctap_state.persistent_store.aaguid().unwrap()); expected_auth_data.extend(&[0x00, 0x20]); @@ -1131,9 +1168,43 @@ mod test { // The expected response is split to only assert the non-random parts. assert_eq!(fmt, "packed"); let mut expected_auth_data = vec![ - 0xA3, 0x79, 0xA6, 0xF6, 0xEE, 0xAF, 0xB9, 0xA5, 0x5E, 0x37, 0x8C, 0x11, 0x80, - 0x34, 0xE2, 0x75, 0x1E, 0x68, 0x2F, 0xAB, 0x9F, 0x2D, 0x30, 0xAB, 0x13, 0xD2, - 0x12, 0x55, 0x86, 0xCE, 0x19, 0x47, 0x41, 0x00, 0x00, 0x00, 0x00, + 0xA3, + 0x79, + 0xA6, + 0xF6, + 0xEE, + 0xAF, + 0xB9, + 0xA5, + 0x5E, + 0x37, + 0x8C, + 0x11, + 0x80, + 0x34, + 0xE2, + 0x75, + 0x1E, + 0x68, + 0x2F, + 0xAB, + 0x9F, + 0x2D, + 0x30, + 0xAB, + 0x13, + 0xD2, + 0x12, + 0x55, + 0x86, + 0xCE, + 0x19, + 0x47, + 0x41, + 0x00, + 0x00, + 0x00, + INITIAL_SIGNATURE_COUNTER as u8, ]; expected_auth_data.extend(&ctap_state.persistent_store.aaguid().unwrap()); expected_auth_data.extend(&[0x00, CREDENTIAL_ID_BASE_SIZE as u8]); @@ -1280,9 +1351,43 @@ mod test { // The expected response is split to only assert the non-random parts. assert_eq!(fmt, "packed"); let mut expected_auth_data = vec![ - 0xA3, 0x79, 0xA6, 0xF6, 0xEE, 0xAF, 0xB9, 0xA5, 0x5E, 0x37, 0x8C, 0x11, 0x80, - 0x34, 0xE2, 0x75, 0x1E, 0x68, 0x2F, 0xAB, 0x9F, 0x2D, 0x30, 0xAB, 0x13, 0xD2, - 0x12, 0x55, 0x86, 0xCE, 0x19, 0x47, 0xC1, 0x00, 0x00, 0x00, 0x00, + 0xA3, + 0x79, + 0xA6, + 0xF6, + 0xEE, + 0xAF, + 0xB9, + 0xA5, + 0x5E, + 0x37, + 0x8C, + 0x11, + 0x80, + 0x34, + 0xE2, + 0x75, + 0x1E, + 0x68, + 0x2F, + 0xAB, + 0x9F, + 0x2D, + 0x30, + 0xAB, + 0x13, + 0xD2, + 0x12, + 0x55, + 0x86, + 0xCE, + 0x19, + 0x47, + 0xC1, + 0x00, + 0x00, + 0x00, + INITIAL_SIGNATURE_COUNTER as u8, ]; expected_auth_data.extend(&ctap_state.persistent_store.aaguid().unwrap()); expected_auth_data.extend(&[0x00, CREDENTIAL_ID_MAX_SIZE as u8]); @@ -1329,9 +1434,43 @@ mod test { // The expected response is split to only assert the non-random parts. assert_eq!(fmt, "packed"); let mut expected_auth_data = vec![ - 0xA3, 0x79, 0xA6, 0xF6, 0xEE, 0xAF, 0xB9, 0xA5, 0x5E, 0x37, 0x8C, 0x11, 0x80, - 0x34, 0xE2, 0x75, 0x1E, 0x68, 0x2F, 0xAB, 0x9F, 0x2D, 0x30, 0xAB, 0x13, 0xD2, - 0x12, 0x55, 0x86, 0xCE, 0x19, 0x47, 0xC1, 0x00, 0x00, 0x00, 0x00, + 0xA3, + 0x79, + 0xA6, + 0xF6, + 0xEE, + 0xAF, + 0xB9, + 0xA5, + 0x5E, + 0x37, + 0x8C, + 0x11, + 0x80, + 0x34, + 0xE2, + 0x75, + 0x1E, + 0x68, + 0x2F, + 0xAB, + 0x9F, + 0x2D, + 0x30, + 0xAB, + 0x13, + 0xD2, + 0x12, + 0x55, + 0x86, + 0xCE, + 0x19, + 0x47, + 0xC1, + 0x00, + 0x00, + 0x00, + INITIAL_SIGNATURE_COUNTER as u8, ]; expected_auth_data.extend(&ctap_state.persistent_store.aaguid().unwrap()); expected_auth_data.extend(&[0x00, 0x20]); @@ -1374,6 +1513,7 @@ mod test { response: Result, expected_user: PublicKeyCredentialUserEntity, flags: u8, + signature_counter: u32, expected_number_of_credentials: Option, ) { match response.unwrap() { @@ -1384,11 +1524,16 @@ mod test { number_of_credentials, .. } = get_assertion_response; - let expected_auth_data = vec![ + let mut expected_auth_data = vec![ 0xA3, 0x79, 0xA6, 0xF6, 0xEE, 0xAF, 0xB9, 0xA5, 0x5E, 0x37, 0x8C, 0x11, 0x80, 0x34, 0xE2, 0x75, 0x1E, 0x68, 0x2F, 0xAB, 0x9F, 0x2D, 0x30, 0xAB, 0x13, 0xD2, - 0x12, 0x55, 0x86, 0xCE, 0x19, 0x47, flags, 0x00, 0x00, 0x00, 0x01, + 0x12, 0x55, 0x86, 0xCE, 0x19, 0x47, flags, 0x00, 0x00, 0x00, 0x00, ]; + let signature_counter_position = expected_auth_data.len() - 4; + BigEndian::write_u32( + &mut expected_auth_data[signature_counter_position..], + signature_counter, + ); assert_eq!(auth_data, expected_auth_data); assert_eq!(user, Some(expected_user)); assert_eq!(number_of_credentials, expected_number_of_credentials); @@ -1400,6 +1545,7 @@ mod test { fn check_assertion_response( response: Result, expected_user_id: Vec, + signature_counter: u32, expected_number_of_credentials: Option, ) { let expected_user = PublicKeyCredentialUserEntity { @@ -1412,6 +1558,7 @@ mod test { response, expected_user, 0x00, + signature_counter, expected_number_of_credentials, ); } @@ -1444,7 +1591,11 @@ mod test { DUMMY_CHANNEL_ID, DUMMY_CLOCK_VALUE, ); - check_assertion_response(get_assertion_response, vec![0x1D], None); + let signature_counter = ctap_state + .persistent_store + .global_signature_counter() + .unwrap(); + check_assertion_response(get_assertion_response, vec![0x1D], signature_counter, None); } #[test] @@ -1637,7 +1788,11 @@ mod test { DUMMY_CHANNEL_ID, DUMMY_CLOCK_VALUE, ); - check_assertion_response(get_assertion_response, vec![0x1D], None); + let signature_counter = ctap_state + .persistent_store + .global_signature_counter() + .unwrap(); + check_assertion_response(get_assertion_response, vec![0x1D], signature_counter, None); let credential = PublicKeyCredentialSource { key_type: PublicKeyCredentialType::PublicKey, @@ -1740,10 +1895,26 @@ mod test { DUMMY_CHANNEL_ID, DUMMY_CLOCK_VALUE, ); - check_assertion_response_with_user(get_assertion_response, user2, 0x04, Some(2)); + let signature_counter = ctap_state + .persistent_store + .global_signature_counter() + .unwrap(); + check_assertion_response_with_user( + get_assertion_response, + user2, + 0x04, + signature_counter, + Some(2), + ); let get_assertion_response = ctap_state.process_get_next_assertion(DUMMY_CLOCK_VALUE); - check_assertion_response_with_user(get_assertion_response, user1, 0x04, None); + check_assertion_response_with_user( + get_assertion_response, + user1, + 0x04, + signature_counter, + None, + ); let get_assertion_response = ctap_state.process_get_next_assertion(DUMMY_CLOCK_VALUE); assert_eq!( @@ -1800,13 +1971,22 @@ mod test { DUMMY_CHANNEL_ID, DUMMY_CLOCK_VALUE, ); - check_assertion_response(get_assertion_response, vec![0x03], Some(3)); + let signature_counter = ctap_state + .persistent_store + .global_signature_counter() + .unwrap(); + check_assertion_response( + get_assertion_response, + vec![0x03], + signature_counter, + Some(3), + ); let get_assertion_response = ctap_state.process_get_next_assertion(DUMMY_CLOCK_VALUE); - check_assertion_response(get_assertion_response, vec![0x02], None); + check_assertion_response(get_assertion_response, vec![0x02], signature_counter, None); let get_assertion_response = ctap_state.process_get_next_assertion(DUMMY_CLOCK_VALUE); - check_assertion_response(get_assertion_response, vec![0x01], None); + check_assertion_response(get_assertion_response, vec![0x01], signature_counter, None); let get_assertion_response = ctap_state.process_get_next_assertion(DUMMY_CLOCK_VALUE); assert_eq!( @@ -2042,4 +2222,26 @@ mod test { .is_none()); } } + + #[test] + fn test_signature_counter() { + let mut rng = ThreadRng256 {}; + let user_immediately_present = |_| Ok(()); + let mut ctap_state = CtapState::new(&mut rng, user_immediately_present, DUMMY_CLOCK_VALUE); + + let mut last_counter = ctap_state + .persistent_store + .global_signature_counter() + .unwrap(); + assert!(last_counter > 0); + for _ in 0..100 { + assert!(ctap_state.increment_global_signature_counter().is_ok()); + let next_counter = ctap_state + .persistent_store + .global_signature_counter() + .unwrap(); + assert!(next_counter > last_counter); + last_counter = next_counter; + } + } } diff --git a/src/ctap/storage.rs b/src/ctap/storage.rs index 0e4941a..911bdd7 100644 --- a/src/ctap/storage.rs +++ b/src/ctap/storage.rs @@ -18,6 +18,7 @@ use crate::ctap::data_formats::{CredentialProtectionPolicy, PublicKeyCredentialS use crate::ctap::key_material; use crate::ctap::pin_protocol_v1::PIN_AUTH_LENGTH; use crate::ctap::status_code::Ctap2StatusCode; +use crate::ctap::INITIAL_SIGNATURE_COUNTER; use crate::embedded_flash::{self, StoreConfig, StoreEntry, StoreError}; use alloc::string::String; #[cfg(any(test, feature = "ram_storage", feature = "with_ctap2_1"))] @@ -366,16 +367,16 @@ impl PersistentStore { Ok(self .store .find_one(&Key::GlobalSignatureCounter) - .map_or(0, |(_, entry)| { + .map_or(INITIAL_SIGNATURE_COUNTER, |(_, entry)| { u32::from_ne_bytes(*array_ref!(entry.data, 0, 4)) })) } - pub fn incr_global_signature_counter(&mut self) -> Result<(), Ctap2StatusCode> { + pub fn incr_global_signature_counter(&mut self, increment: u32) -> Result<(), Ctap2StatusCode> { let mut buffer = [0; core::mem::size_of::()]; match self.store.find_one(&Key::GlobalSignatureCounter) { None => { - buffer.copy_from_slice(&1u32.to_ne_bytes()); + buffer.copy_from_slice(&(increment + INITIAL_SIGNATURE_COUNTER).to_ne_bytes()); self.store.insert(StoreEntry { tag: GLOBAL_SIGNATURE_COUNTER, data: &buffer, @@ -385,7 +386,7 @@ impl PersistentStore { Some((index, entry)) => { let value = u32::from_ne_bytes(*array_ref!(entry.data, 0, 4)); // In hopes that servers handle the wrapping gracefully. - buffer.copy_from_slice(&value.wrapping_add(1).to_ne_bytes()); + buffer.copy_from_slice(&value.wrapping_add(increment).to_ne_bytes()); self.store.replace( index, StoreEntry { @@ -1136,6 +1137,28 @@ mod test { assert_eq!(persistent_store._min_pin_length_rp_ids().unwrap(), rp_ids); } + #[test] + fn test_global_signature_counter() { + let mut rng = ThreadRng256 {}; + let mut persistent_store = PersistentStore::new(&mut rng); + + let mut counter_value = 1; + assert_eq!( + persistent_store.global_signature_counter().unwrap(), + counter_value + ); + for increment in 1..10 { + assert!(persistent_store + .incr_global_signature_counter(increment) + .is_ok()); + counter_value += increment; + assert_eq!( + persistent_store.global_signature_counter().unwrap(), + counter_value + ); + } + } + #[test] fn test_serialize_deserialize_credential() { let mut rng = ThreadRng256 {}; From 21b8ad18cecb47fb465b2870fa45570f767d238f Mon Sep 17 00:00:00 2001 From: Fabian Kaczmarczyck Date: Fri, 4 Dec 2020 13:41:56 +0100 Subject: [PATCH 22/23] fix clippy warning in apdu --- 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 b39d7fb..949151e 100644 --- a/src/ctap/apdu.rs +++ b/src/ctap/apdu.rs @@ -226,7 +226,7 @@ impl TryFrom<&[u8]> for APDU { } } - return Err(ApduStatusCode::SW_INTERNAL_EXCEPTION); + Err(ApduStatusCode::SW_INTERNAL_EXCEPTION) } } From 0b55ff3c3ad995b34113d71808bf33fa7828e1bf Mon Sep 17 00:00:00 2001 From: Fabian Kaczmarczyck Date: Fri, 4 Dec 2020 14:57:11 +0100 Subject: [PATCH 23/23] fixes formatting --- src/ctap/ctap1.rs | 5 +- src/ctap/mod.rs | 164 +++++----------------------------------------- 2 files changed, 17 insertions(+), 152 deletions(-) diff --git a/src/ctap/ctap1.rs b/src/ctap/ctap1.rs index 5919431..e434c36 100644 --- a/src/ctap/ctap1.rs +++ b/src/ctap/ctap1.rs @@ -388,7 +388,6 @@ impl Ctap1Command { mod test { use super::super::{key_material, CREDENTIAL_ID_BASE_SIZE, USE_SIGNATURE_COUNTER}; use super::*; - use byteorder::{BigEndian, ByteOrder}; use crypto::rng256::ThreadRng256; use crypto::Hash256; @@ -645,9 +644,7 @@ mod test { fn check_signature_counter(response: &[u8; 4], signature_counter: u32) { if USE_SIGNATURE_COUNTER { - let mut signature_counter_bytes = [0u8; 4]; - BigEndian::write_u32(&mut signature_counter_bytes, signature_counter); - assert_eq!(response, &signature_counter_bytes); + assert_eq!(u32::from_be_bytes(*response), signature_counter); } else { assert_eq!(response, &[0x00, 0x00, 0x00, 0x00]); } diff --git a/src/ctap/mod.rs b/src/ctap/mod.rs index 56cca9a..ea42caf 100644 --- a/src/ctap/mod.rs +++ b/src/ctap/mod.rs @@ -1097,44 +1097,11 @@ mod test { // The expected response is split to only assert the non-random parts. assert_eq!(fmt, "packed"); let mut expected_auth_data = vec![ - 0xA3, - 0x79, - 0xA6, - 0xF6, - 0xEE, - 0xAF, - 0xB9, - 0xA5, - 0x5E, - 0x37, - 0x8C, - 0x11, - 0x80, - 0x34, - 0xE2, - 0x75, - 0x1E, - 0x68, - 0x2F, - 0xAB, - 0x9F, - 0x2D, - 0x30, - 0xAB, - 0x13, - 0xD2, - 0x12, - 0x55, - 0x86, - 0xCE, - 0x19, - 0x47, - 0x41, - 0x00, - 0x00, - 0x00, - INITIAL_SIGNATURE_COUNTER as u8, + 0xA3, 0x79, 0xA6, 0xF6, 0xEE, 0xAF, 0xB9, 0xA5, 0x5E, 0x37, 0x8C, 0x11, 0x80, + 0x34, 0xE2, 0x75, 0x1E, 0x68, 0x2F, 0xAB, 0x9F, 0x2D, 0x30, 0xAB, 0x13, 0xD2, + 0x12, 0x55, 0x86, 0xCE, 0x19, 0x47, 0x41, 0x00, 0x00, 0x00, ]; + expected_auth_data.push(INITIAL_SIGNATURE_COUNTER as u8); expected_auth_data.extend(&ctap_state.persistent_store.aaguid().unwrap()); expected_auth_data.extend(&[0x00, 0x20]); assert_eq!( @@ -1168,44 +1135,11 @@ mod test { // The expected response is split to only assert the non-random parts. assert_eq!(fmt, "packed"); let mut expected_auth_data = vec![ - 0xA3, - 0x79, - 0xA6, - 0xF6, - 0xEE, - 0xAF, - 0xB9, - 0xA5, - 0x5E, - 0x37, - 0x8C, - 0x11, - 0x80, - 0x34, - 0xE2, - 0x75, - 0x1E, - 0x68, - 0x2F, - 0xAB, - 0x9F, - 0x2D, - 0x30, - 0xAB, - 0x13, - 0xD2, - 0x12, - 0x55, - 0x86, - 0xCE, - 0x19, - 0x47, - 0x41, - 0x00, - 0x00, - 0x00, - INITIAL_SIGNATURE_COUNTER as u8, + 0xA3, 0x79, 0xA6, 0xF6, 0xEE, 0xAF, 0xB9, 0xA5, 0x5E, 0x37, 0x8C, 0x11, 0x80, + 0x34, 0xE2, 0x75, 0x1E, 0x68, 0x2F, 0xAB, 0x9F, 0x2D, 0x30, 0xAB, 0x13, 0xD2, + 0x12, 0x55, 0x86, 0xCE, 0x19, 0x47, 0x41, 0x00, 0x00, 0x00, ]; + expected_auth_data.push(INITIAL_SIGNATURE_COUNTER as u8); expected_auth_data.extend(&ctap_state.persistent_store.aaguid().unwrap()); expected_auth_data.extend(&[0x00, CREDENTIAL_ID_BASE_SIZE as u8]); assert_eq!( @@ -1351,44 +1285,11 @@ mod test { // The expected response is split to only assert the non-random parts. assert_eq!(fmt, "packed"); let mut expected_auth_data = vec![ - 0xA3, - 0x79, - 0xA6, - 0xF6, - 0xEE, - 0xAF, - 0xB9, - 0xA5, - 0x5E, - 0x37, - 0x8C, - 0x11, - 0x80, - 0x34, - 0xE2, - 0x75, - 0x1E, - 0x68, - 0x2F, - 0xAB, - 0x9F, - 0x2D, - 0x30, - 0xAB, - 0x13, - 0xD2, - 0x12, - 0x55, - 0x86, - 0xCE, - 0x19, - 0x47, - 0xC1, - 0x00, - 0x00, - 0x00, - INITIAL_SIGNATURE_COUNTER as u8, + 0xA3, 0x79, 0xA6, 0xF6, 0xEE, 0xAF, 0xB9, 0xA5, 0x5E, 0x37, 0x8C, 0x11, 0x80, + 0x34, 0xE2, 0x75, 0x1E, 0x68, 0x2F, 0xAB, 0x9F, 0x2D, 0x30, 0xAB, 0x13, 0xD2, + 0x12, 0x55, 0x86, 0xCE, 0x19, 0x47, 0xC1, 0x00, 0x00, 0x00, ]; + expected_auth_data.push(INITIAL_SIGNATURE_COUNTER as u8); expected_auth_data.extend(&ctap_state.persistent_store.aaguid().unwrap()); expected_auth_data.extend(&[0x00, CREDENTIAL_ID_MAX_SIZE as u8]); assert_eq!( @@ -1434,44 +1335,11 @@ mod test { // The expected response is split to only assert the non-random parts. assert_eq!(fmt, "packed"); let mut expected_auth_data = vec![ - 0xA3, - 0x79, - 0xA6, - 0xF6, - 0xEE, - 0xAF, - 0xB9, - 0xA5, - 0x5E, - 0x37, - 0x8C, - 0x11, - 0x80, - 0x34, - 0xE2, - 0x75, - 0x1E, - 0x68, - 0x2F, - 0xAB, - 0x9F, - 0x2D, - 0x30, - 0xAB, - 0x13, - 0xD2, - 0x12, - 0x55, - 0x86, - 0xCE, - 0x19, - 0x47, - 0xC1, - 0x00, - 0x00, - 0x00, - INITIAL_SIGNATURE_COUNTER as u8, + 0xA3, 0x79, 0xA6, 0xF6, 0xEE, 0xAF, 0xB9, 0xA5, 0x5E, 0x37, 0x8C, 0x11, 0x80, + 0x34, 0xE2, 0x75, 0x1E, 0x68, 0x2F, 0xAB, 0x9F, 0x2D, 0x30, 0xAB, 0x13, 0xD2, + 0x12, 0x55, 0x86, 0xCE, 0x19, 0x47, 0xC1, 0x00, 0x00, 0x00, ]; + expected_auth_data.push(INITIAL_SIGNATURE_COUNTER as u8); expected_auth_data.extend(&ctap_state.persistent_store.aaguid().unwrap()); expected_auth_data.extend(&[0x00, 0x20]); assert_eq!(