From 48d0a2ab04129fddd3cab87d71b29d608b299878 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Thu, 28 Nov 2019 17:09:37 +0000 Subject: [PATCH 1/8] Use slice::copy_from_slice in Transaction::change_pin --- src/transaction.rs | 31 +++---------------------------- 1 file changed, 3 insertions(+), 28 deletions(-) diff --git a/src/transaction.rs b/src/transaction.rs index a55e1c4..ebe5a01 100644 --- a/src/transaction.rs +++ b/src/transaction.rs @@ -16,8 +16,6 @@ use crate::{ use log::{error, trace}; use std::convert::TryInto; #[cfg(feature = "untested")] -use std::ptr; -#[cfg(feature = "untested")] use zeroize::Zeroizing; /// Exclusive transaction with the YubiKey's PC/SC card. @@ -187,7 +185,6 @@ impl<'tx> Transaction<'tx> { #[cfg(feature = "untested")] pub fn change_pin(&self, action: i32, current_pin: &[u8], new_pin: &[u8]) -> Result<(), Error> { let mut templ = [0, Ins::ChangeReference.code(), 0, 0x80]; - let mut indata = Zeroizing::new([0u8; 16]); if current_pin.len() > CB_PIN_MAX || new_pin.len() > CB_PIN_MAX { return Err(Error::SizeError); @@ -199,31 +196,9 @@ impl<'tx> Transaction<'tx> { templ[3] = 0x81; } - unsafe { - ptr::copy(current_pin.as_ptr(), indata.as_mut_ptr(), current_pin.len()); - - if current_pin.len() < CB_PIN_MAX { - ptr::write_bytes( - indata.as_mut_ptr().add(current_pin.len()), - 0xff, - CB_PIN_MAX - current_pin.len(), - ); - } - - ptr::copy( - new_pin.as_ptr(), - indata.as_mut_ptr().offset(8), - new_pin.len(), - ); - - if new_pin.len() < CB_PIN_MAX { - ptr::write_bytes( - indata.as_mut_ptr().offset(8).add(new_pin.len()), - 0xff, - CB_PIN_MAX - new_pin.len(), - ); - } - } + let mut indata = Zeroizing::new([0xff; CB_PIN_MAX * 2]); + indata[0..current_pin.len()].copy_from_slice(current_pin); + indata[CB_PIN_MAX..CB_PIN_MAX + new_pin.len()].copy_from_slice(new_pin); let status_words = self .transfer_data(&templ, indata.as_ref(), 0xFF)? From afb6a9479e904eae086008b75913c8114c901e68 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Thu, 28 Nov 2019 17:14:36 +0000 Subject: [PATCH 2/8] Use slice::copy_within in read_certificate --- src/certificate.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/certificate.rs b/src/certificate.rs index a4e0ab8..71a3db4 100644 --- a/src/certificate.rs +++ b/src/certificate.rs @@ -40,7 +40,6 @@ use crate::{ Buffer, }; use log::error; -use std::ptr; use zeroize::Zeroizing; /// Certificates @@ -124,10 +123,7 @@ pub(crate) fn read_certificate(txn: &Transaction<'_>, slot: SlotId) -> Result Date: Thu, 28 Nov 2019 17:49:17 +0000 Subject: [PATCH 3/8] Rewrite metadata::set_item without unsafe Also re-introduces some comments that were lost during corrosion. --- src/metadata.rs | 108 +++++++++++++++++++----------------------------- 1 file changed, 43 insertions(+), 65 deletions(-) diff --git a/src/metadata.rs b/src/metadata.rs index 7249ad3..5c8def9 100644 --- a/src/metadata.rs +++ b/src/metadata.rs @@ -83,35 +83,25 @@ pub(crate) fn set_item( tag: u8, p_item: &[u8], ) -> Result<(), Error> { - let mut p_temp: *mut u8 = data.as_mut_ptr(); let mut cb_temp: usize = 0; 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; - while p_temp < data[*pcb_data..].as_mut_ptr() { - unsafe { - tag_temp = *p_temp; - p_temp = p_temp.add(1); + let mut offset = 0; - cb_len = get_length( - slice::from_raw_parts( - p_temp, - data.as_mut_ptr() as usize + data.len() - p_temp as usize, - ), - &mut cb_temp, - ); - p_temp = p_temp.add(cb_len); + while offset < *pcb_data { + tag_temp = data[offset]; + offset += 1; - if tag_temp == tag { - break; - } + cb_len = get_length(&data[offset..], &mut cb_temp); + offset += cb_len; - p_temp = p_temp.add(cb_temp); + if tag_temp == tag { + break; } + + offset += cb_temp; } if tag_temp != tag { @@ -120,75 +110,63 @@ pub(crate) fn set_item( return Ok(()); } - unsafe { - p_temp = data.as_mut_ptr().add(*pcb_data); - cb_len = get_length_size(cb_item); + // We did not find an existing tag, append + offset = *pcb_data; + cb_len = get_length_size(cb_item); - if (*pcb_data + cb_len + cb_item) > cb_data_max { - return Err(Error::GenericError); - } - - *p_temp = tag; - p_temp = p_temp.add(1); - p_temp = p_temp.add(set_length( - slice::from_raw_parts_mut( - p_temp, - data.as_ptr() as usize + data.len() - p_temp as usize, - ), - cb_item, - )); - - ptr::copy(p_item.as_ptr(), p_temp, 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; return Ok(()); } - if cb_temp == cb_item { - unsafe { - ptr::copy(p_item.as_ptr(), p_temp, cb_item); - } + // Found tag + // Check length, if it matches, overwrite + if cb_temp == cb_item { + data[offset..offset + cb_item].copy_from_slice(p_item); return Ok(()); } - p_next = unsafe { p_temp.add(cb_temp) }; - cb_moved = (cb_item as isize - cb_temp as isize) + // Length doesn't match, expand/shrink to fit + let next_offset = offset + cb_temp; + // Must be signed to have negative offsets + let cb_moved: isize = (cb_item as isize - cb_temp as isize) + if cb_item != 0 { get_length_size(cb_item) as isize } else { + // For tag, if deleting -1 } + // Accounts for different length encoding - cb_len as isize; - if (*pcb_data + cb_moved as usize) > cb_data_max { + // If length would cause buffer overflow, return error + if (*pcb_data as isize + cb_moved) as usize > cb_data_max { return Err(Error::GenericError); } - unsafe { - ptr::copy( - p_next, - p_next.offset(cb_moved), - *pcb_data - p_next as usize - data.as_ptr() as usize, - ); - } - - *pcb_data += cb_moved as usize; + // Move remaining data + data.copy_within( + next_offset..*pcb_data, + (next_offset as isize + cb_moved) as usize, + ); + *pcb_data = (*pcb_data as isize + cb_moved) as usize; + // Re-encode item and insert if cb_item != 0 { - unsafe { - p_temp = p_temp.offset(-(cb_len as isize)); - p_temp = p_temp.add(set_length( - slice::from_raw_parts_mut( - p_temp, - data.as_ptr() as usize + data.len() - p_temp as usize, - ), - cb_item, - )); - ptr::copy(p_item.as_ptr(), p_temp, cb_item); - } + offset -= cb_len; + offset += set_length(&mut data[offset..], cb_item); + data[offset..offset + cb_item].copy_from_slice(p_item); } Ok(()) From 8b86a0f57887dbbdc314a608575493a07a159980 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Thu, 28 Nov 2019 17:55:51 +0000 Subject: [PATCH 4/8] Rewrite metadata::get_item without unsafe --- src/metadata.rs | 49 +++++++++++++++++++------------------------------ 1 file changed, 19 insertions(+), 30 deletions(-) diff --git a/src/metadata.rs b/src/metadata.rs index 5c8def9..8204346 100644 --- a/src/metadata.rs +++ b/src/metadata.rs @@ -31,48 +31,37 @@ // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. use crate::{consts::*, error::Error, serialization::*, transaction::Transaction, Buffer}; -use std::{ptr, slice}; +use std::ptr; use zeroize::Zeroizing; /// Get metadata item pub(crate) fn get_item(data: &[u8], tag: u8) -> Result<&[u8], Error> { - let mut p_temp: *const u8 = data.as_ptr(); let mut cb_temp: usize = 0; - let mut tag_temp: u8; + let mut offset = 0; - unsafe { - while p_temp < data.as_ptr().add(data.len()) { - tag_temp = *p_temp; - p_temp = p_temp.add(1); + while offset < data.len() { + let tag_temp = data[offset]; + offset += 1; - let p_slice = slice::from_raw_parts( - p_temp, - data.as_ptr() as usize + data.len() - p_temp as usize, - ); - - if !has_valid_length( - p_slice, - data.as_ptr().add(data.len()) as usize - p_temp as usize, - ) { - return Err(Error::SizeError); - } - - p_temp = p_temp.add(get_length(p_slice, &mut cb_temp)); - - if tag_temp == tag { - // found tag - break; - } - - p_temp = p_temp.add(cb_temp); + if !has_valid_length(&data[offset..], data.len() - 1) { + return Err(Error::SizeError); } - if p_temp < data.as_ptr().add(data.len()) { - return Ok(slice::from_raw_parts(p_temp, cb_temp)); + offset += get_length(&data[offset..], &mut cb_temp); + + if tag_temp == tag { + // found tag + break; } + + offset += cb_temp; } - Err(Error::GenericError) + if offset < data.len() { + Ok(&data[offset..offset + cb_temp]) + } else { + Err(Error::GenericError) + } } /// Set metadata item From 7c08674facd15592233545c13c4594a1a4728c7a Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Thu, 28 Nov 2019 17:58:19 +0000 Subject: [PATCH 5/8] Use slice::copy_within in metadata::read --- src/metadata.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/metadata.rs b/src/metadata.rs index 8204346..5cadbc5 100644 --- a/src/metadata.rs +++ b/src/metadata.rs @@ -31,7 +31,6 @@ // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. use crate::{consts::*, error::Error, serialization::*, transaction::Transaction, Buffer}; -use std::ptr; use zeroize::Zeroizing; /// Get metadata item @@ -186,10 +185,7 @@ pub(crate) fn read(txn: &Transaction<'_>, tag: u8) -> Result { return Err(Error::GenericError); } - unsafe { - ptr::copy(data.as_ptr().add(offset), data.as_mut_ptr(), pcb_data); - } - + data.copy_within(offset..offset + pcb_data, 0); data.truncate(pcb_data); Ok(data) } From 1935216cf33b3e99653f9e450baa8784ee8d41cb Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Thu, 28 Nov 2019 18:13:19 +0000 Subject: [PATCH 6/8] Rewrite MsRoots::read without unsafe --- src/msroots.rs | 52 +++++++++++++++----------------------------------- 1 file changed, 15 insertions(+), 37 deletions(-) diff --git a/src/msroots.rs b/src/msroots.rs index f05ed70..c6911d0 100644 --- a/src/msroots.rs +++ b/src/msroots.rs @@ -39,7 +39,6 @@ use crate::{consts::*, error::Error, serialization::*, yubikey::YubiKey}; use log::error; -use std::{ptr, slice}; /// `msroots` file: PKCS#7-formatted certificate store for enterprise trust roots pub struct MsRoots(Vec); @@ -51,33 +50,22 @@ impl MsRoots { } /// Read `msroots` file from YubiKey - pub fn read(yubikey: &mut YubiKey) -> Result, Error> { - let mut len: usize = 0; - let mut ptr: *mut u8; - let mut tag: u8; - let mut offset: usize = 0; - - let mut results = vec![]; + pub fn read(yubikey: &mut YubiKey) -> Result, Error> { let cb_data = yubikey.obj_size_max(); let txn = yubikey.begin_transaction()?; // allocate first page - let mut p_data = vec![0u8; cb_data]; + let mut data = Vec::with_capacity(cb_data); for object_id in YKPIV_OBJ_MSROOTS1..YKPIV_OBJ_MSROOTS5 { - let mut buf = txn.fetch_object(object_id)?; + let buf = txn.fetch_object(object_id)?; let cb_buf = buf.len(); - ptr = buf.as_mut_ptr(); - if cb_buf < CB_OBJ_TAG_MIN { - return Ok(results); + return Ok(None); } - unsafe { - tag = *ptr; - ptr = ptr.add(1); - } + let tag = buf[0]; if (TAG_MSROOTS_MID != tag || YKPIV_OBJ_MSROOTS5 == object_id) && (TAG_MSROOTS_END != tag) @@ -85,38 +73,28 @@ impl MsRoots { // the current object doesn't contain a valid part of a msroots file // treat condition as object isn't found - return Ok(results); + return Ok(None); } - unsafe { - ptr = ptr.add(get_length( - slice::from_raw_parts(ptr, buf.as_ptr() as usize + buf.len() - ptr as usize), - &mut len, - )); - } + let mut len: usize = 0; + let offset = 1 + get_length(&buf[1..], &mut len); // check that decoded length represents object contents - if len > cb_buf - (ptr as isize - buf.as_mut_ptr() as isize) as usize { - return Ok(results); + if len > cb_buf - offset { + return Ok(None); } - unsafe { - ptr::copy(ptr, p_data.as_mut_ptr().add(offset), len); - } - - offset += len; - - match MsRoots::new(&p_data[..offset]) { - Ok(msroots) => results.push(msroots), - Err(res) => error!("error parsing msroots: {:?}", res), - } + data.extend_from_slice(&buf[offset..offset + len]); if tag == TAG_MSROOTS_END { break; } } - Ok(results) + MsRoots::new(&data).map(Some).map_err(|e| { + error!("error parsing msroots: {:?}", e); + e + }) } /// Write `msroots` file to YubiKey From 8240575bb48c55a6a0fd2d9dd7d7b12af451c44c Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Thu, 28 Nov 2019 23:44:16 +0000 Subject: [PATCH 7/8] Rewrite YubiKey::import_private_key without unsafe --- src/yubikey.rs | 189 ++++++++++++++++--------------------------------- 1 file changed, 62 insertions(+), 127 deletions(-) diff --git a/src/yubikey.rs b/src/yubikey.rs index c6c4cae..ec38cfb 100644 --- a/src/yubikey.rs +++ b/src/yubikey.rs @@ -51,7 +51,6 @@ use std::fmt::{self, Display}; #[cfg(feature = "untested")] use std::{ convert::TryInto, - ptr, slice, time::{SystemTime, UNIX_EPOCH}, }; #[cfg(feature = "untested")] @@ -618,177 +617,113 @@ impl YubiKey { pin_policy: u8, touch_policy: u8, ) -> Result<(), Error> { - // TODO(tarcieri): get rid of legacy pointers - let (p, p_len) = match p { - Some(slice) => (slice.as_ptr(), slice.len()), - None => (ptr::null(), 0), - }; - - let (q, q_len) = match q { - Some(slice) => (slice.as_ptr(), slice.len()), - None => (ptr::null(), 0), - }; - - let (dp, dp_len) = match dp { - Some(slice) => (slice.as_ptr(), slice.len()), - None => (ptr::null(), 0), - }; - - let (dq, dq_len) = match dq { - Some(slice) => (slice.as_ptr(), slice.len()), - None => (ptr::null(), 0), - }; - - let (qinv, qinv_len) = match qinv { - Some(slice) => (slice.as_ptr(), slice.len()), - None => (ptr::null(), 0), - }; - - let (ec_data, ec_data_len) = match ec_data { - Some(slice) => (slice.as_ptr(), slice.len()), - None => (ptr::null(), 0), - }; - let mut key_data = Zeroizing::new(vec![0u8; 1024]); - let mut in_ptr: *mut u8 = key_data.as_mut_ptr(); let templ = [0, Ins::ImportKey.code(), algorithm, key]; - let mut elem_len: u32 = 0; - let mut params: [*const u8; 5] = [ptr::null(); 5]; - let mut lens = [0usize; 5]; - let n_params: u8; - let param_tag: i32; if key == YKPIV_KEY_CARDMGM || key < YKPIV_KEY_RETIRED1 - || key > YKPIV_KEY_RETIRED20 && (key < YKPIV_KEY_AUTHENTICATION) - || key > YKPIV_KEY_CARDAUTH && (key != YKPIV_KEY_ATTESTATION) + || (key > YKPIV_KEY_RETIRED20 && key < YKPIV_KEY_AUTHENTICATION) + || (key > YKPIV_KEY_CARDAUTH && key != YKPIV_KEY_ATTESTATION) { return Err(Error::KeyError); } if pin_policy != YKPIV_PINPOLICY_DEFAULT - && (pin_policy != YKPIV_PINPOLICY_NEVER) - && (pin_policy != YKPIV_PINPOLICY_ONCE) - && (pin_policy != YKPIV_PINPOLICY_ALWAYS) + && pin_policy != YKPIV_PINPOLICY_NEVER + && pin_policy != YKPIV_PINPOLICY_ONCE + && pin_policy != YKPIV_PINPOLICY_ALWAYS { return Err(Error::GenericError); } if touch_policy != YKPIV_TOUCHPOLICY_DEFAULT - && (touch_policy != YKPIV_TOUCHPOLICY_NEVER) - && (touch_policy != YKPIV_TOUCHPOLICY_ALWAYS) - && (touch_policy != YKPIV_TOUCHPOLICY_CACHED) + && touch_policy != YKPIV_TOUCHPOLICY_NEVER + && touch_policy != YKPIV_TOUCHPOLICY_ALWAYS + && touch_policy != YKPIV_TOUCHPOLICY_CACHED { return Err(Error::GenericError); } - match algorithm { - YKPIV_ALGO_RSA1024 | YKPIV_ALGO_RSA2048 => { - if p_len + q_len + dp_len + dq_len + qinv_len >= 1024 { - return Err(Error::SizeError); - } else { - if algorithm == YKPIV_ALGO_RSA1024 { - elem_len = 64; + let (elem_len, params, param_tag) = match algorithm { + YKPIV_ALGO_RSA1024 | YKPIV_ALGO_RSA2048 => match (p, q, dp, dq, qinv) { + (Some(p), Some(q), Some(dp), Some(dq), Some(qinv)) => { + if p.len() + q.len() + dp.len() + dq.len() + qinv.len() >= key_data.len() { + return Err(Error::SizeError); } - if algorithm == YKPIV_ALGO_RSA2048 { - elem_len = 128; + ( + match algorithm { + YKPIV_ALGO_RSA1024 => 64, + YKPIV_ALGO_RSA2048 => 128, + }, + vec![p, q, dp, dq, qinv], + 0x01, + ) + } + _ => return Err(Error::GenericError), + }, + YKPIV_ALGO_ECCP256 | YKPIV_ALGO_ECCP384 => match ec_data { + Some(ec_data) => { + if ec_data.len() >= key_data.len() { + // This can never be true, but check to be explicit. + return Err(Error::SizeError); } - if p.is_null() || q.is_null() || dp.is_null() || dq.is_null() || qinv.is_null() - { - return Err(Error::GenericError); - } - - params[0] = p; - lens[0] = p_len; - params[1] = q; - lens[1] = q_len; - params[2] = dp; - lens[2] = dp_len; - params[3] = dq; - lens[3] = dq_len; - params[4] = qinv; - lens[4] = qinv_len; - param_tag = 0x1; - n_params = 5u8; + ( + match algorithm { + YKPIV_ALGO_ECCP256 => 32, + YKPIV_ALGO_ECCP384 => 48, + }, + vec![ec_data], + 0x06, + ) } - } - YKPIV_ALGO_ECCP256 | YKPIV_ALGO_ECCP384 => { - if ec_data_len >= key_data.len() { - return Err(Error::SizeError); - } - - if algorithm == YKPIV_ALGO_ECCP256 { - elem_len = 32; - } else if algorithm == YKPIV_ALGO_ECCP384 { - elem_len = 48; - } - - if ec_data.is_null() { - return Err(Error::GenericError); - } - - params[0] = ec_data; - lens[0] = ec_data_len; - param_tag = 0x6; - n_params = 1; - } + _ => return Err(Error::GenericError), + }, _ => return Err(Error::AlgorithmError), - } + }; - for i in 0..n_params { - unsafe { - *in_ptr = (param_tag + i as i32) as u8; - in_ptr = in_ptr.offset(1); + let mut offset = 0; - in_ptr = in_ptr.add(set_length( - slice::from_raw_parts_mut( - in_ptr, - key_data.as_mut_ptr() as usize - in_ptr as usize, - ), - elem_len as usize, - )); - } + for (i, param) in params.into_iter().enumerate() { + key_data[offset] = param_tag + i as u8; + offset += 1; - let padding = elem_len as usize - lens[i as usize]; - let remaining = (key_data.as_mut_ptr() as usize) + 1024 - in_ptr as usize; + 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); } - unsafe { - ptr::write_bytes(in_ptr, 0, padding); - in_ptr = in_ptr.add(padding); - ptr::copy(params[i as usize], in_ptr, lens[i as usize]); - in_ptr = in_ptr.add(lens[i as usize]); + 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(); } if pin_policy != YKPIV_PINPOLICY_DEFAULT { - unsafe { - *in_ptr = YKPIV_PINPOLICY_TAG; - *in_ptr.add(1) = 0x01; - *in_ptr.add(2) = pin_policy; - in_ptr = in_ptr.add(3); - } + key_data[offset] = YKPIV_PINPOLICY_TAG; + key_data[offset + 1] = 0x01; + key_data[offset + 2] = pin_policy; + offset += 3; } if touch_policy != YKPIV_TOUCHPOLICY_DEFAULT { - unsafe { - *in_ptr = YKPIV_TOUCHPOLICY_TAG; - *in_ptr.add(1) = 0x01; - *in_ptr.add(2) = touch_policy; - in_ptr = in_ptr.add(3); - } + key_data[offset] = YKPIV_TOUCHPOLICY_TAG; + key_data[offset + 1] = 0x01; + key_data[offset + 2] = touch_policy; + offset += 3; } let txn = self.begin_transaction()?; - let len = in_ptr as usize - key_data.as_mut_ptr() as usize; let status_words = txn - .transfer_data(&templ, &key_data[..len], 256)? + .transfer_data(&templ, &key_data[..offset], 256)? .status_words(); match status_words { From 1db929c10fd7e87e89d07dbe329679c39e2f663c Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Fri, 29 Nov 2019 00:09:08 +0000 Subject: [PATCH 8/8] Mark excluded nested match branches as unreachable --- src/yubikey.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/yubikey.rs b/src/yubikey.rs index ec38cfb..f1f5c0f 100644 --- a/src/yubikey.rs +++ b/src/yubikey.rs @@ -655,6 +655,7 @@ impl YubiKey { match algorithm { YKPIV_ALGO_RSA1024 => 64, YKPIV_ALGO_RSA2048 => 128, + _ => unreachable!(), }, vec![p, q, dp, dq, qinv], 0x01, @@ -673,6 +674,7 @@ impl YubiKey { match algorithm { YKPIV_ALGO_ECCP256 => 32, YKPIV_ALGO_ECCP384 => 48, + _ => unreachable!(), }, vec![ec_data], 0x06,