Re: [PATCH v2 6/8] CryptoPkg/TlsLib: TlsSetVerifyHost: parse IP address literals as such (CVE-2019-14553)

["Wang, Jian J" <jian.j.wang@intel.com>]

I'm aware of the change is necessary for the solution. But I'm not expert on TLS.
I'd others to give more professional opinions.

Acked-by: Jian J Wang <jian.j.wang@intel.com>

Regards,
Jian

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Saturday, October 26, 2019 1:37 PM
> To: edk2-devel-groups-io <devel@edk2.groups.io>
> Cc: David Woodhouse <dwmw2@infradead.org>; Wang, Jian J
> <jian.j.wang@intel.com>; Wu, Jiaxin <jiaxin.wu@intel.com>; Sivaraman Nainar
> <sivaramann@amiindia.co.in>; Lu, XiaoyuX <xiaoyux.lu@intel.com>
> Subject: [PATCH v2 6/8] CryptoPkg/TlsLib: TlsSetVerifyHost: parse IP address
> literals as such (CVE-2019-14553)
> 
> Using the inet_pton() function that we imported in the previous patches,
> recognize if "HostName" is an IP address literal, and then parse it into
> binary representation. Passing the latter to OpenSSL for server
> certificate validation is important, per RFC-2818
> <https://tools.ietf.org/html/rfc2818#section-3.1>:
> 
> > In some cases, the URI is specified as an IP address rather than a
> > hostname. In this case, the iPAddress subjectAltName must be present in
> > the certificate and must exactly match the IP in the URI.
> 
> Note: we cannot use X509_VERIFY_PARAM_set1_ip_asc() because in the
> OpenSSL
> version that is currently consumed by edk2, said function depends on
> sscanf() for parsing IPv4 literals. In
> "CryptoPkg/Library/BaseCryptLib/SysCall/CrtWrapper.c", we only provide an
> empty -- always failing -- stub for sscanf(), however.
> 
> 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>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=960
> CVE: CVE-2019-14553
> Suggested-by: David Woodhouse <dwmw2@infradead.org>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> 
> Notes:
>     v2:
>     - new patch
> 
>  CryptoPkg/Library/TlsLib/TlsConfig.c | 28 +++++++++++++++++---
>  1 file changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/CryptoPkg/Library/TlsLib/TlsConfig.c
> b/CryptoPkg/Library/TlsLib/TlsConfig.c
> index 2bf5aee7c093..307eb57896dc 100644
> --- a/CryptoPkg/Library/TlsLib/TlsConfig.c
> +++ b/CryptoPkg/Library/TlsLib/TlsConfig.c
> @@ -516,22 +516,42 @@ TlsSetVerifyHost (
>    IN     UINT32                   Flags,
>    IN     CHAR8                    *HostName
>    )
>  {
> -  TLS_CONNECTION  *TlsConn;
> +  TLS_CONNECTION    *TlsConn;
> +  X509_VERIFY_PARAM *VerifyParam;
> +  UINTN             BinaryAddressSize;
> +  UINT8             BinaryAddress[MAX (NS_INADDRSZ, NS_IN6ADDRSZ)];
> +  INTN              ParamStatus;
> 
>    TlsConn = (TLS_CONNECTION *) Tls;
>    if (TlsConn == NULL || TlsConn->Ssl == NULL || HostName == NULL) {
>       return EFI_INVALID_PARAMETER;
>    }
> 
>    SSL_set_hostflags(TlsConn->Ssl, Flags);
> 
> -  if (SSL_set1_host(TlsConn->Ssl, HostName) == 0) {
> -    return EFI_ABORTED;
> +  VerifyParam = SSL_get0_param (TlsConn->Ssl);
> +  ASSERT (VerifyParam != NULL);
> +
> +  BinaryAddressSize = 0;
> +  if (inet_pton (AF_INET6, HostName, BinaryAddress) == 1) {
> +    BinaryAddressSize = NS_IN6ADDRSZ;
> +  } else if (inet_pton (AF_INET, HostName, BinaryAddress) == 1) {
> +    BinaryAddressSize = NS_INADDRSZ;
> +  }
> +
> +  if (BinaryAddressSize > 0) {
> +    DEBUG ((DEBUG_VERBOSE, "%a:%a: parsed \"%a\" as an IPv%c address "
> +      "literal\n", gEfiCallerBaseName, __FUNCTION__, HostName,
> +      (UINTN)((BinaryAddressSize == NS_IN6ADDRSZ) ? '6' : '4')));
> +    ParamStatus = X509_VERIFY_PARAM_set1_ip (VerifyParam, BinaryAddress,
> +                    BinaryAddressSize);
> +  } else {
> +    ParamStatus = X509_VERIFY_PARAM_set1_host (VerifyParam, HostName, 0);
>    }
> 
> -  return EFI_SUCCESS;
> +  return (ParamStatus == 1) ? EFI_SUCCESS : EFI_ABORTED;
>  }
> 
>  /**
>    Sets a TLS/SSL session ID to be used during TLS/SSL connect.
> --
> 2.19.1.3.g30247aa5d201
>