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 37A4A21A04830 for ; Fri, 31 Mar 2017 01:47:47 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 65CCFC04BD3A; Fri, 31 Mar 2017 08:47:46 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 65CCFC04BD3A Authentication-Results: ext-mx07.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx07.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=lersek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 65CCFC04BD3A Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-31.phx2.redhat.com [10.3.116.31]) by smtp.corp.redhat.com (Postfix) with ESMTP id E8CD384958; Fri, 31 Mar 2017 08:47:44 +0000 (UTC) To: "Zhang, Chao B" , "edk2-devel@lists.01.org" References: <20170330020045.21452-1-chao.b.zhang@intel.com> Cc: "Fu, Siyuan" , "Long, Qin" From: Laszlo Ersek Message-ID: Date: Fri, 31 Mar 2017 10:47:43 +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: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Fri, 31 Mar 2017 08:47:46 +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: Fri, 31 Mar 2017 08:47:47 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 03/31/17 07:11, Zhang, Chao B wrote: > Laszlo: > I checked my patch. It did contains such info. I use index 1, 2 in front of some comment line. Ah! That is a good explanation actually; I have burned myself with the same in the past too. The thing is, if you use the hash mark (#) at the beginning of any line, possibly preceded by whitespace only, then GIT will consider that line a comment, and it will remove it from the commit message before the patch is committed! It is unfortunately very non-intuitive. I recommend writing "Nr.1" and "Nr.2." instead of "#1" and "#2". Or maybe, in a listing, - #1 foo - #2 bar Thanks! Laszlo > Not sure it is the reason why these info are filtered. I will send patch again. > > > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek > Sent: Friday, March 31, 2017 12:31 AM > To: Zhang, Chao B ; edk2-devel@lists.01.org > Cc: Fu, Siyuan ; Long, Qin > Subject: Re: [edk2] [PATCH] SecureBoot UI Update > > 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/SecureBootConf >> ig.vfr >> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf >> ig.vfr >> index 02ddf4a..e153eca 100644 >> --- >> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf >> ig.vfr >> +++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBoot >> +++ Config.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/SecureBootConf >> igImpl.c >> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf >> igImpl.c >> index 6f58729..17fe120 100644 >> --- >> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf >> igImpl.c >> +++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBoot >> +++ ConfigImpl.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/SecureBootConf >> igImpl.h >> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf >> igImpl.h >> index bea9470..f9b75e6 100644 >> --- >> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf >> igImpl.h >> +++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBoot >> +++ ConfigImpl.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/SecureBootConf >> igNvData.h >> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf >> igNvData.h >> index df4d72e..c3dc92c 100644 >> --- >> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf >> igNvData.h >> +++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBoot >> +++ ConfigNvData.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/SecureBootConf >> igStrings.uni >> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf >> igStrings.uni >> index af6d83b..96a02b3 100644 >> --- >> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf >> igStrings.uni >> +++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBoot >> +++ ConfigStrings.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" >> > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel >