Acked-by: Jiewen Yao <jiewen....@intel.com> > -----Original Message----- > From: Xu, Min M <min.m...@intel.com> > Sent: Friday, April 23, 2021 9:36 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 > > 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 (#74383): https://edk2.groups.io/g/devel/message/74383 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] -=-=-=-=-=-=-=-=-=-=-=-
