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

["Yao, Jiewen" <jiewen.yao@intel.com>]

Acked-by: Jiewen Yao <Jiewen.yao@intel.com>

> -----Original Message-----
> From: Xu, Min M <min.m.xu@intel.com>
> Sent: Friday, April 23, 2021 9:36 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
> 
> 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