[RFC v1 5/4] CryptoPkg/TlsLib: accept peer certs via both DNS names and IP addresses

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

SSL_set1_host() in TlsSetVerifyHost() ignores GEN_IP entries in the peer
certificate's Subject Alternative Name (SAN) extension. This leads to the
rejection of any valid peer certificate that matches the dot-decimal IPv4,
or colon-hexadecimal IPv6, host part of an URL *only* through SAN/GEN_IP,
and not through the Common Name.

Based on David's guidance, replace SSL_set1_host() in TlsSetVerifyHost()
with application specific data ("ExData") that is associated with the SSL
object. Namely, pass the host part of the URL as "application specific
data" into a new peer certificate verification callback. In the callback,
first try to parse the host part of the URL as a numeric IP address, for
certificate subject verification. If that parsing fails, fall back to
interpreting the host part as a DNS hostname.

Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Richard Levitte <levitte@openssl.org>
Cc: Sivaraman Nainar <sivaramann@amiindia.co.in>
Ref: http://mid.mail-archive.com/B4DE137BDB63634BAC03BD9DE765F197028B24CA23@VENUS1.in.megatrends.com
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=960
Ref: https://edk2.groups.io/g/devel/message/42022
Suggested-by: David Woodhouse <dwmw2@infradead.org>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
    Unfortunately, there are two problems with this patch:
    
    (1) X509_VERIFY_PARAM_set1_ip_asc() does not accept IPv4 addresses in
        dot-decimal notation (unless I messed up the code). My log file
        contains:
    
    > TlsDxe:TlsCertVerify: verifying peer certificate with DNS hostname "192.168.124.2"
    > TlsDxe:TlsCertVerify: peer certificate accepted
    
    (2) X509_VERIFY_PARAM_set1_ip_asc() does accept IPv6 addresses. However,
        in that case, the server certificate that I had generated with
        "genkey" (where I entered the IPv6 address in the Common Name field)
        is rejected:
    
    > TlsDxe:TlsCertVerify: verifying peer certificate with numerical IP address "fd33:eb1b:9b36::2"
    > TlsDxe:TlsCertVerify: peer certificate rejected
    > TlsDoHandshake SSL_HANDSHAKE_ERROR State=0x4 SSL_ERROR_SSL
    > TlsDoHandshake ERROR 0x1416F086=L14:F16F:R86
    
        If I do not apply the present patch on top of Jiaxin's v1 4/4 (at
        <http://mid.mail-archive.com/20190927034441.3096-5-Jiaxin.wu@intel.com>),
        then the certificate is accepted fine.
    
    Not sure how to address these.

 CryptoPkg/Library/TlsLib/TlsLib.inf       |   1 +
 CryptoPkg/Library/TlsLib/InternalTlsLib.h |  33 +++
 CryptoPkg/Library/TlsLib/TlsConfig.c      |  17 +-
 CryptoPkg/Library/TlsLib/TlsExData.c      | 301 ++++++++++++++++++++
 CryptoPkg/Library/TlsLib/TlsInit.c        |  35 ++-
 5 files changed, 385 insertions(+), 2 deletions(-)

diff --git a/CryptoPkg/Library/TlsLib/TlsLib.inf b/CryptoPkg/Library/TlsLib/TlsLib.inf
index 2f3ce695c33e..1f65eea516d4 100644
--- a/CryptoPkg/Library/TlsLib/TlsLib.inf
+++ b/CryptoPkg/Library/TlsLib/TlsLib.inf
@@ -24,12 +24,13 @@ [Defines]
 
 [Sources]
   InternalTlsLib.h
   TlsInit.c
   TlsConfig.c
   TlsProcess.c
+  TlsExData.c
 
 [Packages]
   MdePkg/MdePkg.dec
   CryptoPkg/CryptoPkg.dec
 
 [LibraryClasses]
diff --git a/CryptoPkg/Library/TlsLib/InternalTlsLib.h b/CryptoPkg/Library/TlsLib/InternalTlsLib.h
index ce7f4ced4a30..c8762befd31c 100644
--- a/CryptoPkg/Library/TlsLib/InternalTlsLib.h
+++ b/CryptoPkg/Library/TlsLib/InternalTlsLib.h
@@ -34,8 +34,41 @@ typedef struct {
   //
   // Memory BIO for the TLS/SSL Writing operations.
   //
   BIO                             *OutBio;
 } TLS_CONNECTION;
 
+//
+// See the documentation for "mPeerSubjectNameKey",
+// TlsPeerSubjectNameDuplicate(), TlsPeerSubjectNameFree(), and TlsCertVerify()
+// in "TlsExData.c".
+//
+extern INT32 mPeerSubjectNameKey;
+
+INT32
+TlsPeerSubjectNameDuplicate (
+  OUT    CRYPTO_EX_DATA       *DestinationExData,
+  IN     CONST CRYPTO_EX_DATA *SourceExData,
+  IN OUT VOID                 *PeerSubjectNameAddress,
+  IN     INT32                ExDataType,
+  IN     long                 ArgLong,
+  IN     VOID                 *ArgPtr
+  );
+
+VOID
+TlsPeerSubjectNameFree (
+  IN VOID           *ParentSsl,
+  IN VOID           *PeerSubjectName OPTIONAL,
+  IN CRYPTO_EX_DATA *ExData,
+  IN INT32          ExDataType,
+  IN long           ArgLong,
+  IN VOID           *ArgPtr
+  );
+
+INT32
+TlsCertVerify (
+  IN X509_STORE_CTX *PeerCertificateChain,
+  IN VOID           *Arg
+  );
+
 #endif
 
diff --git a/CryptoPkg/Library/TlsLib/TlsConfig.c b/CryptoPkg/Library/TlsLib/TlsConfig.c
index 2bf5aee7c093..114168dfb020 100644
--- a/CryptoPkg/Library/TlsLib/TlsConfig.c
+++ b/CryptoPkg/Library/TlsLib/TlsConfig.c
@@ -504,32 +504,47 @@ TlsSetVerify (
   @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.
+  @retval  EFI_OUT_OF_RESOURCES  Memory allocation failure.
 
 **/
 EFI_STATUS
 EFIAPI
 TlsSetVerifyHost (
   IN     VOID                     *Tls,
   IN     UINT32                   Flags,
   IN     CHAR8                    *HostName
   )
 {
   TLS_CONNECTION  *TlsConn;
+  CHAR8           *PeerSubjectName;
 
   TlsConn = (TLS_CONNECTION *) Tls;
   if (TlsConn == NULL || TlsConn->Ssl == NULL || HostName == NULL) {
      return EFI_INVALID_PARAMETER;
   }
 
+  PeerSubjectName = AllocateCopyPool (
+                      AsciiStrSize (HostName),
+                      HostName
+                      );
+  if (PeerSubjectName == NULL) {
+    return EFI_OUT_OF_RESOURCES;
+  }
+
   SSL_set_hostflags(TlsConn->Ssl, Flags);
 
-  if (SSL_set1_host(TlsConn->Ssl, HostName) == 0) {
+  if (SSL_set_ex_data (
+        TlsConn->Ssl,
+        mPeerSubjectNameKey,
+        PeerSubjectName
+        ) == 0) {
+    FreePool (PeerSubjectName);
     return EFI_ABORTED;
   }
 
   return EFI_SUCCESS;
 }
 
diff --git a/CryptoPkg/Library/TlsLib/TlsExData.c b/CryptoPkg/Library/TlsLib/TlsExData.c
new file mode 100644
index 000000000000..9671234f8416
--- /dev/null
+++ b/CryptoPkg/Library/TlsLib/TlsExData.c
@@ -0,0 +1,301 @@
+/** @file
+  OpenSSL callback functions for:
+
+  - duplicating and freeing the Peer Subject Name strings that we associate
+    with SSL objects as application data ("ExData"),
+
+  - verifying peer certificates against the Subject Name stings associated with
+    SSL objects.
+
+  Copyright (C) 2019, Red Hat, Inc.
+
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#include "InternalTlsLib.h"
+
+//
+// We attach the Subject Name that we expect the peer certificate to match to
+// the SSL object as an application-specific datum. This type of
+// application-specific data first needs to be registered with OpenSSL. The
+// registration identifier is stored in the object below.
+//
+// We define the associated data type as (CHAR8*), pointing to a
+// dynamically-allocated, NUL-terminated ASCII string. The string may contain a
+// DNS hostname, or an IPv4 address in dotted decimal notation, or an IPv6
+// address in colon-separated hexadecimal notation (without the surrounding
+// brackets used in URLs). The condensed "::" notation is supported for IPv6
+// addresses.
+//
+INT32 mPeerSubjectNameKey;
+
+/**
+  OpenSSL callback function for duplicating the Subject Name when its parent
+  SSL object is duplicated.
+
+  Because this function is an OpenSSL callback, it must not be declared EFIAPI.
+
+  @param[out] DestinationExData          The ExData object in the new SSL
+                                         object. DestinationExData is the
+                                         dictionary in which
+                                         mPeerSubjectNameKey identifies the new
+                                         (duplicated) subject name. Ignored.
+
+  @param[in] SourceExData                The ExData object in the original SSL
+                                         object. SourceExData is the dictionary
+                                         in which mPeerSubjectNameKey
+                                         identifies the subject name to
+                                         duplicate. Ignored.
+
+  @param[in,out] PeerSubjectNameAddress  On input,
+                                         *(VOID**)PeerSubjectNameAddress points
+                                         to the Subject Name in SourceExData.
+                                         On output,
+                                         *(VOID**)PeerSubjectNameAddress points
+                                         to the newly allocated copy of the
+                                         Subject Name, to be stored in
+                                         DestinationExData. On input,
+                                         PeerSubjectNameAddress must not be
+                                         NULL, but
+                                         *(VOID**)PeerSubjectNameAddress may be
+                                         NULL.
+
+  @param[in] ExDataType                  Equals mPeerSubjectNameKey. Ignored.
+
+  @param[in] ArgLong                     Zero; ignored.
+
+  @param[in] ArgPtr                      NULL; ignored.
+
+  @retval 0  Memory allocation failure.
+
+  @retval 1  Successful duplication (including a NULL subject name, when
+             nothing is done).
+**/
+INT32
+TlsPeerSubjectNameDuplicate (
+  OUT    CRYPTO_EX_DATA       *DestinationExData,
+  IN     CONST CRYPTO_EX_DATA *SourceExData,
+  IN OUT VOID                 *PeerSubjectNameAddress,
+  IN     INT32                ExDataType,
+  IN     long                 ArgLong,
+  IN     VOID                 *ArgPtr
+  )
+{
+  CHAR8 *PeerSubjectName;
+  CHAR8 *NewPeerSubjectName;
+
+  //
+  // Assert that these input parameters match what we passed to
+  // SSL_get_ex_new_index() in TlsInitialize().
+  //
+  ASSERT (ExDataType == mPeerSubjectNameKey);
+  ASSERT (ArgLong == 0);
+  ASSERT (ArgPtr == NULL);
+
+  //
+  // Further assert non-nullity for PeerSubjectNameAddress.
+  //
+  ASSERT (PeerSubjectNameAddress != NULL);
+
+  PeerSubjectName = *(VOID **)PeerSubjectNameAddress;
+  if (PeerSubjectName == NULL) {
+    DEBUG ((DEBUG_VERBOSE, "%a:%a: nothing to copy\n", gEfiCallerBaseName,
+      __FUNCTION__));
+    //
+    // Exit with success.
+    //
+    return 1;
+  }
+
+  NewPeerSubjectName = AllocateCopyPool (
+                         AsciiStrSize (PeerSubjectName),
+                         PeerSubjectName
+                         );
+  if (NewPeerSubjectName == NULL) {
+    DEBUG ((DEBUG_ERROR, "%a:%a: failed to allocate memory\n",
+      gEfiCallerBaseName, __FUNCTION__));
+    return 0;
+  }
+
+  *(VOID **)PeerSubjectNameAddress = NewPeerSubjectName;
+  DEBUG ((DEBUG_VERBOSE,
+    "%a:%a: copied peer subject name \"%a\" from %p to %p\n",
+    gEfiCallerBaseName, __FUNCTION__, PeerSubjectName, (VOID *)PeerSubjectName,
+    (VOID *)NewPeerSubjectName));
+  return 1;
+}
+
+/**
+  OpenSSL callback function for freeing the Subject Name when its parent SSL
+  object is freed.
+
+  Because this function is an OpenSSL callback, it must not be declared EFIAPI.
+
+  @param[in] ParentSsl        The parent SSL object being freed. Ignored.
+
+  @param[in] PeerSubjectName  The subject name to release. May be NULL.
+
+  @param[in] ExData           The ExData object in ParentSsl. ExData is the
+                              dictionary in which mPeerSubjectNameKey
+                              identifies the subject name to release. Ignored.
+
+  @param[in] ExDataType       Equals mPeerSubjectNameKey. Ignored.
+
+  @param[in] ArgLong          Zero; ignored.
+
+  @param[in] ArgPtr           NULL; ignored.
+**/
+VOID
+TlsPeerSubjectNameFree (
+  IN VOID           *ParentSsl,
+  IN VOID           *PeerSubjectName OPTIONAL,
+  IN CRYPTO_EX_DATA *ExData,
+  IN INT32          ExDataType,
+  IN long           ArgLong,
+  IN VOID           *ArgPtr
+  )
+{
+  //
+  // Assert that these input parameters match what we passed to
+  // SSL_get_ex_new_index() in TlsInitialize().
+  //
+  ASSERT (ExDataType == mPeerSubjectNameKey);
+  ASSERT (ArgLong == 0);
+  ASSERT (ArgPtr == NULL);
+
+  if (PeerSubjectName == NULL) {
+    return;
+  }
+
+  DEBUG ((DEBUG_VERBOSE, "%a:%a: freeing peer subject name \"%a\" at %p\n",
+    gEfiCallerBaseName, __FUNCTION__, (CHAR8 *)PeerSubjectName,
+    PeerSubjectName));
+  FreePool (PeerSubjectName);
+}
+
+/**
+  OpenSSL callback function for discovering and verifying the X509 peer
+  certificate chain during SSL/TLS handshake.
+
+  This function wraps the X509_verify_cert() OpenSSL function; it ensures that
+  both DNS host names and numeric IPv4/IPv6 addresses are matched in peer
+  certificates as Subject Names.
+
+  Because this function is an OpenSSL callback, it must not be declared EFIAPI.
+
+  @param[in] PeerCertificateChain  The certificate chain of the peer to verify.
+                                   The function checks whether
+                                   PeerCertificateChain matches the Peer
+                                   Subject Name that we've associated with the
+                                   SSL object of the network connection.
+
+  @param[in] Arg                   NULL; ignored.
+
+  @retval 1  Verification success.
+
+  @retval 0  Verification failure.
+**/
+INT32
+TlsCertVerify (
+  IN X509_STORE_CTX *PeerCertificateChain,
+  IN VOID           *Arg
+  )
+{
+  SSL               *Ssl;
+  X509_VERIFY_PARAM *VerifyParams;
+  CHAR8             *SubjectName;
+  INT32             ParamStatus;
+  INT32             VerifyStatus;
+
+  //
+  // Assert that these input parameters match what we passed to
+  // SSL_CTX_set_cert_verify_callback() in TlsCtxNew().
+  //
+  ASSERT (Arg == NULL);
+
+  //
+  // Retrieve the SSL object associated with the network connection for which
+  // the peer certificate is being verified in the SSL/TLS handshake.
+  //
+  Ssl = X509_STORE_CTX_get_ex_data (
+          PeerCertificateChain,
+          SSL_get_ex_data_X509_STORE_CTX_idx ()
+          );
+  if (Ssl == NULL) {
+    DEBUG ((DEBUG_ERROR, "%a:%a: SSL object not found\n", gEfiCallerBaseName,
+      __FUNCTION__));
+    //
+    // Reject the certificate.
+    //
+    return 0;
+  }
+
+  //
+  // Fetch the certificate verification parameters.
+  //
+  VerifyParams = X509_STORE_CTX_get0_param (PeerCertificateChain);
+  if (VerifyParams == NULL) {
+    DEBUG ((DEBUG_ERROR, "%a:%a: verification parameters not found\n",
+      gEfiCallerBaseName, __FUNCTION__));
+    return 0;
+  }
+
+  //
+  // Retrieve the Peer Subject Name that we *may* have associated with the SSL
+  // object in TlsSetVerifyHost().
+  //
+  SubjectName = SSL_get_ex_data (Ssl, mPeerSubjectNameKey);
+  //
+  // If SubjectName is NULL or empty, explicitly clear the list of host names
+  // in VerifyParams, and perform no name checks on the peer certificate.
+  //
+  // Otherwise, attempt to parse the Peer Subject Name as an IPv4 or IPv6
+  // address. If this succeeds, then the parsed address is used for verifying
+  // the peer certificate.
+  //
+  // Otherwise, verify the peer certificate with SubjectName taken as a DNS
+  // hostname.
+  //
+  if (SubjectName == NULL || SubjectName[0] == '\0') {
+    ParamStatus = X509_VERIFY_PARAM_set1_host (VerifyParams, SubjectName, 0);
+
+    DEBUG ((DEBUG_WARN, "%a:%a: verifying peer certificate without subject "
+      "name check (MITM risk)!\n", gEfiCallerBaseName, __FUNCTION__));
+  } else {
+    ParamStatus = X509_VERIFY_PARAM_set1_ip_asc (VerifyParams, SubjectName);
+
+    if (ParamStatus == 1) {
+      DEBUG ((DEBUG_VERBOSE,
+        "%a:%a: verifying peer certificate with numerical IP address \"%a\"\n",
+        gEfiCallerBaseName, __FUNCTION__, SubjectName));
+    } else {
+      ParamStatus = X509_VERIFY_PARAM_set1_host (VerifyParams, SubjectName, 0);
+
+      DEBUG ((DEBUG_VERBOSE,
+        "%a:%a: verifying peer certificate with DNS hostname \"%a\"\n",
+        gEfiCallerBaseName, __FUNCTION__, SubjectName));
+    }
+  }
+
+  if (ParamStatus == 0) {
+    DEBUG ((DEBUG_ERROR,
+      "%a:%a: unexpected failure to set verification parameters\n",
+      gEfiCallerBaseName, __FUNCTION__));
+    //
+    // Reject the certificate.
+    //
+    return 0;
+  }
+
+  VerifyStatus = X509_verify_cert (PeerCertificateChain);
+
+  if (VerifyStatus > 0) {
+    DEBUG ((DEBUG_VERBOSE, "%a:%a: peer certificate accepted\n",
+      gEfiCallerBaseName, __FUNCTION__));
+    return 1;
+  }
+
+  DEBUG ((DEBUG_ERROR, "%a:%a: peer certificate rejected\n",
+    gEfiCallerBaseName, __FUNCTION__));
+  return 0;
+}
diff --git a/CryptoPkg/Library/TlsLib/TlsInit.c b/CryptoPkg/Library/TlsLib/TlsInit.c
index f9ad6f6b946c..c7918364a4c7 100644
--- a/CryptoPkg/Library/TlsLib/TlsInit.c
+++ b/CryptoPkg/Library/TlsLib/TlsInit.c
@@ -24,29 +24,53 @@ BOOLEAN
 EFIAPI
 TlsInitialize (
   VOID
   )
 {
   INTN            Ret;
+  BOOLEAN         RandomIsSeeded;
 
   //
   // Performs initialization of crypto and ssl library, and loads required
   // algorithms.
   //
   Ret = OPENSSL_init_ssl (
           OPENSSL_INIT_LOAD_SSL_STRINGS | OPENSSL_INIT_LOAD_CRYPTO_STRINGS,
           NULL
           );
   if (Ret != 1) {
     return FALSE;
   }
+  //
+  // OPENSSL_init_ssl() cannot, and need not, be rolled back, if the rest of
+  // this function fails.
+  //
+
+  mPeerSubjectNameKey = SSL_get_ex_new_index (
+                          0,                           // "argl": unneeded
+                          NULL,                        // "argp": unneeded
+                          NULL,                        // "new_func": unneeded
+                          TlsPeerSubjectNameDuplicate, // "dup_func"
+                          TlsPeerSubjectNameFree       // "free_func"
+                          );
+  if (mPeerSubjectNameKey == -1) {
+    return FALSE;
+  }
 
   //
   // Initialize the pseudorandom number generator.
   //
-  return RandomSeed (NULL, 0);
+  RandomIsSeeded = RandomSeed (NULL, 0);
+  if (!RandomIsSeeded) {
+    goto DeregisterPeerSubjectName;
+  }
+  return TRUE;
+
+DeregisterPeerSubjectName:
+  CRYPTO_free_ex_index (CRYPTO_EX_INDEX_SSL, mPeerSubjectNameKey);
+  return FALSE;
 }
 
 /**
   Free an allocated SSL_CTX object.
 
   @param[in]  TlsCtx    Pointer to the SSL_CTX object to be released.
@@ -103,12 +127,21 @@ TlsCtxNew (
   //
   // Treat as minimum accepted versions by setting the minimal bound.
   // Client can use higher TLS version if server supports it
   //
   SSL_CTX_set_min_proto_version (TlsCtx, ProtoVersion);
 
+  //
+  // Set peer certificate verification procedure.
+  //
+  SSL_CTX_set_cert_verify_callback (
+    TlsCtx,
+    TlsCertVerify,
+    NULL           // "arg": unneeded
+    );
+
   return (VOID *) TlsCtx;
 }
 
 /**
   Free an allocated TLS object.
 
-- 
2.19.1.3.g30247aa5d201