Re: [PATCH v8 11/14] SecurityPkg: Allow VariablePolicy state to delete authenticated variables

["Yao, Jiewen" <jiewen.yao@intel.com>]

Reviewed-by: Jiewen Yao <Jiewen.yao@intel.com>

> -----Original Message-----
> From: Bret Barkelew <bret@corthon.com>
> Sent: Wednesday, September 23, 2020 2:08 PM
> To: devel@edk2.groups.io
> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>;
> Chao Zhang <chao.b.zhang@intel.com>; Bret Barkelew
> <brbarkel@microsoft.com>; Bi, Dandan <dandan.bi@intel.com>
> Subject: [PATCH v8 11/14] SecurityPkg: Allow VariablePolicy state to delete
> authenticated variables
> 
> From: Bret Barkelew <brbarkel@microsoft.com>
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=2522
> 
> Causes AuthService to check
> IsVariablePolicyEnabled() before enforcing
> write protections to allow variable deletion
> when policy engine is disabled.
> 
> Only allows deletion, not modification.
> 
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Chao Zhang <chao.b.zhang@intel.com>
> Cc: Bret Barkelew <brbarkel@microsoft.com>
> Signed-off-by: Bret Barkelew <brbarkel@microsoft.com>
> Reviewed-by: Dandan Bi <dandan.bi@intel.com>
> Acked-by: Jian J Wang <jian.j.wang@intel.com>
> ---
>  SecurityPkg/Library/AuthVariableLib/AuthService.c       | 30 ++++++++++++++++-
> ---
>  SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf |  2 ++
>  2 files changed, 26 insertions(+), 6 deletions(-)
> 
> diff --git a/SecurityPkg/Library/AuthVariableLib/AuthService.c
> b/SecurityPkg/Library/AuthVariableLib/AuthService.c
> index 2f60331f2c04..4fb609504db7 100644
> --- a/SecurityPkg/Library/AuthVariableLib/AuthService.c
> +++ b/SecurityPkg/Library/AuthVariableLib/AuthService.c
> @@ -19,12 +19,16 @@
>    to verify the signature.
> 
> 
> 
>  Copyright (c) 2009 - 2019, Intel Corporation. All rights reserved.<BR>
> 
> +Copyright (c) Microsoft Corporation.
> 
>  SPDX-License-Identifier: BSD-2-Clause-Patent
> 
> 
> 
>  **/
> 
> 
> 
>  #include "AuthServiceInternal.h"
> 
> 
> 
> +#include <Protocol/VariablePolicy.h>
> 
> +#include <Library/VariablePolicyLib.h>
> 
> +
> 
>  //
> 
>  // Public Exponent of RSA Key.
> 
>  //
> 
> @@ -217,9 +221,12 @@ NeedPhysicallyPresent(
>    IN     EFI_GUID       *VendorGuid
> 
>    )
> 
>  {
> 
> -  if ((CompareGuid (VendorGuid, &gEfiSecureBootEnableDisableGuid) &&
> (StrCmp (VariableName, EFI_SECURE_BOOT_ENABLE_NAME) == 0))
> 
> -    || (CompareGuid (VendorGuid, &gEfiCustomModeEnableGuid) && (StrCmp
> (VariableName, EFI_CUSTOM_MODE_NAME) == 0))) {
> 
> -    return TRUE;
> 
> +  // If the VariablePolicy engine is disabled, allow deletion of any authenticated
> variables.
> 
> +  if (IsVariablePolicyEnabled()) {
> 
> +    if ((CompareGuid (VendorGuid, &gEfiSecureBootEnableDisableGuid) &&
> (StrCmp (VariableName, EFI_SECURE_BOOT_ENABLE_NAME) == 0))
> 
> +      || (CompareGuid (VendorGuid, &gEfiCustomModeEnableGuid) && (StrCmp
> (VariableName, EFI_CUSTOM_MODE_NAME) == 0))) {
> 
> +      return TRUE;
> 
> +    }
> 
>    }
> 
> 
> 
>    return FALSE;
> 
> @@ -842,7 +849,8 @@ ProcessVariable (
>               &OrgVariableInfo
> 
>               );
> 
> 
> 
> -  if ((!EFI_ERROR (Status)) && IsDeleteAuthVariable (OrgVariableInfo.Attributes,
> Data, DataSize, Attributes) && UserPhysicalPresent()) {
> 
> +  // If the VariablePolicy engine is disabled, allow deletion of any authenticated
> variables.
> 
> +  if ((!EFI_ERROR (Status)) && IsDeleteAuthVariable (OrgVariableInfo.Attributes,
> Data, DataSize, Attributes) && (UserPhysicalPresent()
> || !IsVariablePolicyEnabled())) {
> 
>      //
> 
>      // Allow the delete operation of common authenticated variable(AT or AW) at
> user physical presence.
> 
>      //
> 
> @@ -1920,6 +1928,12 @@ VerifyTimeBasedPayload (
>    PayloadPtr = SigData + SigDataSize;
> 
>    PayloadSize = DataSize - OFFSET_OF_AUTHINFO2_CERT_DATA - (UINTN)
> SigDataSize;
> 
> 
> 
> +  // If the VariablePolicy engine is disabled, allow deletion of any authenticated
> variables.
> 
> +  if (PayloadSize == 0 && (Attributes & EFI_VARIABLE_APPEND_WRITE) == 0
> && !IsVariablePolicyEnabled()) {
> 
> +    VerifyStatus = TRUE;
> 
> +    goto Exit;
> 
> +  }
> 
> +
> 
>    //
> 
>    // Construct a serialization buffer of the values of the VariableName,
> VendorGuid and Attributes
> 
>    // parameters of the SetVariable() call and the TimeStamp component of the
> 
> @@ -2173,8 +2187,12 @@ VerifyTimeBasedPayload (
>  Exit:
> 
> 
> 
>    if (AuthVarType == AuthVarTypePk || AuthVarType == AuthVarTypePriv) {
> 
> -    Pkcs7FreeSigners (TopLevelCert);
> 
> -    Pkcs7FreeSigners (SignerCerts);
> 
> +    if (TopLevelCert != NULL) {
> 
> +        Pkcs7FreeSigners (TopLevelCert);
> 
> +    }
> 
> +    if (SignerCerts != NULL) {
> 
> +        Pkcs7FreeSigners (SignerCerts);
> 
> +    }
> 
>    }
> 
> 
> 
>    if (!VerifyStatus) {
> 
> diff --git a/SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf
> b/SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf
> index 8d4ce14df494..8eadeebcebd7 100644
> --- a/SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf
> +++ b/SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf
> @@ -3,6 +3,7 @@
>  #
> 
>  #  Copyright (c) 2015 - 2016, Intel Corporation. All rights reserved.<BR>
> 
>  #  Copyright (c) 2018, ARM Limited. All rights reserved.<BR>
> 
> +#  Copyright (c) Microsoft Corporation.
> 
>  #
> 
>  #  SPDX-License-Identifier: BSD-2-Clause-Patent
> 
>  #
> 
> @@ -41,6 +42,7 @@ [LibraryClasses]
>    MemoryAllocationLib
> 
>    BaseCryptLib
> 
>    PlatformSecureLib
> 
> +  VariablePolicyLib
> 
> 
> 
>  [Guids]
> 
>    ## CONSUMES            ## Variable:L"SetupMode"
> 
> --
> 2.28.0.windows.1