From e18828d0487a7df9065f49a48ca2a8ac2e43fd13 Mon Sep 17 00:00:00 2001
From: Tony Arcieri <bascule@gmail.com>
Date: Mon, 25 Nov 2019 07:19:20 -0800
Subject: [PATCH] Apply suggestions from code review

@str4d's suggested fixes

Co-Authored-By: str4d <thestr4d@gmail.com>
---
 src/cccid.rs     | 2 +-
 src/config.rs    | 1 +
 src/consts.rs    | 5 ++++-
 src/container.rs | 2 +-
 src/key.rs       | 6 ++++--
 src/metadata.rs  | 3 +++
 src/msroots.rs   | 7 ++++---
 7 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/src/cccid.rs b/src/cccid.rs
index 4de4cca..ceb30d8 100644
--- a/src/cccid.rs
+++ b/src/cccid.rs
@@ -70,7 +70,7 @@ impl CCCID {
         }
 
         let mut cccid = [0u8; YKPIV_CCCID_SIZE];
-        cccid.copy_from_slice(&response[CHUID_GUID_OFFS..(CHUID_GUID_OFFS + YKPIV_CCCID_SIZE)]);
+        cccid.copy_from_slice(&response[CCC_ID_OFFS..(CCC_ID_OFFS + YKPIV_CCCID_SIZE)]);
         Ok(CCCID(cccid))
     }
 
diff --git a/src/config.rs b/src/config.rs
index 1524d50..a368754 100644
--- a/src/config.rs
+++ b/src/config.rs
@@ -117,6 +117,7 @@ impl Config {
                     );
                 }
 
+                // Always favor protected MGM
                 config.mgm_type = MgmType::Protected;
             }
         }
diff --git a/src/consts.rs b/src/consts.rs
index 4321488..15ecfc0 100644
--- a/src/consts.rs
+++ b/src/consts.rs
@@ -45,11 +45,14 @@ pub const CB_ADMIN_SALT: usize = 16;
 pub const CB_ATR_MAX: usize = 33;
 
 pub const CB_BUF_MAX_NEO: usize = 2048;
+pub const CB_BUF_MAX_YK4: usize = 3072;
+pub const CB_BUF_MAX: usize = CB_BUF_MAX_YK4;
 
 pub const CB_ECC_POINTP256: usize = 65;
 pub const CB_ECC_POINTP384: usize = 97;
 
-pub const CB_OBJ_MAX: usize = 3063;
+pub const CB_OBJ_MAX_YK4: usize = CB_BUF_MAX_YK4 - 9;
+pub const CB_OBJ_MAX: usize = CB_OBJ_MAX_YK4;
 pub const CB_OBJ_MAX_NEO: usize = CB_BUF_MAX_NEO - 9;
 
 pub const CB_OBJ_TAG_MIN: usize = 2; // 1 byte tag + 1 byte len
diff --git a/src/container.rs b/src/container.rs
index 8a8659a..72e6b33 100644
--- a/src/container.rs
+++ b/src/container.rs
@@ -146,7 +146,7 @@ impl Container {
             return Err(Error::ParseError);
         }
 
-        let mut name = [0u16; 40];
+        let mut name = [0u16; CONTAINER_NAME_LEN];
         let name_bytes_len = CONTAINER_NAME_LEN * 2;
 
         for (i, chunk) in bytes[..name_bytes_len].chunks_exact(2).enumerate() {
diff --git a/src/key.rs b/src/key.rs
index 64ece9a..4a8f376 100644
--- a/src/key.rs
+++ b/src/key.rs
@@ -288,9 +288,9 @@ pub fn generate(
                 return Err(Error::KeyError);
             }
             StatusWords::IncorrectParamError => {
-                if pin_policy != 0 {
+                if pin_policy != YKPIV_PINPOLICY_DEFAULT {
                     error!("{} (pin policy not supported?)", err_msg);
-                } else if touch_policy != 0 {
+                } else if touch_policy != YKPIV_TOUCHPOLICY_DEFAULT {
                     error!("{} (touch policy not supported?)", err_msg);
                 } else {
                     error!("{} (algorithm not supported?)", err_msg);
@@ -324,6 +324,7 @@ pub fn generate(
             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 {
                 error!("failed to parse public key structure (public exponent)");
@@ -352,6 +353,7 @@ pub fn generate(
                 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];
diff --git a/src/metadata.rs b/src/metadata.rs
index a1cc249..feb9705 100644
--- a/src/metadata.rs
+++ b/src/metadata.rs
@@ -81,6 +81,7 @@ pub(crate) fn set_item(
     let mut tag_temp: u8 = 0;
     let mut cb_len: usize = 0;
     let cb_item = p_item.len();
+    // Must be signed to have negative offsets
     let cb_moved: isize;
     let p_next: *mut u8;
 
@@ -108,6 +109,7 @@ pub(crate) fn set_item(
 
     if tag_temp != tag {
         if cb_item == 0 {
+            // We've been asked to delete an existing item that isn't in the blob
             return Ok(());
         }
 
@@ -238,6 +240,7 @@ pub(crate) fn write(
     };
 
     if data.is_empty() {
+        // Deleting metadata
         return txn.save_object(obj_id, &[]);
     }
 
diff --git a/src/msroots.rs b/src/msroots.rs
index afdedcd..ac427b3 100644
--- a/src/msroots.rs
+++ b/src/msroots.rs
@@ -134,7 +134,8 @@ impl MsRoots {
             return txn.save_object(YKPIV_OBJ_MSROOTS1, &[]);
         }
 
-        n_objs = (data_len / (cb_obj_max - 4)) + 1;
+        // Calculate number of objects required to store blob
+        n_objs = (data_len / (cb_obj_max - CB_OBJ_TAG_MAX)) + 1;
 
         if n_objs > 5 {
             return Err(Error::SizeError);
@@ -143,8 +144,8 @@ impl MsRoots {
         for i in 0..n_objs {
             offset = 0;
 
-            data_chunk = if cb_obj_max - 4 < data_len - data_offset {
-                cb_obj_max - 4
+            data_chunk = if cb_obj_max - CB_OBJ_TAG_MAX < data_len - data_offset {
+                cb_obj_max - CB_OBJ_TAG_MAX
             } else {
                 data_len - data_offset
             };