Vboot Reference: Refactor Code.

Vboot Reference: Refactor Code.

This CL does the following:
1) It adds a SignatureBuf function which uses the OpenSSL library to generate RSA signature. This is more robust than the previous way of invoking the command line "openssl" utility and capturing its output. No more unnecessary temporary files for signature operations.
2) It adds functions that allow direct manipulation of binary verified Firmware and Kernel Image blobs in memory.
3) It changes the structure field members for FirmwareImage to make it consistent with KernelImage. Now it's clearer which key is used when.
4) Minor bug fixes and slightly improved API for dealing verified boot firmware and kernel images.
5) Renames the RSA_verify function to prevent conflicts with OpenSSL since it's linked into the firmware utility binary.

[gauravsh, 2010-03-02 04:23:04]

This review might seem big, but it's mostly a lot of refactoring. Let me know if
you need me to add another reviewer to this. :)

[sosa, 2010-03-02 05:03:07]

http://codereview.chromium.org/661353/diff/1/2
File src/platform/vboot_reference/common/utility_stub.c (right):

http://codereview.chromium.org/661353/diff/1/2#newcode63
src/platform/vboot_reference/common/utility_stub.c:63: state->remaining_len =
-1;
return NULL; ?

http://codereview.chromium.org/661353/diff/1/3
File src/platform/vboot_reference/crypto/rsa.c (right):

http://codereview.chromium.org/661353/diff/1/3#newcode130
src/platform/vboot_reference/crypto/rsa.c:130: int RSAVerify(const RSAPublicKey
*key,
Spacing

http://codereview.chromium.org/661353/diff/1/7
File src/platform/vboot_reference/include/kernel_image.h (right):

http://codereview.chromium.org/661353/diff/1/7#newcode85
src/platform/vboot_reference/include/kernel_image.h:85: * Caller owns the
returned pointer and mus Free() it.
must*

http://codereview.chromium.org/661353/diff/1/7#newcode206
src/platform/vboot_reference/include/kernel_image.h:206: int
AddKernelSignature(KernelImage* image, const char* kernel_sigining_key_file);
one char too looong

http://codereview.chromium.org/661353/diff/1/18
File src/platform/vboot_reference/utils/firmware_image.c (right):

http://codereview.chromium.org/661353/diff/1/18#newcode78
src/platform/vboot_reference/utils/firmware_image.c:78: StatefulMemcpy(&st,
&image->firmware_sign_algorithm,
trailing space

http://codereview.chromium.org/661353/diff/1/18#newcode105
src/platform/vboot_reference/utils/firmware_image.c:105: StatefulMemcpy(&st,
&image->firmware_key_version, FIELD_LEN(firmware_key_version));
80 char

http://codereview.chromium.org/661353/diff/1/18#newcode109
src/platform/vboot_reference/utils/firmware_image.c:109: StatefulMemcpy(&st,
image->firmware_key_signature, FIELD_LEN(firmware_key_signature));
80 char

http://codereview.chromium.org/661353/diff/1/18#newcode325
src/platform/vboot_reference/utils/firmware_image.c:325: header_blob +
(base_header_checksum_offset + firmware_sign_key_len),
80 char

http://codereview.chromium.org/661353/diff/1/18#newcode411
src/platform/vboot_reference/utils/firmware_image.c:411: firmware_sign_key =
RSAPublicKeyFromBuf(firmware_sign_key_ptr, firmware_sign_key_len);
80 char

http://codereview.chromium.org/661353/diff/1/18#newcode417
src/platform/vboot_reference/utils/firmware_image.c:417: if ((error_code =
VerifyFirmwarePreamble(firmware_sign_key, preamble_ptr, algorithm,
80 char

http://codereview.chromium.org/661353/diff/1/18#newcode428
src/platform/vboot_reference/utils/firmware_image.c:428: if ((error_code =
VerifyFirmwareData(firmware_sign_key, firmware_ptr, firmware_len,
80 char

http://codereview.chromium.org/661353/diff/1/18#newcode509
src/platform/vboot_reference/utils/firmware_image.c:509: signature_size,
image->firmware_sign_algorithm,
if (*

http://codereview.chromium.org/661353/diff/1/19
File src/platform/vboot_reference/utils/firmware_utility.cc (right):

http://codereview.chromium.org/661353/diff/1/19#newcode165
src/platform/vboot_reference/utils/firmware_utility.cc:165:
image_->firmware_sign_key = BufferFromFile(firmware_sign_key_pub_file_.c_str(),
80 char

http://codereview.chromium.org/661353/diff/1/19#newcode207
src/platform/vboot_reference/utils/firmware_utility.cc:207:
if(!AddFirmwareSignature(image_, firmware_sign_key_file_.c_str(),
if (

http://codereview.chromium.org/661353/diff/1/20
File src/platform/vboot_reference/utils/kernel_image.c (right):

http://codereview.chromium.org/661353/diff/1/20#newcode183
src/platform/vboot_reference/utils/kernel_image.c:183: StatefulMemcpy_r(&st,
&image->header_checksum, FIELD_LEN(header_checksum));
Check remaining length?

http://codereview.chromium.org/661353/diff/1/20#newcode209
src/platform/vboot_reference/utils/kernel_image.c:209: return config_blob;
Check remaining length?

http://codereview.chromium.org/661353/diff/1/20#newcode594
src/platform/vboot_reference/utils/kernel_image.c:594:
if(!RSAVerify(kernel_sign_key, image->kernel_signature,
if (

http://codereview.chromium.org/661353/diff/1/20#newcode643
src/platform/vboot_reference/utils/kernel_image.c:643: if(!(config_signature =
SignatureBuf(config_blob,
if (

http://codereview.chromium.org/661353/diff/1/21
File src/platform/vboot_reference/utils/signature_digest.c (right):

http://codereview.chromium.org/661353/diff/1/21#newcode59
src/platform/vboot_reference/utils/signature_digest.c:59: goto failure;
Goto's are not allowed in our codebase.  Try refactoring with if's / else's.  I
know some kernel code likes to do the, if fail, status != 0 and then check if
status = 0 before every if i.e. status_success

http://codereview.chromium.org/661353/diff/1/22
File src/platform/vboot_reference/utils/verify_data.c (right):

http://codereview.chromium.org/661353/diff/1/22#newcode84
src/platform/vboot_reference/utils/verify_data.c:84: if(RSAVerify(key,
signature, sig_len, algorithm, digest)) {
if (

[gauravsh, 2010-03-02 08:53:23]

http://codereview.chromium.org/661353/diff/1/2
File src/platform/vboot_reference/common/utility_stub.c (right):

http://codereview.chromium.org/661353/diff/1/2#newcode63
src/platform/vboot_reference/common/utility_stub.c:63: state->remaining_len =
-1;
On 2010/03/02 05:03:07, sosa wrote:
> return NULL; ?

Yes, thanks for catching that. Done.

http://codereview.chromium.org/661353/diff/1/3
File src/platform/vboot_reference/crypto/rsa.c (right):

http://codereview.chromium.org/661353/diff/1/3#newcode130
src/platform/vboot_reference/crypto/rsa.c:130: int RSAVerify(const RSAPublicKey
*key,
On 2010/03/02 05:03:07, sosa wrote:
> Spacing
Done.

http://codereview.chromium.org/661353/diff/1/7
File src/platform/vboot_reference/include/kernel_image.h (right):

http://codereview.chromium.org/661353/diff/1/7#newcode85
src/platform/vboot_reference/include/kernel_image.h:85: * Caller owns the
returned pointer and mus Free() it.
On 2010/03/02 05:03:07, sosa wrote:
> must*

Done. For all.

http://codereview.chromium.org/661353/diff/1/7#newcode206
src/platform/vboot_reference/include/kernel_image.h:206: int
AddKernelSignature(KernelImage* image, const char* kernel_sigining_key_file);
On 2010/03/02 05:03:07, sosa wrote:
> one char too looong

Fixed.

http://codereview.chromium.org/661353/diff/1/18
File src/platform/vboot_reference/utils/firmware_image.c (right):

http://codereview.chromium.org/661353/diff/1/18#newcode78
src/platform/vboot_reference/utils/firmware_image.c:78: StatefulMemcpy(&st,
&image->firmware_sign_algorithm,
On 2010/03/02 05:03:07, sosa wrote:
> trailing space
doesn't exist?

http://codereview.chromium.org/661353/diff/1/18#newcode105
src/platform/vboot_reference/utils/firmware_image.c:105: StatefulMemcpy(&st,
&image->firmware_key_version, FIELD_LEN(firmware_key_version));
On 2010/03/02 05:03:07, sosa wrote:
> 80 char

Done.

http://codereview.chromium.org/661353/diff/1/18#newcode109
src/platform/vboot_reference/utils/firmware_image.c:109: StatefulMemcpy(&st,
image->firmware_key_signature, FIELD_LEN(firmware_key_signature));
On 2010/03/02 05:03:07, sosa wrote:
> 80 char
Done.

http://codereview.chromium.org/661353/diff/1/18#newcode325
src/platform/vboot_reference/utils/firmware_image.c:325: header_blob +
(base_header_checksum_offset + firmware_sign_key_len),
On 2010/03/02 05:03:07, sosa wrote:
> 80 char
Done.

http://codereview.chromium.org/661353/diff/1/18#newcode411
src/platform/vboot_reference/utils/firmware_image.c:411: firmware_sign_key =
RSAPublicKeyFromBuf(firmware_sign_key_ptr, firmware_sign_key_len);
On 2010/03/02 05:03:07, sosa wrote:
> 80 char

Done.

http://codereview.chromium.org/661353/diff/1/18#newcode417
src/platform/vboot_reference/utils/firmware_image.c:417: if ((error_code =
VerifyFirmwarePreamble(firmware_sign_key, preamble_ptr, algorithm,
On 2010/03/02 05:03:07, sosa wrote:
> 80 char
Done.

http://codereview.chromium.org/661353/diff/1/18#newcode428
src/platform/vboot_reference/utils/firmware_image.c:428: if ((error_code =
VerifyFirmwareData(firmware_sign_key, firmware_ptr, firmware_len,
On 2010/03/02 05:03:07, sosa wrote:
> 80 char
Done.

http://codereview.chromium.org/661353/diff/1/18#newcode509
src/platform/vboot_reference/utils/firmware_image.c:509: signature_size,
image->firmware_sign_algorithm,
On 2010/03/02 05:03:07, sosa wrote:
> if (*
Done.

http://codereview.chromium.org/661353/diff/1/19
File src/platform/vboot_reference/utils/firmware_utility.cc (right):

http://codereview.chromium.org/661353/diff/1/19#newcode165
src/platform/vboot_reference/utils/firmware_utility.cc:165:
image_->firmware_sign_key = BufferFromFile(firmware_sign_key_pub_file_.c_str(),
On 2010/03/02 05:03:07, sosa wrote:
> 80 char
Done.

http://codereview.chromium.org/661353/diff/1/19#newcode207
src/platform/vboot_reference/utils/firmware_utility.cc:207:
if(!AddFirmwareSignature(image_, firmware_sign_key_file_.c_str(),
On 2010/03/02 05:03:07, sosa wrote:
> if (
Done.

http://codereview.chromium.org/661353/diff/1/20
File src/platform/vboot_reference/utils/kernel_image.c (right):

http://codereview.chromium.org/661353/diff/1/20#newcode183
src/platform/vboot_reference/utils/kernel_image.c:183: StatefulMemcpy_r(&st,
&image->header_checksum, FIELD_LEN(header_checksum));
On 2010/03/02 05:03:07, sosa wrote:
> Check remaining length?
Thanks for catching that. Done.

http://codereview.chromium.org/661353/diff/1/20#newcode209
src/platform/vboot_reference/utils/kernel_image.c:209: return config_blob;
On 2010/03/02 05:03:07, sosa wrote:
> Check remaining length?

Done.

http://codereview.chromium.org/661353/diff/1/20#newcode594
src/platform/vboot_reference/utils/kernel_image.c:594:
if(!RSAVerify(kernel_sign_key, image->kernel_signature,
On 2010/03/02 05:03:07, sosa wrote:
> if (
Done.

http://codereview.chromium.org/661353/diff/1/20#newcode643
src/platform/vboot_reference/utils/kernel_image.c:643: if(!(config_signature =
SignatureBuf(config_blob,
On 2010/03/02 05:03:07, sosa wrote:
> if (

Done.

http://codereview.chromium.org/661353/diff/1/21
File src/platform/vboot_reference/utils/signature_digest.c (right):

http://codereview.chromium.org/661353/diff/1/21#newcode59
src/platform/vboot_reference/utils/signature_digest.c:59: goto failure;
On 2010/03/02 05:03:07, sosa wrote:
> Goto's are not allowed in our codebase.  Try refactoring with if's / else's. 
I
> know some kernel code likes to do the, if fail, status != 0 and then check if
> status = 0 before every if i.e. status_success
I mostly use goto only in C as it simplifies guaranteed temporary object
destruction in case of errors. I have refactored the code to remove the use of
goto.

http://codereview.chromium.org/661353/diff/1/22
File src/platform/vboot_reference/utils/verify_data.c (right):

http://codereview.chromium.org/661353/diff/1/22#newcode84
src/platform/vboot_reference/utils/verify_data.c:84: if(RSAVerify(key,
signature, sig_len, algorithm, digest)) {
On 2010/03/02 05:03:07, sosa wrote:
> if (
Done.

[sosa, 2010-03-02 20:22:14]

lgtm