This patch is good to me. Reviewed-by: Min Xu <min.m...@intel.com> > -----Original Message----- > From: Gao, Jiaqi <jiaqi....@intel.com> > Sent: Monday, April 19, 2021 9:31 AM > To: Xu, Min M <min.m...@intel.com>; devel@edk2.groups.io > Cc: Yao, Jiewen <jiewen....@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...@intel.com> > Sent: Monday, April 19, 2021 7:52 AM > To: Gao, Jiaqi <jiaqi....@intel.com>; devel@edk2.groups.io > Cc: Yao, Jiewen <jiewen....@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....@intel.com> > > Sent: Friday, April 16, 2021 3:56 PM > > To: devel@edk2.groups.io > > Cc: Gao, Jiaqi <jiaqi....@intel.com>; Xu, Min M <min.m...@intel.com>; > > Yao, Jiewen <jiewen....@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...@intel.com> > > Cc: Jiewen Yao <jiewen....@intel.com> > > Signed-off-by: Jiaqi Gao <jiaqi....@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
