Age | Commit message (Collapse) | Author |
|
CertList->SignatureSize is of type UINT32 which is always positive.
If CertList->SignatureListSize == 0, then
CertList->SignatureListSize <= CertList->SignatureSize
is also true.
Remove the redundant CertList->SignatureListSize == 0 checks.
A message "Corrupted signature list" is better suited then
"Invalid MOK detected! Ignoring MOK List." in this case.
Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
|
|
Within previous versions of shim the MokListTrusted var did not
exist. The user had to opt in to using the feature.
Change the default behavior to an opt out model. Since old
shims will not have the BS MokListTrusted set, use inverse
logic that sets the MokListTrustedRT to 1 when the boot
service variable is missing.
Many Linux distros carry out of tree patches to trust the mok
keys by default. These out of tree patches can be dropped
when using a Linux kernel that supports MokListTrustedRT.
Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
|
|
- one missing free
- one minor deadcode issue
- two unchecked allocations
- one debug hexdump of a variable we just freed
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Introduce a new MOK variable called MokListTrustedRT. It allows an end-user
to decide if they want to trust MOKList keys within the soon to be booted
Linux kernel. This variable does not change any functionality within shim
itself. When Linux boots, if MokListTrustedRT is set and
EFI_VARIABLE_NON_VOLATILE is not set, keys in MokListRT are loaded into the
.machine keyring instead of the .platform keyring.
Signed-off-by: Eric Snowberg <eric.snowberg@oracle.com>
|
|
Several places in e.g. MokManager and our console library use
ST->ConOut->ClearScreen directly, without checking for the existence of
a console output device.
This patch adds function to our console library to do that correctly,
instead of using the bug-prone ad hoc implementation everywhere.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
This just makes one less thing we have to make sure is the same between
the test harnesses and the runtime code.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
This re-structures our includes so we can be sure everything is always
including all the system headers in a uniform, predictable way.
Temporarily it also adds a bunch of junk at all the places we use
variadic functions to specifically pick either the MS (cdecl) or ELF
ABIs.
I'm not 100% sure that's all correct (see later patch) but it's enough
to allow this to build.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Move the countdown function from MokManager to console.c to make the
function public
Also make console_save_and_set_mode() and console_restore_mode() public
Signed-off-by: Gary Lin <glin@suse.com>
|
|
The license statements in our source files were getting to be a giant
mess, and mostly they all just say the same thing. I've switched most
of it to SPDX labels, but left copyright statements in place (where they
were not obviously incorrect copy-paste jobs that I did...).
If there's some change here you don't think is valid, let me know and
we can fix it up together.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
This fixes several codepaths where MokList and MokListX are supposed to
be deleted, but are not. It also adds debug logging to much of the
deletion codepath.
|
|
Signed-off-by: Peter Jones <pjones@redhat.com>
Upstream: pr#212
|
|
Signed-off-by: Peter Jones <pjones@redhat.com>
Upstream: pr#212
|
|
When compiling with GCC 9, there are a couple of errors of the form
MokManager.c: In function ‘write_back_mok_list’:
MokManager.c:1056:19: error: taking address of packed member of ‘struct <anonymous>’ may result in an unaligned pointer value [-Werror=address-of-packed-member]
1056 | if (CompareGuid(&(list[i].Type), &X509_GUID) == 0)
| ^~~~~~~~~~~~~~~
Copying the member of the packed struct to a temporary variable and
pointing to that variable solves the problem.
Upstream-commit-id: 58532e12e9a
|
|
There are lots of hi-dpi laptops nowadays, as doing mok enrollment, the font
is too small to see.
https://bugs.launchpad.net/ubuntu/+source/shim/+bug/1822043
This patch checks if the resolution is larger than Full HD (1920x1080) and
current console output columns and rows is in a good mode. Then swith the
console output to a better mode.
Signed-off-by: Ivan Hu <ivan.hu@canonical.com>
Upstream-commit-id: cf05af6d899
|
|
Fix the errors from gcc9 '-Werror=address-of-packed-member'
https://github.com/rhboot/shim/issues/161
Signed-off-by: Gary Lin <glin@suse.com>
Upstream-commit-id: aaa09b35e73
|
|
In MokManager we get a lot of these:
../src/MokManager.c:1063:19: error: taking address of packed member of ‘struct <anonymous>’ may result in an unaligned pointer value [-Werror=address-of-packed-member]
1063 | if (CompareGuid(&(list[i].Type), &X509_GUID) == 0)
| ^~~~~~~~~~~~~~~
The reason for this is that gnu-efi takes EFI_GUID * as its argument
instead of VOID *, and there's nothing telling the compiler that it
doesn't have alignment constraints on the input, so the compiler wants
it to have 16-byte alignment.
Just use CompareMem() for these, as that's all CompareGuid is calling
anyway.
Signed-off-by: Peter Jones <pjones@redhat.com>
Upstream-commit-id: 08c14376b59
|
|
When writing MokList with EFI_VARIABLE_APPEND_WRITE, some HP laptops
may just return EFI_SUCCESS without writing the content into the flash,
so we have no way to detect if MokList is updated or not. Now we always
read MokList first and write it back with the new content.
https://github.com/rhboot/shim/issues/105
Signed-off-by: Gary Lin <glin@suse.com>
Upstream-commit-id: f442c8424b4
|
|
This timeout can have the values [-1,0..0x7fff]; where -1 means "no timeout",
with MokManager going directly to the menu, and is capped to 0x7fff to avoid
unecessary long timeouts. The default remains 10, which will be used whenever
the MokTimeout variable isn't set.
Signed-off-by: Mathieu Trudel-Lapierre <mathieu.trudel-lapierre@canonical.com>
Upstream-commit-id: 93708c11083
|
|
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
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>
|
|
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>
|
|
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 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>
|
|
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>
|
|
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>
|
|
Lindent gets confused by these, and they're hard to read anyway.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Obviously, these are not correct. Most of them are just useless; one
can be changed to a more useful test.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
OpenSSL changes quite a bit of the key validation, and most of the keys
I can find in the wild aren't marked as trusted by the new checker.
Intel noticed this too: https://github.com/vathpela/edk2/commit/f536d7c3ed
but instead of fixing the compatibility error, they switched their test
data to match the bug.
So that's pretty broken.
For now, I'm reverting OpenSSL 1.1.0e, because we need those certs in
the wild to work.
This reverts commit 513cbe2aea689bf968f171f894f3d4cdb43524d5.
This reverts commit e9cc33d6f2b7f35c6f5e349fd83fb9ae0bc66226.
This reverts commit 80d49f758ead0180bfe6161931838e0578248303.
This reverts commit 9bc647e2b23bcfd69a0077c0717fbc454c919a57.
This reverts commit ae75df6232ad30f3e8736e9449692d58a7439260.
This reverts commit e883479f35644d17db7efed710657c8543cfcb68.
This reverts commit 97469449fda5ba933a64280917e776487301a127.
This reverts commit e39692647f78e13d757ddbfdd36f440d5f526050.
This reverts commit 0f3dfc01e2d5e7df882c963dd8dc4a0dfbfc96ad.
This reverts commit 4da6ac819510c7cc4ba21d7a735d69b45daa5873.
This reverts commit d064bd7eef201f26cb926450a76260b5187ac689.
This reverts commit 9bc86cfd6f9387f0da9d5c0102b6aa5627e91c91.
This reverts commit ab9a05a10f16b33f7ee1e9da360c7801eebdb9d2.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Rather than looping once through the possible actions (MokNew, MokDel, etc.),
revise the logic so that instead of rebooting immediately we get back to the
main menu setting a flag to replace "Continue booting" with a proper reboot.
Getting back to the menu means we can go make other changes before rebooting.
For instance, you might want to enable validation, but beforehand you also
need to enroll a MOK. You can already do so from userland; except the requests
were cleared as soon as one of them was processed.
This involves some extra cleanup of the states to avoid running the same
request more than once, removing the option from the menu once it's done, and
changing prompting functions to return an EFI_STATUS so we can better track
whether the process has succeeded.
Signed-off-by: Mathieu Trudel-Lapierre <mathieu.trudel-lapierre@canonical.com>
|
|
X509_get_notBefore -> X509_getm_notBefore
X509_get_notAfter -> X509_getm_notAfter
Signed-off-by: Gary Lin <glin@suse.com>
|
|
Signed-off-by: Mathieu Trudel-Lapierre <mathieu.trudel-lapierre@canonical.com>
|
|
new_data in write_db() wasn't freed after SetVariable.
Signed-off-by: Gary Lin <glin@suse.com>
|
|
Try to append the MOK/MOKX list first and then fallback to the normal
SetVariable if the firmware doesn't support EFI_VARIABLE_APPEND_WRITE.
Signed-off-by: Gary Lin <glin@suse.com>
|
|
We got the bug report about the usage of APPEND_WRITE that may cause the
failure when writing a variable in Lenovo machines. Although
EFI_VARIABLE_APPEND_WRITE already exists in the UEFI spec for years,
unfortunately, some vendors just ignore it and never implement the
attribute. This commit removes the usage of EFI_VARIABLE_APPEND_WRITE to
make MokManager work on those machines.
https://github.com/rhinstaller/shim/issues/55
Signed-off-by: Gary Lin <glin@suse.com>
|
|
Also update Cryptlib to edk2 r19218
- Undefine NO_BUILTIN_VA_FUNCS in Cryptlib/OpenSSL/ for x86_64 to use
the gcc builtins and remove all EFIAPI from the functions
- Move the most of defines into the headers instead of Makefile
- Remove the global variable 'timeval'
- Remove the unused code: crypto/pqueue/* and crypto/ts/*
- Include bn.h in MokManager.c due to the changes in openssl
Signed-off-by: Gary Lin <glin@suse.com>
|
|
My favorite part of -Wsign-compare is how it shows different results on
different arches for no obvious reason.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Nobody should be deploying SHA-1. No hardware deploys it, and the rate
of change on https://en.wikipedia.org/wiki/SHA-1#Attacks is wildly
uninspiring.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
Patch from Johannes Segitz <jsegitz@suse.com>
|
|
Signed-off-by: Gary Ching-Pang Lin <glin@suse.com>
|
|
Signed-off-by: Gary Ching-Pang Lin <glin@suse.com>
|
|
Signed-off-by: Gary Ching-Pang Lin <glin@suse.com>
|
|
There are some functions that the return value and the type
didn't match.
Signed-off-by: Gary Ching-Pang Lin <glin@suse.com>
|