Re: [PATCH 03/13] NetworkPkg/TlsDxe: verify DataSize for EfiTlsCipherList

["Fu, Siyuan" <siyuan.fu@intel.com>]

Reviewed-by: Fu Siyuan <siyuan.fu@intel.com>



> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Tuesday, April 3, 2018 10:52 PM
> To: edk2-devel-01 <edk2-devel@lists.01.org>
> Cc: Wu, Jiaxin <jiaxin.wu@intel.com>; Fu, Siyuan <siyuan.fu@intel.com>
> Subject: [PATCH 03/13] NetworkPkg/TlsDxe: verify DataSize for
> EfiTlsCipherList
> 
> TlsSetSessionData() shouldn't just ignore an incomplete EFI_TLS_CIPHER
> element at the end of "Data":
> 
> - Generally speaking, malformed input for a security API is best rejected
>   explicitly.
> 
> - Specifically speaking, the size of EFI_TLS_CIPHER is 2 bytes. If
>   DataSize is 1 on input, then the initial check for (DataSize == 0) will
>   fail, but then TlsSetCipherList() will be called with CipherNum=0.
> 
> Return EFI_INVALID_PARAMETER from TlsSetSessionData() if "Data" doesn't
> contain a whole number of EFI_TLS_CIPHER elements. While at it, introduce
> the dedicated variable CipherCount.
> 
> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
> Cc: Siyuan Fu <siyuan.fu@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=915
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  NetworkPkg/TlsDxe/TlsProtocol.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/NetworkPkg/TlsDxe/TlsProtocol.c
> b/NetworkPkg/TlsDxe/TlsProtocol.c
> index ad4c922c60bd..a5f95a098345 100644
> --- a/NetworkPkg/TlsDxe/TlsProtocol.c
> +++ b/NetworkPkg/TlsDxe/TlsProtocol.c
> @@ -35,12 +35,13 @@ EFI_TLS_PROTOCOL  mTlsProtocol = {
> 
>    @retval EFI_SUCCESS             The TLS session data is set
> successfully.
>    @retval EFI_INVALID_PARAMETER   One or more of the following conditions
> is TRUE:
>                                    This is NULL.
>                                    Data is NULL.
>                                    DataSize is 0.
> +                                  DataSize is invalid for DataType.
>    @retval EFI_UNSUPPORTED         The DataType is unsupported.
>    @retval EFI_ACCESS_DENIED       If the DataType is one of below:
>                                    EfiTlsClientRandom
>                                    EfiTlsServerRandom
>                                    EfiTlsKeyMaterial
>    @retval EFI_NOT_READY           Current TLS session state is NOT
> @@ -56,12 +57,13 @@ TlsSetSessionData (
>    IN     UINTN                         DataSize
>    )
>  {
>    EFI_STATUS                Status;
>    TLS_INSTANCE              *Instance;
>    UINT16                    *CipherId;
> +  UINTN                     CipherCount;
>    UINTN                     Index;
> 
>    EFI_TPL                   OldTpl;
> 
>    Status = EFI_SUCCESS;
>    CipherId = NULL;
> @@ -97,23 +99,29 @@ TlsSetSessionData (
>        goto ON_EXIT;
>      }
> 
>      Status = TlsSetConnectionEnd (Instance->TlsConn,
> *((EFI_TLS_CONNECTION_END *) Data));
>      break;
>    case EfiTlsCipherList:
> +    if (DataSize % sizeof (EFI_TLS_CIPHER) != 0) {
> +      Status = EFI_INVALID_PARAMETER;
> +      goto ON_EXIT;
> +    }
> +
>      CipherId = AllocatePool (DataSize);
>      if (CipherId == NULL) {
>        Status = EFI_OUT_OF_RESOURCES;
>        goto ON_EXIT;
>      }
> 
> -    for (Index = 0; Index < DataSize / sizeof (EFI_TLS_CIPHER); Index++)
> {
> +    CipherCount = DataSize / sizeof (EFI_TLS_CIPHER);
> +    for (Index = 0; Index < CipherCount; Index++) {
>        *(CipherId +Index) = HTONS (*(((UINT16 *) Data) + Index));
>      }
> 
> -    Status = TlsSetCipherList (Instance->TlsConn, CipherId, DataSize /
> sizeof (EFI_TLS_CIPHER));
> +    Status = TlsSetCipherList (Instance->TlsConn, CipherId, CipherCount);
> 
>      FreePool (CipherId);
>      break;
>    case EfiTlsCompressionMethod:
>      //
>      // TLS seems only define one CompressionMethod.null, which specifies
> that data exchanged via the
> --
> 2.14.1.3.gb7cf6e02401b
>