Re: [PATCH 1/1] SecurityPkg/DxeImageVerificationLib: Check result of GetEfiGlobalVariable2

["Marvin Häuser" <mhaeuser@posteo.de>]

[-- Attachment #1: Type: text/plain, Size: 3541 bytes --]


> On 28. Feb 2023, at 07:12, Gerd Hoffmann <kraxel@redhat.com> wrote:
> 
>   Hi,
> 
>>> (a) the SecureBoot variable is not present (EFI_NOT_FOUND) according to
>>>    the return value, or
>> 
>> @Maintainers Would there be any objection to drop this and skip the SB checks only when explicitly disabled?
>> Please explicitly respond even if not, so we don't end up with everyone silently agreeing, but forgetting about the patch after. Thanks! :)
> 
> I hold back v2, waiting for an answer here.
> 
>>> -  if (*SecureBoot == SECURE_BOOT_MODE_DISABLE) {
>>> -    FreePool (SecureBoot);
>>> +  if ((VarStatus == EFI_SUCCESS) && (SecureBoot == SECURE_BOOT_MODE_DISABLE)) {
>> 
>> I would check the attributes here as well. They should be BS | RT, but
>> explicitly not NV. This would force the SB checks in case a malicious
>> actor somehow managed to store a persistent disable-value variable (be
>> that a bug, physical access, or other means).
> 
> Like this (incremental fixup)?

Sorry, I formulated it a bit vague - what I meant is that the attributes should be exactly BS | RT (i.e., equal to), but I see how adding it must not be NV sounds like it should be just those three Bits checked. Otherwise, yes, thanks a lot!

It’s a read-only status reporting variable, so even with future changes, setting any of the other attributes wouldn’t make much sense. BS | RT is what the spec currently dictates (there is a table here: https://uefi.org/specs/UEFI/2.10/03_Boot_Manager.html).

> 
> Do we have macros for variable attribute checking?
> Havn't seen anything while skimming variable-related headers ...

Don’t think so. Not sure there is much attribute-checking done to begin with outside VarCheckLib. The only stack I’ve seen doing this extensively is recent-years Mac EFI.

Best regards,
Marvin

> 
> take care,
>  Gerd
> 
> diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> index f29a27e5a053..79c784f77ac8 100644
> --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> @@ -1688,6 +1688,7 @@ DxeImageVerificationHandler (
>   EFI_STATUS                    HashStatus;
>   EFI_STATUS                    DbStatus;
>   EFI_STATUS                    VarStatus;
> +  UINT32                        VarAttr;
>   BOOLEAN                       IsFound;
> 
>   SignatureList     = NULL;
> @@ -1745,7 +1746,7 @@ DxeImageVerificationHandler (
>   }
> 
>   SecureBootSize = sizeof (SecureBoot);
> -  VarStatus      = gRT->GetVariable (EFI_SECURE_BOOT_MODE_NAME, &gEfiGlobalVariableGuid, NULL, &SecureBootSize, &SecureBoot);
> +  VarStatus      = gRT->GetVariable (EFI_SECURE_BOOT_MODE_NAME, &gEfiGlobalVariableGuid, &VarAttr, &SecureBootSize, &SecureBoot);
>   //
>   // Skip verification if SecureBoot variable doesn't exist.
>   //
> @@ -1756,7 +1757,12 @@ DxeImageVerificationHandler (
>   //
>   // Skip verification if SecureBoot is disabled but not AuditMode
>   //
> -  if ((VarStatus == EFI_SUCCESS) && (SecureBoot == SECURE_BOOT_MODE_DISABLE)) {
> +  if ((VarStatus == EFI_SUCCESS) &&
> +      !(VarAttr & EFI_VARIABLE_NON_VOLATILE) &&
> +      (VarAttr & EFI_VARIABLE_BOOTSERVICE_ACCESS) &&
> +      (VarAttr & EFI_VARIABLE_RUNTIME_ACCESS) &&
> +      (SecureBoot == SECURE_BOOT_MODE_DISABLE))
> +  {
>     return EFI_SUCCESS;
>   }
> 
> 

[-- Attachment #2: Type: text/html, Size: 6691 bytes --]