From e73607e662820339d3ae74eefdfb5099f516df29 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Sun, 8 Dec 2019 17:12:19 +0000 Subject: [PATCH 1/5] Rename Certificate::new to Certificate::from_bytes --- src/certificate.rs | 4 ++-- src/key.rs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/certificate.rs b/src/certificate.rs index 3f70452..133b5b5 100644 --- a/src/certificate.rs +++ b/src/certificate.rs @@ -152,7 +152,7 @@ impl Certificate { return Err(Error::InvalidObject); } - Certificate::new(buf) + Certificate::from_bytes(buf) } /// Write this certificate into the YubiKey in the given slot @@ -170,7 +170,7 @@ impl Certificate { } /// Initialize a local certificate struct from the given bytebuffer - pub fn new(cert: impl Into) -> Result { + pub fn from_bytes(cert: impl Into) -> Result { let cert = cert.into(); if cert.is_empty() { diff --git a/src/key.rs b/src/key.rs index 84826d2..4aadbcf 100644 --- a/src/key.rs +++ b/src/key.rs @@ -410,7 +410,7 @@ impl Key { }; if !buf.is_empty() { - let cert = Certificate::new(buf)?; + let cert = Certificate::from_bytes(buf)?; keys.push(Key { slot, cert }); } } From 4c2ecea7210d51f1d51214a5c8f9757724b511f6 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Sun, 8 Dec 2019 17:23:17 +0000 Subject: [PATCH 2/5] Replace GeneratedKey with PublicKeyInfo --- src/key.rs | 61 +++++++++++++++++------------------------------------- 1 file changed, 19 insertions(+), 42 deletions(-) diff --git a/src/key.rs b/src/key.rs index 4aadbcf..fd4628a 100644 --- a/src/key.rs +++ b/src/key.rs @@ -49,13 +49,18 @@ use std::convert::TryFrom; #[cfg(feature = "untested")] use crate::{ apdu::{Ins, StatusWords}, + certificate::PublicKeyInfo, policy::{PinPolicy, TouchPolicy}, serialization::*, settings, Buffer, CB_OBJ_MAX, }; #[cfg(feature = "untested")] +use elliptic_curve::weierstrass::PublicKey as EcPublicKey; +#[cfg(feature = "untested")] use log::{error, warn}; #[cfg(feature = "untested")] +use rsa::{BigUint, RSAPublicKey}; +#[cfg(feature = "untested")] use zeroize::Zeroizing; #[cfg(feature = "untested")] @@ -429,43 +434,6 @@ impl Key { } } -/// Information about a generated key -// TODO(tarcieri): this could use some more work -#[cfg(feature = "untested")] -#[derive(Clone, Debug, Eq, PartialEq)] -pub enum GeneratedKey { - /// RSA keys - Rsa { - /// RSA algorithm - algorithm: AlgorithmId, - - /// Modulus - modulus: Vec, - - /// Exponent - exp: Vec, - }, - /// ECC keys - Ecc { - /// ECC algorithm - algorithm: AlgorithmId, - - /// Public curve point (i.e. public key) - point: Vec, - }, -} - -#[cfg(feature = "untested")] -impl GeneratedKey { - /// Get the algorithm - pub fn algorithm(&self) -> AlgorithmId { - *match self { - GeneratedKey::Rsa { algorithm, .. } => algorithm, - GeneratedKey::Ecc { algorithm, .. } => algorithm, - } - } -} - /// Generate key #[cfg(feature = "untested")] #[allow(clippy::cognitive_complexity)] @@ -475,7 +443,7 @@ pub fn generate( algorithm: AlgorithmId, pin_policy: PinPolicy, touch_policy: TouchPolicy, -) -> Result { +) -> Result { // Keygen messages // TODO(tarcieri): extract these into an I18N-handling type? const SZ_SETTING_ROCA: &str = "Enable_Unsafe_Keygen_ROCA"; @@ -596,10 +564,13 @@ pub fn generate( } let exp = exp_tlv.value.to_vec(); - Ok(GeneratedKey::Rsa { + Ok(PublicKeyInfo::Rsa { algorithm, - modulus, - exp, + pubkey: RSAPublicKey::new( + BigUint::from_bytes_be(&modulus), + BigUint::from_bytes_be(&exp), + ) + .map_err(|_| Error::InvalidObject)?, }) } AlgorithmId::EccP256 | AlgorithmId::EccP384 => { @@ -623,7 +594,13 @@ pub fn generate( } let point = tlv.value.to_vec(); - Ok(GeneratedKey::Ecc { algorithm, point }) + + if let AlgorithmId::EccP256 = algorithm { + EcPublicKey::from_bytes(point).map(PublicKeyInfo::EcP256) + } else { + EcPublicKey::from_bytes(point).map(PublicKeyInfo::EcP384) + } + .ok_or(Error::InvalidObject) } } } From 41b10d1f23bd8876bbbe2cb987533395d5b9140c Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Wed, 11 Dec 2019 00:51:09 +0000 Subject: [PATCH 3/5] Convert certificate info into an enum --- src/certificate.rs | 58 ++++++++++++++++++++++++++++++++-------------- 1 file changed, 41 insertions(+), 17 deletions(-) diff --git a/src/certificate.rs b/src/certificate.rs index 133b5b5..ba78bba 100644 --- a/src/certificate.rs +++ b/src/certificate.rs @@ -44,6 +44,7 @@ use elliptic_curve::weierstrass::{ }; use log::error; use rsa::{PublicKey, RSAPublicKey}; +use std::convert::TryFrom; use std::fmt; use x509_parser::{parse_x509_der, x509::SubjectPublicKeyInfo}; use zeroize::Zeroizing; @@ -57,17 +58,43 @@ const OID_EC_PUBLIC_KEY: &str = "1.2.840.10045.2.1"; const OID_NIST_P256: &str = "1.2.840.10045.3.1.7"; const OID_NIST_P384: &str = "1.3.132.0.34"; -#[allow(dead_code)] -const CERTINFO_UNCOMPRESSED: u8 = 0; -#[cfg(feature = "untested")] -const CERTINFO_GZIP: u8 = 1; - const TAG_CERT: u8 = 0x70; #[cfg(feature = "untested")] const TAG_CERT_COMPRESS: u8 = 0x71; #[cfg(feature = "untested")] const TAG_CERT_LRC: u8 = 0xFE; +/// Information about how a [`Certificate`] is stored within a YubiKey. +#[derive(Clone, Copy, Debug, PartialEq)] +pub enum CertInfo { + /// The certificate is uncompressed. + Uncompressed, + + /// The certificate is gzip-compressed. + Gzip, +} + +impl TryFrom for CertInfo { + type Error = Error; + + fn try_from(value: u8) -> Result { + match value { + 0x00 => Ok(CertInfo::Uncompressed), + 0x01 => Ok(CertInfo::Gzip), + _ => Err(Error::InvalidObject), + } + } +} + +impl From for u8 { + fn from(certinfo: CertInfo) -> u8 { + match certinfo { + CertInfo::Uncompressed => 0x00, + CertInfo::Gzip => 0x01, + } + } +} + /// Information about a public key within a [`Certificate`]. #[derive(Clone, Eq, PartialEq)] pub enum PublicKeyInfo { @@ -157,7 +184,12 @@ impl Certificate { /// Write this certificate into the YubiKey in the given slot #[cfg(feature = "untested")] - pub fn write(&self, yubikey: &mut YubiKey, slot: SlotId, certinfo: u8) -> Result<(), Error> { + pub fn write( + &self, + yubikey: &mut YubiKey, + slot: SlotId, + certinfo: CertInfo, + ) -> Result<(), Error> { let txn = yubikey.begin_transaction()?; write_certificate(&txn, slot, Some(&self.data), certinfo) } @@ -166,7 +198,7 @@ impl Certificate { #[cfg(feature = "untested")] pub fn delete(yubikey: &mut YubiKey, slot: SlotId) -> Result<(), Error> { let txn = yubikey.begin_transaction()?; - write_certificate(&txn, slot, None, 0) + write_certificate(&txn, slot, None, CertInfo::Uncompressed) } /// Initialize a local certificate struct from the given bytebuffer @@ -244,7 +276,7 @@ pub(crate) fn write_certificate( txn: &Transaction<'_>, slot: SlotId, data: Option<&[u8]>, - certinfo: u8, + certinfo: CertInfo, ) -> Result<(), Error> { let object_id = slot.object_id(); @@ -258,15 +290,7 @@ pub(crate) fn write_certificate( let mut offset = Tlv::write(&mut buf, TAG_CERT, data)?; // write compression info and LRC trailer - offset += Tlv::write( - &mut buf, - TAG_CERT_COMPRESS, - if certinfo == CERTINFO_GZIP { - &[0x01] - } else { - &[0x00] - }, - )?; + offset += Tlv::write(&mut buf, TAG_CERT_COMPRESS, &[certinfo.into()])?; offset += Tlv::write(&mut buf, TAG_CERT_LRC, &[])?; txn.save_object(object_id, &buf[..offset]) From 2eff313064096bc9be1989d4dbdc0e8a15b26c93 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Wed, 11 Dec 2019 02:24:18 +0000 Subject: [PATCH 4/5] Fix bug in key::generate and document weirdness Bug was introduced in #73 when starting offsets were overlooked. Digging into why they were there led to uncovering the weird not-quite-ASN.1 format that the YubiKey returns generated pubkeys in. --- src/key.rs | 40 ++++++++++++++++++++++++++++++++++++++-- src/serialization.rs | 2 ++ 2 files changed, 40 insertions(+), 2 deletions(-) diff --git a/src/key.rs b/src/key.rs index fd4628a..fcd37e7 100644 --- a/src/key.rs +++ b/src/key.rs @@ -548,9 +548,41 @@ pub fn generate( } } + // TODO(str4d): Response is wrapped in an ASN.1 TLV: + // + // 0x7f 0x49 -> Application | Constructed | 0x49 match algorithm { AlgorithmId::Rsa1024 | AlgorithmId::Rsa2048 => { - let (data, modulus_tlv) = Tlv::parse(response.data())?; + // It appears that the inner application-specific value returned by the + // YubiKey is constructed such that RSA pubkeys can be parsed in two ways: + // + // - Use a full ASN.1 parser on the entire datastructure: + // + // RSA 1024: + // [127, 73, 129, 136, 129, 129, 128, [ 128 octets ], 130, 3, 1, 0, 1] + // | tag | len:136 |0x81| len:128 | modulus |0x82|len3| exp | + // + // RSA 2048: + // [127, 73, 130, 1, 9, 129, 130, 1, 0, [ 256 octets ], 130, 3, 1, 0, 1] + // | tag | len:265 |0x81| len:256 | modulus |0x82|len3| exp | + // + // - Skip the first 5 bytes and use crate::serialize::get_length during TLV + // parsing (which treats 128 as a single-byte definite length instead of an + // indefinite length): + // + // RSA 1024: + // [127, 73, 129, 136, 129, 129, 128, [ 128 octets ], 130, 3, 1, 0, 1] + // | |0x81|len128| modulus |0x82|len3| exp | + // + // RSA 2048: + // [127, 73, 130, 1, 9, 129, 130, 1, 0, [ 256 octets ], 130, 3, 1, 0, 1] + // | |0x81| len:256 | modulus |0x82|len3| exp | + // + // Because of the above, treat this for now as a 2-byte ASN.1 tag with a + // 3-byte length. + let data = &response.data()[5..]; + + let (data, modulus_tlv) = Tlv::parse(data)?; if modulus_tlv.tag != TAG_RSA_MODULUS { error!("Failed to parse public key structure (modulus)"); return Err(Error::ParseError); @@ -574,13 +606,17 @@ pub fn generate( }) } AlgorithmId::EccP256 | AlgorithmId::EccP384 => { + // 2-byte ASN.1 tag, 1-byte length (because all supported EC pubkey lengths + // are shorter than 128 bytes, fitting into a definite short ASN.1 length). + let data = &response.data()[3..]; + let len = if let AlgorithmId::EccP256 = algorithm { CB_ECC_POINTP256 } else { CB_ECC_POINTP384 }; - let (_, tlv) = Tlv::parse(response.data())?; + let (_, tlv) = Tlv::parse(data)?; if tlv.tag != TAG_ECC_POINT { error!("failed to parse public key structure"); diff --git a/src/serialization.rs b/src/serialization.rs index 06b24c1..e36f171 100644 --- a/src/serialization.rs +++ b/src/serialization.rs @@ -156,6 +156,8 @@ pub(crate) fn set_length(buffer: &mut [u8], length: usize) -> Result usize { + // This is not valid ASN.1 (0x80 is the indefinite length marker). + // See comment in key::generate for more context. if buffer[0] < 0x81 { *len = buffer[0] as usize; 1 From d113c1f4b9ad489498d8d68e5a3cb7d985b5a1c5 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Wed, 11 Dec 2019 02:44:02 +0000 Subject: [PATCH 5/5] impl<'a> TryFrom<&'a [u8]> for Certificate --- src/certificate.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/certificate.rs b/src/certificate.rs index ba78bba..fb8261b 100644 --- a/src/certificate.rs +++ b/src/certificate.rs @@ -169,6 +169,14 @@ pub struct Certificate { data: Buffer, } +impl<'a> TryFrom<&'a [u8]> for Certificate { + type Error = Error; + + fn try_from(bytes: &'a [u8]) -> Result { + Self::from_bytes(bytes.to_vec()) + } +} + impl Certificate { /// Read a certificate from the given slot in the YubiKey pub fn read(yubikey: &mut YubiKey, slot: SlotId) -> Result {