Topics

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


Bret Barkelew
 

https://bugzilla.tianocore.org/show_bug.cgi?id=3D2522

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@...>
Cc: Jian J Wang <jian.j.wang@...>
Cc: Chao Zhang <chao.b.zhang@...>
Cc: Bret Barkelew <brbarkel@...>
Signed-off-by: Bret Barkelew <brbarkel@...>
---
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/SecurityPk=
g/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.=0D
=0D
Copyright (c) 2009 - 2019, Intel Corporation. All rights reserved.<BR>=0D
+Copyright (c) Microsoft Corporation.=0D
SPDX-License-Identifier: BSD-2-Clause-Patent=0D
=0D
**/=0D
=0D
#include "AuthServiceInternal.h"=0D
=0D
+#include <Protocol/VariablePolicy.h>=0D
+#include <Library/VariablePolicyLib.h>=0D
+=0D
//=0D
// Public Exponent of RSA Key.=0D
//=0D
@@ -217,9 +221,12 @@ NeedPhysicallyPresent(
IN EFI_GUID *VendorGuid=0D
)=0D
{=0D
- if ((CompareGuid (VendorGuid, &gEfiSecureBootEnableDisableGuid) && (StrC=
mp (VariableName, EFI_SECURE_BOOT_ENABLE_NAME) =3D=3D 0))=0D
- || (CompareGuid (VendorGuid, &gEfiCustomModeEnableGuid) && (StrCmp (Va=
riableName, EFI_CUSTOM_MODE_NAME) =3D=3D 0))) {=0D
- return TRUE;=0D
+ // If the VariablePolicy engine is disabled, allow deletion of any authe=
nticated variables.=0D
+ if (IsVariablePolicyEnabled()) {=0D
+ if ((CompareGuid (VendorGuid, &gEfiSecureBootEnableDisableGuid) && (St=
rCmp (VariableName, EFI_SECURE_BOOT_ENABLE_NAME) =3D=3D 0))=0D
+ || (CompareGuid (VendorGuid, &gEfiCustomModeEnableGuid) && (StrCmp (=
VariableName, EFI_CUSTOM_MODE_NAME) =3D=3D 0))) {=0D
+ return TRUE;=0D
+ }=0D
}=0D
=0D
return FALSE;=0D
@@ -842,7 +849,8 @@ ProcessVariable (
&OrgVariableInfo=0D
);=0D
=0D
- if ((!EFI_ERROR (Status)) && IsDeleteAuthVariable (OrgVariableInfo.Attri=
butes, Data, DataSize, Attributes) && UserPhysicalPresent()) {=0D
+ // If the VariablePolicy engine is disabled, allow deletion of any authe=
nticated variables.=0D
+ if ((!EFI_ERROR (Status)) && IsDeleteAuthVariable (OrgVariableInfo.Attri=
butes, Data, DataSize, Attributes) && (UserPhysicalPresent() || !IsVariable=
PolicyEnabled())) {=0D
//=0D
// Allow the delete operation of common authenticated variable(AT or A=
W) at user physical presence.=0D
//=0D
@@ -1960,6 +1968,12 @@ VerifyTimeBasedPayload (
=0D
CopyMem (Buffer, PayloadPtr, PayloadSize);=0D
=0D
+ // If the VariablePolicy engine is disabled, allow deletion of any authe=
nticated variables.=0D
+ if (PayloadSize =3D=3D 0 && (Attributes & EFI_VARIABLE_APPEND_WRITE) =3D=
=3D 0 && !IsVariablePolicyEnabled()) {=0D
+ VerifyStatus =3D TRUE;=0D
+ goto Exit;=0D
+ }=0D
+=0D
if (AuthVarType =3D=3D AuthVarTypePk) {=0D
//=0D
// Verify that the signature has been made with the current Platform K=
ey (no chaining for PK).=0D
diff --git a/SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf b/Secu=
rityPkg/Library/AuthVariableLib/AuthVariableLib.inf
index 8d4ce14df494..8eadeebcebd7 100644
--- a/SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf
+++ b/SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf
@@ -3,6 +3,7 @@
#=0D
# Copyright (c) 2015 - 2016, Intel Corporation. All rights reserved.<BR>=
=0D
# Copyright (c) 2018, ARM Limited. All rights reserved.<BR>=0D
+# Copyright (c) Microsoft Corporation.=0D
#=0D
# SPDX-License-Identifier: BSD-2-Clause-Patent=0D
#=0D
@@ -41,6 +42,7 @@ [LibraryClasses]
MemoryAllocationLib=0D
BaseCryptLib=0D
PlatformSecureLib=0D
+ VariablePolicyLib=0D
=0D
[Guids]=0D
## CONSUMES ## Variable:L"SetupMode"=0D
--=20
2.28.0.windows.1


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@...>; Jian J Wang
<jian.j.wang@...>; Chao Zhang <chao.b.zhang@...>
主题: [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@...>
Cc: Jian J Wang <jian.j.wang@...>
Cc: Chao Zhang <chao.b.zhang@...>
Cc: Bret Barkelew <brbarkel@...>
Signed-off-by: Bret Barkelew <brbarkel@...>
---
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@...>


+

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@...]
-=-=-=-=-=-=


Bret Barkelew
 

These are both great points and I’ll make both changes.

 

Thanks!

 

- Bret

 

From: Yao, Jiewen via groups.io
Sent: Tuesday, September 15, 2020 7:52 PM
To: Yao, Jiewen; gaoliming; devel@edk2.groups.io; Wang, Jian J; bret@...; Bi, Dandan
Cc: Wu, Hao A; liming.gao; Justen, Jordan L; 'Laszlo Ersek'; 'Ard Biesheuvel'; 'Andrew Fish'; Ni, Ray
Subject: [EXTERNAL] Re: [edk2-devel] [PATCH v7 11/14] SecurityPkg: Allow VariablePolicy state to delete authenticated variables

 

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
> 时间: 2020828 13:51
> 收件人: devel@edk2.groups.io
> 抄送: Jiewen Yao <jiewen.yao@...>; Jian J Wang
> <jian.j.wang@...>; Chao Zhang <chao.b.zhang@...>
> : [edk2-devel] [PATCH v7 11/14] SecurityPkg: Allow VariablePolicy state
> to delete authenticated variables
>
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2522&amp;data=02%7C01%7Cbret.barkelew%40microsoft.com%7C8f1aee9f15a14900a3d508d859eb7222%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637358215630854973&amp;sdata=JyDdXC9JRdBdqfmhAc6fI4R5xhh70wcD0NnNIgwfF3w%3D&amp;reserved=0
>
> 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@...>
> Cc: Jian J Wang <jian.j.wang@...>
> Cc: Chao Zhang <chao.b.zhang@...>
> Cc: Bret Barkelew <brbarkel@...>
> Signed-off-by: Bret Barkelew <brbarkel@...>
> ---
>  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@...>


> +
>
>    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://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2Fmessage%2F64723&amp;data=02%7C01%7Cbret.barkelew%40microsoft.com%7C8f1aee9f15a14900a3d508d859eb7222%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637358215630864971&amp;sdata=JpxigghyweSgGoxS3lF6P6giUqI6WkIkDfX6%2FTqcog4%3D&amp;reserved=0
> Mute This Topic: https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgroups.io%2Fmt%2F76468137%2F4905953&amp;data=02%7C01%7Cbret.barkelew%40microsoft.com%7C8f1aee9f15a14900a3d508d859eb7222%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637358215630864971&amp;sdata=FUAT8j4ic9AokVjXWRNAgC2GRTKg581rra%2BHcnJ%2F%2BBc%3D&amp;reserved=0
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2Funsub&amp;data=02%7C01%7Cbret.barkelew%40microsoft.com%7C8f1aee9f15a14900a3d508d859eb7222%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637358215630864971&amp;sdata=kTY8I2s1WrJMc0r43wy0d1LwHJfnVe3WuYnx%2BCMuT4k%3D&amp;reserved=0
> [gaoliming@...]
> -=-=-=-=-=-=
>
>