summaryrefslogtreecommitdiff
path: root/shim.c
AgeCommit message (Collapse)Author
2020-09-09Fix buffer overrun due DEFAULT_LOADER length miscalculationJavier Martinez Canillas
The DEFAULT_LOADER is a UCS-2 string and the StrLen() function returns the number of UCS-2 encoded characters in the string. But the allocated memory is in bytes, so only half of the needed memory to store it is allocated. This leads to a buffer overrun when the StrCpy() function attempts to copy the DEFAULT_LOADER to the allocated buffer. Fixes: 354bd9b1931 ("Actually check for errors from set_second_stage()") Reported-by: Stuart Hayes <stuart_hayes@dell.com> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
2020-07-25Implement lennysz's suggestions for MokListRTPeter Jones
Signed-off-by: Peter Jones <pjones@redhat.com>
2020-07-25Improve debug output somePeter Jones
Signed-off-by: Peter Jones <pjones@redhat.com> Upstream: pr#213
2020-07-23Handle binaries with multiple signatures.Peter Jones
This adds support for multiple signatures. It first tries validating the binary by hash, first against our dbx lists, then against our db lists. If it isn't allowed or rejected at that step, it continues to the normal routine of checking all the signatures. At this point it does *not* reject a binary just because a signature is by a cert on a dbx list, though that will override any db list that certificate is listed on. If at any point any assertion about the binary or signature list being well-formed fails, the binary is immediately rejected, though we do allow skipping over signatures which have an unsupported sig->Hdr.wCertificateType. Signed-off-by: Peter Jones <pjones@redhat.com> Upstream: pr#210
2020-07-23Add support for vendor_db built-in shim authorized list.Peter Jones
Potential new signing strategies ( for example signing grub, fwupdate and vmlinuz with separate certificates ) require shim to support a vendor provided bundle of trusted certificates and hashes, which allows shim to trust EFI binaries matching either certificate by signature or hash in the vendor_db. Functionality is similar to vendor_dbx. This also improves the mirroring quite a bit. Upstream: pr#206
2020-07-23Make cert.S not impossible to read.Peter Jones
Signed-off-by: Peter Jones <pjones@redhat.com> Upstream: pr#206
2020-07-23Fix some volatile usage gcc whines about.Peter Jones
Signed-off-by: Peter Jones <pjones@redhat.com> Upstream: pr#212
2020-07-23tpm: Include information about PE/COFF images in the TPM Event LogJavier Martinez Canillas
The "TCG PC Client Specific Platform Firmware Profile Specification" says that when measuring a PE/COFF image, the TCG_PCR_EVENT2 structure Event field MUST contain a UEFI_IMAGE_LOAD_EVENT structure. Currently an empty UEFI_IMAGE_LOAD_EVENT structure is passed so users only have the hash of the PE/COFF image, but not information such the file path of the binary. Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> Upstream-commit-id: c252b9ee94c
2020-07-23shim: Update EFI_LOADED_IMAGE with the second stage loader file pathJavier Martinez Canillas
When shim loads the second stage loader (e.g: GRUB) the FilePath field of the EFI_LOADED_IMAGE structure isn't updated with the path of the loaded binary. So it still contains the file path of the shim binary. This isn't a problem since the file path is currently not used. But should be used to set the DevicePath field of the EFI_IMAGE_LOAD_EVENT structure that is logged when measuring the PE/COFF binaries. In that case the TPM Event Log will have an incorrect file path for the measured binary, i.e: $ hexdump -Cv /sys/kernel/security/tpm0/binary_bios_measurements ... 00000a50 00 00 00 00 00 00 04 04 34 00 5c 00 45 00 46 00 |........4.\.E.F.| 00000a60 49 00 5c 00 72 00 65 00 64 00 68 00 61 00 74 00 |I.\.r.e.d.h.a.t.| 00000a70 5c 00 73 00 68 00 69 00 6d 00 78 00 36 00 34 00 |\.s.h.i.m.x.6.4.| 00000a80 2e 00 65 00 66 00 69 00 00 00 7f ff 04 00 00 00 |..e.f.i.........| 00000a90 00 00 00 00 00 00 af 08 00 00 00 0d 00 00 00 b5 |................| 00000aa0 cd d0 8f bb 16 31 e2 80 8b e8 58 75 c9 89 18 95 |.....1....Xu....| 00000ab0 d2 de 15 15 00 00 00 67 72 75 62 5f 63 6d 64 20 |.......grub_cmd | 00000ac0 73 65 74 20 70 61 67 65 72 3d 31 00 08 00 00 00 |set pager=1.....| ... So update the EFI_LOADED_IMAGE structure with the second stage loader file path to have the correct value in the log, i.e: $ hexdump -Cv /sys/kernel/security/tpm0/binary_bios_measurements ... 00000a50 00 00 00 00 00 00 04 04 34 00 5c 00 45 00 46 00 |........4.\.E.F.| 00000a60 49 00 5c 00 72 00 65 00 64 00 68 00 61 00 74 00 |I.\.r.e.d.h.a.t.| 00000a70 5c 00 67 00 72 00 75 00 62 00 78 00 36 00 34 00 |\.g.r.u.b.x.6.4.| 00000a80 2e 00 65 00 66 00 69 00 00 00 7f ff 04 00 00 00 |..e.f.i.........| 00000a90 00 00 00 00 00 00 af 08 00 00 00 0d 00 00 00 b5 |................| 00000aa0 cd d0 8f bb 16 31 e2 80 8b e8 58 75 c9 89 18 95 |.....1....Xu....| 00000ab0 d2 de 15 15 00 00 00 67 72 75 62 5f 63 6d 64 20 |.......grub_cmd | 00000ac0 73 65 74 20 70 61 67 65 72 3d 31 00 08 00 00 00 |set pager=1.....| ... Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> Upstream-commit-id: cd7d42d493d
2020-07-23Actually check for errors from set_second_stage()Peter Jones
This changes shim_init() to check for errors from set_second_stage(). In order to make that work, it also does the following: - correctly /always/ allocate second_stage, not sometimes allocate and sometimes point at .data - test for LoadOptionSize == 0 and return success - print an error message for the failure so we can see it. Signed-off-by: Peter Jones <pjones@redhat.com> Upstream-commit-id: 354bd9b1931
2020-07-23Slightly better debugging messagesPeter Jones
Signed-off-by: Peter Jones <pjones@redhat.com> Upstream-commit-id: 173d35fe8f5
2020-07-23OpenSSL: always provide OBJ_create() with name strings.Peter Jones
Some versions of OpenSSL seem to go back and forth as to whether NULL for these names are okay. Don't risk it. Signed-off-by: Peter Jones <pjones@redhat.com> Upstream-commit-id: 46b76a01717
2020-07-23Fix a use of strlen() instead of Strlen()Peter Jones
Signed-off-by: Peter Jones <pjones@redhat.com> Upstream-commit-id: 1870bae7960
2020-07-23Hook exit when shim_lock protocol installedStuart Hayes
A recent commit moved where the shim_lock protocol is loaded and unloaded, but did not move where exit was hooked and unhooked. Exit needs to be hooked when the protocol is installed, so that the protocol will be uninstalled on exit. Otherwise, the system can crash if, for example, shim loads grub, the user exits grub, shim is run again, which installs a second instance of the protocol, and then grub tries to use the shim_lock protocol that was installed by the first instance of shim. Signed-off-by: Stuart Hayes <stuart.w.hayes@gmail.com> Upstream-commit-id: 06c92591e94
2020-07-23shim: Rework pause functions and add read_counter()Peter Jones
Signed-off-by: Peter Jones <pjones@redhat.com> Upstream-commit-id: fc6b0bca84e
2020-07-23Make EFI variable copying fatal only on secureboot enabled systemsPatrick Uiterwijk
I have come across systems that are unwilling to reserve enough memory for a MokListRT big enough for big certificates. This seems to be the case with firmware implementations that do not support secureboot, which is probably the reason they went with much lower variable storage. This patch set makes sure we can still boot on those systems, by only making the copy action fatal if the system has secure boot enabled, or if the error was anything other than EFI_INVALID_PARAMETER. Signed-off-by: Patrick Uiterwijk <patrick@puiterwijk.org> Upstream-commit-id: 741c61abba7
2020-07-23shim: only include shim_cert.h in shim.cGary Lin
The shim_cert array was declared as a static array, and every user of shim_cert.h would create a shim_cert array for its own and grow the file size. To remove the unnecessary duplicate shim_cert arrays, this commit declares shim_cert in shim.c while other users still can access the array through the external variables: build_cert and build_cert_size. Signed-off-by: Gary Lin <glin@suse.com> Upstream-commit-id: 4e2d62f0f4e
2020-07-23Fix apparent typo in ARM 32-on-64 codedann frazier
The architecture is aarch64, not arch64. Fixes: 750584c20775 ("Make 64-on-32 maybe work on x86_64.") Signed-off-by: dann frazier <dann.frazier@canonical.com> Upstream-commit-id: e9f67aaa75a
2020-07-23Fix for "Section 0 has negative size" error when loading fbaa64.efiMaran Wilson
The current code is incorrectly failing to load the fbaa64.efi image found in Arm servers even though the UEFI shell code is able to properly load and execute the same image. The problem is due to the presence of a section header that has zero size and address and marked "discardable" in the fbaa64.efi image. Although there is already a check further down in the code to look for the discardable bit and skip further verification checks if set, we never get to that point due to the "end < base" check at the start of the loop. Here is a dump of the fbaa64.efi image as compiled on an Arm machine from the latest code in this repo: % # First I used hexedit to change header byte from 'AA' to '86' % # so that objdump was able to correctly parse the file: % objdump -x -m aarch64 fbaa64.efi fbaa64.efi: file format pei-x86-64 fbaa64.efi architecture: i386:x86-64, flags 0x00000103: HAS_RELOC, EXEC_P, D_PAGED start address 0x0000000000000148 Characteristics 0x20e executable line numbers stripped symbols stripped debugging information removed Time/Date Wed Dec 31 16:00:00 1969 Magic 020b (PE32+) MajorLinkerVersion 2 MinorLinkerVersion 20 SizeOfCode 000b15d0 SizeOfInitializedData 00000000 SizeOfUninitializedData 00000000 AddressOfEntryPoint 0000000000000148 BaseOfCode 0000000000000148 ImageBase 0000000000000000 SectionAlignment 0000000000000020 FileAlignment 0000000000000008 MajorOSystemVersion 0 MinorOSystemVersion 0 MajorImageVersion 0 MinorImageVersion 0 MajorSubsystemVersion 0 MinorSubsystemVersion 0 Win32Version 00000000 SizeOfImage 000b1718 SizeOfHeaders 00000148 CheckSum 00000000 Subsystem 0000000a (EFI application) DllCharacteristics 00000000 SizeOfStackReserve 0000000000000000 SizeOfStackCommit 0000000000000000 SizeOfHeapReserve 0000000000000000 SizeOfHeapCommit 0000000000000000 LoaderFlags 00000000 NumberOfRvaAndSizes 00000006 The Data Directory Entry 0 0000000000000000 00000000 Export Directory [.edata (or where ever we found it)] Entry 1 0000000000000000 00000000 Import Directory [parts of .idata] Entry 2 0000000000000000 00000000 Resource Directory [.rsrc] Entry 3 0000000000000000 00000000 Exception Directory [.pdata] Entry 4 0000000000000000 00000000 Security Directory Entry 5 0000000000000000 00000000 Base Relocation Directory [.reloc] Entry 6 0000000000000000 00000000 Debug Directory Entry 7 0000000000000000 00000000 Description Directory Entry 8 0000000000000000 00000000 Special Directory Entry 9 0000000000000000 00000000 Thread Storage Directory [.tls] Entry a 0000000000000000 00000000 Load Configuration Directory Entry b 0000000000000000 00000000 Bound Import Directory Entry c 0000000000000000 00000000 Import Address Table Directory Entry d 0000000000000000 00000000 Delay Import Directory Entry e 0000000000000000 00000000 CLR Runtime Header Entry f 0000000000000000 00000000 Reserved Sections: Idx Name Size VMA LMA File off Algn 0 .reloc 00000000 0000000000000000 0000000000000000 00000000 2**0 ALLOC, LOAD, READONLY, DATA 1 .text 000b15d0 0000000000000148 0000000000000148 00000148 2**4 CONTENTS, ALLOC, LOAD, CODE SYMBOL TABLE: no symbols Signed-off-by: Maran Wilson <maran.wilson@oracle.com> Reviewed-by: Aaron Young <aaron.young@oracle.com> Reviewed-by: Jack Schwartz <jack.schwartz@oracle.com> Upstream-commit-id: 6df7a8f5609
2020-07-23shim: Prevent shim to set itself as a second stage loaderJavier Martinez Canillas
When shim is invoked from a relative path (e.g: from the UEFI shell), the Loaded Image handle LoadOptions can be set to the binary relative path. But the is_our_path() function only checks if LoadOptions is set to the absolute path of shim to ignore it. So if a relative path is there, shim would set itself as the secondary loader and invoke itself in a loop. To prevent that, use the path in LoadOptions to calculate the absolute path and compare it with the one in the Loader Image handle FilePath. Resolves: bz#1622485 Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> Reviewed-by: Maran Wilson maran.wilson@oracle.com Tested-by: Maran Wilson maran.wilson@oracle.com Upstream-commit-id: e563bc3dcd1
2020-07-23shim: Properly generate absolute paths from relative image pathsJavier Martinez Canillas
The generate_path_from_image_path() doesn't properly handle the case when shim is invoked using a relative path (e.g: from the EFI shell). In that function, always the last component is stripped from absolute file path to calculate the dirname, and this is concatenated with the image path. But if the path is a relative one, the function will wrongly concatenate the dirname with the relative image path, i.e: Shell> FS0: FS0:\> cd EFI FS0:\EFI\> BOOT\BOOTX64.EFI Failed to open \EFI\BOOT\BOOT\BOOTX64.EFI - Not found Failed to load image \EFI\BOOT\BOOT\BOOTX64.EFI: Not found start_image() returned Not found Calculate the image path basename and concatenate that with the dirname. Signed-off-by: Javier Martinez Canillas <javierm@redhat.com> Reviewed-by: Maran Wilson maran.wilson@oracle.com Tested-by: Maran Wilson maran.wilson@oracle.com Upstream-commit-id: a625fa5096c
2020-07-23shim: Extend invalid reloc size warning messagePaul Menzel
Knowing the value of the reloc directory size is helpful for debugging, cf. issue #131 [1], [1]: https://github.com/rhboot/shim/issues/131 Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de> Upstream-commit-id: dd3230d07f3
2020-07-23Make some things dprint() instead of console_print()Peter Jones
Signed-off-by: Peter Jones <pjones@redhat.com> Upstream-commit-id: dad59f8c0f36
2018-04-05Audit get_variable() calls for correct FreePool() use.Peter Jones
Signed-off-by: Peter Jones <pjones@redhat.com>
2018-04-05Make handle_image() use console_print() not console_notify() on successPeter Jones
Signed-off-by: Peter Jones <pjones@redhat.com>
2018-04-05Get rid of dprinta(), it's uselessPeter Jones
Signed-off-by: Peter Jones <pjones@redhat.com>
2018-04-04Make the 'something has gone seriously wrong' message less ambiguousPeter Jones
Signed-off-by: Peter Jones <pjones@redhat.com>
2018-04-04read_header(): fix the case where signatures have been removed.Peter Jones
Signed-off-by: Peter Jones <pjones@redhat.com>
2018-03-23Revert "Allow shim to handle multiple trusted certificates"Peter Jones
This was merged before it was really ready - verify_trusted_cert needs to check each certificate against vendor_dbx, "dbx", and "MokListX", or else it can enable a blacklisted certificate accidentally. This reverts commit 8721bbe6fb1bfdfbc8bd16e05673929e4cbbdedc.
2018-03-19Fix i386 pointer type error.Peter Jones
Signed-off-by: Peter Jones <pjones@redhat.com>
2018-03-15Avoid a minor scan-build complaint.Peter Jones
scan-build doesn't like it when we assign return values but don't use them. Signed-off-by: Peter Jones <pjones@redhat.com>
2018-03-14Allow shim to handle multiple trusted certificatesMichael Brown
Allow shim to perform verification against a list of trusted certificates by simply concatenating the DER files. Signed-off-by: Michael Brown <mbrown@fensystems.co.uk>
2018-03-14Allow memory allocated by handle_image() to be freedMichael Brown
There is currently no way for a caller of handle_image() to free the memory allocated to hold the relocated executable. Fix by adding the allocated memory address and number of pages as returned parameters from handle_image(). Signed-off-by: Michael Brown <mbrown@fensystems.co.uk>
2018-03-14Remove global entry_point variableMichael Brown
Treat entry_point as a returned parameter from handle_image(), rather than using a global variable. Signed-off-by: Michael Brown <mbrown@fensystems.co.uk>
2018-03-14Do not modify original imageMichael Brown
relocate_coff() currently modifies the PE header within the raw data. This appears to be unnecessary, and causes a verification failure if a second attempt is made to verify the same data buffer. Signed-off-by: Michael Brown <mbrown@fensystems.co.uk>
2018-03-12console: Do not set EFI console to textmode until something is printedHans de Goede
Remove the setup_console(1) calls from shim and instead make lib/console.c make that call when necessary. This avoids shim forcing the EFI console to switch to text-mode if nothing is printed. This commit also modifies MokManager to work the same way for consistency, even though MokManager will always print something. Signed-off-by: Hans de Goede <hdegoede@redhat.com>
2018-03-12console: Add console_print and console_print_at helpersHans de Goede
This is a preparation commit for removing the setup_console(1) calls from MokManager and shim so that we don't force the EFI console to switch to text-mode. This commit replaces all direct calls to Print / PrintAt with calls to the new helpers (no functional changes) so that we can delay calling setup_console(1) till the first Print call in a follow-up patch. Signed-off-by: Hans de Goede <hdegoede@redhat.com>
2018-03-12shim: Don't overwrite EFI_LOADED_IMAGE's LoadOptions when not neededTamas K Lengyel
When the firmware is using EFI_LOAD_OPTION to specify options for the secondary loader, the shim will properly detect that and return in set_second_stage. Later howerer in handle_image EFI_LOADED_IMAGE is being overwritten with load_option irrespective of the fact that load_option was never set. This effectively prevents the EFI_LOAD_OPTION from reaching the secondary loader. Only overwrite EFI_LOADED_IMAGE's LoadOptions when load_option is not NULL solves the problem. Signed-off-by: Tamas K Lengyel <lengyelt@ainfosec.com>
2018-03-12Fix a minor merge error.Peter Jones
Signed-off-by: Peter Jones <pjones@redhat.com>
2018-03-12shim: Make our variable validation and mirroring table driven.Peter Jones
This makes it so shim's idea of Mok variables all resides in one table of data, and we don't need a bunch of nearly identical ad-hoc functions to handle each of them. Signed-off-by: Peter Jones <pjones@redhat.com>
2018-03-12shim: make everything use a common perror() call.Peter Jones
Signed-off-by: Peter Jones <pjones@redhat.com>
2018-03-12shim: Improve the bounds checking of ImageAddress()Peter Jones
Make ImageAddress() directly check for overflow in its math. Signed-off-by: Peter Jones <pjones@redhat.com>
2018-03-12shim: main(): Don't save the value from mok_ignore_db()Peter Jones
We don't really care if setting MokIgnoreDB fails; don't save the return. Signed-off-by: Peter Jones <pjones@redhat.com>
2018-03-12shim: generate_hash(): make clang-analyzer not get confused.Peter Jones
clang-analyzer thinks that because we're not checking for NULL from ImageAddress() it can be NULL, but doesn't realize we've already checked that value once before. Check it again, it can't hurt. Signed-off-by: Peter Jones <pjones@redhat.com>
2018-03-12shim: Mitigate a minor clang-analyzer complaint.Peter Jones
Clang believes "SumOfBytesHashed" is never used, because the thing that uses that computed value is #if 0'd currently. Just swizzle the #if's so that line is also not compiled. Signed-off-by: Peter Jones <pjones@redhat.com>
2018-03-12Don't use uefi_call_wrapper(), ever.Peter Jones
I'm pretty done with typing uefi_call_wrapper() and counting arguments every time. Instead, just make the compiler error if we don't have ms_abi. Also, make it so nothing can use uefi_call_wrapper() directly. Signed-off-by: Peter Jones <pjones@redhat.com>
2018-03-12shim: Use EFI_ERROR() instead of comparing to EFI_SUCCESS everywhere.Peter Jones
Also consistently name our status variable "efi_status" unless there's a good reason not to, such as already having another one of those. Signed-off-by: Peter Jones <pjones@redhat.com>
2018-03-12shim: relocate_coff(): get rid of "FixupData" stuffPeter Jones
"FixupData" in the edk2 tree is a log of the relocations that happened, which is allocated by the "client" calling relocate, and written into while it does relocations. Since we never allocate that log anywhere, FixupData is always NULL, and so covscan says: 318 case EFI_IMAGE_REL_BASED_HIGH: 319 Fixup16 = (UINT16 *) Fixup; 320 *Fixup16 = (UINT16) (*Fixup16 + ((UINT16) ((UINT32) Adjust >> 16))); null: At condition FixupData != NULL, the value of FixupData must be NULL. dead_error_condition: The condition FixupData != NULL cannot be true. 321 if (FixupData != NULL) { CID 182859 (#1 of 4): Logically dead code (DEADCODE)dead_error_begin: Execution cannot reach this statement: *((UINT16 *)FixupData) = *F.... 322 *(UINT16 *) FixupData = *Fixup16; 323 FixupData = FixupData + sizeof (UINT16); 324 } 325 break; And it's right; all four occurrances are deadcode that never do anything but confuse the reader. Kill it with fire. Signed-off-by: Peter Jones <pjones@redhat.com>
2018-03-12shim: check_db_cert_in_ram(): clear openssl errors /before/ returning.Peter Jones
Covscan says: 455 if (IsFound) { 456 tpm_measure_variable(dbname, guid, CertSize, Cert->SignatureData); 457 return DATA_FOUND; CID 182850 (#1 of 1): Structurally dead code (UNREACHABLE)unreachable: This code cannot be reached: drain_openssl_errors();. 458 drain_openssl_errors(); 459 } else { 460 LogError(L"AuthenticodeVerify(): %d\n", IsFound); 461 } And, well... woops. Signed-off-by: Peter Jones <pjones@redhat.com>
2018-03-12shim: ensure generate_hash() never operates on a negative (signed) number.Peter Jones
Covscan noticed: 746static EFI_STATUS generate_hash (char *data, unsigned int datasize_in, 747 PE_COFF_LOADER_IMAGE_CONTEXT *context, 748 UINT8 *sha256hash, UINT8 *sha1hash) 749 750{ ... 764 CID 182849 (#1 of 1): Unsigned compared against 0 (NO_EFFECT)unsigned_compare: This less-than-zero comparison of an unsigned value is never true. datasize_in < 0U. 765 if (datasize_in < 0) { 766 perror(L"Invalid data size\n"); 767 return EFI_INVALID_PARAMETER; 768 } And I guess that's a fair point, but some of the callers take the size as a signed integer. So we should be handling that on all the input cases instead of getting that far. Signed-off-by: Peter Jones <pjones@redhat.com>
generated by cgit v1.2.3 (git 2.39.1) at 2025-09-02 02:28:36 +0000