From 2eff313064096bc9be1989d4dbdc0e8a15b26c93 Mon Sep 17 00:00:00 2001
From: Jack Grigg <thestr4d@gmail.com>
Date: Wed, 11 Dec 2019 02:24:18 +0000
Subject: [PATCH] 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, Erro
 /// Parse length tag, returning the size of the length tag itself as the
 /// returned value, and setting the len parameter to the parsed length.
 pub(crate) fn get_length(buffer: &[u8], len: &mut usize) -> 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