X-Git-Url: https://git.proxmox.com/?p=mirror_edk2.git;a=blobdiff_plain;f=SecurityPkg%2FVariableAuthenticated%2FSecureBootConfigDxe%2FSecureBootConfigImpl.c;h=2eaf24633db15d880c95c23ae493b12ed551de49;hp=6f587291e226ed901b58b438fbc5c3ba8923a660;hb=9d9b8b77bc54718a4debfea3a7a3dd59bba16840;hpb=c035e37335ae43229d7e68de74a65f2c01ebc0af

diff --git a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c
index 6f587291e2..2eaf24633d 100644
--- a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c
+++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c
@@ -96,6 +96,31 @@ CHAR16* mSupportX509Suffix = L"*.cer/der/crt";
 
 SECUREBOOT_CONFIG_PRIVATE_DATA  *gSecureBootPrivateData = NULL;
 
+/**
+  This code cleans up enrolled file by closing file & free related resources attached to
+  enrolled file.
+
+  @param[in] FileContext            FileContext cached in SecureBootConfig driver
+
+**/
+VOID
+CloseEnrolledFile(
+  IN SECUREBOOT_FILE_CONTEXT *FileContext
+)
+{
+  if (FileContext->FHandle != NULL) {
+    CloseFile (FileContext->FHandle);
+    FileContext->FHandle = NULL;
+  }
+
+  if (FileContext->FileName != NULL){
+    FreePool(FileContext->FileName);
+    FileContext->FileName = NULL;
+  }
+  FileContext->FileType = UNKNOWN_FILE_TYPE;
+
+}
+
 /**
   This code checks if the FileSuffix is one of the possible DER-encoded certificate suffix.
 
@@ -119,6 +144,61 @@ IsDerEncodeCertificate (
   return FALSE;
 }
 
+/**
+  This code checks if the file content complies with EFI_VARIABLE_AUTHENTICATION_2 format
+The function reads file content but won't open/close given FileHandle.
+
+  @param[in] FileHandle            The FileHandle to be checked
+
+  @retval    TRUE            The content is EFI_VARIABLE_AUTHENTICATION_2 format.
+  @retval    FALSE          The content is NOT a EFI_VARIABLE_AUTHENTICATION_2 format.
+
+**/
+BOOLEAN
+IsAuthentication2Format (
+  IN   EFI_FILE_HANDLE    FileHandle
+)
+{
+  EFI_STATUS                     Status;
+  EFI_VARIABLE_AUTHENTICATION_2  *Auth2;
+  BOOLEAN                        IsAuth2Format;
+
+  IsAuth2Format = FALSE;
+
+  //
+  // Read the whole file content
+  //
+  Status = ReadFileContent(
+             FileHandle,
+             (VOID **) &mImageBase,
+             &mImageSize,
+             0
+             );
+  if (EFI_ERROR (Status)) {
+    goto ON_EXIT;
+  }
+
+  Auth2 = (EFI_VARIABLE_AUTHENTICATION_2 *)mImageBase;
+  if (Auth2->AuthInfo.Hdr.wCertificateType != WIN_CERT_TYPE_EFI_GUID) {
+    goto ON_EXIT;
+  }
+
+  if (CompareGuid(&gEfiCertPkcs7Guid, &Auth2->AuthInfo.CertType)) {
+    IsAuth2Format = TRUE;
+  }
+
+ON_EXIT:
+  //
+  // Do not close File. simply check file content
+  //
+  if (mImageBase != NULL) {
+    FreePool (mImageBase);
+    mImageBase = NULL;
+  }
+
+  return IsAuth2Format;
+}
+
 /**
   Set Secure Boot option into variable space.
 
@@ -474,10 +554,7 @@ ON_EXIT:
     FreePool(PkCert);
   }
 
-  if (Private->FileContext->FHandle != NULL) {
-    CloseFile (Private->FileContext->FHandle);
-    Private->FileContext->FHandle = NULL;
-  }
+  CloseEnrolledFile(Private->FileContext);
 
   return Status;
 }
@@ -654,13 +731,7 @@ EnrollRsa2048ToKek (
 
 ON_EXIT:
 
-  CloseFile (Private->FileContext->FHandle);
-  Private->FileContext->FHandle = NULL;
-
-  if (Private->FileContext->FileName != NULL){
-    FreePool(Private->FileContext->FileName);
-    Private->FileContext->FileName = NULL;
-  }
+  CloseEnrolledFile(Private->FileContext);
 
   if (Private->SignatureGUID != NULL) {
     FreePool (Private->SignatureGUID);
@@ -781,13 +852,7 @@ EnrollX509ToKek (
 
 ON_EXIT:
 
-  CloseFile (Private->FileContext->FHandle);
-  if (Private->FileContext->FileName != NULL){
-    FreePool(Private->FileContext->FileName);
-    Private->FileContext->FileName = NULL;
-  }
-
-  Private->FileContext->FHandle = NULL;
+  CloseEnrolledFile(Private->FileContext);
 
   if (Private->SignatureGUID != NULL) {
     FreePool (Private->SignatureGUID);
@@ -821,7 +886,7 @@ EnrollKeyExchangeKey (
   EFI_STATUS  Status;
   UINTN       NameLength;
 
-  if ((Private->FileContext->FileName == NULL) || (Private->SignatureGUID == NULL)) {
+  if ((Private->FileContext->FHandle == NULL) || (Private->FileContext->FileName == NULL) || (Private->SignatureGUID == NULL)) {
     return EFI_INVALID_PARAMETER;
   }
 
@@ -844,6 +909,11 @@ EnrollKeyExchangeKey (
   } else if (CompareMem (FilePostFix, L".pbk",4) == 0) {
     return EnrollRsa2048ToKek (Private);
   } else {
+    //
+    // File type is wrong, simply close it
+    //
+    CloseEnrolledFile(Private->FileContext);
+
     return EFI_INVALID_PARAMETER;
   }
 }
@@ -955,13 +1025,7 @@ EnrollX509toSigDB (
 
 ON_EXIT:
 
-  CloseFile (Private->FileContext->FHandle);
-  if (Private->FileContext->FileName != NULL){
-    FreePool(Private->FileContext->FileName);
-    Private->FileContext->FileName = NULL;
-  }
-
-  Private->FileContext->FHandle = NULL;
+  CloseEnrolledFile(Private->FileContext);
 
   if (Private->SignatureGUID != NULL) {
     FreePool (Private->SignatureGUID);
@@ -1519,13 +1583,8 @@ EnrollX509HashtoSigDB (
   }
 
 ON_EXIT:
-  CloseFile (Private->FileContext->FHandle);
-  if (Private->FileContext->FileName != NULL){
-    FreePool(Private->FileContext->FileName);
-    Private->FileContext->FileName = NULL;
-  }
 
-  Private->FileContext->FHandle = NULL;
+  CloseEnrolledFile(Private->FileContext);
 
   if (Private->SignatureGUID != NULL) {
     FreePool (Private->SignatureGUID);
@@ -1837,12 +1896,12 @@ HashPeImage (
     //
     // Use PE32 offset.
     //
-    HashSize = (UINTN) ((UINT8 *) (&mNtHeader.Pe32->OptionalHeader.CheckSum) - HashBase);
+    HashSize = (UINTN) (&mNtHeader.Pe32->OptionalHeader.CheckSum) - (UINTN) HashBase;
   } else {
     //
     // Use PE32+ offset.
     //
-    HashSize = (UINTN) ((UINT8 *) (&mNtHeader.Pe32Plus->OptionalHeader.CheckSum) - HashBase);
+    HashSize = (UINTN) (&mNtHeader.Pe32Plus->OptionalHeader.CheckSum) - (UINTN) HashBase;
   }
 
   Status  = mHash[HashAlg].HashUpdate(HashCtx, HashBase, HashSize);
@@ -1859,13 +1918,13 @@ HashPeImage (
     // Use PE32 offset.
     //
     HashBase = (UINT8 *) &mNtHeader.Pe32->OptionalHeader.CheckSum + sizeof (UINT32);
-    HashSize = (UINTN) ((UINT8 *) (&mNtHeader.Pe32->OptionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_SECURITY]) - HashBase);
+    HashSize = (UINTN) (&mNtHeader.Pe32->OptionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_SECURITY]) - (UINTN) HashBase;
   } else {
     //
     // Use PE32+ offset.
     //
     HashBase = (UINT8 *) &mNtHeader.Pe32Plus->OptionalHeader.CheckSum + sizeof (UINT32);
-    HashSize = (UINTN) ((UINT8 *) (&mNtHeader.Pe32Plus->OptionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_SECURITY]) - HashBase);
+    HashSize = (UINTN) (&mNtHeader.Pe32Plus->OptionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_SECURITY]) - (UINTN) HashBase;
   }
 
   Status  = mHash[HashAlg].HashUpdate(HashCtx, HashBase, HashSize);
@@ -1881,13 +1940,13 @@ HashPeImage (
     // Use PE32 offset
     //
     HashBase = (UINT8 *) &mNtHeader.Pe32->OptionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_SECURITY + 1];
-    HashSize = mNtHeader.Pe32->OptionalHeader.SizeOfHeaders - (UINTN) ((UINT8 *) (&mNtHeader.Pe32->OptionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_SECURITY + 1]) - mImageBase);
+    HashSize = mNtHeader.Pe32->OptionalHeader.SizeOfHeaders - ((UINTN) (&mNtHeader.Pe32->OptionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_SECURITY + 1]) - (UINTN) mImageBase);
   } else {
     //
     // Use PE32+ offset.
     //
     HashBase = (UINT8 *) &mNtHeader.Pe32Plus->OptionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_SECURITY + 1];
-    HashSize = mNtHeader.Pe32Plus->OptionalHeader.SizeOfHeaders - (UINTN) ((UINT8 *) (&mNtHeader.Pe32Plus->OptionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_SECURITY + 1]) - mImageBase);
+    HashSize = mNtHeader.Pe32Plus->OptionalHeader.SizeOfHeaders - ((UINTN) (&mNtHeader.Pe32Plus->OptionalHeader.DataDirectory[EFI_IMAGE_DIRECTORY_ENTRY_SECURITY + 1]) - (UINTN) mImageBase);
   }
 
   Status  = mHash[HashAlg].HashUpdate(HashCtx, HashBase, HashSize);
@@ -2066,6 +2125,107 @@ HashPeImageByType (
   return EFI_SUCCESS;
 }
 
+/**
+  Enroll a new executable's signature into Signature Database.
+
+  @param[in] PrivateData     The module's private data.
+  @param[in] VariableName    Variable name of signature database, must be
+                             EFI_IMAGE_SECURITY_DATABASE, EFI_IMAGE_SECURITY_DATABASE1
+                             or EFI_IMAGE_SECURITY_DATABASE2.
+
+  @retval   EFI_SUCCESS            New signature is enrolled successfully.
+  @retval   EFI_INVALID_PARAMETER  The parameter is invalid.
+  @retval   EFI_UNSUPPORTED        Unsupported command.
+  @retval   EFI_OUT_OF_RESOURCES   Could not allocate needed resources.
+
+**/
+EFI_STATUS
+EnrollAuthentication2Descriptor (
+  IN SECUREBOOT_CONFIG_PRIVATE_DATA *Private,
+  IN CHAR16                         *VariableName
+  )
+{
+  EFI_STATUS                        Status;
+  VOID                              *Data;
+  UINTN                             DataSize;
+  UINT32                            Attr;
+
+  Data = NULL;
+
+  //
+  // DBT only support DER-X509 Cert Enrollment
+  //
+  if (StrCmp (VariableName, EFI_IMAGE_SECURITY_DATABASE2) == 0) {
+    return EFI_UNSUPPORTED;
+  }
+
+  //
+  // Read the whole file content
+  //
+  Status = ReadFileContent(
+             Private->FileContext->FHandle,
+             (VOID **) &mImageBase,
+             &mImageSize,
+             0
+             );
+  if (EFI_ERROR (Status)) {
+    goto ON_EXIT;
+  }
+  ASSERT (mImageBase != NULL);
+
+  Attr = EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_RUNTIME_ACCESS
+         | EFI_VARIABLE_BOOTSERVICE_ACCESS | EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS;
+
+  //
+  // Check if SigDB variable has been already existed.
+  // If true, use EFI_VARIABLE_APPEND_WRITE attribute to append the
+  // new signature data to original variable
+  //
+  DataSize = 0;
+  Status = gRT->GetVariable(
+                  VariableName,
+                  &gEfiImageSecurityDatabaseGuid,
+                  NULL,
+                  &DataSize,
+                  NULL
+                  );
+  if (Status == EFI_BUFFER_TOO_SMALL) {
+    Attr |= EFI_VARIABLE_APPEND_WRITE;
+  } else if (Status != EFI_NOT_FOUND) {
+    goto ON_EXIT;
+  }
+
+  //
+  // Diretly set AUTHENTICATION_2 data to SetVariable
+  //
+  Status = gRT->SetVariable(
+                  VariableName,
+                  &gEfiImageSecurityDatabaseGuid,
+                  Attr,
+                  mImageSize,
+                  mImageBase
+                  );
+
+  DEBUG((DEBUG_INFO, "Enroll AUTH_2 data to Var:%s Status: %x\n", VariableName, Status));
+
+ON_EXIT:
+
+  CloseEnrolledFile(Private->FileContext);
+
+  if (Data != NULL) {
+    FreePool (Data);
+  }
+
+  if (mImageBase != NULL) {
+    FreePool (mImageBase);
+    mImageBase = NULL;
+  }
+
+  return Status;
+
+}
+
+
 /**
   Enroll a new executable's signature into Signature Database.
 
@@ -2235,13 +2395,7 @@ EnrollImageSignatureToSigDB (
 
 ON_EXIT:
 
-  CloseFile (Private->FileContext->FHandle);
-  Private->FileContext->FHandle = NULL;
-
-  if (Private->FileContext->FileName != NULL){
-    FreePool(Private->FileContext->FileName);
-    Private->FileContext->FileName = NULL;
-  }
+  CloseEnrolledFile(Private->FileContext);
 
   if (Private->SignatureGUID != NULL) {
     FreePool (Private->SignatureGUID);
@@ -2305,9 +2459,11 @@ EnrollSignatureDatabase (
     // Supports DER-encoded X509 certificate.
     //
     return EnrollX509toSigDB (Private, VariableName);
+  } else if (IsAuthentication2Format(Private->FileContext->FHandle)){
+    return EnrollAuthentication2Descriptor(Private, VariableName);
+  } else {
+    return EnrollImageSignatureToSigDB (Private, VariableName);
   }
-
-  return EnrollImageSignatureToSigDB (Private, VariableName);
 }
 
 /**
@@ -2936,11 +3092,13 @@ UpdateSecureBootString(
 /**
   This function extracts configuration from variable.
 
+  @param[in]       Private      Point to SecureBoot configuration driver private data.
   @param[in, out]  ConfigData   Point to SecureBoot configuration private data.
 
 **/
 VOID
 SecureBootExtractConfigFromVariable (
+  IN SECUREBOOT_CONFIG_PRIVATE_DATA  *Private,
   IN OUT SECUREBOOT_CONFIGURATION    *ConfigData
   )
 {
@@ -2965,7 +3123,11 @@ SecureBootExtractConfigFromVariable (
   ConfigData->RevocationTime.Hour   = CurrTime.Hour;
   ConfigData->RevocationTime.Minute = CurrTime.Minute;
   ConfigData->RevocationTime.Second = 0;
-
+  if (Private->FileContext->FHandle != NULL) {
+    ConfigData->FileEnrollType = Private->FileContext->FileType;
+  } else {
+    ConfigData->FileEnrollType = UNKNOWN_FILE_TYPE;
+  }
 
   //
   // If it is Physical Presence User, set the PhysicalPresent to true.
@@ -3088,10 +3250,12 @@ SecureBootExtractConfig (
     return EFI_NOT_FOUND;
   }
 
+  ZeroMem(&Configuration, sizeof(SECUREBOOT_CONFIGURATION));
+
   //
   // Get Configuration from Variable.
   //
-  SecureBootExtractConfigFromVariable (&Configuration);
+  SecureBootExtractConfigFromVariable (PrivateData, &Configuration);
 
   BufferSize = sizeof (SECUREBOOT_CONFIGURATION);
   ConfigRequest = Request;
@@ -3166,9 +3330,10 @@ SecureBootRouteConfig (
        OUT EFI_STRING                          *Progress
   )
 {
-  SECUREBOOT_CONFIGURATION   IfrNvData;
-  UINTN                      BufferSize;
-  EFI_STATUS                 Status;
+  SECUREBOOT_CONFIGURATION          IfrNvData;
+  UINTN                             BufferSize;
+  SECUREBOOT_CONFIG_PRIVATE_DATA    *PrivateData;
+  EFI_STATUS                        Status;
 
   if (Configuration == NULL || Progress == NULL) {
     return EFI_INVALID_PARAMETER;
@@ -3179,10 +3344,12 @@ SecureBootRouteConfig (
     return EFI_NOT_FOUND;
   }
 
+  PrivateData = SECUREBOOT_CONFIG_PRIVATE_FROM_THIS (This);
+
   //
   // Get Configuration from Variable.
   //
-  SecureBootExtractConfigFromVariable (&IfrNvData);
+  SecureBootExtractConfigFromVariable (PrivateData, &IfrNvData);
 
   //
   // Map the Configuration to the configuration block.
@@ -3248,6 +3415,7 @@ SecureBootCallback (
 {
   EFI_INPUT_KEY                   Key;
   EFI_STATUS                      Status;
+  RETURN_STATUS                   RStatus;
   SECUREBOOT_CONFIG_PRIVATE_DATA  *Private;
   UINTN                           BufferSize;
   SECUREBOOT_CONFIGURATION        *IfrNvData;
@@ -3258,6 +3426,9 @@ SecureBootCallback (
   UINT8                           *SetupMode;
   CHAR16                          PromptString[100];
   EFI_DEVICE_PATH_PROTOCOL        *File;
+  UINTN                           NameLength;
+  UINT16                          *FilePostFix;
+  SECUREBOOT_CONFIG_PRIVATE_DATA  *PrivateData;
 
   Status           = EFI_SUCCESS;
   SecureBootEnable = NULL;
@@ -3290,8 +3461,20 @@ SecureBootCallback (
       // Update secure boot strings when opening this form
       //
       Status = UpdateSecureBootString(Private);
-      SecureBootExtractConfigFromVariable (IfrNvData);
+      SecureBootExtractConfigFromVariable (Private, IfrNvData);
       mIsEnterSecureBootForm = TRUE;
+    } else {
+      //
+      // When entering SecureBoot OPTION Form
+      // always close opened file & free resource
+      //
+      if ((QuestionId == KEY_SECURE_BOOT_PK_OPTION) ||
+          (QuestionId == KEY_SECURE_BOOT_KEK_OPTION) ||
+          (QuestionId == KEY_SECURE_BOOT_DB_OPTION) ||
+          (QuestionId == KEY_SECURE_BOOT_DBX_OPTION) ||
+          (QuestionId == KEY_SECURE_BOOT_DBT_OPTION)) {
+        CloseEnrolledFile(Private->FileContext);
+      }
     }
     goto EXIT;
   }
@@ -3345,6 +3528,7 @@ SecureBootCallback (
     case KEY_SECURE_BOOT_DB_OPTION:
     case KEY_SECURE_BOOT_DBX_OPTION:
     case KEY_SECURE_BOOT_DBT_OPTION:
+      PrivateData = SECUREBOOT_CONFIG_PRIVATE_FROM_THIS (This);
       //
       // Clear Signature GUID.
       //
@@ -3356,6 +3540,11 @@ SecureBootCallback (
         }
       }
 
+      //
+      // Cleanup VFRData once leaving PK/KEK/DB/DBX/DBT enroll/delete page
+      //
+      SecureBootExtractConfigFromVariable (PrivateData, IfrNvData);
+
       if (QuestionId == KEY_SECURE_BOOT_DB_OPTION) {
         LabelId = SECUREBOOT_ENROLL_SIGNATURE_TO_DB;
       } else if (QuestionId == KEY_SECURE_BOOT_DBX_OPTION) {
@@ -3393,6 +3582,38 @@ SecureBootCallback (
 
     case SECUREBOOT_ENROLL_SIGNATURE_TO_DBX:
       ChooseFile (NULL, NULL, UpdateDBXFromFile, &File);
+
+      if (Private->FileContext->FHandle != NULL) {
+        //
+        // Parse the file's postfix.
+        //
+        NameLength = StrLen (Private->FileContext->FileName);
+        if (NameLength <= 4) {
+          return FALSE;
+        }
+        FilePostFix = Private->FileContext->FileName + NameLength - 4;
+
+        if (IsDerEncodeCertificate (FilePostFix)) {
+          //
+          // Supports DER-encoded X509 certificate.
+          //
+          IfrNvData->FileEnrollType = X509_CERT_FILE_TYPE;
+        } else if (IsAuthentication2Format(Private->FileContext->FHandle)){
+          IfrNvData->FileEnrollType = AUTHENTICATION_2_FILE_TYPE;
+        } else {
+          IfrNvData->FileEnrollType = PE_IMAGE_FILE_TYPE;
+        }
+        Private->FileContext->FileType = IfrNvData->FileEnrollType;
+
+        //
+        // Clean up Certificate Format if File type is not X509 DER
+        //
+        if (IfrNvData->FileEnrollType != X509_CERT_FILE_TYPE) {
+          IfrNvData->CertificateFormat = HASHALG_RAW;
+        }
+        DEBUG((DEBUG_ERROR, "IfrNvData->FileEnrollType %d\n", Private->FileContext->FileType));
+      }
+
       break;
 
     case SECUREBOOT_ENROLL_SIGNATURE_TO_DBT:
@@ -3502,7 +3723,12 @@ SecureBootCallback (
           L"Enrollment failed! Same certificate had already been in the dbx!",
           NULL
           );
-          break;
+
+        //
+        // Cert already exists in DBX. Close opened file before exit.
+        //
+        CloseEnrolledFile(Private->FileContext);
+        break;
       }
 
       if ((IfrNvData != NULL) && (IfrNvData->CertificateFormat < HASHALG_MAX)) {
@@ -3513,6 +3739,7 @@ SecureBootCallback (
                    &IfrNvData->RevocationTime,
                    IfrNvData->AlwaysRevocation
                    );
+        IfrNvData->CertificateFormat = HASHALG_RAW;
       } else {
         Status = EnrollSignatureDatabase (Private, EFI_IMAGE_SECURITY_DATABASE1);
       }
@@ -3521,7 +3748,7 @@ SecureBootCallback (
           EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE,
           &Key,
           L"ERROR: Unsupported file type!",
-          L"Only supports DER-encoded X509 certificate and executable EFI image",
+          L"Only supports DER-encoded X509 certificate, AUTH_2 format data & executable EFI image",
           NULL
           );
       }
@@ -3602,14 +3829,7 @@ SecureBootCallback (
     case KEY_VALUE_NO_SAVE_AND_EXIT_DB:
     case KEY_VALUE_NO_SAVE_AND_EXIT_DBX:
     case KEY_VALUE_NO_SAVE_AND_EXIT_DBT:
-      if (Private->FileContext->FHandle != NULL) {
-        CloseFile (Private->FileContext->FHandle);
-        Private->FileContext->FHandle = NULL;
-        if (Private->FileContext->FileName!= NULL){
-          FreePool(Private->FileContext->FileName);
-          Private->FileContext->FileName = NULL;
-        }
-      }
+      CloseEnrolledFile(Private->FileContext);
 
       if (Private->SignatureGUID != NULL) {
         FreePool (Private->SignatureGUID);
@@ -3630,18 +3850,14 @@ SecureBootCallback (
     case KEY_SECURE_BOOT_SIGNATURE_GUID_DBX:
     case KEY_SECURE_BOOT_SIGNATURE_GUID_DBT:
       ASSERT (Private->SignatureGUID != NULL);
-      Status = StringToGuid (
-                 IfrNvData->SignatureGuid,
-                 StrLen (IfrNvData->SignatureGuid),
-                 Private->SignatureGUID
-                 );
-      if (EFI_ERROR (Status)) {
+      RStatus = StrToGuid (IfrNvData->SignatureGuid, Private->SignatureGUID);
+      if (RETURN_ERROR (RStatus) || (IfrNvData->SignatureGuid[GUID_STRING_LENGTH] != L'\0')) {
+        Status = EFI_INVALID_PARAMETER;
         break;
       }
 
       *ActionRequest = EFI_BROWSER_ACTION_REQUEST_FORM_APPLY;
       break;
-
     case KEY_SECURE_BOOT_DELETE_PK:
       GetVariable2 (EFI_SETUP_MODE_NAME, &gEfiGlobalVariableGuid, (VOID**)&SetupMode, NULL);
       if (SetupMode == NULL || (*SetupMode) == SETUP_MODE) {