Reviewed-by: Jiewen Yao <jiewen....@intel.com> > -----Original Message----- > From: Bret Barkelew <b...@corthon.com> > Sent: Wednesday, September 23, 2020 2:08 PM > To: devel@edk2.groups.io > Cc: Yao, Jiewen <jiewen....@intel.com>; Wang, Jian J <jian.j.w...@intel.com>; > Chao Zhang <chao.b.zh...@intel.com>; Bret Barkelew > <brbar...@microsoft.com>; Bi, Dandan <dandan...@intel.com> > Subject: [PATCH v8 11/14] SecurityPkg: Allow VariablePolicy state to delete > authenticated variables > > From: Bret Barkelew <brbar...@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....@intel.com> > Cc: Jian J Wang <jian.j.w...@intel.com> > Cc: Chao Zhang <chao.b.zh...@intel.com> > Cc: Bret Barkelew <brbar...@microsoft.com> > Signed-off-by: Bret Barkelew <brbar...@microsoft.com> > Reviewed-by: Dandan Bi <dandan...@intel.com> > Acked-by: Jian J Wang <jian.j.w...@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
-=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#65509): https://edk2.groups.io/g/devel/message/65509 Mute This Topic: https://groups.io/mt/77029629/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
