From 5f5f72b6d13f31da3a54569f75d16e85f134451d Mon Sep 17 00:00:00 2001 From: Kamran Khan Date: Mon, 30 Nov 2020 02:04:52 -0800 Subject: [PATCH 01/18] 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 02/18] 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 03/18] 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 04/18] 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 05/18] 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 06/18] 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 07/18] 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 08/18] 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 09/18] 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 10/18] 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 11/18] 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 12/18] 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 13/18] 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 14/18] 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 15/18] 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 16/18] 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 17/18] 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 18/18] 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);