Re: [Patch] [UDK2017] SecurityPkg OpalPasswordSupportLib: Add check to avoid potential buffer overflow.

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

Reviewed-by: jiewen.yao@intel.com

> -----Original Message-----
> From: Dong, Eric
> Sent: Wednesday, August 1, 2018 10:33 AM
> To: edk2-devel@lists.01.org
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
> Subject: RE: [edk2] [Patch] [UDK2017] SecurityPkg OpalPasswordSupportLib:
> Add check to avoid potential buffer overflow.
> 
> Hi Hao & Jiewen,
> 
> This patch has been verified by the original reporter, also pass regression test
> done by myself.
> 
> Thanks,
> Eric
> 
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Eric
> > Dong
> > Sent: Tuesday, July 31, 2018 1:16 PM
> > To: edk2-devel@lists.01.org
> > Cc: Wu, Hao A <hao.a.wu@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
> > Subject: [edk2] [Patch] [UDK2017] SecurityPkg OpalPasswordSupportLib:
> Add
> > check to avoid potential buffer overflow.
> >
> > Current code not check the CommunicationBuffer size before use it. Attacker
> > can read beyond the end of the (untrusted) commbuffer into controlled
> > memory. Attacker can get access outside of valid SMM memory regions. This
> > patch add check before use it.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Eric Dong <eric.dong@intel.com>
> > Cc: Yao Jiewen <jiewen.yao@intel.com>
> > Cc: Wu Hao <hao.a.wu@intel.com>
> > ---
> >  .../Include/Library/OpalPasswordSupportLib.h       |  3 +-
> >  .../OpalPasswordSupportLib.c                       | 55
> +++++++++++++++-------
> >  .../OpalPasswordSupportNotify.h                    |  2 +-
> >  .../Tcg/Opal/OpalPasswordSmm/OpalPasswordSmm.h     |  6 +--
> >  4 files changed, 42 insertions(+), 24 deletions(-)
> >
> > diff --git a/SecurityPkg/Include/Library/OpalPasswordSupportLib.h
> > b/SecurityPkg/Include/Library/OpalPasswordSupportLib.h
> > index e616c763f0..ad45e9e3b7 100644
> > --- a/SecurityPkg/Include/Library/OpalPasswordSupportLib.h
> > +++ b/SecurityPkg/Include/Library/OpalPasswordSupportLib.h
> > @@ -19,6 +19,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY
> > KIND, EITHER EXPRESS OR IMPLIED.
> >  #include <Protocol/DevicePath.h>
> >  #include <Library/TcgStorageOpalLib.h>
> >
> > +#define OPAL_PASSWORD_MAX_LENGTH         32
> >
> >  #pragma pack(1)
> >
> > @@ -76,7 +77,7 @@ typedef struct {
> >  typedef struct {
> >    LIST_ENTRY                 Link;
> >
> > -  UINT8                      Password[32];
> > +  UINT8
> Password[OPAL_PASSWORD_MAX_LENGTH];
> >    UINT8                      PasswordLength;
> >
> >    EFI_DEVICE_PATH_PROTOCOL   OpalDevicePath;
> > diff --git
> > a/SecurityPkg/Library/OpalPasswordSupportLib/OpalPasswordSupportLib.c
> > b/SecurityPkg/Library/OpalPasswordSupportLib/OpalPasswordSupportLib.c
> > index 837582359e..e377e9ca79 100644
> > ---
> a/SecurityPkg/Library/OpalPasswordSupportLib/OpalPasswordSupportLib.c
> > +++
> b/SecurityPkg/Library/OpalPasswordSupportLib/OpalPasswordSupportLib.
> > +++ c
> > @@ -14,8 +14,6 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY
> > KIND, EITHER EXPRESS OR IMPLIED.
> >
> >  #include "OpalPasswordSupportNotify.h"
> >
> > -#define OPAL_PASSWORD_MAX_LENGTH         32
> > -
> >  LIST_ENTRY           mDeviceList = INITIALIZE_LIST_HEAD_VARIABLE
> > (mDeviceList);
> >  BOOLEAN              gInSmm = FALSE;
> >  EFI_GUID             gOpalPasswordNotifyProtocolGuid =
> > OPAL_PASSWORD_NOTIFY_PROTOCOL_GUID;
> > @@ -663,34 +661,53 @@ SmmOpalPasswordHandler (
> >    EFI_STATUS                        Status;
> >    OPAL_SMM_COMMUNICATE_HEADER       *SmmFunctionHeader;
> >    UINTN                             TempCommBufferSize;
> > -  UINT8                             *NewPassword;
> > -  UINT8                             PasswordLength;
> > -  EFI_DEVICE_PATH_PROTOCOL          *DevicePath;
> > +  UINTN                             RemainedDevicePathSize;
> > +  OPAL_COMM_DEVICE_LIST             *DeviceBuffer;
> >
> >    if (CommBuffer == NULL || CommBufferSize == NULL) {
> >      return EFI_SUCCESS;
> >    }
> >
> > +  Status = EFI_SUCCESS;
> > +  DeviceBuffer = NULL;
> > +
> >    TempCommBufferSize = *CommBufferSize;
> >    if (TempCommBufferSize < OFFSET_OF
> > (OPAL_SMM_COMMUNICATE_HEADER, Data)) {
> >      return EFI_SUCCESS;
> >    }
> > -
> > -  Status   = EFI_SUCCESS;
> > -  SmmFunctionHeader     = (OPAL_SMM_COMMUNICATE_HEADER
> > *)CommBuffer;
> > -
> > -  DevicePath = &((OPAL_COMM_DEVICE_LIST*)(SmmFunctionHeader-
> > >Data))->OpalDevicePath;
> > -  PasswordLength = ((OPAL_COMM_DEVICE_LIST*)(SmmFunctionHeader-
> > >Data))->PasswordLength;
> > -  NewPassword = ((OPAL_COMM_DEVICE_LIST*)(SmmFunctionHeader-
> > >Data))->Password;
> > +  SmmFunctionHeader = (OPAL_SMM_COMMUNICATE_HEADER
> > *)CommBuffer;
> >
> >    switch (SmmFunctionHeader->Function) {
> >      case SMM_FUNCTION_SET_OPAL_PASSWORD:
> > -        if (OpalPasswordIsFullZero (NewPassword) || PasswordLength == 0)
> {
> > -          Status = EFI_INVALID_PARAMETER;
> > -          goto EXIT;
> > -        }
> > +      if (TempCommBufferSize <= OFFSET_OF
> > (OPAL_SMM_COMMUNICATE_HEADER, Data) + OFFSET_OF
> > (OPAL_COMM_DEVICE_LIST, OpalDevicePath)) {
> > +        return EFI_SUCCESS;
> > +      }
> > +
> > +      DeviceBuffer = AllocateCopyPool (TempCommBufferSize - OFFSET_OF
> > (OPAL_SMM_COMMUNICATE_HEADER, Data), SmmFunctionHeader->Data);
> > +      if (DeviceBuffer == NULL) {
> > +        Status = EFI_OUT_OF_RESOURCES;
> > +        goto EXIT;
> > +      }
> >
> > -        Status = OpalSavePasswordToSmm (DevicePath, NewPassword,
> > PasswordLength);
> > +      //
> > +      // Validate the DevicePath.
> > +      //
> > +      RemainedDevicePathSize = TempCommBufferSize - OFFSET_OF
> > (OPAL_SMM_COMMUNICATE_HEADER, Data)
> > +                                    - OFFSET_OF
> (OPAL_COMM_DEVICE_LIST,
> > OpalDevicePath);
> > +      if (!IsDevicePathValid(&DeviceBuffer->OpalDevicePath,
> > RemainedDevicePathSize) ||
> > +          (RemainedDevicePathSize != GetDevicePathSize (&DeviceBuffer-
> > >OpalDevicePath))) {
> > +        Status = EFI_SUCCESS;
> > +        goto EXIT;
> > +      }
> > +
> > +      if (OpalPasswordIsFullZero (DeviceBuffer->Password) ||
> > +          DeviceBuffer->PasswordLength == 0 ||
> > +          DeviceBuffer->PasswordLength >
> OPAL_PASSWORD_MAX_LENGTH) {
> > +        Status = EFI_INVALID_PARAMETER;
> > +        goto EXIT;
> > +      }
> > +
> > +      Status = OpalSavePasswordToSmm (&DeviceBuffer->OpalDevicePath,
> > + DeviceBuffer->Password, DeviceBuffer->PasswordLength);
> >        break;
> >
> >      default:
> > @@ -701,6 +718,10 @@ SmmOpalPasswordHandler (
> >  EXIT:
> >    SmmFunctionHeader->ReturnStatus = Status;
> >
> > +  if (DeviceBuffer != NULL) {
> > +    FreePool (DeviceBuffer);
> > +  }
> > +
> >    //
> >    // Return EFI_SUCCESS cause only one handler can be trigged.
> >    // so return EFI_WARN_INTERRUPT_SOURCE_PENDING to make all
> handler
> > can be trigged.
> > diff --git
> >
> a/SecurityPkg/Library/OpalPasswordSupportLib/OpalPasswordSupportNotify.h
> >
> b/SecurityPkg/Library/OpalPasswordSupportLib/OpalPasswordSupportNotify.
> h
> > index f0ad3a1136..d543faed5d 100644
> > ---
> >
> a/SecurityPkg/Library/OpalPasswordSupportLib/OpalPasswordSupportNotify.h
> > +++
> > b/SecurityPkg/Library/OpalPasswordSupportLib/OpalPasswordSupportNoti
> > +++ fy.h
> > @@ -41,7 +41,7 @@ typedef struct {
> >  } OPAL_SMM_COMMUNICATE_HEADER;
> >
> >  typedef struct {
> > -  UINT8                      Password[32];
> > +  UINT8
> Password[OPAL_PASSWORD_MAX_LENGTH];
> >    UINT8                      PasswordLength;
> >
> >    EFI_DEVICE_PATH_PROTOCOL   OpalDevicePath;
> > diff --git a/SecurityPkg/Tcg/Opal/OpalPasswordSmm/OpalPasswordSmm.h
> > b/SecurityPkg/Tcg/Opal/OpalPasswordSmm/OpalPasswordSmm.h
> > index ce88786fab..bc559f0bd1 100644
> > --- a/SecurityPkg/Tcg/Opal/OpalPasswordSmm/OpalPasswordSmm.h
> > +++ b/SecurityPkg/Tcg/Opal/OpalPasswordSmm/OpalPasswordSmm.h
> > @@ -64,10 +64,6 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF
> > ANY KIND, EITHER EXPRESS OR IMPLIED.
> >  // The payload Length of HDD related ATA commands  //
> >  #define HDD_PAYLOAD                      512
> > -//
> > -// According to ATA spec, the max Length of hdd password is 32 bytes -//
> > -#define OPAL_PASSWORD_MAX_LENGTH         32
> >
> >  extern VOID                              *mBuffer;
> >
> > @@ -124,7 +120,7 @@ typedef struct {
> >
> >    UINT32                                   NvmeNamespaceId;
> >
> > -  UINT8                                    Password[32];
> > +  UINT8
> Password[OPAL_PASSWORD_MAX_LENGTH];
> >    UINT8                                    PasswordLength;
> >
> >    UINT32                                   Length;
> > --
> > 2.15.0.windows.1
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel