Extract TLV parsing into serialization::Tlv

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<Buffer, Error> {
-    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<Bu
         }
     };
 
-    if buf.len() < CB_OBJ_TAG_MIN {
+    // TODO(str4d): Check the rest of the buffer (TAG_CERT_COMPRESS and TAG_CERT_LRC)
-        // TODO(tarcieri): is this really ok?
-        return Ok(Zeroizing::new(vec![]));
-    }
-
     if buf[0] == TAG_CERT {
-        let offset = 1 + get_length(&buf[1..], &mut len);
+        Tlv::parse_single(buf, TAG_CERT).or_else(|_| {
-
-        if len > buf.len() - offset {
             // TODO(tarcieri): is this really ok?
-            return Ok(Zeroizing::new(vec![]));
+            Ok(Zeroizing::new(vec![]))
-        }
+        })
-
+    } else {
-        buf.copy_within(offset..offset + len, 0);
+        Ok(buf)
-        buf.truncate(len);
     }
-
-    Ok(buf)
 }
 
 /// Write certificate

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 (data, modulus_tlv) = Tlv::parse(response.data())?;
-            let mut len = 0;
+            if modulus_tlv.tag != TAG_RSA_MODULUS {
-
-            if data[offset] != TAG_RSA_MODULUS {
                 error!("Failed to parse public key structure (modulus)");
                 return Err(Error::ParseError);
             }
+            let modulus = modulus_tlv.value.to_vec();
 
-            offset += 1;
+            let (_, exp_tlv) = Tlv::parse(data)?;
-            offset += get_length(&data[offset..], &mut len);
+            if exp_tlv.tag != TAG_RSA_EXP {
-            let modulus = data[offset..(offset + len)].to_vec();
-            offset += len;
-
-            if data[offset] != 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];
+            if tlv.value.len() != len {
-            offset += 1;
-
-            if len_byte as usize != 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 })
         }
     }

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,
+    error::Error, serialization::*, transaction::Transaction, Buffer, TAG_ADMIN, TAG_PROTECTED,
-    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> {
+pub(crate) fn get_item(mut data: &[u8], tag: u8) -> Result<&[u8], Error> {
-    let mut cb_temp: usize = 0;
+    while !data.is_empty() {
-    let mut offset = 0;
+        let (remaining, tlv) = Tlv::parse(data)?;
+        data = remaining;
 
-    while offset < data.len() {
+        if tlv.tag == tag {
-        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 {
             // found tag
-            break;
+            return Ok(tlv.value);
         }
-
-        offset += cb_temp;
     }
 
-    if offset < data.len() {
+    Err(Error::GenericError)
-        Ok(&data[offset..offset + cb_temp])
-    } else {
-        Err(Error::GenericError)
-    }
 }
 
 /// Set metadata item
@@ -180,26 +164,8 @@ pub(crate) fn read(txn: &Transaction<'_>, tag: u8) -> Result<Buffer, Error> {
         _ => return Err(Error::InvalidObject),
     };
 
-    let mut data = txn.fetch_object(obj_id)?;
+    let data = txn.fetch_object(obj_id)?;
-
+    Tlv::parse_single(data, tag)
-    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)
 }
 
 /// Write metadata

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::{
+use crate::{error::Error, key::SlotId, serialization::*, yubikey::YubiKey, CB_OBJ_MAX};
-    error::Error, key::SlotId, serialization::*, yubikey::YubiKey, CB_OBJ_MAX, CB_OBJ_TAG_MIN,
-};
 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 {
+        let (_, tlv) = match Tlv::parse(&response) {
-            // TODO(tarcieri): is this really OK?
+            Ok(res) => res,
-            return Ok(containers);
+            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;
+        for chunk in tlv.value.chunks_exact(CONTAINER_REC_LEN) {
-        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) {
             containers.push(Container::new(chunk)?);
         }
 

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 {
+            let (_, tlv) = match Tlv::parse(&buf) {
-                return Ok(None);
+                Ok(res) => res,
-            }
+                Err(_) => return Ok(None),
+            };
 
-            let tag = buf[0];
+            if (TAG_MSROOTS_MID != tlv.tag || OBJ_MSROOTS5 == object_id)
-
+                && (TAG_MSROOTS_END != tlv.tag)
-            if (TAG_MSROOTS_MID != tag || OBJ_MSROOTS5 == object_id) && (TAG_MSROOTS_END != 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;
+            data.extend_from_slice(tlv.value);
-            let offset = 1 + get_length(&buf[1..], &mut len);
 
-            // check that decoded length represents object contents
+            if tlv.tag == TAG_MSROOTS_END {
-            if len > cb_buf - offset {
-                return Ok(None);
-            }
-
-            data.extend_from_slice(&buf[offset..offset + len]);
-
-            if tag == TAG_MSROOTS_END {
                 break;
             }
         }

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<Buffer, Error> {
+        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 {

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;
+        Ok(Buffer::new(inner_tlv.value.into()))
-        offset += get_length(&data[offset..], &mut len);
-        Ok(Buffer::new(data[offset..(offset + len)].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 (remaining, tlv) = Tlv::parse(response.data())?;
-        let mut outlen = 0;
 
-        if data.len() < 2 || !has_valid_length(&data[1..], data.len() - 1) {
+        if !remaining.is_empty() {
-            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() {
             error!(
                 "invalid length indicated in object: total len is {} but indicated length is {}",
-                data.len(),
+                tlv.value.len() + remaining.len(),
-                outlen
+                tlv.value.len()
             );
 
             return Err(Error::SizeError);
         }
 
-        Ok(Zeroizing::new(
+        Ok(Zeroizing::new(tlv.value.to_vec()))
-            data[(1 + offs)..(1 + offs + outlen)].to_vec(),
-        ))
     }
 
     /// Save an object.