From da828abe3cc31c9865758f0e3463d209bf3f7147 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Tue, 10 Dec 2019 12:10:13 +0000 Subject: [PATCH 1/4] Extract TLV parsing into serialization::Tlv --- src/certificate.rs | 26 +++++++-------------- src/key.rs | 35 +++++++++------------------- src/metadata.rs | 54 ++++++++------------------------------------ src/mscmap.rs | 27 ++++++++-------------- src/msroots.rs | 28 ++++++++--------------- src/serialization.rs | 45 +++++++++++++++++++++++++++++++++++- src/transaction.rs | 36 ++++++++--------------------- 7 files changed, 103 insertions(+), 148 deletions(-) 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. From 363bdc435127c662d2ec35b85fa8d9b79df54828 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Tue, 10 Dec 2019 13:17:01 +0000 Subject: [PATCH 2/4] Extract TLV writing into serialization::Tlv --- src/certificate.rs | 41 ++++++++++------------------------ src/key.rs | 52 ++++++++++++++++++-------------------------- src/metadata.rs | 25 ++++----------------- src/mscmap.rs | 25 +++++---------------- src/msroots.rs | 19 ++++++++-------- src/policy.rs | 24 +++++++------------- src/serialization.rs | 46 +++++++++++++++++++++++++++++++++++++++ src/transaction.rs | 36 ++++++++++++++---------------- 8 files changed, 122 insertions(+), 146 deletions(-) diff --git a/src/certificate.rs b/src/certificate.rs index e920b6a..3f70452 100644 --- a/src/certificate.rs +++ b/src/certificate.rs @@ -246,9 +246,6 @@ pub(crate) fn write_certificate( data: Option<&[u8]>, certinfo: u8, ) -> Result<(), Error> { - let mut buf = [0u8; CB_OBJ_MAX]; - let mut offset = 0; - let object_id = slot.object_id(); if data.is_none() { @@ -257,34 +254,20 @@ pub(crate) fn write_certificate( let data = data.unwrap(); - let mut req_len = 1 /* cert tag */ + 3 /* compression tag + data*/ + 2 /* lrc */; - req_len += set_length(&mut buf, data.len()); - req_len += data.len(); - - if req_len < data.len() || req_len > CB_OBJ_MAX { - return Err(Error::SizeError); - } - - buf[offset] = TAG_CERT; - offset += 1; - offset += set_length(&mut buf[offset..], data.len()); - - buf[offset..(offset + data.len())].copy_from_slice(&data); - - offset += data.len(); + let mut buf = [0u8; CB_OBJ_MAX]; + let mut offset = Tlv::write(&mut buf, TAG_CERT, data)?; // write compression info and LRC trailer - buf[offset] = TAG_CERT_COMPRESS; - buf[offset + 1] = 0x01; - buf[offset + 2] = if certinfo == CERTINFO_GZIP { - 0x01 - } else { - 0x00 - }; - buf[offset + 3] = TAG_CERT_LRC; - buf[offset + 4] = 00; - - offset += 5; + offset += Tlv::write( + &mut buf, + TAG_CERT_COMPRESS, + if certinfo == CERTINFO_GZIP { + &[0x01] + } else { + &[0x00] + }, + )?; + offset += Tlv::write(&mut buf, TAG_CERT_LRC, &[])?; txn.save_object(object_id, &buf[..offset]) } diff --git a/src/key.rs b/src/key.rs index 8eaf67b..84826d2 100644 --- a/src/key.rs +++ b/src/key.rs @@ -379,11 +379,8 @@ impl From for u8 { #[cfg(feature = "untested")] impl AlgorithmId { /// Writes the `AlgorithmId` in the format the YubiKey expects during key generation. - pub(crate) fn write(self, buf: &mut [u8]) -> usize { - buf[0] = 0x80; - buf[1] = 0x01; - buf[2] = self.into(); - 3 + pub(crate) fn write(self, buf: &mut [u8]) -> Result { + Tlv::write(buf, 0x80, &[self.into()]) } } @@ -539,16 +536,15 @@ pub fn generate( let templ = [0, Ins::GenerateAsymmetric.code(), 0, slot.into()]; let mut in_data = [0u8; 11]; - in_data[0] = 0xac; - in_data[1] = 3; // length sans this 2-byte header - assert_eq!(algorithm.write(&mut in_data[2..]), 3); - let mut offset = 5; + let mut offset = Tlv::write_as(&mut in_data, 0xac, 3, |buf| { + assert_eq!(algorithm.write(buf).expect("large enough"), 3); + })?; - let pin_len = pin_policy.write(&mut in_data[offset..]); + let pin_len = pin_policy.write(&mut in_data[offset..])?; in_data[1] += pin_len as u8; offset += pin_len; - let touch_len = touch_policy.write(&mut in_data[offset..]); + let touch_len = touch_policy.write(&mut in_data[offset..])?; in_data[1] += touch_len as u8; offset += touch_len; @@ -695,28 +691,22 @@ pub fn import( let mut offset = 0; for (i, param) in params.into_iter().enumerate() { - key_data[offset] = param_tag + i as u8; - offset += 1; - - offset += set_length(&mut key_data[offset..], elem_len); - - let padding = elem_len - param.len(); - let remaining = key_data.len() - offset; - - if padding > remaining { - return Err(Error::AlgorithmError); - } - - for b in &mut key_data[offset..offset + padding] { - *b = 0; - } - offset += padding; - key_data[offset..offset + param.len()].copy_from_slice(param); - offset += param.len(); + offset += Tlv::write_as( + &mut key_data[offset..], + param_tag + i as u8, + elem_len, + |buf| { + let padding = elem_len - param.len(); + for b in &mut buf[..padding] { + *b = 0; + } + buf[padding..].copy_from_slice(param); + }, + )?; } - offset += pin_policy.write(&mut key_data[offset..]); - offset += touch_policy.write(&mut key_data[offset..]); + offset += pin_policy.write(&mut key_data[offset..])?; + offset += touch_policy.write(&mut key_data[offset..])?; let txn = yubikey.begin_transaction()?; diff --git a/src/metadata.rs b/src/metadata.rs index 641ac08..e2cc3ef 100644 --- a/src/metadata.rs +++ b/src/metadata.rs @@ -95,20 +95,7 @@ pub(crate) fn set_item( } // We did not find an existing tag, append - offset = *pcb_data; - cb_len = get_length_size(cb_item); - - // If length would cause buffer overflow, return error - if (*pcb_data + cb_len + cb_item) > cb_data_max { - return Err(Error::GenericError); - } - - data[offset] = tag; - offset += 1; - offset += set_length(&mut data[offset..], cb_item); - data[offset..offset + cb_item].copy_from_slice(p_item); - - *pcb_data += 1 + cb_len + cb_item; + *pcb_data += Tlv::write(&mut data[*pcb_data..], tag, p_item)?; return Ok(()); } @@ -171,8 +158,6 @@ pub(crate) fn read(txn: &Transaction<'_>, tag: u8) -> Result { /// Write metadata #[cfg(feature = "untested")] pub(crate) fn write(txn: &Transaction<'_>, tag: u8, data: &[u8]) -> Result<(), Error> { - let mut buf = Zeroizing::new(vec![0u8; CB_OBJ_MAX]); - if data.len() > CB_OBJ_MAX - CB_OBJ_TAG_MAX { return Err(Error::GenericError); } @@ -188,12 +173,10 @@ pub(crate) fn write(txn: &Transaction<'_>, tag: u8, data: &[u8]) -> Result<(), E return txn.save_object(obj_id, &[]); } - buf[0] = tag; - let mut offset = set_length(&mut buf[1..], data.len()); - buf[offset..(offset + data.len())].copy_from_slice(data); - offset += data.len(); + let mut buf = Zeroizing::new(vec![0u8; CB_OBJ_MAX]); + let len = Tlv::write(&mut buf, tag, data)?; - txn.save_object(obj_id, &buf[..offset]) + txn.save_object(obj_id, &buf[..len]) } /// Get the size of a length tag for the given length diff --git a/src/mscmap.rs b/src/mscmap.rs index dc58c90..e8ec870 100644 --- a/src/mscmap.rs +++ b/src/mscmap.rs @@ -107,8 +107,6 @@ impl Container { /// Write MS Container Map records. pub fn write_mscmap(yubikey: &mut YubiKey, containers: &[Self]) -> Result<(), Error> { - let mut buf = [0u8; CB_OBJ_MAX]; - let mut offset = 0; let n_containers = containers.len(); let data_len = n_containers * CONTAINER_REC_LEN; @@ -118,24 +116,13 @@ impl Container { return txn.save_object(OBJ_MSCMAP, &[]); } - let req_len = 1 + set_length(&mut buf, data_len) + data_len; + let mut buf = [0u8; CB_OBJ_MAX]; + let offset = Tlv::write_as(&mut buf, TAG_MSCMAP, data_len, |buf| { + for (i, chunk) in buf.chunks_exact_mut(CONTAINER_REC_LEN).enumerate() { + chunk.copy_from_slice(&containers[i].to_bytes()); + } + })?; - if req_len > CB_OBJ_MAX { - return Err(Error::SizeError); - } - - buf[offset] = TAG_MSCMAP; - offset += 1; - offset += set_length(&mut buf[offset..], data_len); - - for (i, chunk) in buf[..data_len] - .chunks_exact_mut(CONTAINER_REC_LEN) - .enumerate() - { - chunk.copy_from_slice(&containers[i].to_bytes()); - } - - offset += data_len; txn.save_object(OBJ_MSCMAP, &buf[..offset]) } diff --git a/src/msroots.rs b/src/msroots.rs index 39c04e5..b7d65b7 100644 --- a/src/msroots.rs +++ b/src/msroots.rs @@ -130,16 +130,15 @@ impl MsRoots { data_len - data_offset }; - buf[offset] = if i == n_objs - 1 { - TAG_MSROOTS_END - } else { - TAG_MSROOTS_MID - }; - - offset += 1; - offset += set_length(&mut buf[offset..], data_chunk); - buf[offset..].copy_from_slice(&data[data_offset..(data_offset + data_chunk)]); - offset += data_chunk; + offset += Tlv::write( + &mut buf, + if i == n_objs - 1 { + TAG_MSROOTS_END + } else { + TAG_MSROOTS_MID + }, + &data[data_offset..(data_offset + data_chunk)], + )?; txn.save_object(OBJ_MSROOTS1 + i as u32, &buf[..offset])?; diff --git a/src/policy.rs b/src/policy.rs index e82c180..a5b2c8d 100644 --- a/src/policy.rs +++ b/src/policy.rs @@ -1,5 +1,7 @@ //! Enums representing key policies. +use crate::{error::Error, serialization::Tlv}; + /// Specifies how often the PIN needs to be entered for access to the credential in a /// given slot. This policy must be set upon key generation or importation, and cannot be /// changed later. @@ -36,15 +38,10 @@ impl PinPolicy { /// Writes the `PinPolicy` in the format the YubiKey expects during key generation or /// importation. #[cfg(feature = "untested")] - pub(crate) fn write(self, buf: &mut [u8]) -> usize { + pub(crate) fn write(self, buf: &mut [u8]) -> Result { match self { - PinPolicy::Default => 0, - _ => { - buf[0] = 0xaa; - buf[1] = 0x01; - buf[2] = self.into(); - 3 - } + PinPolicy::Default => Ok(0), + _ => Tlv::write(buf, 0xaa, &[self.into()]), } } } @@ -86,15 +83,10 @@ impl TouchPolicy { /// Writes the `TouchPolicy` in the format the YubiKey expects during key generation /// or importation. #[cfg(feature = "untested")] - pub(crate) fn write(self, buf: &mut [u8]) -> usize { + pub(crate) fn write(self, buf: &mut [u8]) -> Result { match self { - TouchPolicy::Default => 0, - _ => { - buf[0] = 0xab; - buf[1] = 0x01; - buf[2] = self.into(); - 3 - } + TouchPolicy::Default => Ok(0), + _ => Tlv::write(buf, 0xab, &[self.into()]), } } } diff --git a/src/serialization.rs b/src/serialization.rs index 794a4e3..55fc699 100644 --- a/src/serialization.rs +++ b/src/serialization.rs @@ -77,6 +77,52 @@ impl<'a> Tlv<'a> { buffer.truncate(len); Ok(buffer) } + + /// Writes a TLV to the given buffer. + pub(crate) fn write(buffer: &mut [u8], tag: u8, value: &[u8]) -> Result { + if buffer.len() < CB_OBJ_TAG_MIN { + return Err(Error::SizeError); + } + buffer[0] = tag; + + // TODO: Raise error + let offset = 1 + set_length(&mut buffer[1..], value.len()); + + if buffer.len() < offset + value.len() { + return Err(Error::SizeError); + } + buffer[offset..offset + value.len()].copy_from_slice(value); + + Ok(offset + value.len()) + } + + /// Writes a TLV to the given buffer. + /// + /// `value` is guaranteed to be called with a mutable slice of length `length`. + pub(crate) fn write_as( + buffer: &mut [u8], + tag: u8, + length: usize, + value: Gen, + ) -> Result + where + Gen: FnOnce(&mut [u8]), + { + if buffer.len() < CB_OBJ_TAG_MIN { + return Err(Error::SizeError); + } + buffer[0] = tag; + + // TODO: Raise error + let offset = 1 + set_length(&mut buffer[1..], length); + + if buffer.len() < offset + length { + return Err(Error::SizeError); + } + value(&mut buffer[offset..offset + length]); + + Ok(offset + length) + } } /// Set length diff --git a/src/transaction.rs b/src/transaction.rs index fa465aa..d2dd9d9 100644 --- a/src/transaction.rs +++ b/src/transaction.rs @@ -299,19 +299,21 @@ impl<'tx> Transaction<'tx> { 3 }; - indata[0] = 0x7c; - let mut offset = 1 + set_length(&mut indata[1..], in_len + bytes + 3); - indata[offset] = 0x82; - indata[offset + 1] = 0x00; - indata[offset + 2] = match (algorithm, decipher) { - (AlgorithmId::EccP256, true) | (AlgorithmId::EccP384, true) => 0x85, - _ => 0x81, - }; - - offset += 3; - offset += set_length(&mut indata[offset..], in_len); - indata[offset..(offset + in_len)].copy_from_slice(sign_in); - offset += in_len; + let offset = Tlv::write_as(&mut indata, 0x7c, in_len + bytes + 3, |buf| { + assert_eq!(Tlv::write(buf, 0x82, &[]).expect("large enough"), 2); + assert_eq!( + Tlv::write( + &mut buf[2..], + match (algorithm, decipher) { + (AlgorithmId::EccP256, true) | (AlgorithmId::EccP384, true) => 0x85, + _ => 0x81, + }, + sign_in + ) + .expect("large enough"), + 1 + bytes + in_len + ); + })?; let response = self .transfer_data(&templ, &indata[..offset], 1024) @@ -484,14 +486,8 @@ impl<'tx> Transaction<'tx> { let mut len = data.len(); let mut data_remaining = set_object(object_id, &mut data); - data_remaining[0] = 0x53; - data_remaining = &mut data_remaining[1..]; - - let offset = set_length(data_remaining, indata.len()); + let offset = Tlv::write(data_remaining, 0x53, indata)?; data_remaining = &mut data_remaining[offset..]; - data_remaining[..indata.len()].copy_from_slice(indata); - - data_remaining = &mut data_remaining[indata.len()..]; len -= data_remaining.len(); let status_words = self From 8385dda201d97b3aef8b7af31fec8c6542d53db4 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Tue, 10 Dec 2019 13:22:21 +0000 Subject: [PATCH 3/4] Check buffer length in set_length --- src/metadata.rs | 2 +- src/serialization.rs | 30 +++++++++++++++++++----------- 2 files changed, 20 insertions(+), 12 deletions(-) diff --git a/src/metadata.rs b/src/metadata.rs index e2cc3ef..5c513cb 100644 --- a/src/metadata.rs +++ b/src/metadata.rs @@ -136,7 +136,7 @@ pub(crate) fn set_item( // Re-encode item and insert if cb_item != 0 { offset -= cb_len; - offset += set_length(&mut data[offset..], cb_item); + offset += set_length(&mut data[offset..], cb_item)?; data[offset..offset + cb_item].copy_from_slice(p_item); } diff --git a/src/serialization.rs b/src/serialization.rs index 55fc699..a12cd75 100644 --- a/src/serialization.rs +++ b/src/serialization.rs @@ -85,8 +85,7 @@ impl<'a> Tlv<'a> { } buffer[0] = tag; - // TODO: Raise error - let offset = 1 + set_length(&mut buffer[1..], value.len()); + let offset = 1 + set_length(&mut buffer[1..], value.len())?; if buffer.len() < offset + value.len() { return Err(Error::SizeError); @@ -113,8 +112,7 @@ impl<'a> Tlv<'a> { } buffer[0] = tag; - // TODO: Raise error - let offset = 1 + set_length(&mut buffer[1..], length); + let offset = 1 + set_length(&mut buffer[1..], length)?; if buffer.len() < offset + length { return Err(Error::SizeError); @@ -127,19 +125,29 @@ impl<'a> Tlv<'a> { /// Set length #[cfg(feature = "untested")] -pub(crate) fn set_length(buffer: &mut [u8], length: usize) -> usize { +pub(crate) fn set_length(buffer: &mut [u8], length: usize) -> Result { if length < 0x80 { - buffer[0] = length as u8; - 1 + if buffer.is_empty() { + Err(Error::SizeError) + } else { + buffer[0] = length as u8; + Ok(1) + } } else if length < 0x100 { - buffer[0] = 0x81; - buffer[1] = length as u8; - 2 + if buffer.len() < 2 { + Err(Error::SizeError) + } else { + buffer[0] = 0x81; + buffer[1] = length as u8; + Ok(2) + } + } else if buffer.len() < 3 { + Err(Error::SizeError) } else { buffer[0] = 0x82; buffer[1] = ((length >> 8) & 0xff) as u8; buffer[2] = (length & 0xff) as u8; - 3 + Ok(3) } } From 1bf3b13e525fa198510774259e8f771d550bf55e Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Tue, 10 Dec 2019 13:31:48 +0000 Subject: [PATCH 4/4] Add missing untested feature gates --- src/policy.rs | 1 + src/serialization.rs | 2 ++ 2 files changed, 3 insertions(+) diff --git a/src/policy.rs b/src/policy.rs index a5b2c8d..07fd4ce 100644 --- a/src/policy.rs +++ b/src/policy.rs @@ -1,5 +1,6 @@ //! Enums representing key policies. +#[cfg(feature = "untested")] use crate::{error::Error, serialization::Tlv}; /// Specifies how often the PIN needs to be entered for access to the credential in a diff --git a/src/serialization.rs b/src/serialization.rs index a12cd75..06b24c1 100644 --- a/src/serialization.rs +++ b/src/serialization.rs @@ -79,6 +79,7 @@ impl<'a> Tlv<'a> { } /// Writes a TLV to the given buffer. + #[cfg(feature = "untested")] pub(crate) fn write(buffer: &mut [u8], tag: u8, value: &[u8]) -> Result { if buffer.len() < CB_OBJ_TAG_MIN { return Err(Error::SizeError); @@ -98,6 +99,7 @@ impl<'a> Tlv<'a> { /// Writes a TLV to the given buffer. /// /// `value` is guaranteed to be called with a mutable slice of length `length`. + #[cfg(feature = "untested")] pub(crate) fn write_as( buffer: &mut [u8], tag: u8,