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

More of @str4d's suggested changes

Co-Authored-By: str4d <thestr4d@gmail.com>
---
 src/transaction.rs | 16 ++++++++--------
 src/yubikey.rs     | 16 ++++++++++------
 2 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/src/transaction.rs b/src/transaction.rs
index c3bb244..eab3801 100644
--- a/src/transaction.rs
+++ b/src/transaction.rs
@@ -187,7 +187,7 @@ impl<'tx> Transaction<'tx> {
         let mut templ = [0, YKPIV_INS_CHANGE_REFERENCE, 0, 0x80];
         let mut indata = Zeroizing::new([0u8; 16]);
 
-        if current_pin.len() > 8 || new_pin.len() > 8 {
+        if current_pin.len() > CB_PIN_MAX || new_pin.len() > CB_PIN_MAX {
             return Err(Error::SizeError);
         }
 
@@ -200,11 +200,11 @@ impl<'tx> Transaction<'tx> {
         unsafe {
             ptr::copy(current_pin.as_ptr(), indata.as_mut_ptr(), current_pin.len());
 
-            if current_pin.len() < 8 {
+            if current_pin.len() < CB_PIN_MAX {
                 ptr::write_bytes(
                     indata.as_mut_ptr().add(current_pin.len()),
                     0xff,
-                    8 - current_pin.len(),
+                    CB_PIN_MAX - current_pin.len(),
                 );
             }
 
@@ -214,17 +214,17 @@ impl<'tx> Transaction<'tx> {
                 new_pin.len(),
             );
 
-            if new_pin.len() < 8 {
+            if new_pin.len() < CB_PIN_MAX {
                 ptr::write_bytes(
                     indata.as_mut_ptr().offset(8).add(new_pin.len()),
                     0xff,
-                    8 - new_pin.len(),
+                    CB_PIN_MAX - new_pin.len(),
                 );
             }
         }
 
         let status_words = self
-            .transfer_data(&templ, indata.as_ref(), 255)?
+            .transfer_data(&templ, indata.as_ref(), 0xFF)?
             .status_words();
 
         match status_words {
@@ -428,7 +428,7 @@ impl<'tx> Transaction<'tx> {
 
             if out_data.len() - response.buffer().len() - 2 > max_out {
                 error!(
-                    "output buffer to small: wanted to write {}, max was {}",
+                    "output buffer too small: wanted to write {}, max was {}",
                     out_data.len() - response.buffer().len() - 2,
                     max_out
                 );
@@ -444,7 +444,7 @@ impl<'tx> Transaction<'tx> {
             }
         }
 
-        while sw >> 8 != 0x61 {
+        while sw >> 8 == 0x61 {
             trace!(
                 "The card indicates there is {} bytes more data for us",
                 sw & 0xff
diff --git a/src/yubikey.rs b/src/yubikey.rs
index 525c8a7..8011426 100644
--- a/src/yubikey.rs
+++ b/src/yubikey.rs
@@ -379,7 +379,7 @@ impl YubiKey {
     pub unsafe fn change_pin(&mut self, current_pin: &[u8], new_pin: &[u8]) -> Result<(), Error> {
         {
             let txn = self.begin_transaction()?;
-            txn.change_pin(0, current_pin, new_pin)?;
+            txn.change_pin(CHREF_ACT_CHANGE_PIN, current_pin, new_pin)?;
         }
 
         if !new_pin.is_empty() {
@@ -391,7 +391,7 @@ impl YubiKey {
 
     /// Set PIN last changed
     pub unsafe fn set_pin_last_changed(yubikey: &mut YubiKey) -> Result<(), Error> {
-        let mut data = [0u8; YKPIV_OBJ_MAX_SIZE];
+        let mut data = [0u8; CB_BUF_MAX];
         let max_size = yubikey.obj_size_max();
         let txn = yubikey.begin_transaction()?;
 
@@ -435,7 +435,7 @@ impl YubiKey {
     /// The default PUK code is 12345678.
     pub unsafe fn change_puk(&mut self, current_puk: &[u8], new_puk: &[u8]) -> Result<(), Error> {
         let txn = self.begin_transaction()?;
-        txn.change_pin(2, current_puk, new_puk)
+        txn.change_pin(CHREF_ACT_CHANGE_PUK, current_puk, new_puk)
     }
 
     /// Block PUK: permanently prevent the PIN from becoming unblocked
@@ -449,7 +449,7 @@ impl YubiKey {
 
         while tries_remaining != 0 {
             // 2 -> change puk
-            let res = txn.change_pin(2, &puk, &puk);
+            let res = txn.change_pin(CHREF_ACT_CHANGE_PUK, &puk, &puk);
 
             match res {
                 Ok(()) => puk[0] += 1,
@@ -458,6 +458,8 @@ impl YubiKey {
                     continue;
                 }
                 Err(e) => {
+                    // depending on the firmware, tries may not be set to zero when the PUK is blocked,
+                    // instead, the return code will be PIN_LOCKED and tries will be unset
                     if e != Error::PinLocked {
                         continue;
                     }
@@ -481,7 +483,7 @@ impl YubiKey {
         }
 
         flags[0] |= ADMIN_FLAGS_1_PUK_BLOCKED;
-        let mut data = [0u8; YKPIV_OBJ_MAX_SIZE];
+        let mut data = [0u8; CB_BUF_MAX];
         let mut cb_data: usize = data.len();
 
         if metadata::set_item(
@@ -507,7 +509,7 @@ impl YubiKey {
     /// configured PIN Unblocking Key (PUK).
     pub unsafe fn unblock_pin(&mut self, puk: &[u8], new_pin: &[u8]) -> Result<(), Error> {
         let txn = self.begin_transaction()?;
-        txn.change_pin(1, puk, new_pin)
+        txn.change_pin(CHREF_ACT_UNBLOCK_PIN, puk, new_pin)
     }
 
     /// Fetch an object from the YubiKey
@@ -815,6 +817,8 @@ impl YubiKey {
     /// Reset YubiKey.
     ///
     /// WARNING: this is a destructive operation which will destroy all keys!
+    ///
+    /// The reset function is only available when both pins are blocked.
     pub fn reset_device(&mut self) -> Result<(), Error> {
         let templ = [0, YKPIV_INS_RESET, 0, 0];
         let txn = self.begin_transaction()?;