Fix decoding bug in RSAPrivateKey.

base/crypto/rsa_private_key_win.cc

 // Copyright (c) 2009 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/crypto/rsa_private_key.h"
 
 #include <iostream>
 #include <list>
 
 #include "base/logging.h"
@@ skipping 67 matching lines @@
 
 // Helper to prepend an ASN.1 type header.
 static void PrependTypeHeaderAndLength(uint8 type, uint32 length,
                                        std::list<uint8>* output) {
   PrependLength(length, output);
   output->push_front(type);
 }
 
 // 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.
 #define READ_ASSERT(truth) \
   if (!(truth)) { \
     NOTREACHED(); \
     return false; \
   }
 
 // Read an ASN.1 length field. This also checks that the length does not extend
@@ skipping 77 matching lines @@
     out->push_back(*(*pos + i));
 
   // The last byte can be zero to force positiveness. We can ignore this.
   if (**pos != 0x00)
     out->push_back(**pos);
 
   (*pos) += length;
   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
 
 
 namespace base {
 
 // static
 RSAPrivateKey* RSAPrivateKey::Create(uint16 num_bits) {
   scoped_ptr<RSAPrivateKey> result(new RSAPrivateKey);
   if (!result->InitProvider())
     return NULL;
 
   DWORD flags = CRYPT_EXPORTABLE;
 
   // The size is encoded as the upper 16 bits of the flags. :: sigh ::.
   flags |= (num_bits << 16);
   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();
 }
 
 // static
 RSAPrivateKey* RSAPrivateKey::CreateFromPrivateKeyInfo(
     const std::vector<uint8>& input) {
   scoped_ptr<RSAPrivateKey> result(new RSAPrivateKey);
   if (!result->InitProvider())
     return NULL;
 
   uint8* src = const_cast<uint8*>(&input.front());
   uint8* end = src + input.size();
   int version = -1;
   std::vector<uint8> modulus;
   std::vector<uint8> public_exponent;
   std::vector<uint8> private_exponent;
   std::vector<uint8> prime1;
   std::vector<uint8> prime2;
   std::vector<uint8> exponent1;
   std::vector<uint8> exponent2;
   std::vector<uint8> coefficient;
 
   if (!ReadSequence(&src, end) ||
       !ReadVersion(&src, end) ||
       !ReadAlgorithmIdentifier(&src, end) ||
       !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);
 
   int blob_size = sizeof(PUBLICKEYSTRUC) + sizeof(RSAPUBKEY) + modulus.size() +
                   prime1.size() + prime2.size() +
                   exponent1.size() + exponent2.size() +
                   coefficient.size() + private_exponent.size();
   scoped_array<BYTE> blob(new BYTE[blob_size]);
 
@@ skipping 77 matching lines @@
 
   uint8* pos = blob.get();
   PUBLICKEYSTRUC *publickey_struct = reinterpret_cast<PUBLICKEYSTRUC*>(pos);
   pos += sizeof(PUBLICKEYSTRUC);
 
   RSAPUBKEY *rsa_pub_key = reinterpret_cast<RSAPUBKEY*>(pos);
   pos += sizeof(RSAPUBKEY);
 
   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;
 
   uint8* prime1 = pos;
   pos += primes_size;
   uint8* prime2 = pos;
   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));
 
   std::list<uint8> content;
 
   // Version (always zero)
   uint8 version = 0;
 
   // 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);
   PrependTypeHeaderAndLength(kSequenceTag, content.size(), &content);
   PrependTypeHeaderAndLength(kOctetStringTag, content.size(), &content);
 
   // RSA algorithm OID
   for (size_t i = sizeof(kRsaAlgorithmIdentifier); i > 0; --i)
     content.push_front(kRsaAlgorithmIdentifier[i - 1]);
 
@@ skipping 43 matching lines @@
     return false;
   }
 
   for (size_t i = 0; i < encoded_length; ++i)
     output->push_back(encoded[i]);
 
   return true;
 }
 
 }  // namespace base