From: "Zhang, Chao B" <chao.b.zhang@intel.com>
To: Laszlo Ersek <lersek@redhat.com>,
"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
Cc: "Fu, Siyuan" <siyuan.fu@intel.com>, "Long, Qin" <qin.long@intel.com>
Subject: Re: [PATCH] SecureBoot UI Update
Date: Fri, 31 Mar 2017 05:11:17 +0000 [thread overview]
Message-ID: <FF72C7E4248F3C4E9BDF19D4918E90F249525E7B@shsmsx102.ccr.corp.intel.com> (raw)
In-Reply-To: <be9a4c6c-090c-f187-dc83-22dd7e7f5623@redhat.com>
Laszlo:
I checked my patch. It did contains such info. I use index 1, 2 in front of some comment line.
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 <chao.b.zhang@intel.com>; edk2-devel@lists.01.org
Cc: Fu, Siyuan <siyuan.fu@intel.com>; Long, Qin <qin.long@intel.com>
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 <Guid/FileSystemVolumeLabelInfo.h>
> #include <Guid/ImageAuthentication.h> #include <Guid/FileInfo.h>
> +#include <Guid/WinCertificate.h>
>
> #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
next prev parent reply other threads:[~2017-03-31 5:11 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-03-30 2:00 [PATCH] SecureBoot UI Update Zhang, Chao B
2017-03-30 8:26 ` Long, Qin
2017-03-30 16:31 ` Laszlo Ersek
2017-03-31 5:11 ` Zhang, Chao B [this message]
2017-03-31 8:47 ` Laszlo Ersek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=FF72C7E4248F3C4E9BDF19D4918E90F249525E7B@shsmsx102.ccr.corp.intel.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox