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


Yao, Jiewen
 

Hi Bret
I have minor comment below. Please let me know your thought.


-----邮件原件-----
发件人: bounce+27952+64723+4905953+8761045@groups.io
<bounce+27952+64723+4905953+8761045@groups.io> 代表 Bret Barkelew
发送时间: 2020年8月28日 13:51
收件人: devel@edk2.groups.io
抄送: Jiewen Yao <jiewen.yao@intel.com>; Jian J Wang
<jian.j.wang@intel.com>; Chao Zhang <chao.b.zhang@intel.com>
主题: [edk2-devel] [PATCH v7 11/14] SecurityPkg: Allow VariablePolicy state
to delete authenticated variables

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>
---
SecurityPkg/Library/AuthVariableLib/AuthService.c | 22
++++++++++++++++----
SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf | 2 ++
2 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/SecurityPkg/Library/AuthVariableLib/AuthService.c
b/SecurityPkg/Library/AuthVariableLib/AuthService.c
index 2f60331f2c04..aca9a5620c28 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;

+ }

}
[Jiewen] Looks good.



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())) {
[Jiewen] Looks good.

//

// Allow the delete operation of common authenticated variable(AT or
AW) at user physical presence.

//

@@ -1960,6 +1968,12 @@ VerifyTimeBasedPayload (


CopyMem (Buffer, PayloadPtr, PayloadSize);



+ // 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;

+ }
[Jiewen] I checked the programming context.
If we are going to skip the check, I feel the GetScratchBuffer() and CopyMem () may be avoided.
Also, I do not find any those data are used at Exit.

How about we move the check just after getting PayloadSize?
//
// Find out the new data payload which follows Pkcs7 SignedData directly.
//
PayloadPtr = SigData + SigDataSize;
PayloadSize = DataSize - OFFSET_OF_AUTHINFO2_CERT_DATA - (UINTN) SigDataSize;
I hope it can make logic clearer.


One more thing is about below action at Exit.
Pkcs7FreeSigners (TopLevelCert);
Pkcs7FreeSigners (SignerCerts);

With new short path, we can come here with NULL point for Pkcs7FreeSigners().
I don't know the result if we pass a NULL pointer according to Pkcs7FreeSigners() API definition.
/**
Wrap function to use free() to free allocated memory for certificates.
If this interface is not supported, then ASSERT().
@param[in] Certs Pointer to the certificates to be freed.
**/
VOID
EFIAPI
Pkcs7FreeSigners (
IN UINT8 *Certs
);

I notice the current openssl version BaseCryptoLib implementation will check NULL and return.
We are safe in the default one. But I am not sure about other implementation.

I recommend we either document NULL pointer behavior in Pkcs7FreeSigners(), or add NULL pointer check at Exit to avoid calling Pkcs7FreeSigners().

With above two update, reviewed-by: Jiewen Yao <Jiewen.yao@intel.com>


+

if (AuthVarType == AuthVarTypePk) {

//

// Verify that the signature has been made with the current Platform
Key (no chaining for PK).

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 (#64723): https://edk2.groups.io/g/devel/message/64723
Mute This Topic: https://groups.io/mt/76468137/4905953
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub
[gaoliming@byosoft.com.cn]
-=-=-=-=-=-=

Join devel@edk2.groups.io to automatically receive all group messages.