On 10/30/26 07:13, Ana Bobillo wrote:
From: Ana Bobillo <[email protected]>
efi_image_parse() collects the regions of an image to be hashed for
Secure Boot and for TCG2 measurements. It takes the header size, the
section table, each section's raw data and the certificate table from
the image without checking them against the image size:
- SizeOfHeaders and each section's PointerToRawData and SizeOfRawData
are added as regions even when they run past the end of the image,
which is then read out of bounds when the regions are hashed;
- the section table is read wherever SizeOfOptionalHeader and
NumberOfSections put it;
- the end of the certificate table is computed in 32 bits, so a large
offset wraps around and passes the check;
- errors of efi_image_region_add() are ignored.
All of this happens before the signature is verified.
Call efi_check_pe() first, check each offset and size against the image
size without wrapping, add regions only through a checked helper and fail
on its errors, and require the file alignment to be a power of two. The
padding of a section to the file alignment is hashed only as far as the
image goes, so an image whose last section is not padded is still
accepted. Check the certificate table against the size of
WIN_CERTIFICATE, not of a pointer to it.
barebox, whose EFI loader is based on this file, added the same checks in
4ec1f7166778 ("efi: validate PE parser input ranges"); this is a port to
U-Boot. Unlike there, an optional header with fewer than 16 data
directories is still accepted if it holds the certificate table entry it
declares: U-Boot's own EFI applications have six.
The PECOFF specification does not require any table. Six are needed if
you have a .reloc section which some non-compliant software assumes.
Aren't you describing a bug in barebox?
read_header() in
https://github.com/rhboot/shim/blob/e794a714339b1277abe9f890fc98de6e415681b0/pe-relocate.c#L376
has rather strict tests for PE files. It might be worthwhile to compare
their checks with ours.
Fixes: 4540dabdcaca ("efi_loader: image_loader: support image authentication")
Signed-off-by: Ana Bobillo <[email protected]>
---
lib/efi_loader/efi_image_loader.c | 268 +++++++++++++++++++++---------
1 file changed, 190 insertions(+), 78 deletions(-)
diff --git a/lib/efi_loader/efi_image_loader.c
b/lib/efi_loader/efi_image_loader.c
index e31493d5135..35cf3cd268f 100644
--- a/lib/efi_loader/efi_image_loader.c
+++ b/lib/efi_loader/efi_image_loader.c
@@ -393,6 +393,69 @@ void *efi_prepare_aligned_image(void *efi, u64 *efi_size)
}
}
+/**
+ * pe_range_ok() - check that a range lies within the image
+ * @len: size of the image
+ * @off: offset of the range from the start of the image
+ * @size: size of the range
+ *
+ * Return: true if [@off, @off + @size) lies within [0, @len)
+ */
+static bool pe_range_ok(size_t len, size_t off, size_t size)
+{
+ return off <= len && size <= len - off;
+}
+
+/**
+ * pe_align_size() - round up a section size to the file alignment
+ * @size: size of the raw data of a section
+ * @align: file alignment, a power of two
+ * @aligned: on return @size rounded up to @align
+ *
+ * Return: true on success, false if @align is not a power of two or
+ * the rounded size overflows
+ */
+static bool pe_align_size(u32 size, u32 align, size_t *aligned)
+{
+ size_t val = size;
+
+ if (!align || (align & (align - 1)))
+ return false;
FileAlignment and SectionAlignment should be checked once before the
loop calling this function instead of checking the same value multiple
times.
+
+ if (__builtin_add_overflow(val, (size_t)align - 1, &val))
+ return false;
We are already checking for overflows in pe_range_ok(). Why use
__builtin_add_overflow()?
+
+ *aligned = val & ~((size_t)align - 1);
This whole function can be replaced by using the ALIGN() macro.
+
+ return true;
+}
+
+/**
+ * efi_image_region_add_checked() - add a region of an image
+ * @regs: pointer to array of regions
+ * @efi: pointer to the image
+ * @len: size of the image
+ * @start: offset of the region in the image
+ * @size: size of the region
+ * @nocheck: flag against overlapped regions
+ *
+ * Add the region [@start, @start + @size[ of the image to @regs if it lies
+ * within the image.
+ *
+ * Return: status code
+ */
+static efi_status_t efi_image_region_add_checked(struct efi_image_regions
*regs,
+ const void *efi, size_t len,
+ size_t start, size_t size,
+ int nocheck)
+{
+ if (!pe_range_ok(len, start, size))
+ return EFI_INVALID_PARAMETER;
+
+ return efi_image_region_add(regs, efi + start, efi + start + size,
+ nocheck);
+}
+
/**
* efi_image_parse() - parse a PE image
* @efi: Pointer to image
@@ -401,8 +464,8 @@ void *efi_prepare_aligned_image(void *efi, u64 *efi_size)
* @auth: Pointer to a pointer to authentication data in PE
* @auth_len: Size of @auth
*
- * Parse image binary in PE32(+) format, assuming that sanity of PE image
- * has been checked by a caller.
+ * Parse image binary in PE32(+) format. Each offset and size taken from the
+ * image is checked against @len before it is used.
* On success, an address of authentication data in @efi and its size will
* be returned in @auth and @auth_len, respectively.
*
@@ -411,97 +474,135 @@ void *efi_prepare_aligned_image(void *efi, u64 *efi_size)
bool efi_image_parse(void *efi, size_t len, struct efi_image_regions **regp,
WIN_CERTIFICATE **auth, size_t *auth_len)
{
- struct efi_image_regions *regs;
- IMAGE_DOS_HEADER *dos;
+ struct efi_image_regions *regs = NULL;
IMAGE_NT_HEADERS32 *nt;
- IMAGE_SECTION_HEADER *sections, **sorted;
+ IMAGE_SECTION_HEADER *sections, **sorted = NULL;
int num_regions, num_sections, i;
int ctidx = IMAGE_DIRECTORY_ENTRY_SECURITY;
- u32 align, size, authsz, authoff;
- size_t bytes_hashed;
+ u32 align, authsz, authoff, header_size, num_dirs;
+ size_t bytes_hashed, opt_off, opt_size, min_opt_size;
+ size_t csum_off, subsys_off, dir_off, dir_end;
+ size_t sections_off, sections_size;
- dos = (void *)efi;
- nt = (void *)(efi + dos->e_lfanew);
authoff = 0;
authsz = 0;
/*
- * Count maximum number of regions to be digested.
- * We don't have to have an exact number here.
- * See efi_image_region_add()'s in parsing below.
+ * efi_check_pe() checks the DOS and PE signatures and that the image
+ * is large enough to hold a 64bit NT header, which is longer than a
+ * 32bit one. Every optional header field and data directory entry
+ * read below therefore lies within the image.
*/
- num_regions = 3; /* for header */
- num_regions += nt->FileHeader.NumberOfSections;
- num_regions++; /* for extra */
-
- regs = calloc(sizeof(*regs) + sizeof(struct image_region) * num_regions,
- 1);
- if (!regs)
- goto err;
- regs->max = num_regions;
+ if (efi_check_pe(efi, len, (void **)&nt) != EFI_SUCCESS)
+ return false;
+ opt_off = (u8 *)&nt->OptionalHeader - (u8 *)efi;
+ opt_size = nt->FileHeader.SizeOfOptionalHeader;
- /*
- * Collect data regions for hash calculation
- * 1. File headers
- */
if (nt->OptionalHeader.Magic == IMAGE_NT_OPTIONAL_HDR64_MAGIC) {
IMAGE_NT_HEADERS64 *nt64 = (void *)nt;
IMAGE_OPTIONAL_HEADER64 *opt = &nt64->OptionalHeader;
- /* Skip CheckSum */
- efi_image_region_add(regs, efi, &opt->CheckSum, 0);
- if (nt64->OptionalHeader.NumberOfRvaAndSizes <= ctidx) {
- efi_image_region_add(regs,
- &opt->Subsystem,
- efi + opt->SizeOfHeaders, 0);
- } else {
- /* Skip Certificates Table */
- efi_image_region_add(regs,
- &opt->Subsystem,
- &opt->DataDirectory[ctidx], 0);
- efi_image_region_add(regs,
- &opt->DataDirectory[ctidx] + 1,
- efi + opt->SizeOfHeaders, 0);
-
+ min_opt_size = offsetof(IMAGE_OPTIONAL_HEADER64, DataDirectory);
+ csum_off = opt_off + offsetof(IMAGE_OPTIONAL_HEADER64,
+ CheckSum);
+ subsys_off = opt_off + offsetof(IMAGE_OPTIONAL_HEADER64,
+ Subsystem);
+ dir_off = opt_off + offsetof(IMAGE_OPTIONAL_HEADER64,
+ DataDirectory[ctidx]);
+ header_size = opt->SizeOfHeaders;
+ align = opt->FileAlignment;
+ num_dirs = opt->NumberOfRvaAndSizes;
+ if (num_dirs > ctidx) {
authoff = opt->DataDirectory[ctidx].VirtualAddress;
authsz = opt->DataDirectory[ctidx].Size;
}
-
- bytes_hashed = opt->SizeOfHeaders;
- align = opt->FileAlignment;
} else if (nt->OptionalHeader.Magic == IMAGE_NT_OPTIONAL_HDR32_MAGIC) {
IMAGE_OPTIONAL_HEADER32 *opt = &nt->OptionalHeader;
- /* Skip CheckSum */
- efi_image_region_add(regs, efi, &opt->CheckSum, 0);
- if (nt->OptionalHeader.NumberOfRvaAndSizes <= ctidx) {
- efi_image_region_add(regs,
- &opt->Subsystem,
- efi + opt->SizeOfHeaders, 0);
- } else {
- /* Skip Certificates Table */
- efi_image_region_add(regs, &opt->Subsystem,
- &opt->DataDirectory[ctidx], 0);
- efi_image_region_add(regs,
- &opt->DataDirectory[ctidx] + 1,
- efi + opt->SizeOfHeaders, 0);
-
+ min_opt_size = offsetof(IMAGE_OPTIONAL_HEADER32, DataDirectory);
+ csum_off = opt_off + offsetof(IMAGE_OPTIONAL_HEADER32,
+ CheckSum);
+ subsys_off = opt_off + offsetof(IMAGE_OPTIONAL_HEADER32,
+ Subsystem);
+ dir_off = opt_off + offsetof(IMAGE_OPTIONAL_HEADER32,
+ DataDirectory[ctidx]);
+ header_size = opt->SizeOfHeaders;
+ align = opt->FileAlignment;
+ num_dirs = opt->NumberOfRvaAndSizes;
+ if (num_dirs > ctidx) {
authoff = opt->DataDirectory[ctidx].VirtualAddress;
authsz = opt->DataDirectory[ctidx].Size;
}
-
- bytes_hashed = opt->SizeOfHeaders;
- align = opt->FileAlignment;
} else {
log_err("%s: Invalid optional header magic %x\n", __func__,
nt->OptionalHeader.Magic);
+ return false;
+ }
+ dir_end = dir_off + sizeof(IMAGE_DATA_DIRECTORY);
+
+ /*
+ * The optional header must hold the fields used here, and the
+ * certificate table entry when there is one. The section table
+ * follows the optional header; it and the whole header area
+ * (SizeOfHeaders) must lie within the image, as must the certificate
+ * table.
+ */
+ num_sections = nt->FileHeader.NumberOfSections;
+ sections_off = opt_off + opt_size;
+ sections_size = (size_t)num_sections * sizeof(IMAGE_SECTION_HEADER);
+ if (opt_size < min_opt_size ||
+ (num_dirs > ctidx && sections_off < dir_end) ||
+ !pe_range_ok(len, sections_off, sections_size) ||
+ !pe_range_ok(len, 0, header_size) ||
+ header_size < sections_off + sections_size) {
+ log_err("%s: Invalid PE headers\n", __func__);
+ return false;
+ }
+ if (authsz && !pe_range_ok(len, authoff, authsz)) {
+ log_err("%s: Size for auth too large: %u\n", __func__, authsz);
+ return false;
+ }
+
+ /*
+ * Count maximum number of regions to be digested.
+ * We don't have to have an exact number here.
+ * See efi_image_region_add()'s in parsing below.
+ */
+ num_regions = 3; /* for header */
+ num_regions += num_sections;
+ num_regions++; /* for extra */
Please, avoid this increment by starting at 4.
+
+ regs = calloc(sizeof(*regs) + sizeof(struct image_region) * num_regions,
+ 1);
+ if (!regs)
goto err;
+ regs->max = num_regions;
+
+ /*
+ * Collect data regions for hash calculation
+ * 1. File headers
+ */
+ /* Skip CheckSum */
+ if (efi_image_region_add_checked(regs, efi, len, 0, csum_off, 0))
+ goto err;
+ if (num_dirs <= ctidx) {
+ if (efi_image_region_add_checked(regs, efi, len, subsys_off,
+ header_size - subsys_off, 0))
+ goto err;
+ } else {
+ /* Skip Certificates Table */
+ if (efi_image_region_add_checked(regs, efi, len, subsys_off,
+ dir_off - subsys_off, 0))
+ goto err;
+ if (efi_image_region_add_checked(regs, efi, len, dir_end,
+ header_size - dir_end, 0))
+ goto err;
}
+ bytes_hashed = header_size;
+
/* 2. Sections */
- num_sections = nt->FileHeader.NumberOfSections;
- sections = (void *)((uint8_t *)&nt->OptionalHeader +
- nt->FileHeader.SizeOfOptionalHeader);
+ sections = (void *)((uint8_t *)efi + sections_off);
sorted = calloc(sizeof(IMAGE_SECTION_HEADER *), num_sections);
if (!sorted) {
log_err("%s: Out of memory\n", __func__);
@@ -516,14 +617,26 @@ bool efi_image_parse(void *efi, size_t len, struct
efi_image_regions **regp,
qsort(sorted, num_sections, sizeof(sorted[0]), cmp_pe_section);
for (i = 0; i < num_sections; i++) {
+ size_t size;
+
if (!sorted[i]->SizeOfRawData)
continue;
- size = (sorted[i]->SizeOfRawData + align - 1) & ~(align - 1);
- efi_image_region_add(regs, efi + sorted[i]->PointerToRawData,
- efi + sorted[i]->PointerToRawData + size,
- 0);
- log_debug("section[%d](%s): raw: 0x%x-0x%x, virt: %x-%x\n",
+ if (!pe_range_ok(len, sorted[i]->PointerToRawData,
+ sorted[i]->SizeOfRawData) ||
+ !pe_align_size(sorted[i]->SizeOfRawData, align, &size)) {
The PECOFF specification defines a default of 512 for the FileAlignment.
That is the value you should use if FileAlignment == 0.
+ log_err("%s: Invalid section %d\n", __func__, i);
+ goto err;
+ }
+ /* The padding to the file alignment ends with the image */
+ size = min(size, len - sorted[i]->PointerToRawData);
+ if (efi_image_region_add_checked(regs, efi, len,
+ sorted[i]->PointerToRawData,
+ size, 0)) {
+ log_err("%s: Invalid section %d\n", __func__, i);
+ goto err;
+ }
+ log_debug("section[%d](%s): raw: 0x%x-0x%zx, virt: %x-%x\n",
i, sorted[i]->Name,
sorted[i]->PointerToRawData,
sorted[i]->PointerToRawData + size,
@@ -534,25 +647,23 @@ bool efi_image_parse(void *efi, size_t len, struct
efi_image_regions **regp,
bytes_hashed += size;
}
free(sorted);
+ sorted = NULL;
/* 3. Extra data excluding Certificates Table */
- if (bytes_hashed + authsz < len) {
+ if (bytes_hashed < len && authsz < len - bytes_hashed) {
log_debug("extra data for hash: %zu\n",
len - (bytes_hashed + authsz));
- efi_image_region_add(regs, efi + bytes_hashed,
- efi + len - authsz, 0);
+ if (efi_image_region_add_checked(regs, efi, len, bytes_hashed,
+ len - bytes_hashed - authsz,
+ 0))
+ goto err;
}
/* Return Certificates Table */
if (authsz) {
- if (len < authoff + authsz) {
- log_err("%s: Size for auth too large: %u >= %zu\n",
- __func__, authsz, len - authoff);
- goto err;
- }
- if (authsz < sizeof(*auth)) {
+ if (authsz < sizeof(**auth)) {
log_err("%s: Size for auth too small: %u < %zu\n",
- __func__, authsz, sizeof(*auth));
+ __func__, authsz, sizeof(**auth));
goto err;
}
*auth = efi + authoff;
@@ -569,6 +680,7 @@ bool efi_image_parse(void *efi, size_t len, struct
efi_image_regions **regp,
return true;
err:
+ free(sorted);
Here you are closing a memory leak. But this issue is not described in
the commit message.
Ideally for each issue there would be a separate patch. But at least
mention it in the commit message.
Best regards
Heinrich
free(regs);
return false;
