| Age | Commit message (Collapse) | Author |
|---|
|
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.
|
|
This reverts commit 6aa5a62515d62139a2d3b34626fac8910e864a3d.
Everything Hans said was correct. But StrnCat() is in gnu-efi 3.0.8,
and using just StrCpy() here confuses coverity. I'd rather have a CI
page that's not completely full of chaff, but a little bit of redundancy
in the code.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
scan-build doesn't like it when we assign return values but don't use
them.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
I don't think the x86 binaries clang builds will actually work unless
they just infer -maccumulate-outgoing-args from __attribute__((__ms_abi__),
but it's nice to have the analyzer working.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
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>
|
|
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>
|
|
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>
|
|
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>
|
|
StrnCat is not available in gnu-efi-3.0.5 (I did not check if it does
actually exists in 3.0.6). Moreover using strcat on a buffer where we've
just done: "buf[0] = '\0'" is a bit silly, we might as well drop the 0
termination and just use strcpy.
It seems there also is no StrnCpy in gnu-efi-3.0.5, but we are passing in
a pointer to the end of file_name minus 4, so strcpy will consume only
4 bytes anyways and there is no need for the "n".
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
|
|
The manual merge of the "console: Do not set EFI console to textmode until
something is printed" patch has lead to a bunch of tabs being replaced
with 7 spaces. This commit fixes this.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
|
|
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>
|
|
This is a preparation patch for making setup_console() private.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
|
|
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>
|
|
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
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>
|
|
Signed-off-by: Tamas K Lengyel <lengyelt@ainfosec.com>
|
|
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
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>
|
|
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Make ImageAddress() directly check for overflow in its math.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
We don't really care if setting MokIgnoreDB fails; don't save the return.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
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>
|
|
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>
|
|
clang-analyzer caught this.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
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>
|
|
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>
|
|
"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>
|
|
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>
|
|
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>
|
|
If we open it, we have to close it.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
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>
|
|
Covscan believes all this stuff:
852 bs = 0;
853 rc = uefi_call_wrapper(fh2->Read, 3, fh2, &bs, NULL);
7. Condition rc == (9223372036854775813UL /* 0x8000000000000000UL | 5 */), taking false branch.
8. Condition rc == 0, taking false branch.
15. Condition rc == (9223372036854775813UL /* 0x8000000000000000UL | 5 */), taking false branch.
16. Condition rc == 0, taking true branch.
17. Condition bs != 0, taking true branch.
30. Condition rc == (9223372036854775813UL /* 0x8000000000000000UL | 5 */), taking false branch.
31. Condition rc == 0, taking false branch.
854 if (rc == EFI_BUFFER_TOO_SMALL ||
855 (rc == EFI_SUCCESS && bs != 0)) {
856 buffer = AllocateZeroPool(bs);
18. Condition !buffer, taking false branch.
857 if (!buffer) {
858 Print(L"Could not allocate memory\n");
859 /* sure, this might work, why not? */
860 uefi_call_wrapper(fh2->Close, 1, fh2);
861 uefi_call_wrapper(fh->Close, 1, fh);
862 return EFI_OUT_OF_RESOURCES;
863 }
864
865 rc = uefi_call_wrapper(fh2->Read, 3, fh2, &bs, buffer);
866 }
9. Condition bs == 0, taking false branch.
19. Condition bs == 0, taking false branch.
32. Condition bs == 0, taking false branch.
867 if (bs == 0)
868 break;
869
10. Condition (INTN)rc < 0, taking false branch.
20. Condition (INTN)rc < 0, taking false branch.
33. Condition (INTN)rc < 0, taking false branch.
870 if (EFI_ERROR(rc)) {
871 Print(L"Could not read \\EFI\\: %d\n", rc);
872 if (buffer) {
873 FreePool(buffer);
874 buffer = NULL;
875 }
876 uefi_call_wrapper(fh2->Close, 1, fh2);
877 uefi_call_wrapper(fh->Close, 1, fh);
878 return rc;
879 }
34. alias_transfer: Assigning: fi = buffer.
880 EFI_FILE_INFO *fi = buffer;
881
11. Condition !(fi->Attribute & 16), taking false branch.
21. Condition !(fi->Attribute & 16), taking false branch.
CID 182858 (#1-3 of 3): Explicit null dereferenced (FORWARD_NULL)35. var_deref_op: Dereferencing null pointer fi.
882 if (!(fi->Attribute & EFI_FILE_DIRECTORY)) {
883 FreePool(buffer);
884 buffer = NULL;
885 continue;
886 }
Because it doesn't know that when bs==0, fh2->Read() will return
EFI_BUFFER_TOO_SMALL and set bs to the size we need to allocate, so the
allocation path is always taken. Instead, handle our exit/error paths
directly there, and make the allocation path nonconditional.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Covscan sez:
720 FreePool(buffer);
assignment: Assigning: buffer = NULL.
721 buffer = NULL;
722
723 CHAR16 *bootcsv=NULL, *bootarchcsv=NULL;
724
725 bs = 0;
726 do {
727 bs = 0;
728 rc = uefi_call_wrapper(fh->Read, 3, fh, &bs, NULL);
729 if (EFI_ERROR(rc) && rc != EFI_BUFFER_TOO_SMALL) {
730 Print(L"Could not read \\EFI\\%s\\: %d\n", dirname, rc);
null: At condition buffer, the value of buffer must be NULL.
dead_error_condition: The condition buffer cannot be true.
731 if (buffer)
CID 182851 (#1 of 1): Logically dead code (DEADCODE)dead_error_line:
Execution cannot reach this statement: FreePool(buffer);.
732 FreePool(buffer);
733 return rc;
734 }
And it's right; buffer can never be non-NULL there. So just take that
out.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Covscan believes the following:
782 if ((EFI_ERROR(rc) || !bootarchcsv) && bootcsv) {
783 EFI_FILE_HANDLE fh2;
784 rc = uefi_call_wrapper(fh->Open, 5, fh, &fh2,
785 bootcsv, EFI_FILE_READ_ONLY, 0);
786 if (EFI_ERROR(rc) || fh2 == NULL) {
787 Print(L"Couldn't open \\EFI\\%s\\%s: %d\n",
788 dirname, bootcsv, rc);
789 } else {
CID 182829 (#1 of 1): Unused value (UNUSED_VALUE)returned_value:
Assigning value from try_boot_csv(fh2, dirname, bootcsv) to rc here,
but that stored value is overwritten before it can be used.
790 rc = try_boot_csv(fh2, dirname, bootcsv);
791 uefi_call_wrapper(fh2->Close, 1, fh2);
792 }
793 }
value_overwrite: Overwriting previous write to rc with value 0UL.
794 rc = EFI_SUCCESS;
795
796 return rc;
797}
Which isn't untrue, we just don't happen to be using the return code for
anything, before we intentionally return success to our caller.
So that's annoying, but whatever. Just print the error as well.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Covscan says:
146 UINTN len = 0;
147 CHAR16 *b = NULL;
2. tainted_data_argument: Calling function get_file_size taints argument len.
148 rc = get_file_size(fh2, &len);
3. Condition (INTN)rc < 0, taking false branch.
149 if (EFI_ERROR(rc)) {
150 uefi_call_wrapper(fh2->Close, 1, fh2);
151 return rc;
152 }
153
4. overflow_assign: Assigning overflowed or truncated value (or a value computed from an overflowed or a truncated value) to b.
8. overflow: Add operation overflows on operands len and 2UL. Example value for operand: len = 18446744073709551614.
154 b = AllocateZeroPool(len + 2);
Technically we can't handle a file larger than 0xfffffffffffffffd (on
x86_64) because when we try to allocate the buffer to hold it with a
trailing UCS-2 NUL we overflow to 0. Also our filesystem can't hold a
file bigger than 4GB... So this is probably actually broken on 32-bit
platforms.
This patch limits it to some handy amount like 1024 * PAGE_SIZE, aka
4MB.
Note that this doesn't appear to be exploitable (at least on edk2-based
firmwares), because AllocateZeroPool() has a minimum granularity of 1
page, so even if you overflow it with a 4GB file, we'll get 1 page out
of it and then try to read 1 byte into it, and then it's just going to
be a parse error on the CSV. Even if we error on the sentinal UCS-2 NUL
we put at the end, it'll still be inside of the zeroed page, and it still
won't fault or overwrite any meaningful data.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
At all the places we use fh->GetInfo, covscan can't tell that
fh->GetInfo() will return EFI_BUFFER_TOO_SMALL and we'll allocate on the
first try.
If we just explicitly check for "buffer == NULL" as well, covscan
believes we're doing work we don't need to (which is true!)
So instead, put an rc test to return error for everything else there, so
the allocation isn't in a conditional.
Yet another stupid one, but it's easier to nerf it this way than write
the false-positive rule, and it also hardens against incorrect UEFI
implementations (though we've not seen any yet with the problem this
avoids).
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
clang-analyzer says:
MokManager.c:1431:6: warning: Branch condition evaluates to a garbage value
if (mok)
^~~
MokManager.c:1433:6: warning: Branch condition evaluates to a garbage value
if (del_key)
^~~~~~~
And it's right; if we take the first error exit in the function, those
never get initialized. This patch sets them to NULL to begin with.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Covscan daftly claims:
288. var_compare_op: Comparing MokSB to null implies that MokSB might be null.
2330 if (MokSB) {
2331 menu_strings[i] = L"Change Secure Boot state";
2332 menu_item[i] = MOK_CHANGE_SB;
2333 i++;
2334 }
2335
...
2358 choice = console_select(perform_mok_mgmt, menu_strings, 0);
2359 if (choice < 0)
2360 goto out;
...
2362 switch (menu_item[choice]) {
...
2395 case MOK_CHANGE_SB:
CID 182841 (#1 of 1): Dereference after null check
(FORWARD_NULL)293. var_deref_model: Passing null pointer MokSB to
mok_sb_prompt, which dereferences it. [show details]
2396 efi_status = mok_sb_prompt(MokSB, MokSBSize);
Which is, of course, entirely false, beause for menu_item[choice] to be
MOK_CHANGE_SB, MokSB must be !NULL. And then:
252. Condition efi_status == 0, taking true branch.
2397 if (efi_status == EFI_SUCCESS)
2398 MokSB = NULL;
This guarantees it won't be in the list the next time through the loop.
This adds tests for NULLness before mok_sb_prompt(), just to make it
more clear to covscan what's going on.
Also do the same thing for all of:
MOK_CHANGE_SB
MOK_SET_PW
MOK_CHANGE_DB
MOK_ENROLL_MOKX
MOK_DELETE_MOKX
I also Lindent-ed everything I had to touch.
Three other minor errors are also fixed:
1) the loop in enter_mok_menu() leaked the menu allocations each time
through the loop
2) mok_sb_prompt(), mok_pw_prompt(), and mok_db_prompt() all call
FreePool() on their respective variables (MokSB, etc), and
check_mok_request() also calls FreePool() on these. This sounds
horrible, but it turns out it's not an issue, because they only free
them in their EFI_SUCCESS paths, and enter_mok_menu() resets the
system if any of the mok_XX_prompt() calls actually returned
EFI_SUCCESS, so we never get back to check_mok_request() for it to do
its FreePool() calls.
3) the loop in enter_mok_menu() winds up introducing a double free in
the call to free_menu(), but we also can't hit this bug, because all
the exit paths from the loop are "goto out" (or return error) rather
than actually exiting on the loop conditional.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
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>
|
|
We know it's legit already because we computed the pointer from the end,
but covscan gets confused, and we have StrnCat, so we should just use it
anyway.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
I'm just tired of all the little quirks.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
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>
|
