[edk2-devel] [PATCH v9 11/13] SecurityPkg: Allow VariablePolicy state to delete authenticated variables

[Bret Barkelew]

From: Bret Barkelew <brbarkel at 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 at intel.com>
Cc: Jian J Wang <jian.j.wang at intel.com>
Cc: Chao Zhang <chao.b.zhang at intel.com>
Cc: Bret Barkelew <brbarkel at microsoft.com>
Signed-off-by: Bret Barkelew <brbarkel at microsoft.com>
Reviewed-by: Dandan Bi <dandan.bi at intel.com>
Acked-by: Jian J Wang <jian.j.wang at 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 (#67157): https://edk2.groups.io/g/devel/message/67157
Mute This Topic: https://groups.io/mt/78130781/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-