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/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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#74236): https://edk2.groups.io/g/devel/message/74236 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] -=-=-=-=-=-=-=-=-=-=-=-
