Skip to content

Commit

Permalink
Load concatenated EFI_SIGNATURE_LISTs from shim_certificate.efi
Browse files Browse the repository at this point in the history
For multiple reasons, it may be useful for different keys to be used to
sign different parts of the boot chain (e.g. a different key for GRUB
and the Linux kernel). Allow this by loading concatenated
EFI_SIGNATURE_LISTs from shim_certificate.efi rather than only the
first.

At the same time, be a bit more robust by checking for allocation
failures and overflows due to invalid data in the binary.
Use the smaller of VirtualSize and SizeOfRawData since the latter is
rounded up to the section alignment and therefore may contain
non-certificate data.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
  • Loading branch information
rosslagerwall authored and vathpela committed Feb 4, 2025
1 parent 1125212 commit 2daf1db
Showing 1 changed file with 33 additions and 14 deletions.
47 changes: 33 additions & 14 deletions shim.c
Original file line number Diff line number Diff line change
Expand Up @@ -1488,6 +1488,7 @@ load_cert_file(EFI_HANDLE image_handle, CHAR16 *filename, CHAR16 *PathName)
EFI_SIGNATURE_LIST *certlist;
void *pointer;
UINT32 original;
UINT32 offset;
int datasize = 0;
void *data = NULL;
int i;
Expand All @@ -1505,22 +1506,40 @@ load_cert_file(EFI_HANDLE image_handle, CHAR16 *filename, CHAR16 *PathName)

Section = context.FirstSection;
for (i = 0; i < context.NumberOfSections; i++, Section++) {
UINT32 sec_size = MIN(Section->Misc.VirtualSize, Section->SizeOfRawData);

if (CompareMem(Section->Name, ".db\0\0\0\0\0", 8) == 0) {
original = user_cert_size;
if (Section->SizeOfRawData < sizeof(EFI_SIGNATURE_LIST)) {
continue;
}
pointer = ImageAddress(data, datasize,
Section->PointerToRawData);
if (!pointer) {
continue;
offset = 0;
while ((sec_size - offset) >= sizeof(EFI_SIGNATURE_LIST)) {
UINT8 *tmp;

original = user_cert_size;
pointer = ImageAddress(data, datasize,
Section->PointerToRawData + offset);
if (!pointer) {
break;
}
certlist = pointer;

if (certlist->SignatureListSize < sizeof(EFI_SIGNATURE_LIST) ||
checked_add(offset, certlist->SignatureListSize, &offset) ||
offset > sec_size ||
checked_add(user_cert_size, certlist->SignatureListSize,
&user_cert_size)) {
break;
}

tmp = ReallocatePool(user_cert, original,
user_cert_size);
if (!tmp) {
FreePool(data);
return EFI_OUT_OF_RESOURCES;
}
user_cert = tmp;

CopyMem(user_cert + original, pointer,
certlist->SignatureListSize);
}
certlist = pointer;
user_cert_size += certlist->SignatureListSize;;
user_cert = ReallocatePool(user_cert, original,
user_cert_size);
CopyMem(user_cert + original, pointer,
certlist->SignatureListSize);
}
}
FreePool(data);
Expand Down

1 comment on commit 2daf1db

@jsetje
Copy link
Collaborator

@jsetje jsetje commented on 2daf1db Feb 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for fixing this. FWIW I debugged this before I saw your fix and was able to use Section->Misc.VirtualSize and just copy the whole thing.

Since I still had test binaries around I ran a quick test and confirmed that your math ends up with the same user_cert_size and the same data in user_cert.

I'm mostly leaving this note here in case something else changes and the simplified copy becomes useful.

-                      user_cert_size += certlist->SignatureListSize;;
+                      user_cert_size += Section->Misc.VirtualSize;
                       user_cert = ReallocatePool(user_cert, original,
                                                  user_cert_size);
                       CopyMem(user_cert + original, pointer,
-                               certlist->SignatureListSize);
+                               Section->Misc.VirtualSize);

Please sign in to comment.