RSA signature verification and SHA-1/256/512 reference implementation for verified boot.

RSA signature verification and SHA-1/256/512 reference implementation for verified boot.

Also contains some preliminary tests for these primitives.

[gauravsh, 2010-01-20 01:11:34]

Think this is in a state for a first look.

[mschilder, 2010-01-20 03:17:56]

http://codereview.chromium.org/553023/diff/1/3
File src/platform/vboot_reference/dumpRSAPublicKey.c (right):

http://codereview.chromium.org/553023/diff/1/3#newcode74
src/platform/vboot_reference/dumpRSAPublicKey.c:74: write(1, &nwords,
sizeof(nwords));
So this writes in native endianess for the host. Little endian for x86. But not
on PPC for instance.
Why write binary output? If you'd write decimal ascii, there are no endianess
issues with words, just with word arrays.
If you want to write binary, use some macros like hton, but those like to output
big endian.
Or do you want host endianess? Say bigendian arm, you'd run this on the target?
I usually work around these issues by producing decimal ascii and let the
compiler turn it into words..

http://codereview.chromium.org/553023/diff/1/5
File src/platform/vboot_reference/rsa.c (right):

http://codereview.chromium.org/553023/diff/1/5#newcode46
src/platform/vboot_reference/rsa.c:46: int64_t A = 0;
strange indent here?

http://codereview.chromium.org/553023/diff/1/5#newcode57
src/platform/vboot_reference/rsa.c:57: int i;
indent?

http://codereview.chromium.org/553023/diff/1/5#newcode71
src/platform/vboot_reference/rsa.c:71: uint64_t A = (uint64_t)a * b[0] + c[0];
indent?

http://codereview.chromium.org/553023/diff/1/5#newcode96
src/platform/vboot_reference/rsa.c:96: int i;
indent?

http://codereview.chromium.org/553023/diff/1/5#newcode158
src/platform/vboot_reference/rsa.c:158: if (key->len != siglen_map[sig_type]) {
Does sig_type come from a trusted source? Or is it derived from user-supplied
input?
For the latter, some bounds checking is in order.

http://codereview.chromium.org/553023/diff/1/15
File src/platform/vboot_reference/tests/verify_data.c (right):

http://codereview.chromium.org/553023/diff/1/15#newcode38
src/platform/vboot_reference/tests/verify_data.c:38: read(key_fd, &key->len,
sizeof(key->len));
Again, this requires on-disk endianess and host endianess to be same. So unless
you run the converter and this test on the same host, this code is not portable.

[gauravsh, 2010-01-20 21:54:18]

http://codereview.chromium.org/553023/diff/1/3
File src/platform/vboot_reference/dumpRSAPublicKey.c (right):

http://codereview.chromium.org/553023/diff/1/3#newcode74
src/platform/vboot_reference/dumpRSAPublicKey.c:74: write(1, &nwords,
sizeof(nwords));
On 2010/01/20 03:17:56, mschilder wrote:
> So this writes in native endianess for the host. Little endian for x86. But
not
> on PPC for instance.
> Why write binary output? If you'd write decimal ascii, there are no endianess
> issues with words, just with word arrays.
> If you want to write binary, use some macros like hton, but those like to
output
> big endian.
> Or do you want host endianess? Say bigendian arm, you'd run this on the
target?
> I usually work around these issues by producing decimal ascii and let the
> compiler turn it into words..

Host endianess is fine since this code will be used on X86 and ARM running in
little-endian mode. I am writing a binary blob mainly because this pre-processed
key resides in the firmware where 1) space is a premium, 2) we'd like to avoid
any unnecessary parsing of input - for speed reasons. 

However, since this code will always be run on a host to create a key for use by
the firmware (which will always be running on a little-endian host) , maybe it's
a good idea to  ensure that the key is always dumped as a little endian sequence
of bytes.

http://codereview.chromium.org/553023/diff/1/5
File src/platform/vboot_reference/rsa.c (right):

http://codereview.chromium.org/553023/diff/1/5#newcode46
src/platform/vboot_reference/rsa.c:46: int64_t A = 0;
On 2010/01/20 03:17:56, mschilder wrote:
> strange indent here?


Fixed.

http://codereview.chromium.org/553023/diff/1/5#newcode57
src/platform/vboot_reference/rsa.c:57: int i;
On 2010/01/20 03:17:56, mschilder wrote:
> indent?

Fixed.

http://codereview.chromium.org/553023/diff/1/5#newcode71
src/platform/vboot_reference/rsa.c:71: uint64_t A = (uint64_t)a * b[0] + c[0];
On 2010/01/20 03:17:56, mschilder wrote:
> indent?

Fixed.

http://codereview.chromium.org/553023/diff/1/5#newcode96
src/platform/vboot_reference/rsa.c:96: int i;
On 2010/01/20 03:17:56, mschilder wrote:
> indent?

Fixed.

http://codereview.chromium.org/553023/diff/1/5#newcode158
src/platform/vboot_reference/rsa.c:158: if (key->len != siglen_map[sig_type]) {
On 2010/01/20 03:17:56, mschilder wrote:
> Does sig_type come from a trusted source? Or is it derived from user-supplied
> input?
> For the latter, some bounds checking is in order.

It's read from an externally supplied binary blob header. Added a check.

http://codereview.chromium.org/553023/diff/1/15
File src/platform/vboot_reference/tests/verify_data.c (right):

http://codereview.chromium.org/553023/diff/1/15#newcode38
src/platform/vboot_reference/tests/verify_data.c:38: read(key_fd, &key->len,
sizeof(key->len));
On 2010/01/20 03:17:56, mschilder wrote:
> Again, this requires on-disk endianess and host endianess to be same. So
unless
> you run the converter and this test on the same host, this code is not
portable.

See my comment for dumpRSAPublicKey.c. I can just modify this to ensure that the
key is always read as a sequence of little-endian words (which is our common
case).

[mschilder, 2010-01-20 22:27:44]

http://codereview.chromium.org/553023/diff/1/3
File src/platform/vboot_reference/dumpRSAPublicKey.c (right):

http://codereview.chromium.org/553023/diff/1/3#newcode74
src/platform/vboot_reference/dumpRSAPublicKey.c:74: write(1, &nwords,
sizeof(nwords));
On 2010/01/20 21:54:18, gauravsh wrote:
> On 2010/01/20 03:17:56, mschilder wrote:
> > So this writes in native endianess for the host. Little endian for x86. But
> not
> > on PPC for instance.
> > Why write binary output? If you'd write decimal ascii, there are no
endianess
> > issues with words, just with word arrays.
> > If you want to write binary, use some macros like hton, but those like to
> output
> > big endian.
> > Or do you want host endianess? Say bigendian arm, you'd run this on the
> target?
> > I usually work around these issues by producing decimal ascii and let the
> > compiler turn it into words..
> 
> Host endianess is fine since this code will be used on X86 and ARM running in
> little-endian mode. I am writing a binary blob mainly because this
pre-processed
> key resides in the firmware where 1) space is a premium, 

I am talking about letting the compiler convert the ascii key and embed it in
const data with the code. What point is there to read this root key from 2nd
storage?

Not obvious you would want to separate key from the code. So just compile it in.

Size-wise, compiling it in is exactly the same. It's just that the compiler
converts the consts to proper endianess in the compiled form.

2) we'd like to avoid
> any unnecessary parsing of input - for speed reasons. 
> 
> However, since this code will always be run on a host to create a key for use
by
> the firmware (which will always be running on a little-endian host) , maybe
it's
> a good idea to  ensure that the key is always dumped as a little endian
sequence
> of bytes. 
>

http://codereview.chromium.org/553023/diff/5001/6004
File src/platform/vboot_reference/rsa.c (right):

http://codereview.chromium.org/553023/diff/5001/6004#newcode172
src/platform/vboot_reference/rsa.c:172: buf[i] = sig[i];
Where is size of sig checked? By caller? Perhaps the API should pass in # of
signature bytes.
I guess the caller already inspects sig_type and picks the hash based on that?
Where is that code? Similar range checks on sig_type are needed there.
You have to carefully validate untrusted input, both value and size wise before
accessing / assuming anything.

[gauravsh, 2010-01-20 23:24:13]

http://codereview.chromium.org/553023/diff/1/3
File src/platform/vboot_reference/dumpRSAPublicKey.c (right):

http://codereview.chromium.org/553023/diff/1/3#newcode74
src/platform/vboot_reference/dumpRSAPublicKey.c:74: write(1, &nwords,
sizeof(nwords));
On 2010/01/20 22:27:45, mschilder wrote:
> On 2010/01/20 21:54:18, gauravsh wrote:
> > On 2010/01/20 03:17:56, mschilder wrote:
> > > So this writes in native endianess for the host. Little endian for x86.
But
> > not
> > > on PPC for instance.
> > > Why write binary output? If you'd write decimal ascii, there are no
> endianess
> > > issues with words, just with word arrays.
> > > If you want to write binary, use some macros like hton, but those like to
> > output
> > > big endian.
> > > Or do you want host endianess? Say bigendian arm, you'd run this on the
> > target?
> > > I usually work around these issues by producing decimal ascii and let the
> > > compiler turn it into words..
> > 
> > Host endianess is fine since this code will be used on X86 and ARM running
in
> > little-endian mode. I am writing a binary blob mainly because this
> pre-processed
> > key resides in the firmware where 1) space is a premium, 
> 
> I am talking about letting the compiler convert the ascii key and embed it in
> const data with the code. What point is there to read this root key from 2nd
> storage?

Yes, we can compile in the root key with the code. However, the verification
code isn't always used with the same static root key. 

In particular, the root key is only used to verify the signing key which can
change over time and resides in the writable portion of the firmware. So we do
need a way to ship new pre-processed public signing keys into R/W firmware which
can directly be used by the verification code (which will reside in the
read-only firmware). 

> 
> Not obvious you would want to separate key from the code. So just compile it
in.
> 
> Size-wise, compiling it in is exactly the same. It's just that the compiler
> converts the consts to proper endianess in the compiled form.
> 
> 2) we'd like to avoid
> > any unnecessary parsing of input - for speed reasons. 
> > 
> > However, since this code will always be run on a host to create a key for
use
> by
> > the firmware (which will always be running on a little-endian host) , maybe
> it's
> > a good idea to  ensure that the key is always dumped as a little endian
> sequence
> > of bytes. 
> > 
> 
>

http://codereview.chromium.org/553023/diff/5001/6004
File src/platform/vboot_reference/rsa.c (right):

http://codereview.chromium.org/553023/diff/5001/6004#newcode172
src/platform/vboot_reference/rsa.c:172: buf[i] = sig[i];
On 2010/01/20 22:27:45, mschilder wrote:
> Where is size of sig checked? By caller? Perhaps the API should pass in # of
> signature bytes.

Yes, this assumes it will be checked by the caller before a call. But I agree
with you, a signature length should be passed just to make sure.

> I guess the caller already inspects sig_type and picks the hash based on that?
> Where is that code? Similar range checks on sig_type are needed there.
> You have to carefully validate untrusted input, both value and size wise
before
> accessing / assuming anything.

Yes, it has to be done by the caller.

[gauravsh, 2010-01-21 22:04:52]

Changed to use an RSA public exponent of 65537.

[gauravsh, 2010-01-26 02:25:46]

I refactored some of the code and switched to a different SHA-2 implementation.
Bring it on!

[Chris Masone, 2010-01-26 18:54:29]

On 2010/01/26 02:25:46, gauravsh wrote:
> I refactored some of the code and switched to a different SHA-2
implementation.
> Bring it on!

[Chris Masone, 2010-01-26 18:54:46]

http://codereview.chromium.org/553023/diff/8001/8008
File src/platform/vboot_reference/include/rsa.h (right):

http://codereview.chromium.org/553023/diff/8001/8008#newcode33
src/platform/vboot_reference/include/rsa.h:33: const uint8_t sig_type,
what values can I pass in for sig_type?  is there an enum, or something?

http://codereview.chromium.org/553023/diff/8001/8013
File src/platform/vboot_reference/tests/sha_tests.c (right):

http://codereview.chromium.org/553023/diff/8001/8013#newcode5
src/platform/vboot_reference/tests/sha_tests.c:5: /* FIPS 180-2 Tests for
message digest functions. */
did you get these from somewhere?  maybe say where, if so?

http://codereview.chromium.org/553023/diff/8001/8015
File src/platform/vboot_reference/tests/verify_data.h (right):

http://codereview.chromium.org/553023/diff/8001/8015#newcode13
src/platform/vboot_reference/tests/verify_data.h:13: /* Returns the SHA-1 digest
of [input_file]. */
who owns the response?

http://codereview.chromium.org/553023/diff/8001/8015#newcode16
src/platform/vboot_reference/tests/verify_data.h:16: /* Returns the SHA-256
digest of [input_file]. */
same as above

http://codereview.chromium.org/553023/diff/8001/8015#newcode20
src/platform/vboot_reference/tests/verify_data.h:20: uint8_t* SHA512_file(char
*input_file);
ditto

http://codereview.chromium.org/553023/diff/8001/8015#newcode27
src/platform/vboot_reference/tests/verify_data.h:27: /* Return a signature of
[len] bytes read from [input_file] */
again, who owns?

http://codereview.chromium.org/553023/diff/8001/8017
File src/platform/vboot_reference/utils/dumpRSAPublicKey.c (right):

http://codereview.chromium.org/553023/diff/8001/8017#newcode5
src/platform/vboot_reference/utils/dumpRSAPublicKey.c:5: /* C port of
DumpPublicKey.java from the Android Open source project with support
80 char

[gauravsh, 2010-01-26 20:36:52]

http://codereview.chromium.org/553023/diff/8001/8008
File src/platform/vboot_reference/include/rsa.h (right):

http://codereview.chromium.org/553023/diff/8001/8008#newcode33
src/platform/vboot_reference/include/rsa.h:33: const uint8_t sig_type,
On 2010/01/26 18:54:47, cmasone wrote:
> what values can I pass in for sig_type?  is there an enum, or something?

The sigtype->algorithm mappings are defined by genpadding.sh in padding.h. Since
padding.h is going to remain static (unless we add algorithms), maybe I should
make it a part of the CL?

http://codereview.chromium.org/553023/diff/8001/8013
File src/platform/vboot_reference/tests/sha_tests.c (right):

http://codereview.chromium.org/553023/diff/8001/8013#newcode5
src/platform/vboot_reference/tests/sha_tests.c:5: /* FIPS 180-2 Tests for
message digest functions. */
On 2010/01/26 18:54:47, cmasone wrote:
> did you get these from somewhere?  maybe say where, if so?

I wrote these. The test vectors themselves (input, and expected output) are
described in the FIPS 180-2 standard.

http://codereview.chromium.org/553023/diff/8001/8015
File src/platform/vboot_reference/tests/verify_data.h (right):

http://codereview.chromium.org/553023/diff/8001/8015#newcode13
src/platform/vboot_reference/tests/verify_data.h:13: /* Returns the SHA-1 digest
of [input_file]. */
On 2010/01/26 18:54:47, cmasone wrote:
> who owns the response?

Caller owns. I will add that to the description comment.

http://codereview.chromium.org/553023/diff/8001/8017
File src/platform/vboot_reference/utils/dumpRSAPublicKey.c (right):

http://codereview.chromium.org/553023/diff/8001/8017#newcode5
src/platform/vboot_reference/utils/dumpRSAPublicKey.c:5: /* C port of
DumpPublicKey.java from the Android Open source project with support
On 2010/01/26 18:54:47, cmasone wrote:
> 80 char

Fixed.

[Chris Masone, 2010-01-26 20:59:53]

On 2010/01/26 20:36:52, gauravsh wrote:
> http://codereview.chromium.org/553023/diff/8001/8008
> File src/platform/vboot_reference/include/rsa.h (right):
> 
> http://codereview.chromium.org/553023/diff/8001/8008#newcode33
> src/platform/vboot_reference/include/rsa.h:33: const uint8_t sig_type,
> On 2010/01/26 18:54:47, cmasone wrote:
> > what values can I pass in for sig_type?  is there an enum, or something?
> 
> The sigtype->algorithm mappings are defined by genpadding.sh in padding.h.
Since
> padding.h is going to remain static (unless we add algorithms), maybe I should
> make it a part of the CL?

if it's not going to change, then I'd suggest checking it in.  No harm in
leaving the script in too, though.

> 
> http://codereview.chromium.org/553023/diff/8001/8013
> File src/platform/vboot_reference/tests/sha_tests.c (right):
> 
> http://codereview.chromium.org/553023/diff/8001/8013#newcode5
> src/platform/vboot_reference/tests/sha_tests.c:5: /* FIPS 180-2 Tests for
> message digest functions. */
> On 2010/01/26 18:54:47, cmasone wrote:
> > did you get these from somewhere?  maybe say where, if so?
> 
> I wrote these. The test vectors themselves (input, and expected output) are
> described in the FIPS 180-2 standard.
> 
> http://codereview.chromium.org/553023/diff/8001/8015
> File src/platform/vboot_reference/tests/verify_data.h (right):
> 
> http://codereview.chromium.org/553023/diff/8001/8015#newcode13
> src/platform/vboot_reference/tests/verify_data.h:13: /* Returns the SHA-1
digest
> of [input_file]. */
> On 2010/01/26 18:54:47, cmasone wrote:
> > who owns the response?
> 
> Caller owns. I will add that to the description comment.
> 
> http://codereview.chromium.org/553023/diff/8001/8017
> File src/platform/vboot_reference/utils/dumpRSAPublicKey.c (right):
> 
> http://codereview.chromium.org/553023/diff/8001/8017#newcode5
> src/platform/vboot_reference/utils/dumpRSAPublicKey.c:5: /* C port of
> DumpPublicKey.java from the Android Open source project with support
> On 2010/01/26 18:54:47, cmasone wrote:
> > 80 char
> 
> Fixed.

lgtm after you add padding.h, but I'd like wad@ to make sure that his concerns
were met as well.

[Will Drewry, 2010-01-26 21:45:44]

Thanks for all the clean up and restructuring.  This looks good, but still has
some style inconsistencies which would be good to clean up.

thanks!

http://codereview.chromium.org/553023/diff/8019/8023
File src/platform/vboot_reference/crypto/rsa.c (right):

http://codereview.chromium.org/553023/diff/8019/8023#newcode78
src/platform/vboot_reference/crypto/rsa.c:78: ** Input and output big-endian
byte array in inout.
This doesn't match the other multi-line comments. In the other file, I mentioned
the style I'd recommend using if possible.  Regardless, it needs to be
consistent :)

http://codereview.chromium.org/553023/diff/8019/8023#newcode84
src/platform/vboot_reference/crypto/rsa.c:84: uint32_t* aaR = (uint32_t*)
malloc(key->len * sizeof(uint32_t));
Should you check for allocation failures (!=NULL) or do we just assume that
we'll always have enough memory in the firmware?

That aside, will you need to write your own malloc implementation?  (If so, you
could have the pathological cases handled there so that if malloc fails, it
jumps to the recovery path address :)

http://codereview.chromium.org/553023/diff/8019/8023#newcode135
src/platform/vboot_reference/crypto/rsa.c:135: if (sig_len != (key->len *
sizeof(uint32_t))) {
Will this be the key baked into the firmware?

http://codereview.chromium.org/553023/diff/8019/8023#newcode152
src/platform/vboot_reference/crypto/rsa.c:152: buf[i] = sig[i];
Would it be worth having your own memcpy and strcpy helpers?

http://codereview.chromium.org/553023/diff/8019/8023#newcode173
src/platform/vboot_reference/crypto/rsa.c:173: if (buf[i] != *hash++) {
Is stack memory at a premium?  If not, I'd say go ahead and make a buf cursor,
then make these match.
(Or just include an inlined memcmp helper :)

http://codereview.chromium.org/553023/diff/8019/8024
File src/platform/vboot_reference/crypto/sha1.c (right):

http://codereview.chromium.org/553023/diff/8019/8024#newcode275
src/platform/vboot_reference/crypto/sha1.c:275: /* Convenience function */
Should comment that digest should be pre-allocated of the size SHA1_DIGEST

http://codereview.chromium.org/553023/diff/8019/8024#newcode284
src/platform/vboot_reference/crypto/sha1.c:284: digest[i] = *p++;
memcpy?

http://codereview.chromium.org/553023/diff/8019/8026
File src/platform/vboot_reference/include/padding.h (right):

http://codereview.chromium.org/553023/diff/8019/8026#newcode160
src/platform/vboot_reference/include/padding.h:160: static const char*
algo_strings[kNumAlgorithms] = {
Will these only ever be included in one place or will they be needed elsewhere
where being static on include is a problem?

(E.g., should there be a hand written .h with the extern declarations and an
autogenerated .c with the const, non-static definitions?)

http://codereview.chromium.org/553023/diff/8019/8027
File src/platform/vboot_reference/include/rsa.h (right):

http://codereview.chromium.org/553023/diff/8019/8027#newcode10
src/platform/vboot_reference/include/rsa.h:10: #define RSA1024NUMBYTES 128      
    /* 1024 bit key length */
Comment should be two spaces from the value, not vertically aligned.  This goes
for the struct comments below too.

http://codereview.chromium.org/553023/diff/8019/8028
File src/platform/vboot_reference/include/sha.h (right):

http://codereview.chromium.org/553023/diff/8019/8028#newcode2
src/platform/vboot_reference/include/sha.h:2: Use of this source code is
governed by a BSD-style license that can be
Please use:
/* Comment line 1
 * Comment line 2
 * ...
 * Comment line N
 */

:)

http://codereview.chromium.org/553023/diff/8019/8028#newcode7
src/platform/vboot_reference/include/sha.h:7: #ifndef _SHA_H_
This should follow out C++ style plan:

#ifndef VBOOT_REFERENCE_SHA_H_

http://codereview.chromium.org/553023/diff/8019/8029
File src/platform/vboot_reference/tests/Makefile (right):

http://codereview.chromium.org/553023/diff/8019/8029#newcode1
src/platform/vboot_reference/tests/Makefile:1: SRCS=sha_tests.c verify_data.c
Please include copyright boilerplates in Makefiles as well

http://codereview.chromium.org/553023/diff/8019/8030
File src/platform/vboot_reference/tests/run_tests.sh (right):

http://codereview.chromium.org/553023/diff/8019/8030#newcode7
src/platform/vboot_reference/tests/run_tests.sh:7: # Run tests for cryptographic
routine implementations - Message digests and RSA Signature verification.
Please make this < 80 chars

http://codereview.chromium.org/553023/diff/8019/8030#newcode35
src/platform/vboot_reference/tests/run_tests.sh:35: openssl pkeyutl -in
$1.digest.$i -inkey key_rsa$j.pem -pkeyopt digest:$i > $1.rsa$j\_$i.sig
Line exceeds 80 chars (and in other places too)

http://codereview.chromium.org/553023/diff/8019/8030#newcode47
src/platform/vboot_reference/tests/run_tests.sh:47: ./verify_data
$algorithmcounter key_rsa${rsaalgo}.keyb
${TEST_FILE}.rsa${rsaalgo}_${hashalgo}.sig ${TEST_FILE}
Do you track failures?  Should a non-zero error code be propagated out of this
script?

http://codereview.chromium.org/553023/diff/8019/8032
File src/platform/vboot_reference/tests/sha_tests.c (right):

http://codereview.chromium.org/553023/diff/8019/8032#newcode15
src/platform/vboot_reference/tests/sha_tests.c:15: void SHA1_tests(void)
Inconsistent use of {. In other files, you keep it on the same line.  You should
bump it up here too.

http://codereview.chromium.org/553023/diff/8019/8033
File src/platform/vboot_reference/tests/verify_data.c (right):

http://codereview.chromium.org/553023/diff/8019/8033#newcode40
src/platform/vboot_reference/tests/verify_data.c:40: #ifdef DEBUG
Why not use
#ifndef NDEBUG
?

http://codereview.chromium.org/553023/diff/8019/8036
File src/platform/vboot_reference/utils/dumpRSAPublicKey.c (right):

http://codereview.chromium.org/553023/diff/8019/8036#newcode1
src/platform/vboot_reference/utils/dumpRSAPublicKey.c:1: /* Copyright (c) 2010
The Chromium OS Authors. All rights reserved.
Does this notice conflict with the Apaceh license below?  Also not sure how port
licensing works...  Might be worth asking if you can just include this under our
license.

[gauravsh, 2010-01-26 23:31:06]

Addressed the style issues and responded to some of your comments. I will upload
another patchset which abstracts away the calls to malloc and memcpy in a little
bit.

http://codereview.chromium.org/553023/diff/8019/8023
File src/platform/vboot_reference/crypto/rsa.c (right):

http://codereview.chromium.org/553023/diff/8019/8023#newcode78
src/platform/vboot_reference/crypto/rsa.c:78: ** Input and output big-endian
byte array in inout.
On 2010/01/26 21:45:44, Will Drewry wrote:
> This doesn't match the other multi-line comments. In the other file, I
mentioned
> the style I'd recommend using if possible.  Regardless, it needs to be
> consistent :)

Fixed.

http://codereview.chromium.org/553023/diff/8019/8023#newcode84
src/platform/vboot_reference/crypto/rsa.c:84: uint32_t* aaR = (uint32_t*)
malloc(key->len * sizeof(uint32_t));
On 2010/01/26 21:45:44, Will Drewry wrote:
> Should you check for allocation failures (!=NULL) or do we just assume that
> we'll always have enough memory in the firmware?
> 
> That aside, will you need to write your own malloc implementation?  (If so,
you
> could have the pathological cases handled there so that if malloc fails, it
> jumps to the recovery path address :)

That's a good idea. I will abstract away these calls through a Malloc helper.
For testing, it can simply call the OS malloc. For the firmware implementation,
we can change it to do whatever is required in the firmware. How does that
sound?

http://codereview.chromium.org/553023/diff/8019/8023#newcode135
src/platform/vboot_reference/crypto/rsa.c:135: if (sig_len != (key->len *
sizeof(uint32_t))) {
On 2010/01/26 21:45:44, Will Drewry wrote:
> Will this be the key baked into the firmware?

No, the same routine will be used by various parts of the read-only and
read/write firmware to verify signatures.

http://codereview.chromium.org/553023/diff/8019/8023#newcode152
src/platform/vboot_reference/crypto/rsa.c:152: buf[i] = sig[i];
On 2010/01/26 21:45:44, Will Drewry wrote:
> Would it be worth having your own memcpy and strcpy helpers

Probably. I will just create a Memcpy helper which will just call the linux
memcpy in the present version.

http://codereview.chromium.org/553023/diff/8019/8023#newcode173
src/platform/vboot_reference/crypto/rsa.c:173: if (buf[i] != *hash++) {
On 2010/01/26 21:45:44, Will Drewry wrote:
> Is stack memory at a premium?  If not, I'd say go ahead and make a buf cursor,
> then make these match.
> (Or just include an inlined memcmp helper :)

Stack mem is likely to be a premium. I will add a helper here instead. :)

http://codereview.chromium.org/553023/diff/8019/8024
File src/platform/vboot_reference/crypto/sha1.c (right):

http://codereview.chromium.org/553023/diff/8019/8024#newcode275
src/platform/vboot_reference/crypto/sha1.c:275: /* Convenience function */
On 2010/01/26 21:45:44, Will Drewry wrote:
> Should comment that digest should be pre-allocated of the size SHA1_DIGEST

Fixed comment in header.

http://codereview.chromium.org/553023/diff/8019/8024#newcode284
src/platform/vboot_reference/crypto/sha1.c:284: digest[i] = *p++;
On 2010/01/26 21:45:44, Will Drewry wrote:
> memcpy?

Done.

http://codereview.chromium.org/553023/diff/8019/8026
File src/platform/vboot_reference/include/padding.h (right):

http://codereview.chromium.org/553023/diff/8019/8026#newcode160
src/platform/vboot_reference/include/padding.h:160: static const char*
algo_strings[kNumAlgorithms] = {
On 2010/01/26 21:45:44, Will Drewry wrote:
> Will these only ever be included in one place or will they be needed elsewhere
> where being static on include is a problem?
> 
> (E.g., should there be a hand written .h with the extern declarations and an
> autogenerated .c with the const, non-static definitions?)

These mappings are used at multiple places. I like the idea of separating these
into .h externs and .c definitions. I will make that change with a separate CL.

http://codereview.chromium.org/553023/diff/8019/8027
File src/platform/vboot_reference/include/rsa.h (right):

http://codereview.chromium.org/553023/diff/8019/8027#newcode10
src/platform/vboot_reference/include/rsa.h:10: #define RSA1024NUMBYTES 128      
    /* 1024 bit key length */
On 2010/01/26 21:45:44, Will Drewry wrote:
> Comment should be two spaces from the value, not vertically aligned.  This
goes
> for the struct comments below too.
Done.

http://codereview.chromium.org/553023/diff/8019/8028
File src/platform/vboot_reference/include/sha.h (right):

http://codereview.chromium.org/553023/diff/8019/8028#newcode2
src/platform/vboot_reference/include/sha.h:2: Use of this source code is
governed by a BSD-style license that can be
On 2010/01/26 21:45:44, Will Drewry wrote:
> Please use:
> /* Comment line 1
>  * Comment line 2
>  * ...
>  * Comment line N
>  */
> 
> :)

Done. :)

http://codereview.chromium.org/553023/diff/8019/8028#newcode7
src/platform/vboot_reference/include/sha.h:7: #ifndef _SHA_H_
On 2010/01/26 21:45:44, Will Drewry wrote:
> This should follow out C++ style plan:
> 
> #ifndef VBOOT_REFERENCE_SHA_H_

Done.

http://codereview.chromium.org/553023/diff/8019/8029
File src/platform/vboot_reference/tests/Makefile (right):

http://codereview.chromium.org/553023/diff/8019/8029#newcode1
src/platform/vboot_reference/tests/Makefile:1: SRCS=sha_tests.c verify_data.c
On 2010/01/26 21:45:44, Will Drewry wrote:
> Please include copyright boilerplates in Makefiles as well

Done.

http://codereview.chromium.org/553023/diff/8019/8030
File src/platform/vboot_reference/tests/run_tests.sh (right):

http://codereview.chromium.org/553023/diff/8019/8030#newcode7
src/platform/vboot_reference/tests/run_tests.sh:7: # Run tests for cryptographic
routine implementations - Message digests and RSA Signature verification.
On 2010/01/26 21:45:44, Will Drewry wrote:
> Please make this < 80 chars

Done.

http://codereview.chromium.org/553023/diff/8019/8030#newcode35
src/platform/vboot_reference/tests/run_tests.sh:35: openssl pkeyutl -in
$1.digest.$i -inkey key_rsa$j.pem -pkeyopt digest:$i > $1.rsa$j\_$i.sig
On 2010/01/26 21:45:44, Will Drewry wrote:
> Line exceeds 80 chars (and in other places too)

Fixed.

http://codereview.chromium.org/553023/diff/8019/8030#newcode47
src/platform/vboot_reference/tests/run_tests.sh:47: ./verify_data
$algorithmcounter key_rsa${rsaalgo}.keyb
${TEST_FILE}.rsa${rsaalgo}_${hashalgo}.sig ${TEST_FILE}
On 2010/01/26 21:45:44, Will Drewry wrote:
> Do you track failures?  Should a non-zero error code be propagated out of this
> script?

verify_data outputs whether signature match succeeded or failed. It sets the
return code too but that's not needed for this testing script.

http://codereview.chromium.org/553023/diff/8019/8032
File src/platform/vboot_reference/tests/sha_tests.c (right):

http://codereview.chromium.org/553023/diff/8019/8032#newcode15
src/platform/vboot_reference/tests/sha_tests.c:15: void SHA1_tests(void)
On 2010/01/26 21:45:44, Will Drewry wrote:
> Inconsistent use of {. In other files, you keep it on the same line.  You
should
> bump it up here too.

Yup. Fixed.

http://codereview.chromium.org/553023/diff/8019/8033
File src/platform/vboot_reference/tests/verify_data.c (right):

http://codereview.chromium.org/553023/diff/8019/8033#newcode40
src/platform/vboot_reference/tests/verify_data.c:40: #ifdef DEBUG
On 2010/01/26 21:45:44, Will Drewry wrote:
> Why not use
> #ifndef NDEBUG
> ?

Is the inverted version more standard?

http://codereview.chromium.org/553023/diff/8019/8036
File src/platform/vboot_reference/utils/dumpRSAPublicKey.c (right):

http://codereview.chromium.org/553023/diff/8019/8036#newcode1
src/platform/vboot_reference/utils/dumpRSAPublicKey.c:1: /* Copyright (c) 2010
The Chromium OS Authors. All rights reserved.
On 2010/01/26 21:45:44, Will Drewry wrote:
> Does this notice conflict with the Apaceh license below?  Also not sure how
port
> licensing works...  Might be worth asking if you can just include this under
our
> license.

Resolved offline.

[gauravsh, 2010-01-27 00:11:26]

http://codereview.chromium.org/553023/diff/8019/8023
File src/platform/vboot_reference/crypto/rsa.c (right):

http://codereview.chromium.org/553023/diff/8019/8023#newcode173
src/platform/vboot_reference/crypto/rsa.c:173: if (buf[i] != *hash++) {
On 2010/01/26 23:31:07, gauravsh wrote:
> On 2010/01/26 21:45:44, Will Drewry wrote:
> > Is stack memory at a premium?  If not, I'd say go ahead and make a buf
cursor,
> > then make these match.
> > (Or just include an inlined memcmp helper :)
> 
> Stack mem is likely to be a premium. I will add a helper here instead. :)
> 
> 

Actually, I just realized that memcmp() is a no-go here. There are potential
opportunities for timing attacks (since memcmp() will exit as soon as a failed
match occurs). 

In any case, what step above is likely to take up stack space?

[gauravsh, 2010-01-27 01:21:32]

Abstracted away the memory related functions from the crypto code to a utility.h
interface. I am a bit wary of replacing the padding checks in RSA_verify to use
memcmp (or equivalent). But that can be changed later. PTAL.

[Will Drewry, 2010-01-28 21:32:39]

LGTM with some random nits on style and some checking in test/check code. 
Fix/TODO() what makes sense.

http://codereview.chromium.org/553023/diff/8061/10035
File src/platform/vboot_reference/common/utility_stub.c (right):

http://codereview.chromium.org/553023/diff/8061/10035#newcode5
src/platform/vboot_reference/common/utility_stub.c:5: 
You can merge these two block comments so it is all one file-wide comment.

http://codereview.chromium.org/553023/diff/8061/10035#newcode39
src/platform/vboot_reference/common/utility_stub.c:39: match = 0;
do you want "else match = 1" to make the number of instructions the same either
way?

http://codereview.chromium.org/553023/diff/8061/10035#newcode42
src/platform/vboot_reference/common/utility_stub.c:42: return match;
Do you want it to return 1 when n = 0?

http://codereview.chromium.org/553023/diff/8061/10037
File src/platform/vboot_reference/crypto/genpadding.sh (right):

http://codereview.chromium.org/553023/diff/8061/10037#newcode12
src/platform/vboot_reference/crypto/genpadding.sh:12:
SHA1_Suffix="0x30,0x21,0x30,0x09,0x06,0x05,0x2b,0x0e,0x03,0x02,0x1a,0x05,0x00,0x04,0x14"
It'd be nice if these fit on the 80 char line:
SHA1_Suffix="0x30,0x21,0x30,0x09,0x06,0x05,0x2b,0x0e,0x03,0x02,0x1a,0x05,0x00"\
",0x04,0x14"

same below too.

http://codereview.chromium.org/553023/diff/8061/10038
File src/platform/vboot_reference/crypto/padding.c (right):

http://codereview.chromium.org/553023/diff/8061/10038#newcode5
src/platform/vboot_reference/crypto/padding.c:5: * arrays corresponding to
various combinations of algorithms for RSA signatures.
> 80 chars

That aside -- do you want this checked in or should it be generated at build
time?

(Since then any real style comments aren't so important ;)

http://codereview.chromium.org/553023/diff/8061/10038#newcode95
src/platform/vboot_reference/crypto/padding.c:95: const int kNumAlgorithms = 12;
So should siglen_map and padding_map be kSiglenMap and kPaddingMap too?

http://codereview.chromium.org/553023/diff/8061/10039
File src/platform/vboot_reference/crypto/rsa.c (right):

http://codereview.chromium.org/553023/diff/8061/10039#newcode178
src/platform/vboot_reference/crypto/rsa.c:178: fprintf(stderr, "Digest:
Expecting = %02x Got = %02x\n", padding[i],
As a later todo, you may want to replace these with a macro call defined in
utils. Then it means that it can be replaced as appropriate for any other
debugging situations.
LOG(...)

http://codereview.chromium.org/553023/diff/8061/10044
File src/platform/vboot_reference/include/sha.h (right):

http://codereview.chromium.org/553023/diff/8061/10044#newcode49
src/platform/vboot_reference/include/sha.h:49: uint8_t buf[SHA512_DIGEST_SIZE]; 
/* Used for storing the final digest. */
Any reason why these aren't just pointers and the final buffer is passed in as:

SHA*_init(SHA*_CTX* ctx, uint8_t digest);

Alternately, do you just expect that the caller will access buf[] in the ctx
after the call?

Just seems like we don't need to do any extra copying if we don't have to.

http://codereview.chromium.org/553023/diff/8061/10044#newcode57
src/platform/vboot_reference/include/sha.h:57: and stores it into [digest].
[digest] should be pre-allocated to
multiline comment deviates from norm.

http://codereview.chromium.org/553023/diff/8061/10048
File src/platform/vboot_reference/tests/sha_test_vectors.h (right):

http://codereview.chromium.org/553023/diff/8061/10048#newcode14
src/platform/vboot_reference/tests/sha_test_vectors.h:14: char *multiblock_msg1
= "abcdbcdecdefdefgefghfghighijhijkijkljklmklmnlmnomnopnopq";
couple of > 80 char lines here and there

http://codereview.chromium.org/553023/diff/8061/10049
File src/platform/vboot_reference/tests/sha_tests.c (right):

http://codereview.chromium.org/553023/diff/8061/10049#newcode18
src/platform/vboot_reference/tests/sha_tests.c:18: uint8_t* sha1_digest =
(uint8_t *) malloc(SHA1_DIGEST_SIZE);
(F)free is never called.  Any reason not to just do:
uint8_t sha1_digest[SHA1_DIGEST_SIZE]; ?

http://codereview.chromium.org/553023/diff/8061/10049#newcode36
src/platform/vboot_reference/tests/sha_tests.c:36: uint8_t* sha256_digest =
(uint8_t *) malloc(SHA256_DIGEST_SIZE);
ditto

http://codereview.chromium.org/553023/diff/8061/10049#newcode82
src/platform/vboot_reference/tests/sha_tests.c:82: return 0;
It would be useful if the test functions returned pass or fail and this was
propagated in the return value.  I know autotest can grep for output, but it
makes it easier to write other scripts with tests if the exit code is
meaningful.

Feel free to just add a /* TODO(gauravsh) .. */ for this

http://codereview.chromium.org/553023/diff/8061/10050
File src/platform/vboot_reference/tests/verify_data.c (right):

http://codereview.chromium.org/553023/diff/8061/10050#newcode33
src/platform/vboot_reference/tests/verify_data.c:33: key = (RSAPublicKey *)
malloc(sizeof(RSAPublicKey));
Do you want to use abstract utilities in this one or just assume a linux host?
(Malloc, Open, Read, ...)

http://codereview.chromium.org/553023/diff/8061/10050#newcode179
src/platform/vboot_reference/tests/verify_data.c:179: signature = (uint8_t*)
malloc(len);
A NULL check wouldn't go amiss to narrow down bugs if we get these into an
auto-test suite and it starts crashing :)

(Same in other places)

http://codereview.chromium.org/553023/diff/8061/10050#newcode199
src/platform/vboot_reference/tests/verify_data.c:199: fprintf(stderr, "Usage: %s
<algorithm> <key file> <signature file> <input file>\n\n",
> 80 chars; same on line 201

http://codereview.chromium.org/553023/diff/8061/10050#newcode209
src/platform/vboot_reference/tests/verify_data.c:209: sig_len =
siglen_map[algorithm] * sizeof(uint32_t);
Is it worth checking that algorithm < kNumAlgorithms just to avoid weirdness?

http://codereview.chromium.org/553023/diff/8061/10053
File src/platform/vboot_reference/utils/dumpRSAPublicKey.c (right):

http://codereview.chromium.org/553023/diff/8061/10053#newcode33
src/platform/vboot_reference/utils/dumpRSAPublicKey.c:33: if (modulus != 1024 &&
modulus != 2048 && modulus != 4096 && modulus !=8192) {
needs a space !=8192 -> != 8192

[gauravsh, 2010-01-28 22:38:08]

LGTMed in the previous pass but just in case...

http://codereview.chromium.org/553023/diff/8061/10035
File src/platform/vboot_reference/common/utility_stub.c (right):

http://codereview.chromium.org/553023/diff/8061/10035#newcode5
src/platform/vboot_reference/common/utility_stub.c:5: 
On 2010/01/28 21:32:39, Will Drewry wrote:
> You can merge these two block comments so it is all one file-wide comment.

Done.

http://codereview.chromium.org/553023/diff/8061/10035#newcode39
src/platform/vboot_reference/common/utility_stub.c:39: match = 0;
On 2010/01/28 21:32:39, Will Drewry wrote:
> do you want "else match = 1" to make the number of instructions the same
either
> way?  
Good point. done.

http://codereview.chromium.org/553023/diff/8061/10035#newcode42
src/platform/vboot_reference/common/utility_stub.c:42: return match;
On 2010/01/28 21:32:39, Will Drewry wrote:
> Do you want it to return 1 when n = 0?
Yes (but I think it shouldn't matter).

http://codereview.chromium.org/553023/diff/8061/10037
File src/platform/vboot_reference/crypto/genpadding.sh (right):

http://codereview.chromium.org/553023/diff/8061/10037#newcode12
src/platform/vboot_reference/crypto/genpadding.sh:12:
SHA1_Suffix="0x30,0x21,0x30,0x09,0x06,0x05,0x2b,0x0e,0x03,0x02,0x1a,0x05,0x00,0x04,0x14"
On 2010/01/28 21:32:39, Will Drewry wrote:
> It'd be nice if these fit on the 80 char line:
>
SHA1_Suffix="0x30,0x21,0x30,0x09,0x06,0x05,0x2b,0x0e,0x03,0x02,0x1a,0x05,0x00"\
> ",0x04,0x14"
> 
> same below too.

done.

http://codereview.chromium.org/553023/diff/8061/10038
File src/platform/vboot_reference/crypto/padding.c (right):

http://codereview.chromium.org/553023/diff/8061/10038#newcode5
src/platform/vboot_reference/crypto/padding.c:5: * arrays corresponding to
various combinations of algorithms for RSA signatures.
On 2010/01/28 21:32:39, Will Drewry wrote:
> > 80 chars
> 
> That aside -- do you want this checked in or should it be generated at build
> time?
> 
> (Since then any real style comments aren't so important ;)

I am checking this in. I can fix the 80 char limit for the comments, but harder
to do it for the generated padding arrays below. :)

http://codereview.chromium.org/553023/diff/8061/10038#newcode95
src/platform/vboot_reference/crypto/padding.c:95: const int kNumAlgorithms = 12;
On 2010/01/28 21:32:39, Will Drewry wrote:
> So should siglen_map and padding_map be kSiglenMap and kPaddingMap too?

Even though they are not of a primitive data type (aka arrays)?

http://codereview.chromium.org/553023/diff/8061/10039
File src/platform/vboot_reference/crypto/rsa.c (right):

http://codereview.chromium.org/553023/diff/8061/10039#newcode178
src/platform/vboot_reference/crypto/rsa.c:178: fprintf(stderr, "Digest:
Expecting = %02x Got = %02x\n", padding[i],
On 2010/01/28 21:32:39, Will Drewry wrote:
> As a later todo, you may want to replace these with a macro call defined in
> utils. Then it means that it can be replaced as appropriate for any other
> debugging situations.
> LOG(...)
> 

Done.

http://codereview.chromium.org/553023/diff/8061/10039#newcode178
src/platform/vboot_reference/crypto/rsa.c:178: fprintf(stderr, "Digest:
Expecting = %02x Got = %02x\n", padding[i],
On 2010/01/28 21:32:39, Will Drewry wrote:
> As a later todo, you may want to replace these with a macro call defined in
> utils. Then it means that it can be replaced as appropriate for any other
> debugging situations.
> LOG(...)
> 
Done.

http://codereview.chromium.org/553023/diff/8061/10044
File src/platform/vboot_reference/include/sha.h (right):

http://codereview.chromium.org/553023/diff/8061/10044#newcode49
src/platform/vboot_reference/include/sha.h:49: uint8_t buf[SHA512_DIGEST_SIZE]; 
/* Used for storing the final digest. */
On 2010/01/28 21:32:39, Will Drewry wrote:
> Any reason why these aren't just pointers and the final buffer is passed in
as:

I added it to make it consistent with the SHA1 code (it uses the buf during
intermediate operations).
> 
> SHA*_init(SHA*_CTX* ctx, uint8_t digest);
> 
> Alternately, do you just expect that the caller will access buf[] in the ctx
> after the call?

Yes, with the caveat that buf is only populated after a *_final() call. 
> 
> Just seems like we don't need to do any extra copying if we don't have to.

Agreed. I will work on these efficiency considerations once it's clearer how
these functions will be used exactly in the firmware.

http://codereview.chromium.org/553023/diff/8061/10044#newcode57
src/platform/vboot_reference/include/sha.h:57: and stores it into [digest].
[digest] should be pre-allocated to
On 2010/01/28 21:32:39, Will Drewry wrote:
> multiline comment deviates from norm.
Fixed.

http://codereview.chromium.org/553023/diff/8061/10048
File src/platform/vboot_reference/tests/sha_test_vectors.h (right):

http://codereview.chromium.org/553023/diff/8061/10048#newcode14
src/platform/vboot_reference/tests/sha_test_vectors.h:14: char *multiblock_msg1
= "abcdbcdecdefdefgefghfghighijhijkijkljklmklmnlmnomnopnopq";
On 2010/01/28 21:32:39, Will Drewry wrote:
> couple of > 80 char lines here and there

Fixed.

http://codereview.chromium.org/553023/diff/8061/10049
File src/platform/vboot_reference/tests/sha_tests.c (right):

http://codereview.chromium.org/553023/diff/8061/10049#newcode18
src/platform/vboot_reference/tests/sha_tests.c:18: uint8_t* sha1_digest =
(uint8_t *) malloc(SHA1_DIGEST_SIZE);
On 2010/01/28 21:32:39, Will Drewry wrote:
> (F)free is never called.  Any reason not to just do:
> uint8_t sha1_digest[SHA1_DIGEST_SIZE]; ?

Fixed. yeah no need for (M)malloc()-ing. :)

http://codereview.chromium.org/553023/diff/8061/10049#newcode82
src/platform/vboot_reference/tests/sha_tests.c:82: return 0;
On 2010/01/28 21:32:39, Will Drewry wrote:
> It would be useful if the test functions returned pass or fail and this was
> propagated in the return value.  I know autotest can grep for output, but it
> makes it easier to write other scripts with tests if the exit code is
> meaningful.
> 
> Feel free to just add a /* TODO(gauravsh) .. */ for this

Fixed the tests to return success or failure.

http://codereview.chromium.org/553023/diff/8061/10050
File src/platform/vboot_reference/tests/verify_data.c (right):

http://codereview.chromium.org/553023/diff/8061/10050#newcode33
src/platform/vboot_reference/tests/verify_data.c:33: key = (RSAPublicKey *)
malloc(sizeof(RSAPublicKey));
On 2010/01/28 21:32:39, Will Drewry wrote:
> Do you want to use abstract utilities in this one or just assume a linux host?
> (Malloc, Open, Read, ...)

Yeah, this is mainly for testing right now. I will have something similar for
the actual firmware code.

http://codereview.chromium.org/553023/diff/8061/10050#newcode179
src/platform/vboot_reference/tests/verify_data.c:179: signature = (uint8_t*)
malloc(len);
On 2010/01/28 21:32:39, Will Drewry wrote:
> A NULL check wouldn't go amiss to narrow down bugs if we get these into an
> auto-test suite and it starts crashing :)
> 
> (Same in other places)

Added NULL checks.

http://codereview.chromium.org/553023/diff/8061/10050#newcode199
src/platform/vboot_reference/tests/verify_data.c:199: fprintf(stderr, "Usage: %s
<algorithm> <key file> <signature file> <input file>\n\n",
On 2010/01/28 21:32:39, Will Drewry wrote:
> > 80 chars; same on line 201

Fixed.

http://codereview.chromium.org/553023/diff/8061/10050#newcode209
src/platform/vboot_reference/tests/verify_data.c:209: sig_len =
siglen_map[algorithm] * sizeof(uint32_t);
On 2010/01/28 21:32:39, Will Drewry wrote:
> Is it worth checking that algorithm < kNumAlgorithms just to avoid weirdness?

Yes, missed that. Added.

http://codereview.chromium.org/553023/diff/8061/10053
File src/platform/vboot_reference/utils/dumpRSAPublicKey.c (right):

http://codereview.chromium.org/553023/diff/8061/10053#newcode33
src/platform/vboot_reference/utils/dumpRSAPublicKey.c:33: if (modulus != 1024 &&
modulus != 2048 && modulus != 4096 && modulus !=8192) {
On 2010/01/28 21:32:39, Will Drewry wrote:
> needs a space !=8192 -> != 8192
> 
Fixed.

[mschilder, 2010-02-01 19:40:38]

http://codereview.chromium.org/553023/diff/8061/10053
File src/platform/vboot_reference/utils/dumpRSAPublicKey.c (right):

http://codereview.chromium.org/553023/diff/8061/10053#newcode34
src/platform/vboot_reference/utils/dumpRSAPublicKey.c:34: fprintf(stderr,
"ERROR: Unknown modulus length = %d.\n", modulus);
s/Unknown/Unsupported/?

http://codereview.chromium.org/553023/diff/11002/12031
File src/platform/vboot_reference/crypto/padding.c (right):

http://codereview.chromium.org/553023/diff/11002/12031#newcode92
src/platform/vboot_reference/crypto/padding.c:92:
0x00,0x01,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0x00,0x30,0x51,0x30,0x0d,0x06,0x09,0x60,0x86,0x48,0x01,0x65,0x03,0x04,0x02,0x03,0x05,0x00,0x04,0x40
These are taking up a lot of space at some point. You could replace the literal
bytes with their hash. Using the same hash as the signature algo. Perhaps even
get as clever as:
compute modpowF4; xor hash of data into least significant bytes (should become
all 0 if hash is as expected); hash entire resulting sig buf; compare against
expected const precomputed hash.

http://codereview.chromium.org/553023/diff/11002/12032
File src/platform/vboot_reference/crypto/rsa.c (right):

http://codereview.chromium.org/553023/diff/11002/12032#newcode107
src/platform/vboot_reference/crypto/rsa.c:107: 
extra line?

http://codereview.chromium.org/553023/diff/11002/12032#newcode122
src/platform/vboot_reference/crypto/rsa.c:122: Free(a);
depending on how minimalist/crappy your allocator is, you might get better
efficiency by releasing in reverse order of allocating. So Free(aaR); Free(aR);
Free(a).

[gauravsh, 2010-02-08 20:28:17]

Sorry I didn't realize you had more comments. I have responded below to your
last batch of comments. If you'd like to continue further with the review of the
RSA verification code, I can create a new issue.

Thanks!

http://codereview.chromium.org/553023/diff/8061/10053
File src/platform/vboot_reference/utils/dumpRSAPublicKey.c (right):

http://codereview.chromium.org/553023/diff/8061/10053#newcode34
src/platform/vboot_reference/utils/dumpRSAPublicKey.c:34: fprintf(stderr,
"ERROR: Unknown modulus length = %d.\n", modulus);
On 2010/02/01 19:40:39, mschilder wrote:
> s/Unknown/Unsupported/?
Done.

http://codereview.chromium.org/553023/diff/11002/12031
File src/platform/vboot_reference/crypto/padding.c (right):

http://codereview.chromium.org/553023/diff/11002/12031#newcode92
src/platform/vboot_reference/crypto/padding.c:92:
0x00,0x01,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0xff,0x00,0x30,0x51,0x30,0x0d,0x06,0x09,0x60,0x86,0x48,0x01,0x65,0x03,0x04,0x02,0x03,0x05,0x00,0x04,0x40
On 2010/02/01 19:40:39, mschilder wrote:
> These are taking up a lot of space at some point. You could replace the
literal
> bytes with their hash. Using the same hash as the signature algo. Perhaps even
> get as clever as:
> compute modpowF4; xor hash of data into least significant bytes (should become
> all 0 if hash is as expected); hash entire resulting sig buf; compare against
> expected const precomputed hash.

Yes, I am hoping to reduce the space requirement, probably by RLE encoding the
0xffs.

I like your suggestions for the other optimizations too, but I'd like to keep
the current implementation simple until we have some hard performance numbers
for this.

http://codereview.chromium.org/553023/diff/11002/12032
File src/platform/vboot_reference/crypto/rsa.c (right):

http://codereview.chromium.org/553023/diff/11002/12032#newcode107
src/platform/vboot_reference/crypto/rsa.c:107: 
On 2010/02/01 19:40:39, mschilder wrote:
> extra line?
Fixed.

http://codereview.chromium.org/553023/diff/11002/12032#newcode122
src/platform/vboot_reference/crypto/rsa.c:122: Free(a);
On 2010/02/01 19:40:39, mschilder wrote:
> depending on how minimalist/crappy your allocator is, you might get better
> efficiency by releasing in reverse order of allocating. So Free(aaR);
Free(aR);
> Free(a).

Done.