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 8721bbe6fb1bfdfbc8bd16e05673929e4cbbdedc.

Revert "MokManager: stop using StrnCat"

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>

Fix i386 pointer type error.

Signed-off-by: Peter Jones <pjones@redhat.com>

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>

Work around clang bugs for scan-build.

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>

travis: Fix a typo

Signed-off-by: Peter Jones <pjones@redhat.com>

Fix the working directory we start in.

Signed-off-by: Peter Jones <pjones@redhat.com>

Add some configs for CI using github+travis+docker

Signed-off-by: Peter Jones <pjones@redhat.com>

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>

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>

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>

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>

MokManager: stop using StrnCat

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>

console: Fix indentation

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>

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>

console: Move setup_console() definition to higher in the file

This is a preparation patch for making setup_console() private.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>

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>

Fix syntastic config for include/

Signed-off-by: Peter Jones <pjones@redhat.com>

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>

Make EFI_INCLUDE path configurable during make

Signed-off-by: Tamas K Lengyel <lengyelt@ainfosec.com>

Make sure fallback.efi gets errlog.o as well

Signed-off-by: Peter Jones <pjones@redhat.com>

Fix a minor merge error.

Signed-off-by: Peter Jones <pjones@redhat.com>

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>

shim: make everything use a common perror() call.

Signed-off-by: Peter Jones <pjones@redhat.com>

shim: Improve the bounds checking of ImageAddress()

Make ImageAddress() directly check for overflow in its math.

Signed-off-by: Peter Jones <pjones@redhat.com>

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>

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>

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>

shim: tpm.c: Make sure old_caps is initialized in tpm2 code.

clang-analyzer caught this.

Signed-off-by: Peter Jones <pjones@redhat.com>

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>

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>

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>

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>

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>

fallback: find_boot_options(): don't leak a file handle.

If we open it, we have to close it.

Signed-off-by: Peter Jones <pjones@redhat.com>

fallback: 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>

fallback: find_boot_options(): make the allocation path prettier.

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>

fallback: find_boot_csv(): eliminate dead code.

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>

fallback: find_boot_csv(): Print the error from try_boot_csv()

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>

fallback: read_file(): limit how big the file can be and still be valid

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>

fallback: handle buffer allocations for fh->GetInfo() prettier.

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>

MokManager: get rid of struct menu_item, it is unused.

Signed-off-by: Peter Jones <pjones@redhat.com>

MokManager: Fix a conditional on an uninitialized value in one error path.

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>

MokManager: handle mok parameter allocations better.

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>

MokManager: use EFI_ERROR() instead of comparing to EFI_SUCCESS.

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>

MokManager: check_der_suffix(): use StrnCat() on our suffix checker.

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>

MokManager: Lindent (and other reformats) the whole file.

I'm just tired of all the little quirks.

Signed-off-by: Peter Jones <pjones@redhat.com>

lib/shell.c: minor cleanup

Signed-off-by: Peter Jones <pjones@redhat.com>

lib/simple_file.c: minor cleanup

Signed-off-by: Peter Jones <pjones@redhat.com>

lib: 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>

lib: find_in_variable_esl(): Fix a tiny nitpick clang-analyze has.

clang-analyze believes the following:

311 EFI_STATUS
312 variable_enroll_hash(CHAR16 *var, EFI_GUID owner,
313      UINT8 hash[SHA256_DIGEST_SIZE])
314 {
315 EFI_STATUS efi_status;
316
317 efi_status = find_in_variable_esl(var, owner, hash, SHA256_DIGEST_SIZE);
>               Calling 'find_in_variable_esl' →

260 EFI_STATUS
261 find_in_variable_esl(CHAR16* var, EFI_GUID owner, UINT8 *key, UINTN keylen)
262 {
263 UINTN DataSize;
264 UINT8 *Data;
>               ← 'Data' declared without an initial value →
265 EFI_STATUS efi_status;
266
267 efi_status = get_variable(var, &Data, &DataSize, owner);
>               ← Calling 'get_variable' →

237 EFI_STATUS
238 get_variable(CHAR16 *var, UINT8 **data, UINTN *len, EFI_GUID owner)
239 {
240 return get_variable_attr(var, data, len, owner, NULL);
> ← Calling 'get_variable_attr' →

213 EFI_STATUS
214 get_variable_attr(CHAR16 *var, UINT8 **data, UINTN *len, EFI_GUID owner,
215   UINT32 *attributes)
216 {
217 EFI_STATUS efi_status;
218
219 *len = 0;
220
221 efi_status = GetVariable(var, &owner, NULL, len, NULL);
> ← Calling 'GetVariable' →
> ← Returning from 'GetVariable' →
222 if (efi_status != EFI_BUFFER_TOO_SMALL)
> ← Assuming the condition is true →
> ← Taking true branch →
223 return efi_status;
224
225 *data = AllocateZeroPool(*len);
226 if (!*data)
227 return EFI_OUT_OF_RESOURCES;
228
229 efi_status = GetVariable(var, &owner, attributes, len, *data);
230 if (EFI_ERROR(efi_status)) {
231 FreePool(*data);
232 *data = NULL;
233 }
234 return efi_status;
235 }

And it can't figure out that the first GetVariable() call will, in fact,
always return EFI_BUFFER_TOO_SMALL, and that AllocateZeroPool() will
then *correctly* clobber the two variables we never assigned the value
from.  It also then believes that efi_status might have been returned
/without/ being an error, and thinks that means we'll use the
uninitialized pointer.

This won't happen, but hey, let's make the code better express to the
checker what is intended.

Signed-off-by: Peter Jones <pjones@redhat.com>

lib: simple_file_selector(): remove some dead code.

clang-analyzer correctly believes this:

465                             int i;
466
467 i = StrLen(name) - 1;

                                ^ Value stored to 'i' is never read

468
469 for (i = StrLen(name); i > 0; --i) {
470 if (name[i] == '\\')
471 break;
472 }

And it's right; that's completely dead code.

Signed-off-by: Peter Jones <pjones@redhat.com>

lib: simple_file_selector(): simplify the error path to confuse covscan less.

Because they don't believe code should be defensive against future
changes, covscan believes:

520 out_free:
521        FreePool(dmp);
   CID 182824 (#1 of 1): Dereference before null check
   (REVERSE_INULL)check_after_deref: Null-checking entries suggests that
   it may be null, but it has already been dereferenced on all paths
   leading to the check.
522        if (entries) {
523                free_entries(entries, count);
524                FreePool(entries);
525        }
526 out_free_name:
527        FreePool(name);
528}

Which is technically correct, but still kind of dumb.  So this patch
combines the two error out paths into just being out_free, so that the
first path there is before entries is allocated.  (It also initializes
dmp to NULL and checks that before freeing it.)

I also Lindent-ed that function.

Signed-off-by: Peter Jones <pjones@redhat.com>

Use gcc's offsetof() instead of hacking out our own.

Signed-off-by: Peter Jones <pjones@redhat.com>

CryptLib: Add the AsciiStrCpy() decl.

Signed-off-by: Peter Jones <pjones@redhat.com>

Get rid of all the places we cast to (CHAR16 *[])

Lindent gets confused by these, and they're hard to read anyway.

Signed-off-by: Peter Jones <pjones@redhat.com>

Fix some "if (x < 0)" tests where x is UINTN.

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>

Don't have tons of local guid definitions for no reason at all.

Signed-off-by: Peter Jones <pjones@redhat.com>

lib/variables.c: reformat CreateTimeBasedPayload()

Signed-off-by: Peter Jones <pjones@redhat.com>

Clean up efiauthenticated.h a lot.

Signed-off-by: Peter Jones <pjones@redhat.com>

Make sure all of our include files have proper guards.

... and make them all the same formatting too.

Signed-off-by: Peter Jones <pjones@redhat.com>

Move includes around to clean the source tree up a bit.

Signed-off-by: Peter Jones <pjones@redhat.com>

Rename generate_path() because we have 2 of it.

Signed-off-by: Peter Jones <pjones@redhat.com>

Fix the declaration of find_httpboot() to match its implementation.

Signed-off-by: Peter Jones <pjones@redhat.com>

Add "make scan-build" target.

Signed-off-by: Peter Jones <pjones@redhat.com>

Add a model file for coverity.

This is useful to hide some false positives from the covscan results.
We never build it.

Signed-off-by: Peter Jones <pjones@redhat.com>

Add 'make coverity' target.

Signed-off-by: Peter Jones <pjones@redhat.com>

Split makefiles up a bit

Signed-off-by: Peter Jones <pjones@redhat.com>

Document REQUIRE_TPM

Signed-off-by: Peter Jones <pjones@redhat.com>

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>

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>

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>

Fall-back TPM2 measurement if it fails with PE_COFF_IMAGE flag

Signed-off-by: Tamas K Lengyel <lengyelt@ainfosec.com>

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>

Measure into the TPM even if SecureBoot is off in shim_lock verify

Signed-off-by: Tamas K Lengyel <lengyelt@ainfosec.com>

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>

Add -m64 compiler flag to allow cross-compiling to 64-bit version on 32-bit system

Signed-off-by: Tamas K Lengyel <lengyelt@ainfosec.com>

ident: We don't actually need the hostname or kernel version, and it makes the builds differ.

Signed-off-by: Peter Jones <pjones@redhat.com>

Add proxy dhcp support

Cryptlib: replace CryptPem with CryptPemNull

We don't need the functions in CryptPem.c.

Signed-off-by: Gary Lin <glin@suse.com>

httpboot: include console.h

in_protocol is declared in console.h, so httpboot.c has to include the
header.

Signed-off-by: Gary Lin <glin@suse.com>

httpboot: fix the infinite loop

We should get out of the loop once the uri node is not the last node in
the device path.

Signed-off-by: Gary Lin <glin@suse.com>

httpboot: Amend the device path matching rule

Originally, we check if the last 2 nodes in the device path are
IPv4()/Uri() or IPv6()/Uri() to determine whether httpboot is used or
not. However, since UEFI 2.7, the DNS node will be inserted between the
IP node and the URI node if the server provides the DNS server address.
This commit changes the matching rule to search IP node and URI node
and ignore any node between those two nodes.

Signed-off-by: Gary Lin <glin@suse.com>

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>

Bump the version to 14

Signed-off-by: Peter Jones <pjones@redhat.com>

Don't allow undefined symbols at all.

Signed-off-by: Peter Jones <pjones@redhat.com>

"in_protocol" is used in more than shim.o; make it not static.

Signed-off-by: Peter Jones <pjones@redhat.com>

Don't let openssl() try to call an external abort()

Signed-off-by: Peter Jones <pjones@redhat.com>

Bump the version to 13

shim 13:
- OpenSSL reverted to 1.0.2k to make the cert chaining of existing deployments stay working
- Better PCR usage for TPM
- TPM documentation in README.tpm
- More configurable build via make variables:
  ENABLE_SHIM_CERT
  ENABLE_SHIM_HASH
  ENABLE_SBSIGN
  LIBDIR
  EFIDIR
  VENDOR_CERT_FILE
  VENDOR_DB_FILE
- Better MoK documentation in MokVars.txt
- Better debuginfo generation
- Lots of minor bug fixes.

Signed-off-by: Peter Jones <pjones@redhat.com>

buildid: Check the return values of write() calls

Signed-off-by: Mathieu Trudel-Lapierre <mathieu.trudel-lapierre@canonical.com>

Make shim_cert.h able to be included more safely.

If you build with ENABLE_SHIM_CERT=1, the include chain right now winds
up meaning shim_cert is defined in a header that gets included in
netboot.c as well, which never uses it:

  In file included from shim.h:125:0,
                   from netboot.c:36:
  shim_cert.h:1:14: error: ‘shim_cert’ defined but not used [-Werror=unused-variable]
   static UINT8 shim_cert[] = {
                ^~~~~~~~~
  cc1: all warnings being treated as errors

So make that okay by adding __attribute__((__unused__)) to the variable
decl.

Signed-off-by: Peter Jones <pjones@redhat.com>

buildid: make 'make clean' remove it.

Use iconv for UCS-2 encoding.

Cyphermox discovered that when you run this:

  ( printf "\xff\x00\xfe\x00" ; echo "shimx64.efi,foo,,This is the boot entry for foo" ) | sed -z 's/./&\x00/g'

on some debian machines, printf(1) doesn't interpret the \x.. characters,
and that results in this being the encoded text:

00000000  5c 78 66 66 5c 78 66 65  73 00 68 00 69 00 6d 00  |\xff\xfes.h.i.m.|
00000010  78 00 36 00 34 00 2e 00  65 00 66 00 69 00 2c 00  |x.6.4...e.f.i.,.|
00000020  66 00 6f 00 6f 00 2c 00  2c 00 54 00 68 00 69 00  |f.o.o.,.,.T.h.i.|

which... yeah, that's wrong.  So instead, use iconv instead of
printf+sed to encode it in UCS-2.  Unfortunately, that means we don't
get endian markers, because for some reason iconv(1) doesn't have any way
to say it should include them.  But that's okay; fallback already
handles not having them and just assumes the second byte being \x00
means UCS-2LE.

Signed-off-by: Peter Jones <pjones@redhat.com>

Fix an LDFLAGS issue on arm and aarch64

Commit 1e71734992 inadvertantly switched ARM's
LDFLAGS+=--defsym=EFI_SUBSYSTEM=$(SUBSYSTEM) to be before LDFLAGS is set,
and so it got clobbered away.

Signed-off-by: Peter Jones <pjones@redhat.com>

Try to do a slightly better job at 'make clean'

Signed-off-by: Peter Jones <pjones@redhat.com>

Don't build shim_cert.h in parallel with other targets.

shim_cert.h is required by other pieces (such as netboot.o, cert.o) and
might not be built by the time these targets are reached. In that case the
build would fail as it can't find a required header.

Signed-off-by: Mathieu Trudel-Lapierre <mathieu.trudel-lapierre@canonical.com>

Clean up after BOOT$(ARCH).CSV.

It should not be left around after clean since it's a generated file.

Signed-off-by: Mathieu Trudel-Lapierre <mathieu.trudel-lapierre@canonical.com>

try to show errors more usefully.

Signed-off-by: Peter Jones <pjones@redhat.com>

Add some debugging data to the last malformed binary check...

Signed-off-by: Peter Jones <pjones@redhat.com>

Add some defaults and a syntastic file so vim helps more.

Signed-off-by: Peter Jones <pjones@redhat.com>