Re: [edk2] [PATCH v2 1/2] SecurityPkg/SecureBootConfigDxe: Avoid illegal access

Subject: Re: [edk2] [PATCH v2 1/2] SecurityPkg/SecureBootConfigDxe: Avoid illegal access

From: "Dong, Guo" <guo.dong@intel.com>

To: "edk2-devel@lists.sourceforge.net" <edk2-devel@lists.sourceforge.net>

Date: 2013-08-07 17:11:21

Hi Gary,

Sorry for later response.
This patch looks good to me. Thanks for your contribution.

Reviewed-by: Guo Dong 

Thanks,
Guo
-----Original Message-----
From: Jordan Justen [mailto:jljusten@gmail.com] 
Sent: Tuesday, August 06, 2013 1:06 PM
To: edk2-devel@lists.sourceforge.net
Subject: Re: [edk2] [PATCH v2 1/2] SecurityPkg/SecureBootConfigDxe: Avoid illegal access

Series Reviewed-by: Jordan Justen 

I would like to see a review from the SecurityPkg owner for patch 1 before applying 2 to OvmfPkg.

-Jordan

On Wed, Jul 31, 2013 at 7:21 PM, Gary Ching-Pang Lin  wrote:
> When enrolling the certificate from a file, the suffix check function 
> check the last 4 characters to filter out non-DER files. However, if 
> the length of the file name is less than 4, the address prior to the 
> file name will be accessed while it shouldn't. This commit checks the 
> length of the file name to avoid illegal access.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Gary Ching-Pang Lin 
> ---
>  .../SecureBootConfigDxe/SecureBootConfigImpl.c      | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
>
> diff --git 
> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf
> igImpl.c 
> b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf
> igImpl.c
> index e8beecb..ce511bf 100644
> --- 
> a/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConf
> igImpl.c
> +++ b/SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBoot
> +++ ConfigImpl.c
> @@ -399,6 +399,7 @@ EnrollPlatformKey (
>    UINTN                           DataSize;
>    EFI_SIGNATURE_LIST              *PkCert;
>    UINT16*                         FilePostFix;
> +  UINTN                           NameLength;
>
>    if (Private->FileContext->FileName == NULL) {
>      return EFI_INVALID_PARAMETER;
> @@ -414,7 +415,11 @@ EnrollPlatformKey (
>    //
>    // Parse the file's postfix. Only support DER encoded X.509 certificate files.
>    //
> -  FilePostFix = Private->FileContext->FileName + StrLen 
> (Private->FileContext->FileName) - 4;
> +  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;
> @@ -802,6 +807,7 @@ EnrollKeyExchangeKey (
>    )
>  {
>    UINT16*     FilePostFix;
> +  UINTN       NameLength;
>    EFI_STATUS  Status;
>
>    if ((Private->FileContext->FileName == NULL) || 
> (Private->SignatureGUID == NULL)) { @@ -817,7 +823,11 @@ EnrollKeyExchangeKey (
>    // Parse the file's postfix. Supports DER-encoded X509 certificate,
>    // and .pbk as RSA public key file.
>    //
> -  FilePostFix = Private->FileContext->FileName + StrLen 
> (Private->FileContext->FileName) - 4;
> +  NameLength = StrLen (Private->FileContext->FileName);  if 
> + (NameLength <= 4) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +  FilePostFix = Private->FileContext->FileName + NameLength - 4;
>    if (IsDerEncodeCertificate(FilePostFix)) {
>      return EnrollX509ToKek (Private);
>    } else if (CompareMem (FilePostFix, L".pbk",4) == 0) { @@ -1550,6 
> +1560,7 @@ EnrollSignatureDatabase (
>    )
>  {
>    UINT16*      FilePostFix;
> +  UINTN        NameLength;
>    EFI_STATUS   Status;
>
>    if ((Private->FileContext->FileName == NULL) || 
> (Private->FileContext->FHandle == NULL) || (Private->SignatureGUID == NULL)) { @@ -1564,7 +1575,11 @@ EnrollSignatureDatabase (
>    //
>    // Parse the file's postfix.
>    //
> -  FilePostFix = Private->FileContext->FileName + StrLen 
> (Private->FileContext->FileName) - 4;
> +  NameLength = StrLen (Private->FileContext->FileName);  if 
> + (NameLength <= 4) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +  FilePostFix = Private->FileContext->FileName + NameLength - 4;
>    if (IsDerEncodeCertificate(FilePostFix)) {
>      //
>      // Supports DER-encoded X509 certificate.
> --
> 1.8.1.4
>
>
> ----------------------------------------------------------------------
> -------- Get your SQL database under version control now!
> Version control is standard for application code, but databases havent 
> caught up. So what steps can you take to put your SQL databases under 
> version control? Why should you start doing it? Read more to find out.
> http://pubads.g.doubleclick.net/gampad/clk?id=49501711&iu=/4140/ostg.c
> lktrk _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/edk2-devel

------------------------------------------------------------------------------
Get your SQL database under version control now!
Version control is standard for application code, but databases havent caught up. So what steps can you take to put your SQL databases under version control? Why should you start doing it? Read more to find out.
http://pubads.g.doubleclick.net/gampad/clk?id=48897031&iu=/4140/ostg.clktrk
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel

------------------------------------------------------------------------------
Get 100% visibility into Java/.NET code with AppDynamics Lite!
It's a free troubleshooting tool designed for production.
Get down to code-level detail for bottlenecks, with <2% overhead. 
Download for free and get started troubleshooting in minutes. 
http://pubads.g.doubleclick.net/gampad/clk?id=48897031&iu=/4140/ostg.clktrk
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/edk2-devel