Vboot Reference: Add functions to verify signed kernel images.

Vboot Reference: Add functions to verify signed kernel images.

BUG=670
TEST=Adds kernel_image_test which tests the new functions.

The kernel image verification pretty much exactly mirror the already existing firmware image verification functions except with a few different/additional fields in a signed kernel image. The firmware signing key is the root key equivalent for kernel images.

This CL also moves the image verification tests to a different script. There's some additional cleanup of the code that I will be submitting separately after this and another pending patches get LGTMed and land.

[gauravsh, 2010-02-26 04:56:55]

Randall: Can you have a look at the KernelImage structure and let me know if it
mirrors what we need/decided upon.

Chris: The rest is for your kind perusal and LGTMification.
Thanks!

[gauravsh, 2010-02-26 05:03:05]

Ugh, i just realized that git cl upload also picked up diffs from changes that
are under a separate pending code review (I am working off that as a branch (bad
idea?).

So, to make it clear:
Just review the kernel_image.c, kernel_image.h, kernel_image_tests.c, and 
run_rsa_tests.sh, run_image_verification_tests.sh. Ignore the rest. The changes
to the other files are independent of this change

[Randall Spangler, 2010-02-26 22:38:40]

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

http://codereview.chromium.org/660161/diff/1/5#newcode23
src/platform/vboot_reference/include/kernel_image.h:23: uint32_t
kernel_load_addr;  /* Load address in memory for the kernel image */
Make addresses 64-bit.  EFI BIOS will still be in 64-bit mode when
loading/executing the kernel image entry point.

http://codereview.chromium.org/660161/diff/1/5#newcode25
src/platform/vboot_reference/include/kernel_image.h:25: } kconfig_options;
Config file also contains command line to pass to kernel.

http://codereview.chromium.org/660161/diff/1/5#newcode30
src/platform/vboot_reference/include/kernel_image.h:30: /* Key header */
Add a header_version field, so that we can rev this struct later and still read
old kernels.  For now, just set =1.

http://codereview.chromium.org/660161/diff/1/5#newcode37
src/platform/vboot_reference/include/kernel_image.h:37: uint8_t*
kernel_sign_key;  /* Pre-processed public half of signing key. */
Can you always tell the length of kernel_sign_key based on the value of
kernel_sign_algorithm?

http://codereview.chromium.org/660161/diff/1/5#newcode38
src/platform/vboot_reference/include/kernel_image.h:38: uint16_t
kernel_key_version;  /* Key Version# for preventing rollbacks. */
Swap sign_key and key_version, so the variable-length sign_key is at the end of
the key header?

http://codereview.chromium.org/660161/diff/1/5#newcode41
src/platform/vboot_reference/include/kernel_image.h:41: uint8_t
header_checksum[SHA512_DIGEST_SIZE];  /* SHA-512 Crytographic hash of
Why do we have checksum+signature for the header, but only signature for the
config and kernel data?

[gauravsh, 2010-02-26 23:21:56]

Cleaned up the patch to remove all the leakage from the other CL. Git
interactive rebasing FTW.

[gauravsh, 2010-02-27 01:25:07]

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

http://codereview.chromium.org/660161/diff/1/5#newcode23
src/platform/vboot_reference/include/kernel_image.h:23: uint32_t
kernel_load_addr;  /* Load address in memory for the kernel image */
On 2010/02/26 22:38:41, Randall Spangler wrote:
> Make addresses 64-bit.  EFI BIOS will still be in 64-bit mode when
> loading/executing the kernel image entry point.
Done.

http://codereview.chromium.org/660161/diff/1/5#newcode25
src/platform/vboot_reference/include/kernel_image.h:25: } kconfig_options;
On 2010/02/26 22:38:41, Randall Spangler wrote:
> Config file also contains command line to pass to kernel.

Is there a limit on the size of the command line? Can it just be a fixed length
buffer?

http://codereview.chromium.org/660161/diff/1/5#newcode30
src/platform/vboot_reference/include/kernel_image.h:30: /* Key header */
On 2010/02/26 22:38:41, Randall Spangler wrote:
> Add a header_version field, so that we can rev this struct later and still
read
> old kernels.  For now, just set =1.
Done.

http://codereview.chromium.org/660161/diff/1/5#newcode37
src/platform/vboot_reference/include/kernel_image.h:37: uint8_t*
kernel_sign_key;  /* Pre-processed public half of signing key. */
On 2010/02/26 22:38:41, Randall Spangler wrote:
> Can you always tell the length of kernel_sign_key based on the value of
> kernel_sign_algorithm?
> 
Yes.

http://codereview.chromium.org/660161/diff/1/5#newcode38
src/platform/vboot_reference/include/kernel_image.h:38: uint16_t
kernel_key_version;  /* Key Version# for preventing rollbacks. */
On 2010/02/26 22:38:41, Randall Spangler wrote:
> Swap sign_key and key_version, so the variable-length sign_key is at the end
of
> the key header?
Makes sense. Done.

http://codereview.chromium.org/660161/diff/1/5#newcode41
src/platform/vboot_reference/include/kernel_image.h:41: uint8_t
header_checksum[SHA512_DIGEST_SIZE];  /* SHA-512 Crytographic hash of
On 2010/02/26 22:38:41, Randall Spangler wrote:
> Why do we have checksum+signature for the header, but only signature for the
> config and kernel data?
In dev mode, the key signature is not checked, but only the SHA-512 header
checksum is verified for integrity checking. The other signatures are always
checked (and therefore provide integrity protection) irrespective of the dev
mode, therefore no checksum is required.

[Chris Masone, 2010-02-28 23:02:51]

Mostly just nits, except the one in the destructor in kernel_image.c

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

http://codereview.chromium.org/660161/diff/1/5#newcode36
src/platform/vboot_reference/include/kernel_image.h:36: * signing key. */
indent

http://codereview.chromium.org/660161/diff/1/5#newcode45
src/platform/vboot_reference/include/kernel_image.h:45: * key_version] */
does the | here actually mean bitwise or?

http://codereview.chromium.org/660161/diff/47/1004#newcode37
src/platform/vboot_reference/include/kernel_image.h:37: uint8_t*
kernel_sign_key;  /* Pre-processed public half of signing key. */
indent

http://codereview.chromium.org/660161/diff/47/1004#newcode46
src/platform/vboot_reference/include/kernel_image.h:46: 
do these | mean binary or?

http://codereview.chromium.org/660161/diff/47/1007
File src/platform/vboot_reference/tests/kernel_image_tests.c (right):

http://codereview.chromium.org/660161/diff/47/1007#newcode228
src/platform/vboot_reference/tests/kernel_image_tests.c:228: * TODO(gauravsh):
Add a function function to directly generate a binary
function function?

http://codereview.chromium.org/660161/diff/47/1011
File src/platform/vboot_reference/utils/kernel_image.c (right):

http://codereview.chromium.org/660161/diff/47/1011#newcode22
src/platform/vboot_reference/utils/kernel_image.c:22: /* Macro to determine the
size of a field structure in the KernelImage structure. */
80char

http://codereview.chromium.org/660161/diff/47/1011#newcode44
src/platform/vboot_reference/utils/kernel_image.c:44: }
shouldn't you free |image| at the end?  And don't you need to check that these
aren't NULL?

http://codereview.chromium.org/660161/diff/47/1011#newcode93
src/platform/vboot_reference/utils/kernel_image.c:93: kernel_key_signature_len 
= (siglen_map[image->firmware_sign_algorithm] *
put a TODO to refactor this so that you track signature size in number of bytes,
not number of words.

http://codereview.chromium.org/660161/diff/47/1011#newcode128
src/platform/vboot_reference/utils/kernel_image.c:128: StatefulMemcpy(&st,
&image->options.kernel_len, FIELD_LEN(options.kernel_len));
80char

http://codereview.chromium.org/660161/diff/47/1011#newcode192
src/platform/vboot_reference/utils/kernel_image.c:192: if (-1 == (fd =
creat(input_file, S_IRWXU))) {
maybe tell us what S_IRWXU means?

http://codereview.chromium.org/660161/diff/47/1011#newcode483
src/platform/vboot_reference/utils/kernel_image.c:483:
siglen_map[image->firmware_sign_algorithm] * sizeof(uint32_t),
80char

[gauravsh, 2010-03-01 02:48:05]

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

http://codereview.chromium.org/660161/diff/1/5#newcode36
src/platform/vboot_reference/include/kernel_image.h:36: * signing key. */
On 2010/02/28 23:02:52, cmasone wrote:
> indent

Done.

http://codereview.chromium.org/660161/diff/1/5#newcode45
src/platform/vboot_reference/include/kernel_image.h:45: * key_version] */
On 2010/02/28 23:02:52, cmasone wrote:
> does the | here actually mean bitwise or?
It means concatenation. If it's not clear from the comment, I can change it to
something else.

http://codereview.chromium.org/660161/diff/47/1004#newcode37
src/platform/vboot_reference/include/kernel_image.h:37: uint8_t*
kernel_sign_key;  /* Pre-processed public half of signing key. */
On 2010/02/28 23:02:52, cmasone wrote:
> indent
Done.

http://codereview.chromium.org/660161/diff/47/1007
File src/platform/vboot_reference/tests/kernel_image_tests.c (right):

http://codereview.chromium.org/660161/diff/47/1007#newcode228
src/platform/vboot_reference/tests/kernel_image_tests.c:228: * TODO(gauravsh):
Add a function function to directly generate a binary
On 2010/02/28 23:02:52, cmasone wrote:
> function function?

Done.

http://codereview.chromium.org/660161/diff/47/1011
File src/platform/vboot_reference/utils/kernel_image.c (right):

http://codereview.chromium.org/660161/diff/47/1011#newcode22
src/platform/vboot_reference/utils/kernel_image.c:22: /* Macro to determine the
size of a field structure in the KernelImage structure. */
On 2010/02/28 23:02:52, cmasone wrote:
> 80char
Done.

http://codereview.chromium.org/660161/diff/47/1011#newcode44
src/platform/vboot_reference/utils/kernel_image.c:44: }
On 2010/02/28 23:02:52, cmasone wrote:
> shouldn't you free |image| at the end?  And don't you need to check that these
> aren't NULL?

Yes, I should. Done. 

Free() (which in turn calls free()) doesn't do anything if the passed pointer is
NULL. Those are the semantics of free() itself.

http://codereview.chromium.org/660161/diff/47/1011#newcode93
src/platform/vboot_reference/utils/kernel_image.c:93: kernel_key_signature_len 
= (siglen_map[image->firmware_sign_algorithm] *
On 2010/02/28 23:02:52, cmasone wrote:
> put a TODO to refactor this so that you track signature size in number of
bytes,
> not number of words.
Done.

http://codereview.chromium.org/660161/diff/47/1011#newcode128
src/platform/vboot_reference/utils/kernel_image.c:128: StatefulMemcpy(&st,
&image->options.kernel_len, FIELD_LEN(options.kernel_len));
On 2010/02/28 23:02:52, cmasone wrote:
> 80char
Done.

http://codereview.chromium.org/660161/diff/47/1011#newcode192
src/platform/vboot_reference/utils/kernel_image.c:192: if (-1 == (fd =
creat(input_file, S_IRWXU))) {
On 2010/02/28 23:02:52, cmasone wrote:
> maybe tell us what S_IRWXU means?
Done.

http://codereview.chromium.org/660161/diff/47/1011#newcode483
src/platform/vboot_reference/utils/kernel_image.c:483:
siglen_map[image->firmware_sign_algorithm] * sizeof(uint32_t),
On 2010/02/28 23:02:52, cmasone wrote:
> 80char
Done.

[Chris Masone, 2010-03-01 03:12:17]

lgtm once you fix the comment.

http://codereview.chromium.org/660161/diff/51/1023
File src/platform/vboot_reference/include/kernel_image.h (right):

http://codereview.chromium.org/660161/diff/51/1023#newcode43
src/platform/vboot_reference/include/kernel_image.h:43: * [header_len |
yeah, I think that bitwise OR could be a legit interpretation of this
syntax...if you can make it clear that you mean concatenation here, that'd be
helpful

[gauravsh, 2010-03-01 03:18:12]

Fixed the comment. Submitting...

http://codereview.chromium.org/660161/diff/51/1023
File src/platform/vboot_reference/include/kernel_image.h (right):

http://codereview.chromium.org/660161/diff/51/1023#newcode43
src/platform/vboot_reference/include/kernel_image.h:43: * [header_len |
On 2010/03/01 03:12:17, cmasone wrote:
> yeah, I think that bitwise OR could be a legit interpretation of this
> syntax...if you can make it clear that you mean concatenation here, that'd be
> helpful

Ok, replaced | with a comma and made it clear it's concatenation.