Re: [PATCH] SecurityPkg: Add constraints on PK strength

["Min Xu" <min.m.xu@intel.com>]

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