* Re: [PATCH] SecurityPkg: Add constraints on PK strength
[not found] <a321e530a5e9a0cb5b6f95108c9488b3706fa685.1618559012.git.jiaqi.gao@intel.com>
@ 2021-04-18 23:52 ` Min Xu
2021-04-19 1:30 ` Gao, Jiaqi
0 siblings, 1 reply; 4+ messages in thread
From: Min Xu @ 2021-04-18 23:52 UTC (permalink / raw)
To: Gao, Jiaqi, devel@edk2.groups.io; +Cc: Yao, Jiewen
Have you tested the patch? Would you please post the test result in the mail thread?
Thanks.
> -----Original Message-----
> From: Gao, Jiaqi <jiaqi.gao@intel.com>
> Sent: Friday, April 16, 2021 3:56 PM
> To: devel@edk2.groups.io
> Cc: Gao, Jiaqi <jiaqi.gao@intel.com>; Xu, Min M <min.m.xu@intel.com>; Yao,
> Jiewen <jiewen.yao@intel.com>
> Subject: [PATCH] SecurityPkg: Add constraints on PK strength
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3293
>
> Add constraints on the key strength of enrolled platform key(PK), which must
> be greater than or equal to 2048 bit.PK key strength is required by Intel SDL
> and MSFT, etc. This limitation prevents user from using weak keys as PK.
>
> The original code to check the certificate file type is placed in a new function
> CheckX509Certificate(), which checks if the X.509 certificate meets the
> requirements of encode type, RSA-Key strengh, etc.
>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Signed-off-by: Jiaqi Gao <jiaqi.gao@intel.com>
> ---
> .../SecureBootConfigImpl.c | 165 +++++++++++++++---
> .../SecureBootConfigImpl.h | 21 +++
> 2 files changed, 160 insertions(+), 26 deletions(-)
>
> diff --git
> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigI
> mpl.c
> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigI
> mpl.c
> index 4f01a2ed67..1304e21266 100644
> ---
> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigI
> mpl.c
> +++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootCo
> +++ nfigImpl.c
> @@ -90,6 +90,22 @@ CHAR16* mDerEncodedSuffix[] = { };
> CHAR16* mSupportX509Suffix = L"*.cer/der/crt";
>
> +//
> +// Prompt strings during certificate enrollment.
> +//
> +CHAR16* mX509EnrollPromptTitle[] = {
> + L"",
> + L"ERROR: Unsupported file type!",
> + L"ERROR: Unsupported certificate!",
> + NULL
> +};
> +CHAR16* mX509EnrollPromptString[] = {
> + L"",
> + L"Only DER encoded certificate file (*.cer/der/crt) is supported.",
> + L"Public key length should be equal to or greater than 2048 bits.",
> + NULL
> +};
> +
> SECUREBOOT_CONFIG_PRIVATE_DATA *gSecureBootPrivateData = NULL;
>
> /**
> @@ -383,6 +399,102 @@ SetSecureBootMode (
> );
> }
>
> +/**
> + This code checks if the encode type and key strength of X.509
> + certificate is qualified.
> +
> + @param[in] X509FileContext FileContext of X.509 certificate storing
> + file.
> + @param[out] Error Error type checked in the certificate.
> +
> + @return EFI_SUCCESS The certificate checked successfully.
> + @return EFI_INVALID_PARAMETER The parameter is invalid.
> + @return EFI_OUT_OF_RESOURCES Memory allocation failed.
> +
> +**/
> +EFI_STATUS
> +CheckX509Certificate (
> + IN SECUREBOOT_FILE_CONTEXT* X509FileContext,
> + OUT ENROLL_KEY_ERROR* Error
> +)
> +{
> + EFI_STATUS Status;
> + UINT16* FilePostFix;
> + UINTN NameLength;
> + UINT8* X509Data;
> + UINTN X509DataSize;
> + void* X509PubKey;
> + UINTN PubKeyModSize;
> +
> + if (X509FileContext->FileName == NULL) {
> + *Error = Unsupported_Type;
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + X509Data = NULL;
> + X509DataSize = 0;
> + X509PubKey = NULL;
> + PubKeyModSize = 0;
> +
> + //
> + // Parse the file's postfix. Only support DER encoded X.509 certificate files.
> + //
> + NameLength = StrLen (X509FileContext->FileName); if (NameLength <=
> + 4) {
> + DEBUG ((DEBUG_ERROR, "Wrong X509 NameLength\n"));
> + *Error = Unsupported_Type;
> + return EFI_INVALID_PARAMETER;
> + }
> + FilePostFix = X509FileContext->FileName + NameLength - 4; if
> + (!IsDerEncodeCertificate (FilePostFix)) {
> + DEBUG ((DEBUG_ERROR, "Unsupported file type, only DER encoded
> certificate (%s) is supported.\n", mSupportX509Suffix));
> + *Error = Unsupported_Type;
> + return EFI_INVALID_PARAMETER;
> + }
> + DEBUG ((DEBUG_INFO, "FileName= %s\n", X509FileContext->FileName));
> + DEBUG ((DEBUG_INFO, "FilePostFix = %s\n", FilePostFix));
> +
> + //
> + // Read the certificate file content
> + //
> + Status = ReadFileContent (X509FileContext->FHandle, (VOID**)
> + &X509Data, &X509DataSize, 0); if (EFI_ERROR (Status)) {
> + DEBUG ((DEBUG_ERROR, "Error occured while reading the file.\n"));
> + goto ON_EXIT;
> + }
> +
> + //
> + // Parse the public key context.
> + //
> + if (RsaGetPublicKeyFromX509 (X509Data, X509DataSize, &X509PubKey) ==
> FALSE) {
> + DEBUG ((DEBUG_ERROR, "Error occured while parsing the pubkey from
> certificate.\n"));
> + Status = EFI_INVALID_PARAMETER;
> + *Error = Unsupported_Type;
> + goto ON_EXIT;
> + }
> +
> + //
> + // Parse Module size of public key using interface provided by
> + CryptoPkg, which is // actually the size of public key.
> + //
> + if (X509PubKey != NULL) {
> + RsaGetKey (X509PubKey, RsaKeyN, NULL, &PubKeyModSize);
> + if (PubKeyModSize < CER_PUBKEY_MIN_SIZE) {
> + DEBUG ((DEBUG_ERROR, "Unqualified PK size, key size should be equal to
> or greater than 2048 bits.\n"));
> + Status = EFI_INVALID_PARAMETER;
> + *Error = Unqualified_Key;
> + }
> + RsaFree (X509PubKey);
> + }
> +
> + ON_EXIT:
> + if (X509Data != NULL) {
> + FreePool (X509Data);
> + }
> +
> + return Status;
> +}
> +
> /**
> Generate the PK signature list from the X509 Certificate storing file (.cer)
>
> @@ -461,7 +573,10 @@ ON_EXIT:
>
> The SignatureOwner GUID will be the same with PK's vendorguid.
>
> - @param[in] PrivateData The module's private data.
> + @param[in] PrivateData The module's private data.
> + @param[out] Error Point to the error code which indicates the
> + error during enroll process.
> +
>
> @retval EFI_SUCCESS New PK enrolled successfully.
> @retval EFI_INVALID_PARAMETER The parameter is invalid.
> @@ -477,12 +592,6 @@ EnrollPlatformKey (
> UINT32 Attr;
> UINTN DataSize;
> EFI_SIGNATURE_LIST *PkCert;
> - UINT16* FilePostFix;
> - UINTN NameLength;
> -
> - if (Private->FileContext->FileName == NULL) {
> - return EFI_INVALID_PARAMETER;
> - }
>
> PkCert = NULL;
>
> @@ -491,21 +600,6 @@ EnrollPlatformKey (
> return Status;
> }
>
> - //
> - // Parse the file's postfix. Only support DER encoded X.509 certificate files.
> - //
> - NameLength = StrLen (Private->FileContext->FileName);
> - if (NameLength <= 4) {
> - return EFI_INVALID_PARAMETER;
> - }
> - FilePostFix = Private->FileContext->FileName + NameLength - 4;
> - if (!IsDerEncodeCertificate(FilePostFix)) {
> - DEBUG ((EFI_D_ERROR, "Unsupported file type, only DER encoded
> certificate (%s) is supported.", mSupportX509Suffix));
> - return EFI_INVALID_PARAMETER;
> - }
> - DEBUG ((EFI_D_INFO, "FileName= %s\n", Private->FileContext->FileName));
> - DEBUG ((EFI_D_INFO, "FilePostFix = %s\n", FilePostFix));
> -
> //
> // Prase the selected PK file and generate PK certificate list.
> //
> @@ -4300,12 +4394,14 @@ SecureBootCallback (
> UINT16 *FilePostFix;
> SECUREBOOT_CONFIG_PRIVATE_DATA *PrivateData;
> BOOLEAN GetBrowserDataResult;
> + ENROLL_KEY_ERROR EnrollKeyErrorCode;
>
> Status = EFI_SUCCESS;
> SecureBootEnable = NULL;
> SecureBootMode = NULL;
> SetupMode = NULL;
> File = NULL;
> + EnrollKeyErrorCode = None_Error;
>
> if ((This == NULL) || (Value == NULL) || (ActionRequest == NULL)) {
> return EFI_INVALID_PARAMETER;
> @@ -4718,18 +4814,35 @@ SecureBootCallback (
> }
> break;
> case KEY_VALUE_SAVE_AND_EXIT_PK:
> - Status = EnrollPlatformKey (Private);
> + //
> + // Check the suffix, encode type and the key strength of PK certificate.
> + //
> + Status = CheckX509Certificate (Private->FileContext, &EnrollKeyErrorCode);
> + if (EFI_ERROR (Status)) {
> + if (EnrollKeyErrorCode != None_Error && EnrollKeyErrorCode <
> Enroll_Error_Max) {
> + CreatePopUp (
> + EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE,
> + &Key,
> + mX509EnrollPromptTitle[EnrollKeyErrorCode],
> + mX509EnrollPromptString[EnrollKeyErrorCode],
> + NULL
> + );
> + break;
> + }
> + } else {
> + Status = EnrollPlatformKey (Private);
> + }
> if (EFI_ERROR (Status)) {
> UnicodeSPrint (
> PromptString,
> sizeof (PromptString),
> - L"Only DER encoded certificate file (%s) is supported.",
> - mSupportX509Suffix
> + L"Error status: %x.",
> + Status
> );
> CreatePopUp (
> EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE,
> &Key,
> - L"ERROR: Unsupported file type!",
> + L"ERROR: Enrollment failed!",
> PromptString,
> NULL
> );
> diff --git
> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigI
> mpl.h
> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigI
> mpl.h
> index 1fafae07ac..268f015e8e 100644
> ---
> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigI
> mpl.h
> +++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootCo
> +++ nfigImpl.h
> @@ -93,6 +93,27 @@ extern EFI_IFR_GUID_LABEL *mEndLabel;
> #define HASHALG_RAW 0x00000004
> #define HASHALG_MAX 0x00000004
>
> +//
> +// Certificate public key minimum size (bytes) //
> +#define CER_PUBKEY_MIN_SIZE 256
> +
> +//
> +// Types of errors may occur during certificate enrollment.
> +//
> +typedef enum {
> + None_Error = 0,
> + //
> + // Unsupported_type indicates the certificate type is not supported.
> + //
> + Unsupported_Type,
> + //
> + // Unqualified_key indicates the key strength of certificate is not
> + // strong enough.
> + //
> + Unqualified_Key,
> + Enroll_Error_Max
> +}ENROLL_KEY_ERROR;
>
> typedef struct {
> UINTN Signature;
> --
> 2.31.1.windows.1
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] SecurityPkg: Add constraints on PK strength
2021-04-18 23:52 ` [PATCH] SecurityPkg: Add constraints on PK strength Min Xu
@ 2021-04-19 1:30 ` Gao, Jiaqi
2021-04-23 1:36 ` Min Xu
0 siblings, 1 reply; 4+ messages in thread
From: Gao, Jiaqi @ 2021-04-19 1:30 UTC (permalink / raw)
To: Xu, Min M, devel@edk2.groups.io; +Cc: Yao, Jiewen
Hi,
The patch has been built and tested with several toolchains:
1. GCC5 on Linux, both DEBUG and RELEASE.
2. VS2017 on Windows, both DEBUG and RELEASE.
3. VS2019 on Windows, both DEBUG and RELEASE.
To make sure the program can cope with various input, test cases consist of different PK certificate enrollment , which are:
1. Platform Keys (PKs) with RSA public key length less than 2048 bits, include RSA-512 and RSA-1024, etc. These kind of certificates were rejected during user enrollment.
2. PKs with RSA public key length equal to or greater than 2048 bits, include RSA-2048, RSA-3072 and RSA-4096, etc. These kind of certificates were successfully enrolled.
3. PKs which are not DER encoded, such as PEM encoded certificates with .cer/.der/.crt file suffix.
4. Empty PKs.
5. Empty inputs.
All the test cases were performed as expected. Test cases with unqualified key strength pop up the prompt of unqualified key, and the others with unsupported encode format or illegal input act as previous program.
Best Regards,
Jiaqi
-----Original Message-----
From: Xu, Min M <min.m.xu@intel.com>
Sent: Monday, April 19, 2021 7:52 AM
To: Gao, Jiaqi <jiaqi.gao@intel.com>; devel@edk2.groups.io
Cc: Yao, Jiewen <jiewen.yao@intel.com>
Subject: RE: [PATCH] SecurityPkg: Add constraints on PK strength
Have you tested the patch? Would you please post the test result in the mail thread?
Thanks.
> -----Original Message-----
> From: Gao, Jiaqi <jiaqi.gao@intel.com>
> Sent: Friday, April 16, 2021 3:56 PM
> To: devel@edk2.groups.io
> Cc: Gao, Jiaqi <jiaqi.gao@intel.com>; Xu, Min M <min.m.xu@intel.com>;
> Yao, Jiewen <jiewen.yao@intel.com>
> Subject: [PATCH] SecurityPkg: Add constraints on PK strength
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3293
>
> Add constraints on the key strength of enrolled platform key(PK),
> which must be greater than or equal to 2048 bit.PK key strength is
> required by Intel SDL and MSFT, etc. This limitation prevents user from using weak keys as PK.
>
> The original code to check the certificate file type is placed in a
> new function CheckX509Certificate(), which checks if the X.509
> certificate meets the requirements of encode type, RSA-Key strengh, etc.
>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Signed-off-by: Jiaqi Gao <jiaqi.gao@intel.com>
> ---
> .../SecureBootConfigImpl.c | 165 +++++++++++++++---
> .../SecureBootConfigImpl.h | 21 +++
> 2 files changed, 160 insertions(+), 26 deletions(-)
>
> diff --git
> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf
> igI
> mpl.c
> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf
> igI
> mpl.c
> index 4f01a2ed67..1304e21266 100644
> ---
> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf
> igI
> mpl.c
> +++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBoot
> +++ Co
> +++ nfigImpl.c
> @@ -90,6 +90,22 @@ CHAR16* mDerEncodedSuffix[] = { };
> CHAR16* mSupportX509Suffix = L"*.cer/der/crt";
>
> +//
> +// Prompt strings during certificate enrollment.
> +//
> +CHAR16* mX509EnrollPromptTitle[] = {
> + L"",
> + L"ERROR: Unsupported file type!",
> + L"ERROR: Unsupported certificate!",
> + NULL
> +};
> +CHAR16* mX509EnrollPromptString[] = {
> + L"",
> + L"Only DER encoded certificate file (*.cer/der/crt) is supported.",
> + L"Public key length should be equal to or greater than 2048 bits.",
> + NULL
> +};
> +
> SECUREBOOT_CONFIG_PRIVATE_DATA *gSecureBootPrivateData = NULL;
>
> /**
> @@ -383,6 +399,102 @@ SetSecureBootMode (
> );
> }
>
> +/**
> + This code checks if the encode type and key strength of X.509
> + certificate is qualified.
> +
> + @param[in] X509FileContext FileContext of X.509 certificate storing
> + file.
> + @param[out] Error Error type checked in the certificate.
> +
> + @return EFI_SUCCESS The certificate checked successfully.
> + @return EFI_INVALID_PARAMETER The parameter is invalid.
> + @return EFI_OUT_OF_RESOURCES Memory allocation failed.
> +
> +**/
> +EFI_STATUS
> +CheckX509Certificate (
> + IN SECUREBOOT_FILE_CONTEXT* X509FileContext,
> + OUT ENROLL_KEY_ERROR* Error
> +)
> +{
> + EFI_STATUS Status;
> + UINT16* FilePostFix;
> + UINTN NameLength;
> + UINT8* X509Data;
> + UINTN X509DataSize;
> + void* X509PubKey;
> + UINTN PubKeyModSize;
> +
> + if (X509FileContext->FileName == NULL) {
> + *Error = Unsupported_Type;
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + X509Data = NULL;
> + X509DataSize = 0;
> + X509PubKey = NULL;
> + PubKeyModSize = 0;
> +
> + //
> + // Parse the file's postfix. Only support DER encoded X.509 certificate files.
> + //
> + NameLength = StrLen (X509FileContext->FileName); if (NameLength <=
> + 4) {
> + DEBUG ((DEBUG_ERROR, "Wrong X509 NameLength\n"));
> + *Error = Unsupported_Type;
> + return EFI_INVALID_PARAMETER;
> + }
> + FilePostFix = X509FileContext->FileName + NameLength - 4; if
> + (!IsDerEncodeCertificate (FilePostFix)) {
> + DEBUG ((DEBUG_ERROR, "Unsupported file type, only DER encoded
> certificate (%s) is supported.\n", mSupportX509Suffix));
> + *Error = Unsupported_Type;
> + return EFI_INVALID_PARAMETER;
> + }
> + DEBUG ((DEBUG_INFO, "FileName= %s\n", X509FileContext->FileName));
> + DEBUG ((DEBUG_INFO, "FilePostFix = %s\n", FilePostFix));
> +
> + //
> + // Read the certificate file content // Status = ReadFileContent
> + (X509FileContext->FHandle, (VOID**) &X509Data, &X509DataSize, 0);
> + if (EFI_ERROR (Status)) {
> + DEBUG ((DEBUG_ERROR, "Error occured while reading the file.\n"));
> + goto ON_EXIT;
> + }
> +
> + //
> + // Parse the public key context.
> + //
> + if (RsaGetPublicKeyFromX509 (X509Data, X509DataSize, &X509PubKey)
> + ==
> FALSE) {
> + DEBUG ((DEBUG_ERROR, "Error occured while parsing the pubkey from
> certificate.\n"));
> + Status = EFI_INVALID_PARAMETER;
> + *Error = Unsupported_Type;
> + goto ON_EXIT;
> + }
> +
> + //
> + // Parse Module size of public key using interface provided by
> + CryptoPkg, which is // actually the size of public key.
> + //
> + if (X509PubKey != NULL) {
> + RsaGetKey (X509PubKey, RsaKeyN, NULL, &PubKeyModSize);
> + if (PubKeyModSize < CER_PUBKEY_MIN_SIZE) {
> + DEBUG ((DEBUG_ERROR, "Unqualified PK size, key size should be
> + equal to
> or greater than 2048 bits.\n"));
> + Status = EFI_INVALID_PARAMETER;
> + *Error = Unqualified_Key;
> + }
> + RsaFree (X509PubKey);
> + }
> +
> + ON_EXIT:
> + if (X509Data != NULL) {
> + FreePool (X509Data);
> + }
> +
> + return Status;
> +}
> +
> /**
> Generate the PK signature list from the X509 Certificate storing
> file (.cer)
>
> @@ -461,7 +573,10 @@ ON_EXIT:
>
> The SignatureOwner GUID will be the same with PK's vendorguid.
>
> - @param[in] PrivateData The module's private data.
> + @param[in] PrivateData The module's private data.
> + @param[out] Error Point to the error code which indicates the
> + error during enroll process.
> +
>
> @retval EFI_SUCCESS New PK enrolled successfully.
> @retval EFI_INVALID_PARAMETER The parameter is invalid.
> @@ -477,12 +592,6 @@ EnrollPlatformKey (
> UINT32 Attr;
> UINTN DataSize;
> EFI_SIGNATURE_LIST *PkCert;
> - UINT16* FilePostFix;
> - UINTN NameLength;
> -
> - if (Private->FileContext->FileName == NULL) {
> - return EFI_INVALID_PARAMETER;
> - }
>
> PkCert = NULL;
>
> @@ -491,21 +600,6 @@ EnrollPlatformKey (
> return Status;
> }
>
> - //
> - // Parse the file's postfix. Only support DER encoded X.509 certificate files.
> - //
> - NameLength = StrLen (Private->FileContext->FileName);
> - if (NameLength <= 4) {
> - return EFI_INVALID_PARAMETER;
> - }
> - FilePostFix = Private->FileContext->FileName + NameLength - 4;
> - if (!IsDerEncodeCertificate(FilePostFix)) {
> - DEBUG ((EFI_D_ERROR, "Unsupported file type, only DER encoded
> certificate (%s) is supported.", mSupportX509Suffix));
> - return EFI_INVALID_PARAMETER;
> - }
> - DEBUG ((EFI_D_INFO, "FileName= %s\n",
> Private->FileContext->FileName));
> - DEBUG ((EFI_D_INFO, "FilePostFix = %s\n", FilePostFix));
> -
> //
> // Prase the selected PK file and generate PK certificate list.
> //
> @@ -4300,12 +4394,14 @@ SecureBootCallback (
> UINT16 *FilePostFix;
> SECUREBOOT_CONFIG_PRIVATE_DATA *PrivateData;
> BOOLEAN GetBrowserDataResult;
> + ENROLL_KEY_ERROR EnrollKeyErrorCode;
>
> Status = EFI_SUCCESS;
> SecureBootEnable = NULL;
> SecureBootMode = NULL;
> SetupMode = NULL;
> File = NULL;
> + EnrollKeyErrorCode = None_Error;
>
> if ((This == NULL) || (Value == NULL) || (ActionRequest == NULL)) {
> return EFI_INVALID_PARAMETER;
> @@ -4718,18 +4814,35 @@ SecureBootCallback (
> }
> break;
> case KEY_VALUE_SAVE_AND_EXIT_PK:
> - Status = EnrollPlatformKey (Private);
> + //
> + // Check the suffix, encode type and the key strength of PK certificate.
> + //
> + Status = CheckX509Certificate (Private->FileContext, &EnrollKeyErrorCode);
> + if (EFI_ERROR (Status)) {
> + if (EnrollKeyErrorCode != None_Error && EnrollKeyErrorCode <
> Enroll_Error_Max) {
> + CreatePopUp (
> + EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE,
> + &Key,
> + mX509EnrollPromptTitle[EnrollKeyErrorCode],
> + mX509EnrollPromptString[EnrollKeyErrorCode],
> + NULL
> + );
> + break;
> + }
> + } else {
> + Status = EnrollPlatformKey (Private);
> + }
> if (EFI_ERROR (Status)) {
> UnicodeSPrint (
> PromptString,
> sizeof (PromptString),
> - L"Only DER encoded certificate file (%s) is supported.",
> - mSupportX509Suffix
> + L"Error status: %x.",
> + Status
> );
> CreatePopUp (
> EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE,
> &Key,
> - L"ERROR: Unsupported file type!",
> + L"ERROR: Enrollment failed!",
> PromptString,
> NULL
> );
> diff --git
> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf
> igI
> mpl.h
> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf
> igI
> mpl.h
> index 1fafae07ac..268f015e8e 100644
> ---
> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf
> igI
> mpl.h
> +++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBoot
> +++ Co
> +++ nfigImpl.h
> @@ -93,6 +93,27 @@ extern EFI_IFR_GUID_LABEL *mEndLabel;
> #define HASHALG_RAW 0x00000004
> #define HASHALG_MAX 0x00000004
>
> +//
> +// Certificate public key minimum size (bytes) //
> +#define CER_PUBKEY_MIN_SIZE 256
> +
> +//
> +// Types of errors may occur during certificate enrollment.
> +//
> +typedef enum {
> + None_Error = 0,
> + //
> + // Unsupported_type indicates the certificate type is not supported.
> + //
> + Unsupported_Type,
> + //
> + // Unqualified_key indicates the key strength of certificate is not
> + // strong enough.
> + //
> + Unqualified_Key,
> + Enroll_Error_Max
> +}ENROLL_KEY_ERROR;
>
> typedef struct {
> UINTN Signature;
> --
> 2.31.1.windows.1
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] SecurityPkg: Add constraints on PK strength
2021-04-19 1:30 ` Gao, Jiaqi
@ 2021-04-23 1:36 ` Min Xu
2021-04-23 4:37 ` Yao, Jiewen
0 siblings, 1 reply; 4+ messages in thread
From: Min Xu @ 2021-04-23 1:36 UTC (permalink / raw)
To: Gao, Jiaqi, devel@edk2.groups.io; +Cc: Yao, Jiewen
This patch is good to me.
Reviewed-by: Min Xu <min.m.xu@intel.com>
> -----Original Message-----
> From: Gao, Jiaqi <jiaqi.gao@intel.com>
> Sent: Monday, April 19, 2021 9:31 AM
> To: Xu, Min M <min.m.xu@intel.com>; devel@edk2.groups.io
> Cc: Yao, Jiewen <jiewen.yao@intel.com>
> Subject: RE: [PATCH] SecurityPkg: Add constraints on PK strength
>
> Hi,
>
> The patch has been built and tested with several toolchains:
> 1. GCC5 on Linux, both DEBUG and RELEASE.
> 2. VS2017 on Windows, both DEBUG and RELEASE.
> 3. VS2019 on Windows, both DEBUG and RELEASE.
>
> To make sure the program can cope with various input, test cases consist of
> different PK certificate enrollment , which are:
> 1. Platform Keys (PKs) with RSA public key length less than 2048 bits, include
> RSA-512 and RSA-1024, etc. These kind of certificates were rejected during user
> enrollment.
> 2. PKs with RSA public key length equal to or greater than 2048 bits, include RSA-
> 2048, RSA-3072 and RSA-4096, etc. These kind of certificates were successfully
> enrolled.
> 3. PKs which are not DER encoded, such as PEM encoded certificates
> with .cer/.der/.crt file suffix.
> 4. Empty PKs.
> 5. Empty inputs.
>
> All the test cases were performed as expected. Test cases with unqualified key
> strength pop up the prompt of unqualified key, and the others with unsupported
> encode format or illegal input act as previous program.
>
>
> Best Regards,
> Jiaqi
>
> -----Original Message-----
> From: Xu, Min M <min.m.xu@intel.com>
> Sent: Monday, April 19, 2021 7:52 AM
> To: Gao, Jiaqi <jiaqi.gao@intel.com>; devel@edk2.groups.io
> Cc: Yao, Jiewen <jiewen.yao@intel.com>
> Subject: RE: [PATCH] SecurityPkg: Add constraints on PK strength
>
> Have you tested the patch? Would you please post the test result in the mail
> thread?
> Thanks.
>
> > -----Original Message-----
> > From: Gao, Jiaqi <jiaqi.gao@intel.com>
> > Sent: Friday, April 16, 2021 3:56 PM
> > To: devel@edk2.groups.io
> > Cc: Gao, Jiaqi <jiaqi.gao@intel.com>; Xu, Min M <min.m.xu@intel.com>;
> > Yao, Jiewen <jiewen.yao@intel.com>
> > Subject: [PATCH] SecurityPkg: Add constraints on PK strength
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3293
> >
> > Add constraints on the key strength of enrolled platform key(PK),
> > which must be greater than or equal to 2048 bit.PK key strength is
> > required by Intel SDL and MSFT, etc. This limitation prevents user from using
> weak keys as PK.
> >
> > The original code to check the certificate file type is placed in a
> > new function CheckX509Certificate(), which checks if the X.509
> > certificate meets the requirements of encode type, RSA-Key strengh, etc.
> >
> > Cc: Min Xu <min.m.xu@intel.com>
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Signed-off-by: Jiaqi Gao <jiaqi.gao@intel.com>
> > ---
> > .../SecureBootConfigImpl.c | 165 +++++++++++++++---
> > .../SecureBootConfigImpl.h | 21 +++
> > 2 files changed, 160 insertions(+), 26 deletions(-)
> >
> > diff --git
> > a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf
> > igI
> > mpl.c
> > b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf
> > igI
> > mpl.c
> > index 4f01a2ed67..1304e21266 100644
> > ---
> > a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf
> > igI
> > mpl.c
> > +++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBoot
> > +++ Co
> > +++ nfigImpl.c
> > @@ -90,6 +90,22 @@ CHAR16* mDerEncodedSuffix[] = { };
> > CHAR16* mSupportX509Suffix = L"*.cer/der/crt";
> >
> > +//
> > +// Prompt strings during certificate enrollment.
> > +//
> > +CHAR16* mX509EnrollPromptTitle[] = {
> > + L"",
> > + L"ERROR: Unsupported file type!",
> > + L"ERROR: Unsupported certificate!",
> > + NULL
> > +};
> > +CHAR16* mX509EnrollPromptString[] = {
> > + L"",
> > + L"Only DER encoded certificate file (*.cer/der/crt) is supported.",
> > + L"Public key length should be equal to or greater than 2048 bits.",
> > + NULL
> > +};
> > +
> > SECUREBOOT_CONFIG_PRIVATE_DATA *gSecureBootPrivateData = NULL;
> >
> > /**
> > @@ -383,6 +399,102 @@ SetSecureBootMode (
> > );
> > }
> >
> > +/**
> > + This code checks if the encode type and key strength of X.509
> > + certificate is qualified.
> > +
> > + @param[in] X509FileContext FileContext of X.509 certificate storing
> > + file.
> > + @param[out] Error Error type checked in the certificate.
> > +
> > + @return EFI_SUCCESS The certificate checked successfully.
> > + @return EFI_INVALID_PARAMETER The parameter is invalid.
> > + @return EFI_OUT_OF_RESOURCES Memory allocation failed.
> > +
> > +**/
> > +EFI_STATUS
> > +CheckX509Certificate (
> > + IN SECUREBOOT_FILE_CONTEXT* X509FileContext,
> > + OUT ENROLL_KEY_ERROR* Error
> > +)
> > +{
> > + EFI_STATUS Status;
> > + UINT16* FilePostFix;
> > + UINTN NameLength;
> > + UINT8* X509Data;
> > + UINTN X509DataSize;
> > + void* X509PubKey;
> > + UINTN PubKeyModSize;
> > +
> > + if (X509FileContext->FileName == NULL) {
> > + *Error = Unsupported_Type;
> > + return EFI_INVALID_PARAMETER;
> > + }
> > +
> > + X509Data = NULL;
> > + X509DataSize = 0;
> > + X509PubKey = NULL;
> > + PubKeyModSize = 0;
> > +
> > + //
> > + // Parse the file's postfix. Only support DER encoded X.509 certificate files.
> > + //
> > + NameLength = StrLen (X509FileContext->FileName); if (NameLength <=
> > + 4) {
> > + DEBUG ((DEBUG_ERROR, "Wrong X509 NameLength\n"));
> > + *Error = Unsupported_Type;
> > + return EFI_INVALID_PARAMETER;
> > + }
> > + FilePostFix = X509FileContext->FileName + NameLength - 4; if
> > + (!IsDerEncodeCertificate (FilePostFix)) {
> > + DEBUG ((DEBUG_ERROR, "Unsupported file type, only DER encoded
> > certificate (%s) is supported.\n", mSupportX509Suffix));
> > + *Error = Unsupported_Type;
> > + return EFI_INVALID_PARAMETER;
> > + }
> > + DEBUG ((DEBUG_INFO, "FileName= %s\n", X509FileContext->FileName));
> > + DEBUG ((DEBUG_INFO, "FilePostFix = %s\n", FilePostFix));
> > +
> > + //
> > + // Read the certificate file content // Status = ReadFileContent
> > + (X509FileContext->FHandle, (VOID**) &X509Data, &X509DataSize, 0); if
> > + (EFI_ERROR (Status)) {
> > + DEBUG ((DEBUG_ERROR, "Error occured while reading the file.\n"));
> > + goto ON_EXIT;
> > + }
> > +
> > + //
> > + // Parse the public key context.
> > + //
> > + if (RsaGetPublicKeyFromX509 (X509Data, X509DataSize, &X509PubKey)
> > + ==
> > FALSE) {
> > + DEBUG ((DEBUG_ERROR, "Error occured while parsing the pubkey from
> > certificate.\n"));
> > + Status = EFI_INVALID_PARAMETER;
> > + *Error = Unsupported_Type;
> > + goto ON_EXIT;
> > + }
> > +
> > + //
> > + // Parse Module size of public key using interface provided by
> > + CryptoPkg, which is // actually the size of public key.
> > + //
> > + if (X509PubKey != NULL) {
> > + RsaGetKey (X509PubKey, RsaKeyN, NULL, &PubKeyModSize);
> > + if (PubKeyModSize < CER_PUBKEY_MIN_SIZE) {
> > + DEBUG ((DEBUG_ERROR, "Unqualified PK size, key size should be
> > + equal to
> > or greater than 2048 bits.\n"));
> > + Status = EFI_INVALID_PARAMETER;
> > + *Error = Unqualified_Key;
> > + }
> > + RsaFree (X509PubKey);
> > + }
> > +
> > + ON_EXIT:
> > + if (X509Data != NULL) {
> > + FreePool (X509Data);
> > + }
> > +
> > + return Status;
> > +}
> > +
> > /**
> > Generate the PK signature list from the X509 Certificate storing
> > file (.cer)
> >
> > @@ -461,7 +573,10 @@ ON_EXIT:
> >
> > The SignatureOwner GUID will be the same with PK's vendorguid.
> >
> > - @param[in] PrivateData The module's private data.
> > + @param[in] PrivateData The module's private data.
> > + @param[out] Error Point to the error code which indicates the
> > + error during enroll process.
> > +
> >
> > @retval EFI_SUCCESS New PK enrolled successfully.
> > @retval EFI_INVALID_PARAMETER The parameter is invalid.
> > @@ -477,12 +592,6 @@ EnrollPlatformKey (
> > UINT32 Attr;
> > UINTN DataSize;
> > EFI_SIGNATURE_LIST *PkCert;
> > - UINT16* FilePostFix;
> > - UINTN NameLength;
> > -
> > - if (Private->FileContext->FileName == NULL) {
> > - return EFI_INVALID_PARAMETER;
> > - }
> >
> > PkCert = NULL;
> >
> > @@ -491,21 +600,6 @@ EnrollPlatformKey (
> > return Status;
> > }
> >
> > - //
> > - // Parse the file's postfix. Only support DER encoded X.509 certificate files.
> > - //
> > - NameLength = StrLen (Private->FileContext->FileName);
> > - if (NameLength <= 4) {
> > - return EFI_INVALID_PARAMETER;
> > - }
> > - FilePostFix = Private->FileContext->FileName + NameLength - 4;
> > - if (!IsDerEncodeCertificate(FilePostFix)) {
> > - DEBUG ((EFI_D_ERROR, "Unsupported file type, only DER encoded
> > certificate (%s) is supported.", mSupportX509Suffix));
> > - return EFI_INVALID_PARAMETER;
> > - }
> > - DEBUG ((EFI_D_INFO, "FileName= %s\n",
> > Private->FileContext->FileName));
> > - DEBUG ((EFI_D_INFO, "FilePostFix = %s\n", FilePostFix));
> > -
> > //
> > // Prase the selected PK file and generate PK certificate list.
> > //
> > @@ -4300,12 +4394,14 @@ SecureBootCallback (
> > UINT16 *FilePostFix;
> > SECUREBOOT_CONFIG_PRIVATE_DATA *PrivateData;
> > BOOLEAN GetBrowserDataResult;
> > + ENROLL_KEY_ERROR EnrollKeyErrorCode;
> >
> > Status = EFI_SUCCESS;
> > SecureBootEnable = NULL;
> > SecureBootMode = NULL;
> > SetupMode = NULL;
> > File = NULL;
> > + EnrollKeyErrorCode = None_Error;
> >
> > if ((This == NULL) || (Value == NULL) || (ActionRequest == NULL)) {
> > return EFI_INVALID_PARAMETER;
> > @@ -4718,18 +4814,35 @@ SecureBootCallback (
> > }
> > break;
> > case KEY_VALUE_SAVE_AND_EXIT_PK:
> > - Status = EnrollPlatformKey (Private);
> > + //
> > + // Check the suffix, encode type and the key strength of PK certificate.
> > + //
> > + Status = CheckX509Certificate (Private->FileContext,
> &EnrollKeyErrorCode);
> > + if (EFI_ERROR (Status)) {
> > + if (EnrollKeyErrorCode != None_Error && EnrollKeyErrorCode <
> > Enroll_Error_Max) {
> > + CreatePopUp (
> > + EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE,
> > + &Key,
> > + mX509EnrollPromptTitle[EnrollKeyErrorCode],
> > + mX509EnrollPromptString[EnrollKeyErrorCode],
> > + NULL
> > + );
> > + break;
> > + }
> > + } else {
> > + Status = EnrollPlatformKey (Private);
> > + }
> > if (EFI_ERROR (Status)) {
> > UnicodeSPrint (
> > PromptString,
> > sizeof (PromptString),
> > - L"Only DER encoded certificate file (%s) is supported.",
> > - mSupportX509Suffix
> > + L"Error status: %x.",
> > + Status
> > );
> > CreatePopUp (
> > EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE,
> > &Key,
> > - L"ERROR: Unsupported file type!",
> > + L"ERROR: Enrollment failed!",
> > PromptString,
> > NULL
> > );
> > diff --git
> > a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf
> > igI
> > mpl.h
> > b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf
> > igI
> > mpl.h
> > index 1fafae07ac..268f015e8e 100644
> > ---
> > a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf
> > igI
> > mpl.h
> > +++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBoot
> > +++ Co
> > +++ nfigImpl.h
> > @@ -93,6 +93,27 @@ extern EFI_IFR_GUID_LABEL *mEndLabel;
> > #define HASHALG_RAW 0x00000004
> > #define HASHALG_MAX 0x00000004
> >
> > +//
> > +// Certificate public key minimum size (bytes) //
> > +#define CER_PUBKEY_MIN_SIZE 256
> > +
> > +//
> > +// Types of errors may occur during certificate enrollment.
> > +//
> > +typedef enum {
> > + None_Error = 0,
> > + //
> > + // Unsupported_type indicates the certificate type is not supported.
> > + //
> > + Unsupported_Type,
> > + //
> > + // Unqualified_key indicates the key strength of certificate is not
> > + // strong enough.
> > + //
> > + Unqualified_Key,
> > + Enroll_Error_Max
> > +}ENROLL_KEY_ERROR;
> >
> > typedef struct {
> > UINTN Signature;
> > --
> > 2.31.1.windows.1
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] SecurityPkg: Add constraints on PK strength
2021-04-23 1:36 ` Min Xu
@ 2021-04-23 4:37 ` Yao, Jiewen
0 siblings, 0 replies; 4+ messages in thread
From: Yao, Jiewen @ 2021-04-23 4:37 UTC (permalink / raw)
To: Xu, Min M, Gao, Jiaqi, devel@edk2.groups.io
Acked-by: Jiewen Yao <Jiewen.yao@intel.com>
> -----Original Message-----
> From: Xu, Min M <min.m.xu@intel.com>
> Sent: Friday, April 23, 2021 9:36 AM
> To: Gao, Jiaqi <jiaqi.gao@intel.com>; devel@edk2.groups.io
> Cc: Yao, Jiewen <jiewen.yao@intel.com>
> Subject: RE: [PATCH] SecurityPkg: Add constraints on PK strength
>
> This patch is good to me.
> Reviewed-by: Min Xu <min.m.xu@intel.com>
>
> > -----Original Message-----
> > From: Gao, Jiaqi <jiaqi.gao@intel.com>
> > Sent: Monday, April 19, 2021 9:31 AM
> > To: Xu, Min M <min.m.xu@intel.com>; devel@edk2.groups.io
> > Cc: Yao, Jiewen <jiewen.yao@intel.com>
> > Subject: RE: [PATCH] SecurityPkg: Add constraints on PK strength
> >
> > Hi,
> >
> > The patch has been built and tested with several toolchains:
> > 1. GCC5 on Linux, both DEBUG and RELEASE.
> > 2. VS2017 on Windows, both DEBUG and RELEASE.
> > 3. VS2019 on Windows, both DEBUG and RELEASE.
> >
> > To make sure the program can cope with various input, test cases consist of
> > different PK certificate enrollment , which are:
> > 1. Platform Keys (PKs) with RSA public key length less than 2048 bits, include
> > RSA-512 and RSA-1024, etc. These kind of certificates were rejected during
> user
> > enrollment.
> > 2. PKs with RSA public key length equal to or greater than 2048 bits, include
> RSA-
> > 2048, RSA-3072 and RSA-4096, etc. These kind of certificates were
> successfully
> > enrolled.
> > 3. PKs which are not DER encoded, such as PEM encoded certificates
> > with .cer/.der/.crt file suffix.
> > 4. Empty PKs.
> > 5. Empty inputs.
> >
> > All the test cases were performed as expected. Test cases with unqualified
> key
> > strength pop up the prompt of unqualified key, and the others with
> unsupported
> > encode format or illegal input act as previous program.
> >
> >
> > Best Regards,
> > Jiaqi
> >
> > -----Original Message-----
> > From: Xu, Min M <min.m.xu@intel.com>
> > Sent: Monday, April 19, 2021 7:52 AM
> > To: Gao, Jiaqi <jiaqi.gao@intel.com>; devel@edk2.groups.io
> > Cc: Yao, Jiewen <jiewen.yao@intel.com>
> > Subject: RE: [PATCH] SecurityPkg: Add constraints on PK strength
> >
> > Have you tested the patch? Would you please post the test result in the
> mail
> > thread?
> > Thanks.
> >
> > > -----Original Message-----
> > > From: Gao, Jiaqi <jiaqi.gao@intel.com>
> > > Sent: Friday, April 16, 2021 3:56 PM
> > > To: devel@edk2.groups.io
> > > Cc: Gao, Jiaqi <jiaqi.gao@intel.com>; Xu, Min M <min.m.xu@intel.com>;
> > > Yao, Jiewen <jiewen.yao@intel.com>
> > > Subject: [PATCH] SecurityPkg: Add constraints on PK strength
> > >
> > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3293
> > >
> > > Add constraints on the key strength of enrolled platform key(PK),
> > > which must be greater than or equal to 2048 bit.PK key strength is
> > > required by Intel SDL and MSFT, etc. This limitation prevents user from
> using
> > weak keys as PK.
> > >
> > > The original code to check the certificate file type is placed in a
> > > new function CheckX509Certificate(), which checks if the X.509
> > > certificate meets the requirements of encode type, RSA-Key strengh, etc.
> > >
> > > Cc: Min Xu <min.m.xu@intel.com>
> > > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > > Signed-off-by: Jiaqi Gao <jiaqi.gao@intel.com>
> > > ---
> > > .../SecureBootConfigImpl.c | 165 +++++++++++++++---
> > > .../SecureBootConfigImpl.h | 21 +++
> > > 2 files changed, 160 insertions(+), 26 deletions(-)
> > >
> > > diff --git
> > >
> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf
> > > igI
> > > mpl.c
> > >
> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf
> > > igI
> > > mpl.c
> > > index 4f01a2ed67..1304e21266 100644
> > > ---
> > >
> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf
> > > igI
> > > mpl.c
> > > +++
> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBoot
> > > +++ Co
> > > +++ nfigImpl.c
> > > @@ -90,6 +90,22 @@ CHAR16* mDerEncodedSuffix[] = { };
> > > CHAR16* mSupportX509Suffix = L"*.cer/der/crt";
> > >
> > > +//
> > > +// Prompt strings during certificate enrollment.
> > > +//
> > > +CHAR16* mX509EnrollPromptTitle[] = {
> > > + L"",
> > > + L"ERROR: Unsupported file type!",
> > > + L"ERROR: Unsupported certificate!",
> > > + NULL
> > > +};
> > > +CHAR16* mX509EnrollPromptString[] = {
> > > + L"",
> > > + L"Only DER encoded certificate file (*.cer/der/crt) is supported.",
> > > + L"Public key length should be equal to or greater than 2048 bits.",
> > > + NULL
> > > +};
> > > +
> > > SECUREBOOT_CONFIG_PRIVATE_DATA *gSecureBootPrivateData = NULL;
> > >
> > > /**
> > > @@ -383,6 +399,102 @@ SetSecureBootMode (
> > > );
> > > }
> > >
> > > +/**
> > > + This code checks if the encode type and key strength of X.509
> > > + certificate is qualified.
> > > +
> > > + @param[in] X509FileContext FileContext of X.509 certificate storing
> > > + file.
> > > + @param[out] Error Error type checked in the certificate.
> > > +
> > > + @return EFI_SUCCESS The certificate checked successfully.
> > > + @return EFI_INVALID_PARAMETER The parameter is invalid.
> > > + @return EFI_OUT_OF_RESOURCES Memory allocation failed.
> > > +
> > > +**/
> > > +EFI_STATUS
> > > +CheckX509Certificate (
> > > + IN SECUREBOOT_FILE_CONTEXT* X509FileContext,
> > > + OUT ENROLL_KEY_ERROR* Error
> > > +)
> > > +{
> > > + EFI_STATUS Status;
> > > + UINT16* FilePostFix;
> > > + UINTN NameLength;
> > > + UINT8* X509Data;
> > > + UINTN X509DataSize;
> > > + void* X509PubKey;
> > > + UINTN PubKeyModSize;
> > > +
> > > + if (X509FileContext->FileName == NULL) {
> > > + *Error = Unsupported_Type;
> > > + return EFI_INVALID_PARAMETER;
> > > + }
> > > +
> > > + X509Data = NULL;
> > > + X509DataSize = 0;
> > > + X509PubKey = NULL;
> > > + PubKeyModSize = 0;
> > > +
> > > + //
> > > + // Parse the file's postfix. Only support DER encoded X.509 certificate
> files.
> > > + //
> > > + NameLength = StrLen (X509FileContext->FileName); if (NameLength <=
> > > + 4) {
> > > + DEBUG ((DEBUG_ERROR, "Wrong X509 NameLength\n"));
> > > + *Error = Unsupported_Type;
> > > + return EFI_INVALID_PARAMETER;
> > > + }
> > > + FilePostFix = X509FileContext->FileName + NameLength - 4; if
> > > + (!IsDerEncodeCertificate (FilePostFix)) {
> > > + DEBUG ((DEBUG_ERROR, "Unsupported file type, only DER encoded
> > > certificate (%s) is supported.\n", mSupportX509Suffix));
> > > + *Error = Unsupported_Type;
> > > + return EFI_INVALID_PARAMETER;
> > > + }
> > > + DEBUG ((DEBUG_INFO, "FileName= %s\n", X509FileContext->FileName));
> > > + DEBUG ((DEBUG_INFO, "FilePostFix = %s\n", FilePostFix));
> > > +
> > > + //
> > > + // Read the certificate file content // Status = ReadFileContent
> > > + (X509FileContext->FHandle, (VOID**) &X509Data, &X509DataSize, 0); if
> > > + (EFI_ERROR (Status)) {
> > > + DEBUG ((DEBUG_ERROR, "Error occured while reading the file.\n"));
> > > + goto ON_EXIT;
> > > + }
> > > +
> > > + //
> > > + // Parse the public key context.
> > > + //
> > > + if (RsaGetPublicKeyFromX509 (X509Data, X509DataSize, &X509PubKey)
> > > + ==
> > > FALSE) {
> > > + DEBUG ((DEBUG_ERROR, "Error occured while parsing the pubkey from
> > > certificate.\n"));
> > > + Status = EFI_INVALID_PARAMETER;
> > > + *Error = Unsupported_Type;
> > > + goto ON_EXIT;
> > > + }
> > > +
> > > + //
> > > + // Parse Module size of public key using interface provided by
> > > + CryptoPkg, which is // actually the size of public key.
> > > + //
> > > + if (X509PubKey != NULL) {
> > > + RsaGetKey (X509PubKey, RsaKeyN, NULL, &PubKeyModSize);
> > > + if (PubKeyModSize < CER_PUBKEY_MIN_SIZE) {
> > > + DEBUG ((DEBUG_ERROR, "Unqualified PK size, key size should be
> > > + equal to
> > > or greater than 2048 bits.\n"));
> > > + Status = EFI_INVALID_PARAMETER;
> > > + *Error = Unqualified_Key;
> > > + }
> > > + RsaFree (X509PubKey);
> > > + }
> > > +
> > > + ON_EXIT:
> > > + if (X509Data != NULL) {
> > > + FreePool (X509Data);
> > > + }
> > > +
> > > + return Status;
> > > +}
> > > +
> > > /**
> > > Generate the PK signature list from the X509 Certificate storing
> > > file (.cer)
> > >
> > > @@ -461,7 +573,10 @@ ON_EXIT:
> > >
> > > The SignatureOwner GUID will be the same with PK's vendorguid.
> > >
> > > - @param[in] PrivateData The module's private data.
> > > + @param[in] PrivateData The module's private data.
> > > + @param[out] Error Point to the error code which indicates the
> > > + error during enroll process.
> > > +
> > >
> > > @retval EFI_SUCCESS New PK enrolled successfully.
> > > @retval EFI_INVALID_PARAMETER The parameter is invalid.
> > > @@ -477,12 +592,6 @@ EnrollPlatformKey (
> > > UINT32 Attr;
> > > UINTN DataSize;
> > > EFI_SIGNATURE_LIST *PkCert;
> > > - UINT16* FilePostFix;
> > > - UINTN NameLength;
> > > -
> > > - if (Private->FileContext->FileName == NULL) {
> > > - return EFI_INVALID_PARAMETER;
> > > - }
> > >
> > > PkCert = NULL;
> > >
> > > @@ -491,21 +600,6 @@ EnrollPlatformKey (
> > > return Status;
> > > }
> > >
> > > - //
> > > - // Parse the file's postfix. Only support DER encoded X.509 certificate
> files.
> > > - //
> > > - NameLength = StrLen (Private->FileContext->FileName);
> > > - if (NameLength <= 4) {
> > > - return EFI_INVALID_PARAMETER;
> > > - }
> > > - FilePostFix = Private->FileContext->FileName + NameLength - 4;
> > > - if (!IsDerEncodeCertificate(FilePostFix)) {
> > > - DEBUG ((EFI_D_ERROR, "Unsupported file type, only DER encoded
> > > certificate (%s) is supported.", mSupportX509Suffix));
> > > - return EFI_INVALID_PARAMETER;
> > > - }
> > > - DEBUG ((EFI_D_INFO, "FileName= %s\n",
> > > Private->FileContext->FileName));
> > > - DEBUG ((EFI_D_INFO, "FilePostFix = %s\n", FilePostFix));
> > > -
> > > //
> > > // Prase the selected PK file and generate PK certificate list.
> > > //
> > > @@ -4300,12 +4394,14 @@ SecureBootCallback (
> > > UINT16 *FilePostFix;
> > > SECUREBOOT_CONFIG_PRIVATE_DATA *PrivateData;
> > > BOOLEAN GetBrowserDataResult;
> > > + ENROLL_KEY_ERROR EnrollKeyErrorCode;
> > >
> > > Status = EFI_SUCCESS;
> > > SecureBootEnable = NULL;
> > > SecureBootMode = NULL;
> > > SetupMode = NULL;
> > > File = NULL;
> > > + EnrollKeyErrorCode = None_Error;
> > >
> > > if ((This == NULL) || (Value == NULL) || (ActionRequest == NULL)) {
> > > return EFI_INVALID_PARAMETER;
> > > @@ -4718,18 +4814,35 @@ SecureBootCallback (
> > > }
> > > break;
> > > case KEY_VALUE_SAVE_AND_EXIT_PK:
> > > - Status = EnrollPlatformKey (Private);
> > > + //
> > > + // Check the suffix, encode type and the key strength of PK certificate.
> > > + //
> > > + Status = CheckX509Certificate (Private->FileContext,
> > &EnrollKeyErrorCode);
> > > + if (EFI_ERROR (Status)) {
> > > + if (EnrollKeyErrorCode != None_Error && EnrollKeyErrorCode <
> > > Enroll_Error_Max) {
> > > + CreatePopUp (
> > > + EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE,
> > > + &Key,
> > > + mX509EnrollPromptTitle[EnrollKeyErrorCode],
> > > + mX509EnrollPromptString[EnrollKeyErrorCode],
> > > + NULL
> > > + );
> > > + break;
> > > + }
> > > + } else {
> > > + Status = EnrollPlatformKey (Private);
> > > + }
> > > if (EFI_ERROR (Status)) {
> > > UnicodeSPrint (
> > > PromptString,
> > > sizeof (PromptString),
> > > - L"Only DER encoded certificate file (%s) is supported.",
> > > - mSupportX509Suffix
> > > + L"Error status: %x.",
> > > + Status
> > > );
> > > CreatePopUp (
> > > EFI_LIGHTGRAY | EFI_BACKGROUND_BLUE,
> > > &Key,
> > > - L"ERROR: Unsupported file type!",
> > > + L"ERROR: Enrollment failed!",
> > > PromptString,
> > > NULL
> > > );
> > > diff --git
> > >
> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf
> > > igI
> > > mpl.h
> > >
> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf
> > > igI
> > > mpl.h
> > > index 1fafae07ac..268f015e8e 100644
> > > ---
> > >
> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf
> > > igI
> > > mpl.h
> > > +++
> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBoot
> > > +++ Co
> > > +++ nfigImpl.h
> > > @@ -93,6 +93,27 @@ extern EFI_IFR_GUID_LABEL *mEndLabel;
> > > #define HASHALG_RAW 0x00000004
> > > #define HASHALG_MAX 0x00000004
> > >
> > > +//
> > > +// Certificate public key minimum size (bytes) //
> > > +#define CER_PUBKEY_MIN_SIZE 256
> > > +
> > > +//
> > > +// Types of errors may occur during certificate enrollment.
> > > +//
> > > +typedef enum {
> > > + None_Error = 0,
> > > + //
> > > + // Unsupported_type indicates the certificate type is not supported.
> > > + //
> > > + Unsupported_Type,
> > > + //
> > > + // Unqualified_key indicates the key strength of certificate is not
> > > + // strong enough.
> > > + //
> > > + Unqualified_Key,
> > > + Enroll_Error_Max
> > > +}ENROLL_KEY_ERROR;
> > >
> > > typedef struct {
> > > UINTN Signature;
> > > --
> > > 2.31.1.windows.1
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-04-23 4:37 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <a321e530a5e9a0cb5b6f95108c9488b3706fa685.1618559012.git.jiaqi.gao@intel.com>
2021-04-18 23:52 ` [PATCH] SecurityPkg: Add constraints on PK strength Min Xu
2021-04-19 1:30 ` Gao, Jiaqi
2021-04-23 1:36 ` Min Xu
2021-04-23 4:37 ` Yao, Jiewen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox