[edk2-devel] [PATCH] SecurityPkg: Add constraints on PK strength
Min Xu
min.m.xu at intel.com
Fri Apr 23 01:36:25 UTC 2021
This patch is good to me.
Reviewed-by: Min Xu <min.m.xu at intel.com>
> -----Original Message-----
> From: Gao, Jiaqi <jiaqi.gao at intel.com>
> Sent: Monday, April 19, 2021 9:31 AM
> To: Xu, Min M <min.m.xu at intel.com>; devel at edk2.groups.io
> Cc: Yao, Jiewen <jiewen.yao at 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 at intel.com>
> Sent: Monday, April 19, 2021 7:52 AM
> To: Gao, Jiaqi <jiaqi.gao at intel.com>; devel at edk2.groups.io
> Cc: Yao, Jiewen <jiewen.yao at 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 at intel.com>
> > Sent: Friday, April 16, 2021 3:56 PM
> > To: devel at edk2.groups.io
> > Cc: Gao, Jiaqi <jiaqi.gao at intel.com>; Xu, Min M <min.m.xu at intel.com>;
> > Yao, Jiewen <jiewen.yao at 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 at intel.com>
> > Cc: Jiewen Yao <jiewen.yao at intel.com>
> > Signed-off-by: Jiaqi Gao <jiaqi.gao at 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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74381): https://edk2.groups.io/g/devel/message/74381
Mute This Topic: https://groups.io/mt/82198099/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list