Commit Graph

232 Commits

Author SHA1 Message Date
Peter Jones
1d50318f44 Make some things dprint() instead of console_print()
Signed-off-by: Peter Jones <pjones@redhat.com>
Upstream-commit-id: dad59f8c0f36
2020-07-23 20:51:12 -04:00
Peter Jones
77ebb3d676 Audit get_variable() calls for correct FreePool() use.
Signed-off-by: Peter Jones <pjones@redhat.com>
2018-04-05 14:49:17 -04:00
Peter Jones
510474e72d Make handle_image() use console_print() not console_notify() on success
Signed-off-by: Peter Jones <pjones@redhat.com>
2018-04-05 14:49:17 -04:00
Peter Jones
4ffcfdf4da Get rid of dprinta(), it's useless
Signed-off-by: Peter Jones <pjones@redhat.com>
2018-04-05 14:09:46 -04:00
Peter Jones
9ab48c0c25 Make the 'something has gone seriously wrong' message less ambiguous
Signed-off-by: Peter Jones <pjones@redhat.com>
2018-04-04 16:49:43 -04:00
Peter Jones
9bee22310e read_header(): fix the case where signatures have been removed.
Signed-off-by: Peter Jones <pjones@redhat.com>
2018-04-04 16:49:43 -04:00
Peter Jones
cdbfb5a69e Revert "Allow shim to handle multiple trusted certificates"
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 8721bbe6fb.
2018-03-23 13:55:57 -04:00
Peter Jones
ad6f1747b5 Fix i386 pointer type error.
Signed-off-by: Peter Jones <pjones@redhat.com>
2018-03-19 14:27:58 -04:00
Peter Jones
d737c0273d Avoid a minor scan-build complaint.
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-15 11:23:26 -04:00
Michael Brown
8721bbe6fb Allow shim to handle multiple trusted certificates
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-14 13:48:07 -04:00
Michael Brown
d7daa70e0d Allow memory allocated by handle_image() to be freed
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-14 13:47:52 -04:00
Michael Brown
0a4c7d5af3 Remove global entry_point variable
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-14 13:46:19 -04:00
Michael Brown
5b6253c98e Do not modify original image
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-14 13:44:45 -04:00
Hans de Goede
1ff4a36a23 console: Do not set EFI console to textmode until something is printed
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-12 18:00:41 -04:00
Hans de Goede
1fe31ee1b4 console: Add console_print and console_print_at helpers
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-12 18:00:41 -04:00
Tamas K Lengyel
38b16b200c shim: Don't overwrite EFI_LOADED_IMAGE's LoadOptions when not needed
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-12 16:59:31 -04:00
Peter Jones
589819e607 Fix a minor merge error.
Signed-off-by: Peter Jones <pjones@redhat.com>
2018-03-12 16:23:49 -04:00
Peter Jones
4181a16f62 shim: Make our variable validation and mirroring table driven.
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-12 16:21:43 -04:00
Peter Jones
dd712378a7 shim: make everything use a common perror() call.
Signed-off-by: Peter Jones <pjones@redhat.com>
2018-03-12 16:21:43 -04:00
Peter Jones
568dc4944f shim: Improve the bounds checking of ImageAddress()
Make ImageAddress() directly check for overflow in its math.

Signed-off-by: Peter Jones <pjones@redhat.com>
2018-03-12 16:21:43 -04:00
Peter Jones
91e5f5324e shim: main(): Don't save the value from mok_ignore_db()
We don't really care if setting MokIgnoreDB fails; don't save the return.

Signed-off-by: Peter Jones <pjones@redhat.com>
2018-03-12 16:21:43 -04:00
Peter Jones
f80b30febc shim: generate_hash(): make clang-analyzer not get confused.
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-12 16:21:43 -04:00
Peter Jones
3b56e56a43 shim: Mitigate a minor clang-analyzer complaint.
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-12 16:21:43 -04:00
Peter Jones
9fdca5bbe1 Don't use uefi_call_wrapper(), ever.
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-12 16:21:43 -04:00
Peter Jones
1c2376338d shim: Use EFI_ERROR() instead of comparing to EFI_SUCCESS everywhere.
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-12 16:21:43 -04:00
Peter Jones
ee07a19d7e shim: relocate_coff(): get rid of "FixupData" stuff
"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-12 16:21:43 -04:00
Peter Jones
66a7b53677 shim: check_db_cert_in_ram(): clear openssl errors /before/ returning.
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-12 16:21:43 -04:00
Peter Jones
db2f5cf15d shim: ensure generate_hash() never operates on a negative (signed) number.
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>
2018-03-12 16:21:43 -04:00
Peter Jones
b953468e91 Don't have tons of local guid definitions for no reason at all.
Signed-off-by: Peter Jones <pjones@redhat.com>
2018-03-12 16:21:43 -04:00
Peter Jones
1a44dbb8be Rename generate_path() because we have 2 of it.
Signed-off-by: Peter Jones <pjones@redhat.com>
2018-03-12 16:21:43 -04:00
Hans de Goede
79cdb2a215 Fix failure to boot on systems without a TPM
This commit fixes 2 issues with the TPM support code:

1) Remove "REQUIRE_TPM ?=" line from the Makefile, further down the Makefile
checks if REQUIRE_TPM is undefined, but the above line sets it to an empty
string, which is not the same as undefined. Without this handle_image fails
after the tpm_log_pe() call even if REQUIRE_TPM=1 once was not set when
building the shim

2) When secure-boot is disabled then shim_verify() would exit with the
status of tpm_log_pe(), which on systems with a TPM is an error. Combined
with the recent change to always install the shim protocols, this causes
grub to refuse to boot any kernel since the verify() call now always fails.
This commit fixes this by explicitly setting status = EFI_SUCCESS when
secure-boot is disabled.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
2018-03-08 11:18:33 -05:00
Peter Jones
6c8d08c0af shim: Ignore UEFI LoadOptions that are just NUL characters.
I don't know when or why we ever see this, but it's easy enough to
avoid.

Resolves github issue #95

Signed-off-by: Peter Jones <pjones@redhat.com>
2018-03-06 15:28:00 -05:00
Tamas K Lengyel
e207388577 Install shim_lock protocol even when SecureBoot is off
Currently the shim_lock protocol is only installed when SecureBoot is enabled.
However, having Verify just measure into the TPM without SecureBoot is a useful
feature.

Signed-off-by: Tamas K Lengyel <lengyelt@ainfosec.com>
2018-03-06 14:42:32 -05:00
Tamas K Lengyel
ba06a4362d Add REQUIRE_TPM flag to treat TPM related errors as critical
Currently TPM related errors are being silently discarded.

Signed-off-by: Tamas K Lengyel <lengyelt@ainfosec.com>
2018-03-06 14:42:32 -05:00
Tamas K Lengyel
555ef92650 Measure into the TPM even if SecureBoot is off in shim_lock verify
Signed-off-by: Tamas K Lengyel <lengyelt@ainfosec.com>
2018-03-06 14:37:07 -05:00
Tamas K Lengyel
829d3c8265 Log measurements in PCR4 for applications being verified through shim_lock
Currently the only measurement the shim logs in the TPM is that of the EFI
application it directly loads. However, there are no measurements being taken
of application that are being verified through the shim_lock protocol. In this
patch we extend PCR4 for any binary for which Verify is being called through
the shim_lock protocol.

Signed-off-by: Tamas K Lengyel <lengyelt@ainfosec.com>
2018-03-06 14:37:07 -05:00
Mathieu Trudel-Lapierre
c8ca1c5696 Uninstall shim protocols before re-installing them
Make sure if we chainload things, a chainloaded bootloader will be able to use
the latest systab replacements and protocols. They need to match for things
to validate correctly.

Signed-off-by: Mathieu Trudel-Lapierre <mathieu.trudel-lapierre@canonical.com>
2018-02-01 13:50:44 -05:00
Peter Jones
97a3f6cf94 "in_protocol" is used in more than shim.o; make it not static.
Signed-off-by: Peter Jones <pjones@redhat.com>
2017-12-19 16:52:01 -05:00
Peter Jones
25f6fd08cd try to show errors more usefully.
Signed-off-by: Peter Jones <pjones@redhat.com>
2017-09-13 15:18:28 -04:00
Peter Jones
00753a0a28 Add some debugging data to the last malformed binary check...
Signed-off-by: Peter Jones <pjones@redhat.com>
2017-09-13 15:16:43 -04:00
Peter Jones
1d39ada8cb Revert lots of Cryptlib updates.
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 513cbe2aea.
This reverts commit e9cc33d6f2.
This reverts commit 80d49f758e.
This reverts commit 9bc647e2b2.
This reverts commit ae75df6232.
This reverts commit e883479f35.
This reverts commit 97469449fd.
This reverts commit e39692647f.
This reverts commit 0f3dfc01e2.
This reverts commit 4da6ac8195.
This reverts commit d064bd7eef.
This reverts commit 9bc86cfd6f.
This reverts commit ab9a05a10f.

Signed-off-by: Peter Jones <pjones@redhat.com>
2017-08-31 15:13:58 -04:00
Peter Jones
eae64276ff Drain the OpenSSL error stack and report crypto verification errors
Signed-off-by: Peter Jones <pjones@redhat.com>
2017-08-31 15:13:46 -04:00
Peter Jones
36d20ac0aa Init openssl so we can use its debug facilities.
Signed-off-by: Peter Jones <pjones@redhat.com>
2017-08-31 15:13:45 -04:00
Peter Jones
78f6b007e7 Make msleep() be a thing
Signed-off-by: Peter Jones <pjones@redhat.com>
2017-08-31 15:13:34 -04:00
Peter Jones
207dd7dc60 Add ENABLE_SHIM_CERT to make MokManager/fallback signing optional.
This makes shim not create its own keyring and sign MokManager and
fallback by default.

Signed-off-by: Peter Jones <pjones@redhat.com>
2017-08-11 15:18:39 -04:00
Peter Jones
bdc5d3ec9c Always measure all of MokSBState, MokList, and MokListX
Even if errors occurred, always try to measure all of our Mok entries.
This way we won't fail on e.g. MokList not being set.

Signed-off-by: Peter Jones <pjones@redhat.com>
2017-08-03 11:00:58 -04:00
Matthew Garrett
22f2737535 Measure stage 2 according to spec
We're currently measuring the raw second stage loader into PCR 9, but
we're closer to spec if we measure the semi-parsed PE into PCR 4. The
hash that's logged is the same as the hash used for the Authenticode
validation, so refactor shim.c a little to separate out the hash
generation.
2017-08-03 11:00:58 -04:00
Matthew Garrett
8af7c4caca Extend PCR 7
It's desirable to be able to use PCR 7 for all TPM policy on Secure Boot
systems, but right now Shim doesn't record any information about its
configuration or the signature used to launch the second stage loader. Add
support for that.
2017-08-01 12:54:49 -04:00
Lans Zhang
6d4498fb3b update verification_method if the loaded image is signed by shim/vendor cert
Signed-off-by: Lans Zhang <jia.zhang@windriver.com>
2017-06-15 11:30:11 -04:00
Lans Zhang
71d927270a skip the error message when creating MokListRT if vendor cert is empty
EFI_NOT_FOUND will be returned when creating MokListRT if vendor cert is
empty. This is harmless, meaningless and skippable.

Signed-off-by: Lans Zhang <jia.zhang@windriver.com>
2017-06-15 11:29:51 -04:00