diff --git a/src/certificate.rs b/src/certificate.rs index 85389a4..e920b6a 100644 --- a/src/certificate.rs +++ b/src/certificate.rs @@ -36,7 +36,7 @@ use crate::{ serialization::*, transaction::Transaction, yubikey::YubiKey, - Buffer, CB_OBJ_TAG_MIN, + Buffer, }; use elliptic_curve::weierstrass::{ curve::{NistP256, NistP384}, @@ -217,10 +217,9 @@ impl AsRef<[u8]> for Certificate { /// Read certificate pub(crate) fn read_certificate(txn: &Transaction<'_>, slot: SlotId) -> Result { - let mut len: usize = 0; let object_id = slot.object_id(); - let mut buf = match txn.fetch_object(object_id) { + let buf = match txn.fetch_object(object_id) { Ok(b) => b, Err(_) => { // TODO(tarcieri): is this really ok? @@ -228,24 +227,15 @@ pub(crate) fn read_certificate(txn: &Transaction<'_>, slot: SlotId) -> Result buf.len() - offset { + Tlv::parse_single(buf, TAG_CERT).or_else(|_| { // TODO(tarcieri): is this really ok? - return Ok(Zeroizing::new(vec![])); - } - - buf.copy_within(offset..offset + len, 0); - buf.truncate(len); + Ok(Zeroizing::new(vec![])) + }) + } else { + Ok(buf) } - - Ok(buf) } /// Write certificate diff --git a/src/key.rs b/src/key.rs index 7dbb5dd..8eaf67b 100644 --- a/src/key.rs +++ b/src/key.rs @@ -584,31 +584,22 @@ pub fn generate( } } - let data = Buffer::new(response.data().into()); - match algorithm { AlgorithmId::Rsa1024 | AlgorithmId::Rsa2048 => { - let mut offset = 5; - let mut len = 0; - - if data[offset] != TAG_RSA_MODULUS { + let (data, modulus_tlv) = Tlv::parse(response.data())?; + if modulus_tlv.tag != TAG_RSA_MODULUS { error!("Failed to parse public key structure (modulus)"); return Err(Error::ParseError); } + let modulus = modulus_tlv.value.to_vec(); - offset += 1; - offset += get_length(&data[offset..], &mut len); - let modulus = data[offset..(offset + len)].to_vec(); - offset += len; - - if data[offset] != TAG_RSA_EXP { + let (_, exp_tlv) = Tlv::parse(data)?; + if exp_tlv.tag != TAG_RSA_EXP { error!("failed to parse public key structure (public exponent)"); return Err(Error::ParseError); } + let exp = exp_tlv.value.to_vec(); - offset += 1; - offset += get_length(&data[offset..], &mut len); - let exp = data[offset..(offset + len)].to_vec(); Ok(GeneratedKey::Rsa { algorithm, modulus, @@ -616,30 +607,26 @@ pub fn generate( }) } AlgorithmId::EccP256 | AlgorithmId::EccP384 => { - let mut offset = 3; - let len = if let AlgorithmId::EccP256 = algorithm { CB_ECC_POINTP256 } else { CB_ECC_POINTP384 }; - if data[offset] != TAG_ECC_POINT { + let (_, tlv) = Tlv::parse(response.data())?; + + if tlv.tag != TAG_ECC_POINT { error!("failed to parse public key structure"); return Err(Error::ParseError); } - offset += 1; // the curve point should always be determined by the curve - let len_byte = data[offset]; - offset += 1; - - if len_byte as usize != len { + if tlv.value.len() != len { error!("unexpected length"); return Err(Error::AlgorithmError); } - let point = data[offset..(offset + len)].to_vec(); + let point = tlv.value.to_vec(); Ok(GeneratedKey::Ecc { algorithm, point }) } } diff --git a/src/metadata.rs b/src/metadata.rs index e6877b3..641ac08 100644 --- a/src/metadata.rs +++ b/src/metadata.rs @@ -31,8 +31,7 @@ // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. use crate::{ - error::Error, serialization::*, transaction::Transaction, Buffer, CB_OBJ_TAG_MIN, TAG_ADMIN, - TAG_PROTECTED, + error::Error, serialization::*, transaction::Transaction, Buffer, TAG_ADMIN, TAG_PROTECTED, }; #[cfg(feature = "untested")] @@ -45,33 +44,18 @@ pub const OBJ_ADMIN_DATA: u32 = 0x005f_ff00; pub const OBJ_PRINTED: u32 = 0x005f_c109; /// Get metadata item -pub(crate) fn get_item(data: &[u8], tag: u8) -> Result<&[u8], Error> { - let mut cb_temp: usize = 0; - let mut offset = 0; +pub(crate) fn get_item(mut data: &[u8], tag: u8) -> Result<&[u8], Error> { + while !data.is_empty() { + let (remaining, tlv) = Tlv::parse(data)?; + data = remaining; - while offset < data.len() { - let tag_temp = data[offset]; - offset += 1; - - if !has_valid_length(&data[offset..], data.len() - 1) { - return Err(Error::SizeError); - } - - offset += get_length(&data[offset..], &mut cb_temp); - - if tag_temp == tag { + if tlv.tag == tag { // found tag - break; + return Ok(tlv.value); } - - offset += cb_temp; } - if offset < data.len() { - Ok(&data[offset..offset + cb_temp]) - } else { - Err(Error::GenericError) - } + Err(Error::GenericError) } /// Set metadata item @@ -180,26 +164,8 @@ pub(crate) fn read(txn: &Transaction<'_>, tag: u8) -> Result { _ => return Err(Error::InvalidObject), }; - let mut data = txn.fetch_object(obj_id)?; - - if data.len() < CB_OBJ_TAG_MIN { - return Err(Error::GenericError); - } - - if tag != data[0] { - return Err(Error::GenericError); - } - - let mut pcb_data = 0; - let offset = 1 + get_length(&data[1..], &mut pcb_data); - - if pcb_data > data.len() - offset { - return Err(Error::GenericError); - } - - data.copy_within(offset..offset + pcb_data, 0); - data.truncate(pcb_data); - Ok(data) + let data = txn.fetch_object(obj_id)?; + Tlv::parse_single(data, tag) } /// Write metadata diff --git a/src/mscmap.rs b/src/mscmap.rs index 2511fc4..dc58c90 100644 --- a/src/mscmap.rs +++ b/src/mscmap.rs @@ -33,9 +33,7 @@ // (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, key::SlotId, serialization::*, yubikey::YubiKey, CB_OBJ_MAX, CB_OBJ_TAG_MIN, -}; +use crate::{error::Error, key::SlotId, serialization::*, yubikey::YubiKey, CB_OBJ_MAX}; use log::error; use std::{ convert::{TryFrom, TryInto}, @@ -87,25 +85,20 @@ impl Container { let response = txn.fetch_object(OBJ_MSCMAP)?; let mut containers = vec![]; - if response.len() < CB_OBJ_TAG_MIN { - // TODO(tarcieri): is this really OK? - return Ok(containers); - } + let (_, tlv) = match Tlv::parse(&response) { + Ok(res) => res, + Err(_) => { + // TODO(tarcieri): is this really OK? + return Ok(containers); + } + }; - if response[0] != TAG_MSCMAP { + if tlv.tag != TAG_MSCMAP { // TODO(tarcieri): yubico-piv-tool returned success here? should we? return Err(Error::InvalidObject); } - let mut len = 0; - let offset = 1 + get_length(&response[1..], &mut len); - - if len > response.len() - offset { - // TODO(tarcieri): is this really OK? - return Ok(containers); - } - - for chunk in response[offset..(offset + len)].chunks_exact(CONTAINER_REC_LEN) { + for chunk in tlv.value.chunks_exact(CONTAINER_REC_LEN) { containers.push(Container::new(chunk)?); } diff --git a/src/msroots.rs b/src/msroots.rs index 407c278..39c04e5 100644 --- a/src/msroots.rs +++ b/src/msroots.rs @@ -38,7 +38,7 @@ // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. use crate::{error::Error, serialization::*, yubikey::YubiKey}; -use crate::{CB_OBJ_MAX, CB_OBJ_TAG_MAX, CB_OBJ_TAG_MIN}; +use crate::{CB_OBJ_MAX, CB_OBJ_TAG_MAX}; use log::error; const OBJ_MSROOTS1: u32 = 0x005f_ff11; @@ -71,32 +71,24 @@ impl MsRoots { for object_id in OBJ_MSROOTS1..OBJ_MSROOTS5 { let buf = txn.fetch_object(object_id)?; - let cb_buf = buf.len(); - if cb_buf < CB_OBJ_TAG_MIN { - return Ok(None); - } + let (_, tlv) = match Tlv::parse(&buf) { + Ok(res) => res, + Err(_) => return Ok(None), + }; - let tag = buf[0]; - - if (TAG_MSROOTS_MID != tag || OBJ_MSROOTS5 == object_id) && (TAG_MSROOTS_END != tag) { + if (TAG_MSROOTS_MID != tlv.tag || OBJ_MSROOTS5 == object_id) + && (TAG_MSROOTS_END != tlv.tag) + { // the current object doesn't contain a valid part of a msroots file // treat condition as object isn't found return Ok(None); } - let mut len: usize = 0; - let offset = 1 + get_length(&buf[1..], &mut len); + data.extend_from_slice(tlv.value); - // check that decoded length represents object contents - if len > cb_buf - offset { - return Ok(None); - } - - data.extend_from_slice(&buf[offset..offset + len]); - - if tag == TAG_MSROOTS_END { + if tlv.tag == TAG_MSROOTS_END { break; } } diff --git a/src/serialization.rs b/src/serialization.rs index 9145f3d..794a4e3 100644 --- a/src/serialization.rs +++ b/src/serialization.rs @@ -30,12 +30,55 @@ // (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::ObjectId; +use crate::{error::Error, Buffer, ObjectId, CB_OBJ_TAG_MIN}; pub const OBJ_DISCOVERY: u32 = 0x7e; // TODO(tarcieri): refactor these into better serializers/message builders +/// A Type-Length-Value object that has been parsed from a buffer. +pub(crate) struct Tlv<'a> { + pub(crate) tag: u8, + pub(crate) value: &'a [u8], +} + +impl<'a> Tlv<'a> { + /// Parses a `Tlv` from a buffer, returning the remainder of the buffer. + pub(crate) fn parse(buffer: &'a [u8]) -> Result<(&'a [u8], Self), Error> { + if buffer.len() < CB_OBJ_TAG_MIN || !has_valid_length(&buffer[1..], buffer.len() - 1) { + return Err(Error::SizeError); + } + + let tag = buffer[0]; + + let mut len = 0; + let offset = 1 + get_length(&buffer[1..], &mut len); + + let (value, buffer) = buffer[offset..].split_at(len); + + Ok((buffer, Tlv { tag, value })) + } + + /// Takes a [`Buffer`] containing a single `Tlv` with the given tag, and returns a + /// `Buffer` containing only the value part of the `Tlv`. + pub(crate) fn parse_single(mut buffer: Buffer, tag: u8) -> Result { + if buffer.len() < CB_OBJ_TAG_MIN || !has_valid_length(&buffer[1..], buffer.len() - 1) { + return Err(Error::SizeError); + } + + if tag != buffer[0] { + return Err(Error::GenericError); + }; + + let mut len = 0; + let offset = 1 + get_length(&buffer[1..], &mut len); + + buffer.copy_within(offset..offset + len, 0); + buffer.truncate(len); + Ok(buffer) + } +} + /// Set length #[cfg(feature = "untested")] pub(crate) fn set_length(buffer: &mut [u8], length: usize) -> usize { diff --git a/src/transaction.rs b/src/transaction.rs index 3f31c0c..fa465aa 100644 --- a/src/transaction.rs +++ b/src/transaction.rs @@ -264,7 +264,6 @@ impl<'tx> Transaction<'tx> { let in_len = sign_in.len(); let mut indata = [0u8; 1024]; let templ = [0, Ins::Authenticate.code(), algorithm.into(), key.into()]; - let mut len: usize = 0; match algorithm { AlgorithmId::Rsa1024 | AlgorithmId::Rsa2048 => { @@ -331,25 +330,23 @@ impl<'tx> Transaction<'tx> { } } - let data = response.data(); + let (_, outer_tlv) = Tlv::parse(response.data())?; // skip the first 7c tag - if data[0] != 0x7c { + if outer_tlv.tag != 0x7c { error!("failed parsing signature reply (0x7c byte)"); return Err(Error::ParseError); } - let mut offset = 1 + get_length(&data[1..], &mut len); + let (_, inner_tlv) = Tlv::parse(outer_tlv.value)?; // skip the 82 tag - if data[offset] != 0x82 { + if inner_tlv.tag != 0x82 { error!("failed parsing signature reply (0x82 byte)"); return Err(Error::ParseError); } - offset += 1; - offset += get_length(&data[offset..], &mut len); - Ok(Buffer::new(data[offset..(offset + len)].into())) + Ok(Buffer::new(inner_tlv.value.into())) } /// Send/receive large amounts of data to/from the YubiKey, splitting long @@ -457,32 +454,19 @@ impl<'tx> Transaction<'tx> { } } - let data = Buffer::new(response.data().into()); - let mut outlen = 0; + let (remaining, tlv) = Tlv::parse(response.data())?; - if data.len() < 2 || !has_valid_length(&data[1..], data.len() - 1) { - return Err(Error::SizeError); - } - - let offs = get_length(&data[1..], &mut outlen); - - if offs == 0 { - return Err(Error::SizeError); - } - - if outlen + offs + 1 != data.len() { + if !remaining.is_empty() { error!( "invalid length indicated in object: total len is {} but indicated length is {}", - data.len(), - outlen + tlv.value.len() + remaining.len(), + tlv.value.len() ); return Err(Error::SizeError); } - Ok(Zeroizing::new( - data[(1 + offs)..(1 + offs + outlen)].to_vec(), - )) + Ok(Zeroizing::new(tlv.value.to_vec())) } /// Save an object.