Re: [edk2-devel] [PATCH v2 2/8] CryptoPkg/TlsLib: Add the new API "TlsSetVerifyHost" (CVE-2019-14553)

["Laszlo Ersek" <lersek@redhat.com>]

On 10/26/19 13:51, Philippe Mathieu-Daudé wrote:
> On 10/26/19 7:37 AM, Laszlo Ersek wrote:
>> From: "Wu, Jiaxin" <jiaxin.wu@intel.com>
>>
>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=960
>> CVE: CVE-2019-14553
>> In the patch, we add the new API "TlsSetVerifyHost" for the TLS
>> protocol to set the specified host name that need to be verified.
>>
>> Signed-off-by: Wu Jiaxin <jiaxin.wu@intel.com>
>> Reviewed-by: Ye Ting <ting.ye@intel.com>
>> Reviewed-by: Long Qin <qin.long@intel.com>
>> Reviewed-by: Fu Siyuan <siyuan.fu@intel.com>
>> Acked-by: Laszlo Ersek <lersek@redhat.com>
>> Message-Id: <20190927034441.3096-3-Jiaxin.wu@intel.com>
>> Cc: David Woodhouse <dwmw2@infradead.org>
>> Cc: Jian J Wang <jian.j.wang@intel.com>
>> Cc: Jiaxin Wu <jiaxin.wu@intel.com>
>> Cc: Sivaraman Nainar <sivaramann@amiindia.co.in>
>> Cc: Xiaoyu Lu <xiaoyux.lu@intel.com>
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>
>> Notes:
>>      v2:
>>      - fix whitespace in subject line
>>      - drop Contributed-under line per BZ#1373
>>
>>   CryptoPkg/Include/Library/TlsLib.h   | 20 +++++++++++
>>   CryptoPkg/Library/TlsLib/TlsConfig.c | 38 +++++++++++++++++++-
>>   2 files changed, 57 insertions(+), 1 deletion(-)
>>
>> diff --git a/CryptoPkg/Include/Library/TlsLib.h
>> b/CryptoPkg/Include/Library/TlsLib.h
>> index 9875cb6e746b..3af7d4bc095e 100644
>> --- a/CryptoPkg/Include/Library/TlsLib.h
>> +++ b/CryptoPkg/Include/Library/TlsLib.h
>> @@ -395,8 +395,28 @@ TlsSetVerify (
>>     IN     VOID                     *Tls,
>>     IN     UINT32                   VerifyMode
>>     );
>>   +/**
>> +  Set the specified host name to be verified.
>> +
>> +  @param[in]  Tls           Pointer to the TLS object.
>> +  @param[in]  Flags         The setting flags during the validation.
>> +  @param[in]  HostName      The specified host name to be verified.
>> +
>> +  @retval  EFI_SUCCESS           The HostName setting was set
>> successfully.
>> +  @retval  EFI_INVALID_PARAMETER The parameter is invalid.
>> +  @retval  EFI_ABORTED           Invalid HostName setting.
>> +
>> +**/
>> +EFI_STATUS
>> +EFIAPI
>> +TlsSetVerifyHost (
>> +  IN     VOID                     *Tls,
>> +  IN     UINT32                   Flags,
>> +  IN     CHAR8                    *HostName
>> +  );
>> +
>>   /**
>>     Sets a TLS/SSL session ID to be used during TLS/SSL connect.
>>       This function sets a session ID to be used when the TLS/SSL
>> connection is
>> diff --git a/CryptoPkg/Library/TlsLib/TlsConfig.c
>> b/CryptoPkg/Library/TlsLib/TlsConfig.c
>> index 74b577d60ee3..2bf5aee7c093 100644
>> --- a/CryptoPkg/Library/TlsLib/TlsConfig.c
>> +++ b/CryptoPkg/Library/TlsLib/TlsConfig.c
>> @@ -1,8 +1,8 @@
>>   /** @file
>>     SSL/TLS Configuration Library Wrapper Implementation over OpenSSL.
>>   -Copyright (c) 2016 - 2017, Intel Corporation. All rights reserved.<BR>
>> +Copyright (c) 2016 - 2018, Intel Corporation. All rights reserved.<BR>
>>   (C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR>
>>   SPDX-License-Identifier: BSD-2-Clause-Patent
>>     **/
>> @@ -496,8 +496,44 @@ TlsSetVerify (
>>     //
>>     SSL_set_verify (TlsConn->Ssl, VerifyMode, NULL);
>>   }
>>   +/**
>> +  Set the specified host name to be verified.
>> +
>> +  @param[in]  Tls           Pointer to the TLS object.
>> +  @param[in]  Flags         The setting flags during the validation.
>> +  @param[in]  HostName      The specified host name to be verified.
>> +
>> +  @retval  EFI_SUCCESS           The HostName setting was set
>> successfully.
>> +  @retval  EFI_INVALID_PARAMETER The parameter is invalid.
>> +  @retval  EFI_ABORTED           Invalid HostName setting.
>> +
>> +**/
>> +EFI_STATUS
>> +EFIAPI
>> +TlsSetVerifyHost (
>> +  IN     VOID                     *Tls,
>> +  IN     UINT32                   Flags,
>> +  IN     CHAR8                    *HostName
>> +  )
>> +{
>> +  TLS_CONNECTION  *TlsConn;
>> +
>> +  TlsConn = (TLS_CONNECTION *) Tls;
>> +  if (TlsConn == NULL || TlsConn->Ssl == NULL || HostName == NULL) {
> 
> Nitpicking, I'd check HostName first.

Valid point... but this BZ / CVE has been in flight for ~1.5 years
now... Let me just run with this patch please.

> Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>

Thanks!
Laszlo

> 
>> +     return EFI_INVALID_PARAMETER;
>> +  }
>> +
>> +  SSL_set_hostflags(TlsConn->Ssl, Flags);
>> +
>> +  if (SSL_set1_host(TlsConn->Ssl, HostName) == 0) {
>> +    return EFI_ABORTED;
>> +  }
>> +
>> +  return EFI_SUCCESS;
>> +}
>> +
>>   /**
>>     Sets a TLS/SSL session ID to be used during TLS/SSL connect.
>>       This function sets a session ID to be used when the TLS/SSL
>> connection is
>>
> 
> 
>