Fix decoding bug in RSAPrivateKey.

base/crypto/rsa_private_key_win.cc

Index: base/crypto/rsa_private_key_win.cc
diff --git a/base/crypto/rsa_private_key_win.cc b/base/crypto/rsa_private_key_win.cc
index eba60e822b966620e3843ca8e3f582db8ed77c43..1ca2b727fc1445c3e883019031743c8b2e2ce8a2 100644
--- a/base/crypto/rsa_private_key_win.cc
+++ b/base/crypto/rsa_private_key_win.cc
@@ -85,17 +85,20 @@ static void PrependTypeHeaderAndLength(uint8 type, uint32 length,
 
 // Helper to prepend an ASN.1 integer.
 static void PrependInteger(uint8* val, int num_bytes, std::list<uint8>* data) {
-  // If the MSB is set, we are supposed to add an extra null byte at the front.
-  bool needs_null_byte = (val[num_bytes - 1] & 0x80) != 0;
-  int length = needs_null_byte ? num_bytes + 1 : num_bytes;
+  // Skip any trailing null bytes since the input is little endian.

[Evan Martin, 2009/06/22 16:23:45]

This comment is confusing.  Something like "Strip bytes off the MSB end, which
is the tail since the input is little endian" would be more clear.

+  while (num_bytes > 1 && val[num_bytes - 1] == 0x00)
+    num_bytes--;
 
   PrependBytesInReverseOrder(val, num_bytes, data);
 
-  // Add a null byte to force the integer to be positive if necessary.
-  if (needs_null_byte)
+  // If the MSB is set, we need to add an extra null byte, otherwise the integer
+  // could be interpreted as negative.
+  if ((val[num_bytes - 1] & 0x80) != 0) {

[wtc, 2009/06/22 17:26:18]

Nit: it's less confusing to test 'data' here:
  if (data->front() & 0x80) != 0) {

     data->push_front(0x00);
+    num_bytes++;
+  }
 
-  PrependTypeHeaderAndLength(kIntegerTag, length, data);
+  PrependTypeHeaderAndLength(kIntegerTag, num_bytes, data);
 }
 
 // Helper for error handling during key import.
@@ -193,6 +196,26 @@ static bool ReadInteger(uint8** pos, uint8* end, std::vector<uint8>* out) {
   return true;
 }
 
+static bool ReadIntegerWithExpectedSize(uint8** pos, uint8* end,
+                                        int expected_size,
+                                        std::vector<uint8>* out) {
+  if (!ReadInteger(pos, end, out))
+    return false;
+
+  if (out->size() == expected_size + 1) {
+    READ_ASSERT(out->back() == 0x00);
+    out->pop_back();
+  } else {
+    READ_ASSERT(out->size() <= expected_size);
+  }
+
+  // Pad out any missing bytes with null.

[wtc, 2009/06/22 17:26:18]

Are you sure you want to allow missing bytes?  I don't have
time to check this, so I'm counting on you to make the right
decision.

+  for (size_t i = out->size(); i < expected_size; ++i)
+    out->push_back(0x00);
+
+  return true;
+}
+
 }  // namespace
 
 
@@ -211,12 +234,6 @@ RSAPrivateKey* RSAPrivateKey::Create(uint16 num_bits) {
   if (!CryptGenKey(result->provider_, CALG_RSA_SIGN, flags, &result->key_))
     return NULL;
 
-  std::vector<uint8> out;
-  result->ExportPrivateKey(&out);
-  std::cout << "Generated random key: "
-            << HexEncode(&out.front(), out.size())
-            << "\n";
-
   return result.release();
 }
 
@@ -245,14 +262,20 @@ RSAPrivateKey* RSAPrivateKey::CreateFromPrivateKeyInfo(
       !ReadTypeHeaderAndLength(&src, end, kOctetStringTag, NULL) ||
       !ReadSequence(&src, end) ||
       !ReadVersion(&src, end) ||
-      !ReadInteger(&src, end, &modulus) ||
-      !ReadInteger(&src, end, &public_exponent) ||
-      !ReadInteger(&src, end, &private_exponent) ||
-      !ReadInteger(&src, end, &prime1) ||
-      !ReadInteger(&src, end, &prime2) ||
-      !ReadInteger(&src, end, &exponent1) ||
-      !ReadInteger(&src, end, &exponent2) ||
-      !ReadInteger(&src, end, &coefficient))
+      !ReadInteger(&src, end, &modulus))
+    return false;
+
+  int mod_size = modulus.size();
+  READ_ASSERT(mod_size % 2 == 0);
+  int primes_size = mod_size / 2;
+
+  if (!ReadIntegerWithExpectedSize(&src, end, 4, &public_exponent) ||

[wtc, 2009/06/22 17:26:18]

The expected size of 4 for the public exponent assumes more
than necessary.  Numbers other than 2^16 + 1 can also be used
as public exponents.

+      !ReadIntegerWithExpectedSize(&src, end, mod_size, &private_exponent) ||
+      !ReadIntegerWithExpectedSize(&src, end, primes_size, &prime1) ||
+      !ReadIntegerWithExpectedSize(&src, end, primes_size, &prime2) ||
+      !ReadIntegerWithExpectedSize(&src, end, primes_size, &exponent1) ||
+      !ReadIntegerWithExpectedSize(&src, end, primes_size, &exponent2) ||
+      !ReadIntegerWithExpectedSize(&src, end, primes_size, &coefficient))
     return false;
 
   READ_ASSERT(src == end);
@@ -350,9 +373,6 @@ bool RSAPrivateKey::ExportPrivateKey(std::vector<uint8>* output) {
 
   int mod_size = rsa_pub_key->bitlen / 8;
   int primes_size = rsa_pub_key->bitlen / 16;
-  int exponents_size = primes_size;
-  int coefficient_size = primes_size;
-  int private_exponent_size = mod_size;
 
   uint8* modulus = pos;
   pos += mod_size;
@@ -363,15 +383,15 @@ bool RSAPrivateKey::ExportPrivateKey(std::vector<uint8>* output) {
   pos += primes_size;
 
   uint8* exponent1 = pos;
-  pos += exponents_size;
+  pos += primes_size;
   uint8* exponent2 = pos;
-  pos += exponents_size;
+  pos += primes_size;
 
   uint8* coefficient = pos;
-  pos += coefficient_size;
+  pos += primes_size;
 
   uint8* private_exponent = pos;
-  pos += private_exponent_size;
+  pos += mod_size;
 
   CHECK((pos - blob_length) == reinterpret_cast<BYTE*>(publickey_struct));
 
@@ -382,12 +402,12 @@ bool RSAPrivateKey::ExportPrivateKey(std::vector<uint8>* output) {
 
   // We build up the output in reverse order to prevent having to do copies to
   // figure out the length.
-  PrependInteger(coefficient, coefficient_size, &content);
-  PrependInteger(exponent2, exponents_size, &content);
-  PrependInteger(exponent1, exponents_size, &content);
+  PrependInteger(coefficient, primes_size, &content);
+  PrependInteger(exponent2, primes_size, &content);
+  PrependInteger(exponent1, primes_size, &content);
   PrependInteger(prime2, primes_size, &content);
   PrependInteger(prime1, primes_size, &content);
-  PrependInteger(private_exponent, private_exponent_size, &content);
+  PrependInteger(private_exponent, mod_size, &content);
   PrependInteger(reinterpret_cast<uint8*>(&rsa_pub_key->pubexp), 4, &content);
   PrependInteger(modulus, mod_size, &content);
   PrependInteger(&version, 1, &content);