Fix decoding bug in RSAPrivateKey.

Fix decoding bug in RSAPrivateKey.

We were dropping the most significant byte from the input when
decoding PrivateKeyInfo, whether or not it was part of the
original data. This shouldn't matter, except that we need to
get back the original byte lengths so that we can give them to
CryptoAPI.

BUG=14877
TEST=Stress tested the functions by creating 1000 random private keys, exporting them, then re-importing. Also tried stressing the edge cases in particular around extra trailing null bytes.

[Aaron Boodman, 2009-06-22 02:08:32]

One thing I'm curious about here: how do other people who parse PrivateKeyInfo
determine the key length?

It doesn't seem like there is enough information in the BER-encoded format, but
this must be a common problem, right?

[Aaron Boodman, 2009-06-22 04:44:07]

On 2009/06/22 02:08:32, Aaron Boodman wrote:
> One thing I'm curious about here: how do other people who parse PrivateKeyInfo
> determine the key length?
> 
> It doesn't seem like there is enough information in the BER-encoded format,
but
> this must be a common problem, right?

After some review, I realize that the modulus should tell me the key length
because it is specifically chosen so that the high byte is > 0.

I'll rework this to remove the extra param on CreateFromPrivateKeyInfo()
tomorrow morning.

[Aaron Boodman, 2009-06-22 16:13:39]

OK, here is a much simpler fix.

[Evan Martin, 2009-06-22 16:23:44]

http://codereview.chromium.org/141036/diff/1016/24
File base/crypto/rsa_private_key_win.cc (right):

http://codereview.chromium.org/141036/diff/1016/24#newcode88
Line 88: // Skip any trailing null bytes since the input is little endian.
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.

[Erik does not do reviews, 2009-06-22 16:35:51]

LGTM

I'd like to see two things which could be done in a followup:
(1) file a bug for this and reference it
(2) add a unit test that either does what your TEST= description says or find
some of the keys that were failing before and just test those.

[Aaron Boodman, 2009-06-22 16:44:43]

Cool, I added/referenced a bug. I'll work on adding some worst-case keys to the
tests while the tree is closed.

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

LGTM.

http://codereview.chromium.org/141036/diff/1016/24
File base/crypto/rsa_private_key_win.cc (right):

http://codereview.chromium.org/141036/diff/1016/24#newcode96
Line 96: if ((val[num_bytes - 1] & 0x80) != 0) {
Nit: it's less confusing to test 'data' here:
  if (data->front() & 0x80) != 0) {

http://codereview.chromium.org/141036/diff/1016/24#newcode212
Line 212: // Pad out any missing bytes with null.
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.

http://codereview.chromium.org/141036/diff/1016/24#newcode272
Line 272: if (!ReadIntegerWithExpectedSize(&src, end, 4, &public_exponent) ||
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.