From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 72ABA21DFA8FE for ; Thu, 30 Mar 2017 09:31:20 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id CFB8A7AE92; Thu, 30 Mar 2017 16:31:19 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com CFB8A7AE92 Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=lersek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com CFB8A7AE92 Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-143.phx2.redhat.com [10.3.116.143]) by smtp.corp.redhat.com (Postfix) with ESMTP id 59E1890ECB; Thu, 30 Mar 2017 16:31:18 +0000 (UTC) To: "Zhang, Chao B" , edk2-devel@lists.01.org References: <20170330020045.21452-1-chao.b.zhang@intel.com> Cc: siyuan.fu@intel.com, qin.long@intel.com From: Laszlo Ersek Message-ID: Date: Thu, 30 Mar 2017 18:31:17 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <20170330020045.21452-1-chao.b.zhang@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.25]); Thu, 30 Mar 2017 16:31:20 +0000 (UTC) Subject: Re: [PATCH] SecureBoot UI Update X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 30 Mar 2017 16:31:20 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 03/30/17 04:00, Zhang, Chao B wrote: > --- > .../SecureBootConfigDxe/SecureBootConfig.vfr | 38 +++- > .../SecureBootConfigDxe/SecureBootConfigImpl.c | 196 ++++++++++++++++++++- > .../SecureBootConfigDxe/SecureBootConfigImpl.h | 32 ++++ > .../SecureBootConfigDxe/SecureBootConfigNvData.h | 5 + > .../SecureBootConfigStrings.uni | 13 +- > 5 files changed, 268 insertions(+), 16 deletions(-) (I hope this patch has not been committed yet.) Can you guys please describe the UI update in the commit message? The commit message is useless in this form. What is being changed? And why? Worse, there is no Contributed-under line, and no Signed-off-by line. This patch does not conform to the edk2 contribution rules. Thanks Laszlo > > diff --git a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfig.vfr b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfig.vfr > index 02ddf4a..e153eca 100644 > --- a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfig.vfr > +++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfig.vfr > @@ -455,15 +455,35 @@ formset > maxsize = SECURE_BOOT_GUID_SIZE, > endstring; > > - oneof name = SignatureFormatInDbx, > - varid = SECUREBOOT_CONFIGURATION.CertificateFormat, > - prompt = STRING_TOKEN(STR_DBX_CERTIFICATE_FORMAT_PROMPT), > - help = STRING_TOKEN(STR_DBX_CERTIFICATE_FORMAT_HELP), > - option text = STRING_TOKEN(STR_DBX_CERTIFICATE_FORMAT_SHA256), value = 0x2, flags = DEFAULT; > - option text = STRING_TOKEN(STR_DBX_CERTIFICATE_FORMAT_SHA384), value = 0x3, flags = 0; > - option text = STRING_TOKEN(STR_DBX_CERTIFICATE_FORMAT_SHA512), value = 0x4, flags = 0; > - option text = STRING_TOKEN(STR_DBX_CERTIFICATE_FORMAT_RAW), value = 0x5, flags = 0; > - endoneof; > + disableif NOT ideqval SECUREBOOT_CONFIGURATION.FileEnrollType == 1; > + oneof name = X509SignatureFormatInDbx, > + varid = SECUREBOOT_CONFIGURATION.CertificateFormat, > + prompt = STRING_TOKEN(STR_DBX_CERTIFICATE_FORMAT_PROMPT), > + help = STRING_TOKEN(STR_DBX_CERTIFICATE_FORMAT_HELP), > + option text = STRING_TOKEN(STR_DBX_CERTIFICATE_FORMAT_SHA256), value = 0x2, flags = DEFAULT; > + option text = STRING_TOKEN(STR_DBX_CERTIFICATE_FORMAT_SHA384), value = 0x3, flags = 0; > + option text = STRING_TOKEN(STR_DBX_CERTIFICATE_FORMAT_SHA512), value = 0x4, flags = 0; > + option text = STRING_TOKEN(STR_DBX_CERTIFICATE_FORMAT_RAW), value = 0x5, flags = 0; > + endoneof; > + endif; > + > + disableif NOT ideqval SECUREBOOT_CONFIGURATION.FileEnrollType == 2; > + grayoutif TRUE; > + text > + help = STRING_TOKEN(STR_DBX_CERTIFICATE_FORMAT_PROMPT), // Help string > + text = STRING_TOKEN(STR_DBX_CERTIFICATE_FORMAT_HELP), // Prompt string > + text = STRING_TOKEN(STR_DBX_PE_FORMAT_SHA256); // TextTwo > + endif; > + endif; > + > + suppressif ideqval SECUREBOOT_CONFIGURATION.FileEnrollType == 3; > + grayoutif TRUE; > + text > + help = STRING_TOKEN(STR_DBX_CERTIFICATE_FORMAT_PROMPT), // Help string > + text = STRING_TOKEN(STR_DBX_CERTIFICATE_FORMAT_HELP), // Prompt string > + text = STRING_TOKEN(STR_DBX_AUTH_2_FORMAT); // TextTwo > + endif; > + endif; > > suppressif ideqval SECUREBOOT_CONFIGURATION.CertificateFormat == 5; > checkbox varid = SECUREBOOT_CONFIGURATION.AlwaysRevocation, > diff --git a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c > index 6f58729..17fe120 100644 > --- a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c > +++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.c > @@ -120,6 +120,61 @@ IsDerEncodeCertificate ( > } > > /** > + 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. > > @param[in] VarValue The option of Secure Boot. > @@ -2081,6 +2136,115 @@ HashPeImageByType ( > > **/ > EFI_STATUS > +EnrollAuthenication2Descriptor ( > + 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; > + } > + > + > + DEBUG((DEBUG_ERROR, "DBX update binary %s %x %Attr %x\n",VariableName, mImageSize, Attr)); > + // > + // Diretly set AUTHENTICATION_2 data to SetVariable > + // > + Status = gRT->SetVariable( > + VariableName, > + &gEfiImageSecurityDatabaseGuid, > + Attr, > + mImageSize, > + mImageBase > + ); > + > + DEBUG((DEBUG_ERROR, "DBX update binary status %x\n", Status)); > + > +ON_EXIT: > + > + CloseFile (Private->FileContext->FHandle); > + Private->FileContext->FHandle = NULL; > + > + if (Private->FileContext->FileName != NULL){ > + FreePool(Private->FileContext->FileName); > + Private->FileContext->FileName = NULL; > + } > + > + if (Data != NULL) { > + FreePool (Data); > + } > + > + if (mImageBase != NULL) { > + FreePool (mImageBase); > + mImageBase = NULL; > + } > + > + return Status; > + > +} > + > + > +/** > + 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 > EnrollImageSignatureToSigDB ( > IN SECUREBOOT_CONFIG_PRIVATE_DATA *Private, > IN CHAR16 *VariableName > @@ -2305,10 +2469,12 @@ EnrollSignatureDatabase ( > // Supports DER-encoded X509 certificate. > // > return EnrollX509toSigDB (Private, VariableName); > + } else if (IsAuthentication2Format(Private->FileContext->FHandle)){ > + return EnrollAuthenication2Descriptor(Private, VariableName); > + } else { > + return EnrollImageSignatureToSigDB (Private, VariableName); > } > - > - return EnrollImageSignatureToSigDB (Private, VariableName); > -} > +} > > /** > List all signatures in specified signature database (e.g. KEK/DB/DBX/DBT) > @@ -2957,6 +3123,7 @@ SecureBootExtractConfigFromVariable ( > // Initilize the Date and Time using system time. > // > ConfigData->CertificateFormat = HASHALG_RAW; > + ConfigData->FileEnrollType = UNKNOWN_FILE_TYPE; > ConfigData->AlwaysRevocation = TRUE; > gRT->GetTime (&CurrTime, NULL); > ConfigData->RevocationDate.Year = CurrTime.Year; > @@ -3258,6 +3425,8 @@ SecureBootCallback ( > UINT8 *SetupMode; > CHAR16 PromptString[100]; > EFI_DEVICE_PATH_PROTOCOL *File; > + UINTN NameLength; > + UINT16 *FilePostFix; > > Status = EFI_SUCCESS; > SecureBootEnable = NULL; > @@ -3393,6 +3562,27 @@ SecureBootCallback ( > > case SECUREBOOT_ENROLL_SIGNATURE_TO_DBX: > ChooseFile (NULL, NULL, UpdateDBXFromFile, &File); > + // > + // 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(gSecureBootPrivateData->FileContext->FHandle)){ > + IfrNvData->FileEnrollType = AUTHENCIATION_2_FILE_TYPE; > + } else { > + IfrNvData->FileEnrollType = PE_IMAGE_FILE_TYPE; > + } > + DEBUG((DEBUG_ERROR, "IfrNvData->FileEnrollType %d\n", IfrNvData->FileEnrollType)); > + HiiSetBrowserData(&gSecureBootConfigFormSetGuid, mSecureBootStorageName, sizeof (SECUREBOOT_CONFIGURATION),(UINT8 *)IfrNvData, NULL); > break; > > case SECUREBOOT_ENROLL_SIGNATURE_TO_DBT: > diff --git a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.h b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.h > index bea9470..f9b75e6 100644 > --- a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.h > +++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigImpl.h > @@ -47,6 +47,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > #include > #include > #include > +#include > > #include "SecureBootConfigNvData.h" > > @@ -582,4 +583,35 @@ UpdateDBTFromFile ( > IN EFI_DEVICE_PATH_PROTOCOL *FilePath > ); > > +/** > + This code checks if the FileSuffix is one of the possible DER-encoded certificate suffix. > + > + @param[in] FileSuffix The suffix of the input certificate file > + > + @retval TRUE It's a DER-encoded certificate. > + @retval FALSE It's NOT a DER-encoded certificate. > + > +**/ > +BOOLEAN > +IsDerEncodeCertificate ( > + IN CONST CHAR16 *FileSuffix > +); > + > + > +/** > + 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 > +); > + > + > #endif > diff --git a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigNvData.h b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigNvData.h > index df4d72e..c3dc92c 100644 > --- a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigNvData.h > +++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigNvData.h > @@ -107,6 +107,10 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > #define SECURE_BOOT_GUID_SIZE 36 > #define SECURE_BOOT_GUID_STORAGE_SIZE 37 > > +#define UNKNOWN_FILE_TYPE 0 > +#define X509_CERT_FILE_TYPE 1 > +#define AUTHENCIATION_2_FILE_TYPE 2 > +#define PE_IMAGE_FILE_TYPE 3 > > // > // Nv Data structure referenced by IFR > @@ -123,6 +127,7 @@ typedef struct { > UINT8 CertificateFormat; // The type of the certificate > EFI_HII_DATE RevocationDate; // The revocation date of the certificate > EFI_HII_TIME RevocationTime; // The revocation time of the certificate > + UINT8 FileEnrollType; // File type of sigunature enroll > } SECUREBOOT_CONFIGURATION; > > #endif > diff --git a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigStrings.uni b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigStrings.uni > index af6d83b..96a02b3 100644 > --- a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigStrings.uni > +++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigStrings.uni > @@ -35,10 +35,15 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > > #string STR_DBX_CERTIFICATE_FORMAT_PROMPT #language en-US "Signature Format" > #string STR_DBX_CERTIFICATE_FORMAT_HELP #language en-US "Select the certificate format used to enroll certificate into database." > -#string STR_DBX_CERTIFICATE_FORMAT_SHA256 #language en-US "SHA256" > -#string STR_DBX_CERTIFICATE_FORMAT_SHA384 #language en-US "SHA384" > -#string STR_DBX_CERTIFICATE_FORMAT_SHA512 #language en-US "SHA512" > -#string STR_DBX_CERTIFICATE_FORMAT_RAW #language en-US "RAW" > +#string STR_DBX_CERTIFICATE_FORMAT_SHA256 #language en-US "X509 CERT SHA256" > +#string STR_DBX_CERTIFICATE_FORMAT_SHA384 #language en-US "X509 CERT SHA384" > +#string STR_DBX_CERTIFICATE_FORMAT_SHA512 #language en-US "X509 CERT SHA512" > +#string STR_DBX_CERTIFICATE_FORMAT_RAW #language en-US "X509 CERT" > + > +#string STR_DBX_PE_FORMAT_SHA256 #language en-US "PE Image SHA256" > + > +#string STR_DBX_AUTH_2_FORMAT #language en-US "VARIABLE_AUTHENICATION_2" > + > > #string STR_CERTIFICATE_REVOCATION_TIME_PROMPT #language en-US " Revocation Time" > #string STR_CERTIFICATE_REVOCATION_TIME_HELP #language en-US "Input the revocation time of the certificate" >