Data structure and interface for manipulating and handing firmware images for verified boot.

Data structure and interface for manipulating and handing firmware images for verified boot.

[gauravsh, 2010-02-02 23:42:23]

I am getting started on the interface for dealing with and testing firmware
images for verified boot. Before I write any code implementing these functions,
thought I'd get a sanity check on whether the data structures and functions look
ok. This will likely change as I realize missing elements.

[gauravsh, 2010-02-10 01:53:12]

This is now ready for a look. I have verified(!) that the firmware image
verification code works. I just need to fix the existing test-like code for
testing this API into proper tests. But meanwhile, I'd like to hear your
comments on the other parts. Thanks!

[Randall Spangler, 2010-02-10 21:25:26]

LGTM with one question for now and one todo for later.

http://codereview.chromium.org/564020/diff/17001/17009
File src/platform/vboot_reference/utils/firmware_image.c (right):

http://codereview.chromium.org/564020/diff/17001/17009#newcode129
src/platform/vboot_reference/utils/firmware_image.c:129: image->firmware_data =
(uint8_t*) Malloc(image->firmware_len);
Shouldn't we verify the preamble signature before reading the firmware data? 
Otherwise, a malicious image could specify an artificially large firmware_len.

http://codereview.chromium.org/564020/diff/17001/17009#newcode277
src/platform/vboot_reference/utils/firmware_image.c:277: firmware_digest =
DigestBuf(image->firmware_data,
This serializes (reading the entire firmware into RAM) and (calculating the
digest of the firmware data).  Ok for an initial version; for performance, we
should pipeline reading and digesting.

[gauravsh, 2010-02-10 21:40:20]

http://codereview.chromium.org/564020/diff/17001/17009
File src/platform/vboot_reference/utils/firmware_image.c (right):

http://codereview.chromium.org/564020/diff/17001/17009#newcode129
src/platform/vboot_reference/utils/firmware_image.c:129: image->firmware_data =
(uint8_t*) Malloc(image->firmware_len);
On 2010/02/10 21:25:26, Randall Spangler wrote:
> Shouldn't we verify the preamble signature before reading the firmware data? 
> Otherwise, a malicious image could specify an artificially large firmware_len.

This function is just for reading a firmware image from a file, and will not
actually be used in the firmware. The reference code for the firmware will be
similar to VerifyFirmware() below (in which, I do check the preamble signature
first).

http://codereview.chromium.org/564020/diff/17001/17009#newcode277
src/platform/vboot_reference/utils/firmware_image.c:277: firmware_digest =
DigestBuf(image->firmware_data,
On 2010/02/10 21:25:26, Randall Spangler wrote:
> This serializes (reading the entire firmware into RAM) and (calculating the
> digest of the firmware data).  Ok for an initial version; for performance, we
> should pipeline reading and digesting.

Agreed, I do need to abstract away the reading of data from memory. But as I
said above, this is not exactly the reference code (which will be similar for
most part) but for manipulating and managing verified boot firmware images..
That is coming soon with a different CL.

[gauravsh, 2010-02-10 23:39:47]

Fixed the firmware image tests to act like real tests.

[gauravsh, 2010-02-12 19:06:10]

ping

[Will Drewry, 2010-02-12 23:06:28]

LGTM

In general, it'd be nice to do more validation on images before signature just
to catch weird or mangled headers prior to crashing and/or signing them.

http://codereview.chromium.org/564020/diff/17001/17010
File src/platform/vboot_reference/utils/firmware_utility.c (right):

http://codereview.chromium.org/564020/diff/17001/17010#newcode21
src/platform/vboot_reference/utils/firmware_utility.c:21: /* TODO(gauravsh): TO
BE IMPLEMENTED. */
Any reason this can't be done in C++ like our other userspace code?

http://codereview.chromium.org/564020/diff/17012/21002
File src/platform/vboot_reference/include/firmware_image.h (right):

http://codereview.chromium.org/564020/diff/17012/21002#newcode23
src/platform/vboot_reference/include/firmware_image.h:23: typedef struct
FirmwareImage {
Do you expect these to be __attribute__((packed)) ? (packed, align(...)) ?

http://codereview.chromium.org/564020/diff/17012/21004
File src/platform/vboot_reference/tests/firmware_image_tests.c (right):

http://codereview.chromium.org/564020/diff/17012/21004#newcode39
src/platform/vboot_reference/tests/firmware_image_tests.c:39: image->sign_key =
(uint8_t*) Malloc(RSAProcessedKeySize(image->sign_algorithm));
Line too long

http://codereview.chromium.org/564020/diff/17012/21007
File src/platform/vboot_reference/utils/file_keys.c (right):

http://codereview.chromium.org/564020/diff/17012/21007#newcode35
src/platform/vboot_reference/utils/file_keys.c:35: *len = stat_fd.st_size;
I'm pretty sure st_size is off_t which can be int or int64 depending on the host
architecture.  Unlikely to be a problem, but I'd expect -Wall to give you some
complaints?

http://codereview.chromium.org/564020/diff/17012/21008
File src/platform/vboot_reference/utils/firmware_image.c (right):

http://codereview.chromium.org/564020/diff/17012/21008#newcode38
src/platform/vboot_reference/utils/firmware_image.c:38: FirmwareImage* image) {
- space :)

[gauravsh, 2010-02-12 23:25:41]

I agree there should be more sanity checks in the verification code. The fixed
length headers and (signatures on the length field) somewhat ameliorate
potential problems. And I do some checks - like verifying the signature
algorithm. Is there any particular check in the code that is missing from the
VerifyFirmware code?

http://codereview.chromium.org/564020/diff/17001/17010
File src/platform/vboot_reference/utils/firmware_utility.c (right):

http://codereview.chromium.org/564020/diff/17001/17010#newcode21
src/platform/vboot_reference/utils/firmware_utility.c:21: /* TODO(gauravsh): TO
BE IMPLEMENTED. */
On 2010/02/12 23:06:29, Will Drewry wrote:
> Any reason this can't be done in C++ like our other userspace code?
I think this particular utility can be written in C++ since this will be a part
of the tool used by Google to generate signed firmware images.

http://codereview.chromium.org/564020/diff/17012/21002
File src/platform/vboot_reference/include/firmware_image.h (right):

http://codereview.chromium.org/564020/diff/17012/21002#newcode23
src/platform/vboot_reference/include/firmware_image.h:23: typedef struct
FirmwareImage {
On 2010/02/12 23:06:29, Will Drewry wrote:
> Do you expect these to be __attribute__((packed)) ? (packed, align(...)) ?
The way the firmware image binary is written automatically packs them (but
doesn't align them). So, the code doesn't assumes it, no. I was a bit vary of
using this __attribute__ since different compilers may do it differently.

http://codereview.chromium.org/564020/diff/17012/21004
File src/platform/vboot_reference/tests/firmware_image_tests.c (right):

http://codereview.chromium.org/564020/diff/17012/21004#newcode39
src/platform/vboot_reference/tests/firmware_image_tests.c:39: image->sign_key =
(uint8_t*) Malloc(RSAProcessedKeySize(image->sign_algorithm));
On 2010/02/12 23:06:29, Will Drewry wrote:
> Line too long

Fixed.

http://codereview.chromium.org/564020/diff/17012/21007
File src/platform/vboot_reference/utils/file_keys.c (right):

http://codereview.chromium.org/564020/diff/17012/21007#newcode35
src/platform/vboot_reference/utils/file_keys.c:35: *len = stat_fd.st_size;
On 2010/02/12 23:06:29, Will Drewry wrote:
> I'm pretty sure st_size is off_t which can be int or int64 depending on the
host
> architecture.  Unlikely to be a problem, but I'd expect -Wall to give you some
> complaints?
Surprisingly, it didn't. I think that's because C being more weakly-typed than
C++ doesn't complain about such implicit type conversions.

http://codereview.chromium.org/564020/diff/17012/21008
File src/platform/vboot_reference/utils/firmware_image.c (right):

http://codereview.chromium.org/564020/diff/17012/21008#newcode38
src/platform/vboot_reference/utils/firmware_image.c:38: FirmwareImage* image) {
On 2010/02/12 23:06:29, Will Drewry wrote:
> - space :)
Done.