VBoot Reference: Fix splicing bugs in Firmware and Kernel verification.
BUG=670
TESTS=Adds new tests which verify this doesn't occur anymore. Existing tests still pass.
The existing code computes and verifies signatures on firmware/kernel data and firmware/kernel versions separately. This causes a image splicing bug where it is possible to combine together a version signature from a valid new firmware with firmware data and signature from an older version. The same problem exists with kernel verification.
This CL fixes this by changing the firmware/kernel signatures to also include the version information.
For the Firmware, there's a separate signature on the preamble (which contains the version) but the firmware signature now also includes this preamble in addition to the firmware data.
For the Kernel, there's a separate signature on the kernel config/options (wich also contains the version), but the kernel signature now also includes these config/options in addition to the kernel data.
Review URL: http://codereview.chromium.org/1430001
diff --git a/utils/firmware_image.c b/utils/firmware_image.c
index a197708..a598f54 100644
--- a/utils/firmware_image.c
+++ b/utils/firmware_image.c
@@ -190,7 +190,7 @@
return header_blob;
}
-int GetFirmwarePreambleLen(const FirmwareImage* image) {
+int GetFirmwarePreambleLen(void) {
return (FIELD_LEN(firmware_version) + FIELD_LEN(firmware_len) +
FIELD_LEN(preamble));
}
@@ -199,8 +199,8 @@
uint8_t* preamble_blob = NULL;
MemcpyState st;
- preamble_blob = (uint8_t*) Malloc(GetFirmwarePreambleLen(image));
- st.remaining_len = GetFirmwarePreambleLen(image);
+ preamble_blob = (uint8_t*) Malloc(GetFirmwarePreambleLen());
+ st.remaining_len = GetFirmwarePreambleLen();
st.remaining_buf = preamble_blob;
StatefulMemcpy_r(&st, &image->firmware_version, FIELD_LEN(firmware_version));
@@ -229,7 +229,7 @@
*blob_len = (FIELD_LEN(magic) +
GetFirmwareHeaderLen(image) +
FIELD_LEN(firmware_key_signature) +
- GetFirmwarePreambleLen(image) +
+ GetFirmwarePreambleLen() +
2 * firmware_signature_len +
image->firmware_len);
firmware_blob = (uint8_t*) Malloc(*blob_len);
@@ -243,7 +243,7 @@
StatefulMemcpy_r(&st, header_blob, GetFirmwareHeaderLen(image));
StatefulMemcpy_r(&st, image->firmware_key_signature,
FIELD_LEN(firmware_key_signature));
- StatefulMemcpy_r(&st, preamble_blob, GetFirmwarePreambleLen(image));
+ StatefulMemcpy_r(&st, preamble_blob, GetFirmwarePreambleLen());
StatefulMemcpy_r(&st, image->preamble_signature, firmware_signature_len);
StatefulMemcpy_r(&st, image->firmware_signature, firmware_signature_len);
StatefulMemcpy_r(&st, image->firmware_data, image->firmware_len);
@@ -405,23 +405,36 @@
}
int VerifyFirmwareData(RSAPublicKey* firmware_sign_key,
+ const uint8_t* preamble_start,
const uint8_t* firmware_data_start,
int firmware_len,
int algorithm) {
int signature_len = siglen_map[algorithm];
- if (!RSAVerifyBinary_f(NULL, firmware_sign_key, /* Key to use. */
- firmware_data_start + signature_len, /* Data to
- * verify */
- firmware_len, /* Length of data. */
- firmware_data_start, /* Expected Signature */
- algorithm))
+ uint8_t* digest;
+ DigestContext ctx;
+
+ /* Since the firmware signature is over the preamble and the firmware data,
+ * which does not form a contiguous region of memory, we calculate the
+ * message digest ourselves. */
+ DigestInit(&ctx, algorithm);
+ DigestUpdate(&ctx, preamble_start, GetFirmwarePreambleLen());
+ DigestUpdate(&ctx, firmware_data_start + signature_len, firmware_len);
+ digest = DigestFinal(&ctx);
+ if (!RSAVerifyBinaryWithDigest_f(
+ NULL, firmware_sign_key, /* Key to use. */
+ digest, /* Digest of the data to verify. */
+ firmware_data_start, /* Expected Signature */
+ algorithm)) {
+ Free(digest);
return VERIFY_FIRMWARE_SIGNATURE_FAILED;
+ }
+ Free(digest);
return 0;
}
int VerifyFirmware(const uint8_t* root_key_blob,
const uint8_t* firmware_blob) {
- int error_code;
+ int error_code = 0;
int algorithm; /* Signing key algorithm. */
RSAPublicKey* firmware_sign_key = NULL;
int firmware_sign_key_len, signature_len, header_len, firmware_len;
@@ -464,10 +477,11 @@
}
/* Only continue if firmware data verification succeeds. */
firmware_ptr = (preamble_ptr +
- GetFirmwarePreambleLen(NULL) +
+ GetFirmwarePreambleLen() +
signature_len);
- if ((error_code = VerifyFirmwareData(firmware_sign_key, firmware_ptr,
+ if ((error_code = VerifyFirmwareData(firmware_sign_key, preamble_ptr,
+ firmware_ptr,
firmware_len,
algorithm))) {
RSAPublicKeyFree(firmware_sign_key);
@@ -488,6 +502,7 @@
int signature_size;
int error_code = 0;
DigestContext ctx;
+ DigestContext firmware_ctx;
if (!image)
return VERIFY_FIRMWARE_INVALID_IMAGE;
@@ -546,10 +561,17 @@
goto verify_failure;
}
- /* Verify firmware signature. */
- firmware_digest = DigestBuf(image->firmware_data,
- image->firmware_len,
- image->firmware_sign_algorithm);
+ /* Verify firmware signature - firmware signature is on the contents
+ of firmware preamble + firmware_data. */
+ DigestInit(&firmware_ctx, image->firmware_sign_algorithm);
+ DigestUpdate(&firmware_ctx, (uint8_t*) &image->firmware_version,
+ FIELD_LEN(firmware_version));
+ DigestUpdate(&firmware_ctx, (uint8_t*) &image->firmware_len,
+ FIELD_LEN(firmware_len));
+ DigestUpdate(&firmware_ctx, (uint8_t*) &image->preamble,
+ FIELD_LEN(preamble));
+ DigestUpdate(&firmware_ctx, image->firmware_data, image->firmware_len);
+ firmware_digest = DigestFinal(&firmware_ctx);
if (!RSAVerify(firmware_sign_key, image->firmware_signature,
signature_size, image->firmware_sign_algorithm,
firmware_digest)) {
@@ -591,14 +613,17 @@
}
int AddFirmwareSignature(FirmwareImage* image, const char* signing_key_file) {
- uint8_t* preamble_blob;
- uint8_t* preamble_signature;
- uint8_t* firmware_signature;
+ uint8_t* preamble_blob = NULL;
+ uint8_t* preamble_signature = NULL;
+ uint8_t* firmware_signature = NULL;
+ uint8_t* firmware_buf = NULL;
int signature_len = siglen_map[image->firmware_sign_algorithm];
preamble_blob = GetFirmwarePreambleBlob(image);
+ if (!preamble_blob)
+ return 0;
if (!(preamble_signature = SignatureBuf(preamble_blob,
- GetFirmwarePreambleLen(image),
+ GetFirmwarePreambleLen(),
signing_key_file,
image->firmware_sign_algorithm))) {
Free(preamble_blob);
@@ -607,15 +632,27 @@
image->preamble_signature = (uint8_t*) Malloc(signature_len);
Memcpy(image->preamble_signature, preamble_signature, signature_len);
Free(preamble_signature);
-
- if (!(firmware_signature = SignatureBuf(image->firmware_data,
+ /* Firmware signature must be calculated on preamble + firmware_data
+ * to avoid splicing attacks. */
+ firmware_buf = (uint8_t*) Malloc(GetFirmwarePreambleLen() +
+ image->firmware_len);
+ Memcpy(firmware_buf, preamble_blob, GetFirmwarePreambleLen());
+ Memcpy(firmware_buf + GetFirmwarePreambleLen(), image->firmware_data,
+ image->firmware_len);
+ if (!(firmware_signature = SignatureBuf(firmware_buf,
+ GetFirmwarePreambleLen() +
image->firmware_len,
signing_key_file,
- image->firmware_sign_algorithm)))
+ image->firmware_sign_algorithm))) {
+ Free(preamble_blob);
+ Free(firmware_buf);
return 0;
+ }
image->firmware_signature = (uint8_t*) Malloc(signature_len);
Memcpy(image->firmware_signature, firmware_signature, signature_len);
Free(firmware_signature);
+ Free(firmware_buf);
+ Free(preamble_blob);
return 1;
}
diff --git a/utils/kernel_image.c b/utils/kernel_image.c
index e635291..f92a9f8 100644
--- a/utils/kernel_image.c
+++ b/utils/kernel_image.c
@@ -220,7 +220,7 @@
return header_blob;
}
-int GetKernelConfigLen(const KernelImage* image) {
+int GetKernelConfigLen() {
return (FIELD_LEN(kernel_version) +
FIELD_LEN(options.version) + FIELD_LEN(options.cmd_line) +
FIELD_LEN(options.kernel_len) + FIELD_LEN(options.kernel_load_addr) +
@@ -231,8 +231,8 @@
uint8_t* config_blob = NULL;
MemcpyState st;
- config_blob = (uint8_t*) Malloc(GetKernelConfigLen(image));
- st.remaining_len = GetKernelConfigLen(image);
+ config_blob = (uint8_t*) Malloc(GetKernelConfigLen());
+ st.remaining_len = GetKernelConfigLen();
st.remaining_buf = config_blob;
StatefulMemcpy_r(&st, &image->kernel_version, FIELD_LEN(kernel_version));
@@ -266,7 +266,7 @@
*blob_len = (FIELD_LEN(magic) +
GetKernelHeaderLen(image) +
kernel_key_signature_len +
- GetKernelConfigLen(image) +
+ GetKernelConfigLen() +
2 * kernel_signature_len +
image->options.kernel_len);
kernel_blob = (uint8_t*) Malloc(*blob_len);
@@ -279,7 +279,7 @@
StatefulMemcpy_r(&st, image->magic, FIELD_LEN(magic));
StatefulMemcpy_r(&st, header_blob, GetKernelHeaderLen(image));
StatefulMemcpy_r(&st, image->kernel_key_signature, kernel_key_signature_len);
- StatefulMemcpy_r(&st, config_blob, GetKernelConfigLen(image));
+ StatefulMemcpy_r(&st, config_blob, GetKernelConfigLen());
StatefulMemcpy_r(&st, image->config_signature, kernel_signature_len);
StatefulMemcpy_r(&st, image->kernel_signature, kernel_signature_len);
StatefulMemcpy_r(&st, image->kernel_data, image->options.kernel_len);
@@ -453,7 +453,7 @@
int algorithm,
int* kernel_len) {
uint32_t len, config_len;
- config_len = GetKernelConfigLen(NULL);
+ config_len = GetKernelConfigLen();
if (!RSAVerifyBinary_f(NULL, kernel_sign_key, /* Key to use */
config_blob, /* Data to verify */
config_len, /* Length of data */
@@ -470,17 +470,30 @@
}
int VerifyKernelData(RSAPublicKey* kernel_sign_key,
+ const uint8_t* kernel_config_start,
const uint8_t* kernel_data_start,
int kernel_len,
int algorithm) {
int signature_len = siglen_map[algorithm];
- if (!RSAVerifyBinary_f(NULL, kernel_sign_key, /* Key to use. */
- kernel_data_start + signature_len, /* Data to
- * verify */
- kernel_len, /* Length of data. */
- kernel_data_start, /* Expected Signature */
- algorithm))
+ uint8_t* digest;
+ DigestContext ctx;
+
+ /* Since the kernel signature is computed over the kernel version, options
+ * and data, which does not form a contiguous region of memory, we calculate
+ * the message digest ourselves. */
+ DigestInit(&ctx, algorithm);
+ DigestUpdate(&ctx, kernel_config_start, GetKernelConfigLen());
+ DigestUpdate(&ctx, kernel_data_start + signature_len, kernel_len);
+ digest = DigestFinal(&ctx);
+ if (!RSAVerifyBinaryWithDigest_f(
+ NULL, kernel_sign_key, /* Key to use. */
+ digest, /* Digest of the data to verify. */
+ kernel_data_start, /* Expected Signature */
+ algorithm)) {
+ Free(digest);
return VERIFY_KERNEL_SIGNATURE_FAILED;
+ }
+ Free(digest);
return 0;
}
@@ -536,10 +549,11 @@
}
/* Only continue if kernel data verification succeeds. */
kernel_ptr = (config_ptr +
- GetKernelConfigLen(NULL) + /* Skip config block/signature. */
+ GetKernelConfigLen() + /* Skip config block/signature. */
kernel_signature_len);
- if ((error_code = VerifyKernelData(kernel_sign_key, kernel_ptr, kernel_len,
+ if ((error_code = VerifyKernelData(kernel_sign_key, config_ptr, kernel_ptr,
+ kernel_len,
kernel_sign_algorithm))) {
RSAPublicKeyFree(kernel_sign_key);
return error_code; /* AKA jump to recovery. */
@@ -559,7 +573,7 @@
int kernel_signature_size;
int error_code = 0;
DigestContext ctx;
-
+ DigestContext kernel_ctx;
if (!image)
return VERIFY_KERNEL_INVALID_IMAGE;
@@ -631,10 +645,23 @@
goto verify_failure;
}
- /* Verify firmware signature. */
- kernel_digest = DigestBuf(image->kernel_data,
- image->options.kernel_len,
- image->kernel_sign_algorithm);
+ /* Verify kernel signature - kernel signature is computed on the contents
+ of kernel version + kernel options + kernel_data. */
+ DigestInit(&kernel_ctx, image->kernel_sign_algorithm);
+ DigestUpdate(&kernel_ctx, (uint8_t*) &image->kernel_version,
+ FIELD_LEN(kernel_version));
+ DigestUpdate(&kernel_ctx, (uint8_t*) image->options.version,
+ FIELD_LEN(options.version));
+ DigestUpdate(&kernel_ctx, (uint8_t*) image->options.cmd_line,
+ FIELD_LEN(options.cmd_line));
+ DigestUpdate(&kernel_ctx, (uint8_t*) &image->options.kernel_len,
+ FIELD_LEN(options.kernel_len));
+ DigestUpdate(&kernel_ctx, (uint8_t*) &image->options.kernel_load_addr,
+ FIELD_LEN(options.kernel_load_addr));
+ DigestUpdate(&kernel_ctx, (uint8_t*) &image->options.kernel_entry_addr,
+ FIELD_LEN(options.kernel_entry_addr));
+ DigestUpdate(&kernel_ctx, image->kernel_data, image->options.kernel_len);
+ kernel_digest = DigestFinal(&kernel_ctx);
if (!RSAVerify(kernel_sign_key, image->kernel_signature,
kernel_signature_size, image->kernel_sign_algorithm,
kernel_digest)) {
@@ -682,33 +709,44 @@
uint8_t* config_blob = NULL;
uint8_t* config_signature = NULL;
uint8_t* kernel_signature = NULL;
+ uint8_t* kernel_buf;
int signature_len = siglen_map[image->kernel_sign_algorithm];
config_blob = GetKernelConfigBlob(image);
if (!(config_signature = SignatureBuf(config_blob,
- GetKernelConfigLen(image),
+ GetKernelConfigLen(),
kernel_signing_key_file,
image->kernel_sign_algorithm))) {
fprintf(stderr, "Could not compute signature on the kernel config.\n");
Free(config_blob);
return 0;
}
- Free(config_blob);
image->config_signature = (uint8_t*) Malloc(signature_len);
Memcpy(image->config_signature, config_signature, signature_len);
Free(config_signature);
-
- if (!(kernel_signature = SignatureBuf(image->kernel_data,
+ /* Kernel signature muse be calculated on the kernel version, options and
+ * kernel data to avoid splicing attacks. */
+ kernel_buf = (uint8_t*) Malloc(GetKernelConfigLen() +
+ image->options.kernel_len);
+ Memcpy(kernel_buf, config_blob, GetKernelConfigLen());
+ Memcpy(kernel_buf + GetKernelConfigLen(), image->kernel_data,
+ image->options.kernel_len);
+ if (!(kernel_signature = SignatureBuf(kernel_buf,
+ GetKernelConfigLen() +
image->options.kernel_len,
kernel_signing_key_file,
image->kernel_sign_algorithm))) {
+ Free(config_blob);
+ Free(kernel_buf);
fprintf(stderr, "Could not compute signature on the kernel.\n");
return 0;
}
image->kernel_signature = (uint8_t*) Malloc(signature_len);
Memcpy(image->kernel_signature, kernel_signature, signature_len);
Free(kernel_signature);
+ Free(kernel_buf);
+ Free(config_blob);
return 1;
}