From 64b6a3ff4a2ba45681e79c911d7061c08f1a139c Mon Sep 17 00:00:00 2001 From: Chao Zhang Date: Fri, 10 Jul 2015 06:20:04 +0000 Subject: [PATCH] SecurityPkg: Make time based AuthVariable update atomic 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 Reviewed-by: Yao Jiewen Reviewed-by: Star Zeng git-svn-id: https://svn.code.sf.net/p/edk2/code/trunk/edk2@17919 6f19259b-4bc3-4df7-8a09-765794883524 --- .../Library/AuthVariableLib/AuthService.c | 175 +++++++++++++++--- .../AuthVariableLib/AuthServiceInternal.h | 16 ++ .../Library/AuthVariableLib/AuthVariableLib.c | 9 + 3 files changed, 170 insertions(+), 30 deletions(-) diff --git a/SecurityPkg/Library/AuthVariableLib/AuthService.c b/SecurityPkg/Library/AuthVariableLib/AuthService.c index b3e8933b40..8805f54f9d 100644 --- a/SecurityPkg/Library/AuthVariableLib/AuthService.c +++ b/SecurityPkg/Library/AuthVariableLib/AuthService.c @@ -1205,18 +1205,17 @@ ProcessVariable ( // // Allow the delete operation of common authenticated variable at user physical presence. // - if ((Attributes & EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS) != 0) { + Status = AuthServiceInternalUpdateVariable ( + VariableName, + VendorGuid, + NULL, + 0, + 0 + ); + if (!EFI_ERROR (Status) && ((Attributes & EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS) != 0)) { Status = DeleteCertsFromDb (VariableName, VendorGuid); } - if (!EFI_ERROR (Status)) { - Status = AuthServiceInternalUpdateVariable ( - VariableName, - VendorGuid, - NULL, - 0, - 0 - ); - } + return Status; } @@ -1964,6 +1963,109 @@ InsertCertsToDb ( return Status; } +/** + 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; + EFI_GUID AuthVarGuid; + + Status = EFI_SUCCESS; + + // + // Get corresponding certificates by VendorGuid and VariableName. + // + do { + 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); + + // + // Get VarName tailed with '\0' + // + 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)); + // + // Keep VarGuid aligned + // + CopyMem (&AuthVarGuid, &Ptr->VendorGuid, sizeof(EFI_GUID)); + + // + // Find corresponding time auth variable + // + Status = AuthServiceInternalFindVariable ( + VariableName, + &AuthVarGuid, + (VOID **) &AuthVarData, + &AuthVarDataSize + ); + + if (EFI_ERROR(Status)) { + Status = DeleteCertsFromDb(VariableName, &AuthVarGuid); + CertCleaned = TRUE; + DEBUG((EFI_D_INFO, "Recovery!! Cert for Auth Variable %s Guid %g is removed for consistency\n", VariableName, &AuthVarGuid)); + FreePool(VariableName); + break; + } + + FreePool(VariableName); + Offset = Offset + NodeSize; + } + } while (CertCleaned); + + return Status; +} + /** Process variable with EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS set @@ -2285,16 +2387,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. // @@ -2389,6 +2482,7 @@ VerifyTimeBasedPayloadAndUpdate ( UINTN PayloadSize; EFI_VARIABLE_AUTHENTICATION_2 *CertData; AUTH_VARIABLE_INFO OrgVariableInfo; + BOOLEAN IsDel; ZeroMem (&OrgVariableInfo, sizeof (OrgVariableInfo)); FindStatus = mAuthVarLibContextIn->FindVariable ( @@ -2412,8 +2506,12 @@ VerifyTimeBasedPayloadAndUpdate ( return Status; } - if ((PayloadSize == 0) && (VarDel != NULL)) { - *VarDel = TRUE; + if (!EFI_ERROR(FindStatus) + && (PayloadSize == 0) + && ((Attributes & EFI_VARIABLE_APPEND_WRITE) == 0)) { + IsDel = TRUE; + } else { + IsDel = FALSE; } CertData = (EFI_VARIABLE_AUTHENTICATION_2 *) Data; @@ -2421,12 +2519,29 @@ VerifyTimeBasedPayloadAndUpdate ( // // 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 (IsDel && AuthVarType == AuthVarTypePriv && !EFI_ERROR(Status) ) { + Status = DeleteCertsFromDb (VariableName, VendorGuid); + } + + if (VarDel != NULL) { + if (IsDel && !EFI_ERROR(Status)) { + *VarDel = TRUE; + } else { + *VarDel = FALSE; + } + } + + return Status; } diff --git a/SecurityPkg/Library/AuthVariableLib/AuthServiceInternal.h b/SecurityPkg/Library/AuthVariableLib/AuthServiceInternal.h index 08fff3f552..add05c21cc 100644 --- a/SecurityPkg/Library/AuthVariableLib/AuthServiceInternal.h +++ b/SecurityPkg/Library/AuthVariableLib/AuthServiceInternal.h @@ -186,6 +186,22 @@ DeleteCertsFromDb ( IN EFI_GUID *VendorGuid ); +/** + 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. diff --git a/SecurityPkg/Library/AuthVariableLib/AuthVariableLib.c b/SecurityPkg/Library/AuthVariableLib/AuthVariableLib.c index 0bb09189ee..02df309802 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; + } } // -- 2.39.2