[edk2] [PATCH] SecurityPkg: Make time based AuthVariable update atomic

[Zhang, Chao B]

System may break during time based AuthVariable update, causing certdb inconsistent. 2 ways are used to ensure update atomic.
1. Delete cert in certdb after variable is deleted
2. Clean up certdb on variable initialization

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Chao Zhang <***@intel.com>
---
SecurityPkg/Library/AuthVariableLib/AuthService.c | 151 ++++++++++++++++++---
.../Library/AuthVariableLib/AuthServiceInternal.h | 16 +++
.../Library/AuthVariableLib/AuthVariableLib.c | 9 ++
3 files changed, 154 insertions(+), 22 deletions(-)

diff --git a/SecurityPkg/Library/AuthVariableLib/AuthService.c b/SecurityPkg/Library/AuthVariableLib/AuthService.c
index b3e8933..aa23c88 100644
--- a/SecurityPkg/Library/AuthVariableLib/AuthService.c
+++ b/SecurityPkg/Library/AuthVariableLib/AuthService.c
@@ -1965,6 +1965,101 @@ InsertCertsToDb (
}

/**
+ Clean up signer's certificates for common authenticated variable
+ by corresponding VariableName and VendorGuid from "certdb".
+ Sytem may break down during Timebased Variable update & certdb update,
+ make them inconsistent, this function is called in AuthVariable Init to ensure
+ consistency
+
+ @retval EFI_NOT_FOUND Fail to find matching certs.
+ @retval EFI_SUCCESS Find matching certs and output parameters.
+
+**/
+EFI_STATUS
+CleanCertsFromDb (
+ VOID
+ ){
+ UINT32 Offset;
+ AUTH_CERT_DB_DATA *Ptr;
+ UINT32 NameSize;
+ UINT32 NodeSize;
+ CHAR16 *VariableName;
+ EFI_STATUS Status;
+ BOOLEAN CertCleaned;
+ UINT8 *Data;
+ UINTN DataSize;
+ UINT8 *AuthVarData;
+ UINTN AuthVarDataSize;
+
+ CertCleaned = FALSE;
+ Status = EFI_SUCCESS;
+
+ //
+ // Get corresponding certificates by VendorGuid and VariableName.
+ //
+ while (CertCleaned) {
+ CertCleaned = FALSE;
+
+ //
+ // Get latest variable "certdb"
+ //
+ Status = AuthServiceInternalFindVariable (
+ EFI_CERT_DB_NAME,
+ &gEfiCertDbGuid,
+ (VOID **) &Data,
+ &DataSize
+ );
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ if ((DataSize == 0) || (Data == NULL)) {
+ ASSERT (FALSE);
+ return EFI_NOT_FOUND;
+ }
+
+ Offset = sizeof (UINT32);
+
+ while (Offset < (UINT32) DataSize) {
+ Ptr = (AUTH_CERT_DB_DATA *) (Data + Offset);
+ //
+ // Check whether VendorGuid matches.
+ //
+ NodeSize = ReadUnaligned32 (&Ptr->CertNodeSize);
+ NameSize = ReadUnaligned32 (&Ptr->NameSize);
+
+ VariableName = AllocateZeroPool((NameSize + 1) * sizeof(CHAR16));
+ if (VariableName == NULL) {
+ return EFI_OUT_OF_RESOURCES;
+ }
+ CopyMem (VariableName, (UINT8 *) Ptr + sizeof (AUTH_CERT_DB_DATA), NameSize * sizeof(CHAR16));
+ //
+ // Find corresponding time auth variable
+ //
+ Status = AuthServiceInternalFindVariable (
+ VariableName,
+ &Ptr->VendorGuid,
+ (VOID **) &AuthVarData,
+ &AuthVarDataSize
+ );
+
+ if (EFI_ERROR(Status)) {
+ Status = DeleteCertsFromDb(VariableName, &Ptr->VendorGuid);
+ CertCleaned = TRUE;
+ DEBUG((EFI_D_INFO, "Recovery!! Cert for Auth Variable %s Guid %g is removed for consistency\n", VariableName, Ptr->VendorGuid));
+ FreePool(VariableName);
+ break;
+ }
+
+ FreePool(VariableName);
+ Offset = Offset + NodeSize;
+ }
+ }
+
+ return Status;
+}
+
+/**
Process variable with EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS set

Caution: This function may receive untrusted input.
@@ -2285,16 +2380,7 @@ VerifyTimeBasedPayload (
goto Exit;
}

- //
- // Delete signer's certificates when delete the common authenticated variable.
- //
- if ((PayloadSize == 0) && (OrgTimeStamp != NULL) && ((Attributes & EFI_VARIABLE_APPEND_WRITE) == 0)) {
- Status = DeleteCertsFromDb (VariableName, VendorGuid);
- if (EFI_ERROR (Status)) {
- VerifyStatus = FALSE;
- goto Exit;
- }
- } else if ((OrgTimeStamp == NULL) && (PayloadSize != 0)) {
+ if ((OrgTimeStamp == NULL) && (PayloadSize != 0)) {
//
// Insert signer's certificates when adding a new common authenticated variable.
//
@@ -2412,21 +2498,42 @@ VerifyTimeBasedPayloadAndUpdate (
return Status;
}

- if ((PayloadSize == 0) && (VarDel != NULL)) {
- *VarDel = TRUE;
- }
-
CertData = (EFI_VARIABLE_AUTHENTICATION_2 *) Data;

//
// Final step: Update/Append Variable if it pass Pkcs7Verify
//
- return AuthServiceInternalUpdateVariableWithTimeStamp (
- VariableName,
- VendorGuid,
- PayloadPtr,
- PayloadSize,
- Attributes,
- &CertData->TimeStamp
- );
+ Status = AuthServiceInternalUpdateVariableWithTimeStamp (
+ VariableName,
+ VendorGuid,
+ PayloadPtr,
+ PayloadSize,
+ Attributes,
+ &CertData->TimeStamp
+ );
+
+ //
+ // Delete signer's certificates when delete the common authenticated variable.
+ //
+ if (EFI_ERROR(Status)
+ && (AuthVarType == AuthVarTypePriv)
+ && (PayloadSize == 0)
+ && !EFI_ERROR(FindStatus)
+ && ((Attributes & EFI_VARIABLE_APPEND_WRITE) == 0)) {
+ Status = DeleteCertsFromDb (VariableName, VendorGuid);
+ }
+
+
+ if ((VarDel != NULL)) {
+ if (!EFI_ERROR(Status)
+ && (PayloadSize == 0)
+ && !EFI_ERROR(FindStatus)
+ && ((Attributes & EFI_VARIABLE_APPEND_WRITE) == 0) ) {
+ *VarDel = TRUE;
+ } else {
+ *VarDel = FALSE;
+ }
+ }
+
+ return Status;
}
diff --git a/SecurityPkg/Library/AuthVariableLib/AuthServiceInternal.h b/SecurityPkg/Library/AuthVariableLib/AuthServiceInternal.h
index 08fff3f..add05c2 100644
--- a/SecurityPkg/Library/AuthVariableLib/AuthServiceInternal.h
+++ b/SecurityPkg/Library/AuthVariableLib/AuthServiceInternal.h
@@ -187,6 +187,22 @@ DeleteCertsFromDb (
);

/**
+ Clean up signer's certificates for common authenticated variable
+ by corresponding VariableName and VendorGuid from "certdb".
+ Sytem may break down during Timebased Variable update & certdb update,
+ make them inconsistent, this function is called in AuthVariable Init to ensure
+ consistency
+
+ @retval EFI_NOT_FOUND Fail to find matching certs.
+ @retval EFI_SUCCESS Find matching certs and output parameters.
+
+**/
+EFI_STATUS
+CleanCertsFromDb (
+ VOID
+ );
+
+/**
Filter out the duplicated EFI_SIGNATURE_DATA from the new data by comparing to the original data.

@param[in] Data Pointer to original EFI_SIGNATURE_LIST.
diff --git a/SecurityPkg/Library/AuthVariableLib/AuthVariableLib.c b/SecurityPkg/Library/AuthVariableLib/AuthVariableLib.c
index 0bb0918..02df309 100644
--- a/SecurityPkg/Library/AuthVariableLib/AuthVariableLib.c
+++ b/SecurityPkg/Library/AuthVariableLib/AuthVariableLib.c
@@ -352,6 +352,15 @@ AuthVariableLibInitialize (
if (EFI_ERROR (Status)) {
return Status;
}
+ } else {
+ //
+ // Clean up Certs to make certDB & Time based auth variable consistent
+ //
+ Status = CleanCertsFromDb();
+ if (EFI_ERROR (Status)) {
+ DEBUG ((EFI_D_INFO, "Clean up CertDB fail! Status %x\n", Status));
+ return Status;
+ }
}

//