| Age | Commit message (Collapse) | Author |
|---|
|
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>
|
|
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>
|
|
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>
|
|
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>
|
|
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
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>
|
|
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
... and make them all the same formatting too.
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>
|
|
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
This is useful to hide some false positives from the covscan results.
We never build it.
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>
|
|
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>
|
|
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>
|
|
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>
|
|
Signed-off-by: Tamas K Lengyel <lengyelt@ainfosec.com>
|
|
Currently TPM related errors are being silently discarded.
Signed-off-by: Tamas K Lengyel <lengyelt@ainfosec.com>
|
|
Signed-off-by: Tamas K Lengyel <lengyelt@ainfosec.com>
|
|
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>
|
|
system
Signed-off-by: Tamas K Lengyel <lengyelt@ainfosec.com>
|
|
the builds differ.
Signed-off-by: Peter Jones <pjones@redhat.com>
|
|
|
|
We don't need the functions in CryptPem.c.
Signed-off-by: Gary Lin <glin@suse.com>
|
|
in_protocol is declared in console.h, so httpboot.c has to include the
header.
Signed-off-by: Gary Lin <glin@suse.com>
|
|
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>
|
|
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>
|
|
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>
|
|
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>
|
