Hi Bret
I have minor comment below. Please let me know your thought.
> -----邮件原件-----
> 发件人: bounce+27952+64723+4905953+8761...@groups.io
> <bounce+27952+64723+4905953+8761...@groups.io> 代表 Bret Barkelew
> 发送时间: 2020年8月28日 13:51
> 收件人: devel@edk2.groups.io
> 抄送: Jiewen Yao <jiewen....@intel.com>; Jian J Wang
> <jian.j.w...@intel.com>; Chao Zhang <chao.b.zh...@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....@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>
> ---
> 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....@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+ow...@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub
> [gaolim...@byosoft.com.cn]
> -=-=-=-=-=-=
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#65307): https://edk2.groups.io/g/devel/message/65307
Mute This Topic: https://groups.io/mt/76468137/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
