From d3af2f2d80289f824867ac119afc75fda90b7871 Mon Sep 17 00:00:00 2001 From: Tony Arcieri Date: Tue, 26 Nov 2019 09:06:54 -0800 Subject: [PATCH] Factor `Response` into `apdu` module; improved debugging This commit merges the `apdu` and `response` modules: the responses are APDU responses, and so the two are related. This also moves the `trace` logging into the APDU type, which allows it to display `Debug` output for APDUs and responses, which makes it easier to understand what's going on (and will be even better once instructions are converted into an enum so you can actually see what's happening). --- .github/workflows/rust.yml | 2 +- README.md | 10 +- src/apdu.rs | 170 ++++++++++++++++++++++++++++++++- src/key.rs | 6 +- src/lib.rs | 1 - src/response.rs | 186 ------------------------------------- src/transaction.rs | 44 +++++---- src/yubikey.rs | 18 ++-- 8 files changed, 208 insertions(+), 229 deletions(-) delete mode 100644 src/response.rs diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index 1fadffb8..b37a2bfe 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -149,7 +149,7 @@ jobs: uses: actions-rs/cargo@v1 with: command: clippy - args: -- -D warnings + args: --all-features -- -D warnings # TODO: use actions-rs/audit-check security_audit: diff --git a/README.md b/README.md index 82317d26..5dd7bc92 100644 --- a/README.md +++ b/README.md @@ -112,15 +112,17 @@ To trace every message sent to/from the card i.e. the raw Application Protocol Data Unit (APDU) messages, use the `trace` log level: ``` -running 1 test [INFO yubikey_piv::yubikey] trying to connect to reader 'Yubico YubiKey OTP+FIDO+CCID' [INFO yubikey_piv::yubikey] connected to 'Yubico YubiKey OTP+FIDO+CCID' successfully +[TRACE yubikey_piv::apdu] >>> APDU { cla: 0, ins: 164, p1: 4, p2: 0, lc: 5, data: [160, 0, 0, 3, 8] } [TRACE yubikey_piv::transaction] >>> [0, 164, 4, 0, 5, 160, 0, 0, 3, 8] -[TRACE yubikey_piv::transaction] <<< [97, 17, 79, 6, 0, 0, 16, 0, 1, 0, 121, 7, 79, 5, 160, 0, 0, 3, 8, 144, 0] +[TRACE yubikey_piv::apdu] <<< Response { status_words: Success, data: [97, 17, 79, 6, 0, 0, 16, 0, 1, 0, 121, 7, 79, 5, 160, 0, 0, 3, 8] } +[TRACE yubikey_piv::apdu] >>> APDU { cla: 0, ins: 253, p1: 0, p2: 0, lc: 0, data: [] } [TRACE yubikey_piv::transaction] >>> [0, 253, 0, 0, 0] -[TRACE yubikey_piv::transaction] <<< [5, 1, 2, 144, 0] +[TRACE yubikey_piv::apdu] <<< Response { status_words: Success, data: [5, 1, 2] } +[TRACE yubikey_piv::apdu] >>> APDU { cla: 0, ins: 248, p1: 0, p2: 0, lc: 0, data: [] } [TRACE yubikey_piv::transaction] >>> [0, 248, 0, 0, 0] -[TRACE yubikey_piv::transaction] <<< [0, 115, 0, 178, 144, 0] +[TRACE yubikey_piv::apdu] <<< Response { status_words: Success, data: [0, 115, 0, 178] } test connect ... ok ``` diff --git a/src/apdu.rs b/src/apdu.rs index 47945b5e..2ddf77a7 100644 --- a/src/apdu.rs +++ b/src/apdu.rs @@ -30,7 +30,8 @@ // (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. -use crate::{error::Error, response::Response, transaction::Transaction, Buffer}; +use crate::{error::Error, transaction::Transaction, Buffer}; +use log::trace; use std::fmt::{self, Debug}; use zeroize::{Zeroize, Zeroizing}; @@ -112,11 +113,13 @@ impl APDU { /// Transmit this APDU using the given card transaction pub fn transmit(&self, txn: &Transaction<'_>, recv_len: usize) -> Result { - let response_bytes = txn.transmit(&self.to_bytes(), recv_len)?; - Ok(Response::from_bytes(response_bytes)) + trace!(">>> {:?}", self); + let response = Response::from(txn.transmit(&self.to_bytes(), recv_len)?); + trace!("<<< {:?}", &response); + Ok(response) } - /// Consume this APDU and return a self-zeroizing buffer + /// Serialize this APDU as a self-zeroizing byte buffer pub fn to_bytes(&self) -> Buffer { let mut bytes = Vec::with_capacity(5 + self.data.len()); bytes.push(self.cla); @@ -159,3 +162,162 @@ impl Zeroize for APDU { self.data.zeroize(); } } + +/// APDU responses +#[derive(Debug)] +pub(crate) struct Response { + /// Status words + status_words: StatusWords, + + /// Buffer + data: Vec, +} + +impl Response { + /// Create a new response from the given status words and buffer + #[cfg(feature = "untested")] + pub fn new(status_words: StatusWords, data: Vec) -> Response { + Response { status_words, data } + } + + /// Get the [`StatusWords`] for this response. + pub fn status_words(&self) -> StatusWords { + self.status_words + } + + /// Get the raw [`StatusWords`] code for this response. + #[cfg(feature = "untested")] + pub fn code(&self) -> u32 { + self.status_words.code() + } + + /// Do the status words for this response indicate success? + pub fn is_success(&self) -> bool { + self.status_words.is_success() + } + + /// Borrow the response data + pub fn data(&self) -> &[u8] { + self.data.as_ref() + } +} + +impl AsRef<[u8]> for Response { + fn as_ref(&self) -> &[u8] { + self.data() + } +} + +impl Drop for Response { + fn drop(&mut self) { + self.zeroize(); + } +} + +impl From> for Response { + fn from(mut bytes: Vec) -> Self { + if bytes.len() < 2 { + return Response { + status_words: StatusWords::None, + data: bytes, + }; + } + + let sw = StatusWords::from( + (bytes[bytes.len() - 2] as u32) << 8 | (bytes[bytes.len() - 1] as u32), + ); + + let len = bytes.len() - 2; + bytes.truncate(len); + + Response { + status_words: sw, + data: bytes, + } + } +} + +impl Zeroize for Response { + fn zeroize(&mut self) { + self.data.zeroize(); + } +} + +/// Status Words (SW) are 2-byte values returned by a card command. +/// +/// The first byte of a status word is referred to as SW1 and the second byte +/// of a status word is referred to as SW2. +/// +/// See NIST special publication 800-73-4, section 5.6: +/// +#[derive(Copy, Clone, Debug, Eq, PartialEq)] +pub(crate) enum StatusWords { + /// No status words present in response + None, + + /// Successful execution + Success, + + /// Security status not satisfied + SecurityStatusError, + + /// Authentication method blocked + AuthBlockedError, + + /// Incorrect parameter in command data field + IncorrectParamError, + + // + // Custom Yubico Status Word extensions + // + /// Incorrect card slot error + IncorrectSlotError, + + /// Not supported error + NotSupportedError, + + /// Other/unrecognized status words + Other(u32), +} + +impl StatusWords { + /// Get the numerical response code for these status words + pub fn code(self) -> u32 { + match self { + StatusWords::None => 0, + StatusWords::SecurityStatusError => 0x6982, + StatusWords::AuthBlockedError => 0x6983, + StatusWords::IncorrectParamError => 0x6a80, + StatusWords::IncorrectSlotError => 0x6b00, + StatusWords::NotSupportedError => 0x6d00, + StatusWords::Success => 0x9000, + StatusWords::Other(n) => n, + } + } + + /// Do these status words indicate success? + pub fn is_success(self) -> bool { + self == StatusWords::Success + } +} + +impl From for StatusWords { + fn from(sw: u32) -> Self { + match sw { + 0x0000 => StatusWords::None, + 0x6982 => StatusWords::SecurityStatusError, + 0x6983 => StatusWords::AuthBlockedError, + 0x6a80 => StatusWords::IncorrectParamError, + 0x6b00 => StatusWords::IncorrectSlotError, + 0x6d00 => StatusWords::NotSupportedError, + 0x9000 => StatusWords::Success, + _ => StatusWords::Other(sw), + } + } +} + +impl From for u32 { + fn from(sw: StatusWords) -> u32 { + sw.code() + } +} diff --git a/src/key.rs b/src/key.rs index 4a8f3766..7083b6e4 100644 --- a/src/key.rs +++ b/src/key.rs @@ -38,14 +38,14 @@ // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. use crate::{ + apdu::StatusWords, certificate::{self, Certificate}, consts::*, error::Error, - response::StatusWords, serialization::*, settings, yubikey::YubiKey, - AlgorithmId, ObjectId, + AlgorithmId, Buffer, ObjectId, }; use log::{debug, error, warn}; @@ -309,7 +309,7 @@ pub fn generate( } } - let data = response.into_buffer(); + let data = Buffer::new(response.data().into()); match algorithm { YKPIV_ALGO_RSA1024 | YKPIV_ALGO_RSA2048 => { diff --git a/src/lib.rs b/src/lib.rs index e82e61aa..6c595a46 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -155,7 +155,6 @@ mod metadata; pub mod mgm; #[cfg(feature = "untested")] pub mod msroots; -mod response; #[cfg(feature = "untested")] mod serialization; #[cfg(feature = "untested")] diff --git a/src/response.rs b/src/response.rs deleted file mode 100644 index 8743bf0a..00000000 --- a/src/response.rs +++ /dev/null @@ -1,186 +0,0 @@ -//! Responses to issued commands - -// Adapted from yubico-piv-tool: -// -// -// Copyright (c) 2014-2016 Yubico AB -// All rights reserved. -// -// Redistribution and use in source and binary forms, with or without -// modification, are permitted provided that the following conditions are -// met: -// -// * Redistributions of source code must retain the above copyright -// notice, this list of conditions and the following disclaimer. -// -// * Redistributions in binary form must reproduce the above -// copyright notice, this list of conditions and the following -// disclaimer in the documentation and/or other materials provided -// with the distribution. -// -// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS -// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT -// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR -// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT -// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, -// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT -// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, -// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY -// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT -// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE -// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. - -use crate::Buffer; - -/// Parsed response to a command -pub(crate) struct Response { - /// Status words - status_words: StatusWords, - - /// Buffer - buffer: Buffer, -} - -impl Response { - /// Parse a response from the given buffer - pub fn from_bytes(mut buffer: Buffer) -> Self { - if buffer.len() >= 2 { - let sw = StatusWords::from( - (buffer[buffer.len() - 2] as u32) << 8 | (buffer[buffer.len() - 1] as u32), - ); - - let len = buffer.len() - 2; - buffer.truncate(len); - Response { - status_words: sw, - buffer, - } - } else { - Response { - status_words: StatusWords::None, - buffer, - } - } - } - - /// Create a new response from the given status words and buffer - #[cfg(feature = "untested")] - pub fn new(status_words: StatusWords, buffer: Buffer) -> Response { - Response { - status_words, - buffer, - } - } - - /// Get the [`StatusWords`] for this response. - pub fn status_words(&self) -> StatusWords { - self.status_words - } - - /// Get the raw [`StatusWords`] code for this response. - #[cfg(feature = "untested")] - pub fn code(&self) -> u32 { - self.status_words.code() - } - - /// Do the status words for this response indicate success? - pub fn is_success(&self) -> bool { - self.status_words.is_success() - } - - /// Borrow the response buffer - pub fn buffer(&self) -> &[u8] { - self.buffer.as_ref() - } - - /// Consume this response, returning its buffer - #[cfg(feature = "untested")] - pub fn into_buffer(self) -> Buffer { - self.buffer - } -} - -impl AsRef<[u8]> for Response { - fn as_ref(&self) -> &[u8] { - self.buffer() - } -} - -/// Status Words (SW) are 2-byte values returned by a card command. -/// -/// The first byte of a status word is referred to as SW1 and the second byte -/// of a status word is referred to as SW2. -/// -/// See NIST special publication 800-73-4, section 5.6: -/// -#[derive(Copy, Clone, Debug, Eq, PartialEq)] -pub(crate) enum StatusWords { - /// No status words present in response - None, - - /// Successful execution - Success, - - /// Security status not satisfied - SecurityStatusError, - - /// Authentication method blocked - AuthBlockedError, - - /// Incorrect parameter in command data field - IncorrectParamError, - - // - // Custom Yubico Status Word extensions - // - /// Incorrect card slot error - IncorrectSlotError, - - /// Not supported error - NotSupportedError, - - /// Other/unrecognized status words - Other(u32), -} - -impl StatusWords { - /// Get the numerical response code for these status words - pub fn code(self) -> u32 { - match self { - StatusWords::None => 0, - StatusWords::SecurityStatusError => 0x6982, - StatusWords::AuthBlockedError => 0x6983, - StatusWords::IncorrectParamError => 0x6a80, - StatusWords::IncorrectSlotError => 0x6b00, - StatusWords::NotSupportedError => 0x6d00, - StatusWords::Success => 0x9000, - StatusWords::Other(n) => n, - } - } - - /// Do these status words indicate success? - pub fn is_success(self) -> bool { - self == StatusWords::Success - } -} - -impl From for StatusWords { - fn from(sw: u32) -> Self { - match sw { - 0x0000 => StatusWords::None, - 0x6982 => StatusWords::SecurityStatusError, - 0x6983 => StatusWords::AuthBlockedError, - 0x6a80 => StatusWords::IncorrectParamError, - 0x6b00 => StatusWords::IncorrectSlotError, - 0x6d00 => StatusWords::NotSupportedError, - 0x9000 => StatusWords::Success, - _ => StatusWords::Other(sw), - } - } -} - -impl From for u32 { - fn from(sw: StatusWords) -> u32 { - sw.code() - } -} diff --git a/src/transaction.rs b/src/transaction.rs index 5f9beeaf..4f227c58 100644 --- a/src/transaction.rs +++ b/src/transaction.rs @@ -1,17 +1,18 @@ //! YubiKey PC/SC transactions -use crate::{apdu::APDU, consts::*, error::Error, yubikey::*, Buffer}; +use crate::{apdu::APDU, consts::*, error::Error, yubikey::*}; #[cfg(feature = "untested")] use crate::{ + apdu::{Response, StatusWords}, mgm::MgmKey, - response::{Response, StatusWords}, serialization::*, - ObjectId, + Buffer, ObjectId, }; use log::{error, trace}; use std::convert::TryInto; #[cfg(feature = "untested")] use std::ptr; +#[cfg(feature = "untested")] use zeroize::Zeroizing; /// Exclusive transaction with the YubiKey's PC/SC card. @@ -43,10 +44,10 @@ impl<'tx> Transaction<'tx> { /// single APDU messages at a time. For larger messages that need to be /// split into multiple APDUs, use the [`Transaction::transfer_data`] /// method instead. - pub fn transmit(&self, send_buffer: &[u8], recv_len: usize) -> Result { + pub fn transmit(&self, send_buffer: &[u8], recv_len: usize) -> Result, Error> { trace!(">>> {:?}", send_buffer); - let mut recv_buffer = Zeroizing::new(vec![0u8; recv_len]); + let mut recv_buffer = vec![0u8; recv_len]; let len = self .inner @@ -54,9 +55,6 @@ impl<'tx> Transaction<'tx> { .len(); recv_buffer.truncate(len); - - trace!("<<< {:?}", recv_buffer.as_slice()); - Ok(recv_buffer) } @@ -91,14 +89,14 @@ impl<'tx> Transaction<'tx> { return Err(Error::GenericError); } - if response.buffer().len() < 3 { + if response.data().len() < 3 { return Err(Error::SizeError); } Ok(Version { - major: response.buffer()[0], - minor: response.buffer()[1], - patch: response.buffer()[2], + major: response.data()[0], + minor: response.data()[1], + patch: response.data()[2], }) } @@ -157,7 +155,7 @@ impl<'tx> Transaction<'tx> { resp }; - response.buffer()[..4] + response.data()[..4] .try_into() .map(|serial| Serial::from(u32::from_be_bytes(serial))) .map_err(|_| Error::SizeError) @@ -363,7 +361,7 @@ impl<'tx> Transaction<'tx> { } } - let data = response.buffer(); + let data = response.data(); // skip the first 7c tag if data[0] != 0x7c { @@ -404,7 +402,7 @@ impl<'tx> Transaction<'tx> { max_out: usize, ) -> Result { let mut in_offset = 0; - let mut out_data = Zeroizing::new(vec![]); + let mut out_data = vec![]; let mut sw = 0; loop { @@ -432,17 +430,17 @@ impl<'tx> Transaction<'tx> { sw = response.status_words().code(); - if out_data.len() - response.buffer().len() - 2 > max_out { + if out_data.len() - response.data().len() - 2 > max_out { error!( "output buffer too small: wanted to write {}, max was {}", - out_data.len() - response.buffer().len() - 2, + out_data.len() - response.data().len() - 2, max_out ); return Err(Error::SizeError); } - out_data.extend_from_slice(&response.buffer()[..response.buffer().len() - 2]); + out_data.extend_from_slice(&response.data()[..response.data().len() - 2]); in_offset += this_size; if in_offset >= in_data.len() { @@ -460,20 +458,20 @@ impl<'tx> Transaction<'tx> { sw = response.status_words().code(); if sw != StatusWords::Success.code() && (sw >> 8 != 0x61) { - return Ok(Response::new(sw.into(), Zeroizing::new(vec![]))); + return Ok(Response::new(sw.into(), vec![])); } - if out_data.len() + response.buffer().len() - 2 > max_out { + if out_data.len() + response.data().len() - 2 > max_out { error!( "output buffer too small: wanted to write {}, max was {}", - out_data.len() + response.buffer().len() - 2, + out_data.len() + response.data().len() - 2, max_out ); return Err(Error::SizeError); } - out_data.extend_from_slice(&response.buffer()[..response.buffer().len() - 2]); + out_data.extend_from_slice(&response.data()[..response.data().len() - 2]); } Ok(Response::new(sw.into(), out_data)) @@ -495,7 +493,7 @@ impl<'tx> Transaction<'tx> { return Err(Error::GenericError); } - let data = response.into_buffer(); + let data = Buffer::new(response.data().into()); let mut outlen = 0; if data.len() < 2 || !has_valid_length(&data[1..], data.len() - 1) { diff --git a/src/yubikey.rs b/src/yubikey.rs index 22cfeba4..6327ec07 100644 --- a/src/yubikey.rs +++ b/src/yubikey.rs @@ -35,7 +35,11 @@ #[cfg(feature = "untested")] use crate::{ - apdu::APDU, key::SlotId, metadata, mgm::MgmKey, response::StatusWords, serialization::*, + apdu::{StatusWords, APDU}, + key::SlotId, + metadata, + mgm::MgmKey, + serialization::*, ObjectId, }; use crate::{consts::*, error::Error, transaction::Transaction, Buffer}; @@ -271,12 +275,12 @@ impl YubiKey { .data(&[0x7c, 0x02, 0x80, 0x00]) .transmit(&txn, 261)?; - if !challenge.is_success() || challenge.buffer().len() < 12 { + if !challenge.is_success() || challenge.data().len() < 12 { return Err(Error::AuthenticationError); } // send a response to the cards challenge and a challenge of our own. - let response = mgm_key.decrypt(challenge.buffer()[4..12].try_into().unwrap()); + let response = mgm_key.decrypt(challenge.data()[4..12].try_into().unwrap()); let mut data = [0u8; 22]; data[0] = 0x7c; @@ -308,7 +312,7 @@ impl YubiKey { let response = mgm_key.encrypt(&challenge); use subtle::ConstantTimeEq; - if response.ct_eq(&authentication.buffer()[4..12]).unwrap_u8() != 1 { + if response.ct_eq(&authentication.data()[4..12]).unwrap_u8() != 1 { return Err(Error::AuthenticationError); } @@ -803,11 +807,11 @@ impl YubiKey { } } - if response.buffer()[0] != 0x30 { + if response.data()[0] != 0x30 { return Err(Error::GenericError); } - Ok(response.into_buffer()) + Ok(Buffer::new(response.data().into())) } /// Get an auth challenge @@ -824,7 +828,7 @@ impl YubiKey { return Err(Error::AuthenticationError); } - Ok(response.buffer()[4..12].try_into().unwrap()) + Ok(response.data()[4..12].try_into().unwrap()) } /// Verify an auth response