From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by mx.groups.io with SMTP id smtpd.web10.2940.1571180923936483513 for ; Tue, 15 Oct 2019 16:08:44 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 312133084499; Tue, 15 Oct 2019 23:08:43 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-117-240.ams2.redhat.com [10.36.117.240]) by smtp.corp.redhat.com (Postfix) with ESMTP id 24F5C5D6A9; Tue, 15 Oct 2019 23:08:40 +0000 (UTC) From: "Laszlo Ersek" To: edk2-devel-groups-io Cc: Bret Barkelew , David Woodhouse , Jian J Wang , Jiaxin Wu , Richard Levitte , Sivaraman Nainar Subject: [RFC v1 5/4] CryptoPkg/TlsLib: accept peer certs via both DNS names and IP addresses Date: Wed, 16 Oct 2019 01:08:39 +0200 Message-Id: <20191015230839.27708-1-lersek@redhat.com> In-Reply-To: <20190927034441.3096-1-Jiaxin.wu@intel.com> References: <20190927034441.3096-1-Jiaxin.wu@intel.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.40]); Tue, 15 Oct 2019 23:08:43 +0000 (UTC) Content-Transfer-Encoding: quoted-printable 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 Cc: David Woodhouse Cc: Jian J Wang Cc: Jiaxin Wu Cc: Richard Levitte Cc: Sivaraman Nainar Ref: http://mid.mail-archive.com/B4DE137BDB63634BAC03BD9DE765F197028B24CA= 23@VENUS1.in.megatrends.com Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3D960 Ref: https://edk2.groups.io/g/devel/message/42022 Suggested-by: David Woodhouse Signed-off-by: Laszlo Ersek --- Notes: Unfortunately, there are two problems with this patch: =20 (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: =20 > TlsDxe:TlsCertVerify: verifying peer certificate with DNS hostname = "192.168.124.2" > TlsDxe:TlsCertVerify: peer certificate accepted =20 (2) X509_VERIFY_PARAM_set1_ip_asc() does accept IPv6 addresses. Howev= er, in that case, the server certificate that I had generated with "genkey" (where I entered the IPv6 address in the Common Name fie= ld) is rejected: =20 > TlsDxe:TlsCertVerify: verifying peer certificate with numerical IP = address "fd33:eb1b:9b36::2" > TlsDxe:TlsCertVerify: peer certificate rejected > TlsDoHandshake SSL_HANDSHAKE_ERROR State=3D0x4 SSL_ERROR_SSL > TlsDoHandshake ERROR 0x1416F086=3DL14:F16F:R86 =20 If I do not apply the present patch on top of Jiaxin's v1 4/4 (at ), then the certificate is accepted fine. =20 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/TlsL= ib/TlsLib.inf index 2f3ce695c33e..1f65eea516d4 100644 --- a/CryptoPkg/Library/TlsLib/TlsLib.inf +++ b/CryptoPkg/Library/TlsLib/TlsLib.inf @@ -24,12 +24,13 @@ [Defines] =20 [Sources] InternalTlsLib.h TlsInit.c TlsConfig.c TlsProcess.c + TlsExData.c =20 [Packages] MdePkg/MdePkg.dec CryptoPkg/CryptoPkg.dec =20 [LibraryClasses] diff --git a/CryptoPkg/Library/TlsLib/InternalTlsLib.h b/CryptoPkg/Librar= y/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; =20 +// +// See the documentation for "mPeerSubjectNameKey", +// TlsPeerSubjectNameDuplicate(), TlsPeerSubjectNameFree(), and TlsCertV= erify() +// 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 =20 diff --git a/CryptoPkg/Library/TlsLib/TlsConfig.c b/CryptoPkg/Library/Tls= Lib/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. =20 @retval EFI_SUCCESS The HostName setting was set successful= ly. @retval EFI_INVALID_PARAMETER The parameter is invalid. @retval EFI_ABORTED Invalid HostName setting. + @retval EFI_OUT_OF_RESOURCES Memory allocation failure. =20 **/ EFI_STATUS EFIAPI TlsSetVerifyHost ( IN VOID *Tls, IN UINT32 Flags, IN CHAR8 *HostName ) { TLS_CONNECTION *TlsConn; + CHAR8 *PeerSubjectName; =20 TlsConn =3D (TLS_CONNECTION *) Tls; if (TlsConn =3D=3D NULL || TlsConn->Ssl =3D=3D NULL || HostName =3D=3D= NULL) { return EFI_INVALID_PARAMETER; } =20 + PeerSubjectName =3D AllocateCopyPool ( + AsciiStrSize (HostName), + HostName + ); + if (PeerSubjectName =3D=3D NULL) { + return EFI_OUT_OF_RESOURCES; + } + SSL_set_hostflags(TlsConn->Ssl, Flags); =20 - if (SSL_set1_host(TlsConn->Ssl, HostName) =3D=3D 0) { + if (SSL_set_ex_data ( + TlsConn->Ssl, + mPeerSubjectNameKey, + PeerSubjectName + ) =3D=3D 0) { + FreePool (PeerSubjectName); return EFI_ABORTED; } =20 return EFI_SUCCESS; } =20 diff --git a/CryptoPkg/Library/TlsLib/TlsExData.c b/CryptoPkg/Library/Tls= Lib/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 associ= ate + with SSL objects as application data ("ExData"), + + - verifying peer certificates against the Subject Name stings associat= ed 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 mat= ch 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 co= ntain a +// DNS hostname, or an IPv4 address in dotted decimal notation, or an IP= v6 +// address in colon-separated hexadecimal notation (without the surround= ing +// brackets used in URLs). The condensed "::" notation is supported for = IPv6 +// addresses. +// +INT32 mPeerSubjectNameKey; + +/** + OpenSSL callback function for duplicating the Subject Name when its pa= rent + 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 SS= L + object. DestinationExData is th= e + dictionary in which + mPeerSubjectNameKey identifies = the new + (duplicated) subject name. Igno= red. + + @param[in] SourceExData The ExData object in the origin= al SSL + object. SourceExData is the dic= tionary + in which mPeerSubjectNameKey + identifies the subject name to + duplicate. Ignored. + + @param[in,out] PeerSubjectNameAddress On input, + *(VOID**)PeerSubjectNameAddress= points + to the Subject Name in SourceEx= Data. + 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. Ign= ored. + + @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 =3D=3D mPeerSubjectNameKey); + ASSERT (ArgLong =3D=3D 0); + ASSERT (ArgPtr =3D=3D NULL); + + // + // Further assert non-nullity for PeerSubjectNameAddress. + // + ASSERT (PeerSubjectNameAddress !=3D NULL); + + PeerSubjectName =3D *(VOID **)PeerSubjectNameAddress; + if (PeerSubjectName =3D=3D NULL) { + DEBUG ((DEBUG_VERBOSE, "%a:%a: nothing to copy\n", gEfiCallerBaseNam= e, + __FUNCTION__)); + // + // Exit with success. + // + return 1; + } + + NewPeerSubjectName =3D AllocateCopyPool ( + AsciiStrSize (PeerSubjectName), + PeerSubjectName + ); + if (NewPeerSubjectName =3D=3D NULL) { + DEBUG ((DEBUG_ERROR, "%a:%a: failed to allocate memory\n", + gEfiCallerBaseName, __FUNCTION__)); + return 0; + } + + *(VOID **)PeerSubjectNameAddress =3D NewPeerSubjectName; + DEBUG ((DEBUG_VERBOSE, + "%a:%a: copied peer subject name \"%a\" from %p to %p\n", + gEfiCallerBaseName, __FUNCTION__, PeerSubjectName, (VOID *)PeerSubje= ctName, + (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. Ig= nored. + + @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 =3D=3D mPeerSubjectNameKey); + ASSERT (ArgLong =3D=3D 0); + ASSERT (ArgPtr =3D=3D NULL); + + if (PeerSubjectName =3D=3D 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 ensure= s that + both DNS host names and numeric IPv4/IPv6 addresses are matched in pee= r + 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 wi= th 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 =3D=3D NULL); + + // + // Retrieve the SSL object associated with the network connection for = which + // the peer certificate is being verified in the SSL/TLS handshake. + // + Ssl =3D X509_STORE_CTX_get_ex_data ( + PeerCertificateChain, + SSL_get_ex_data_X509_STORE_CTX_idx () + ); + if (Ssl =3D=3D NULL) { + DEBUG ((DEBUG_ERROR, "%a:%a: SSL object not found\n", gEfiCallerBase= Name, + __FUNCTION__)); + // + // Reject the certificate. + // + return 0; + } + + // + // Fetch the certificate verification parameters. + // + VerifyParams =3D X509_STORE_CTX_get0_param (PeerCertificateChain); + if (VerifyParams =3D=3D 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 t= he SSL + // object in TlsSetVerifyHost(). + // + SubjectName =3D 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 IPv= 6 + // address. If this succeeds, then the parsed address is used for veri= fying + // the peer certificate. + // + // Otherwise, verify the peer certificate with SubjectName taken as a = DNS + // hostname. + // + if (SubjectName =3D=3D NULL || SubjectName[0] =3D=3D '\0') { + ParamStatus =3D X509_VERIFY_PARAM_set1_host (VerifyParams, SubjectNa= me, 0); + + DEBUG ((DEBUG_WARN, "%a:%a: verifying peer certificate without subje= ct " + "name check (MITM risk)!\n", gEfiCallerBaseName, __FUNCTION__)); + } else { + ParamStatus =3D X509_VERIFY_PARAM_set1_ip_asc (VerifyParams, Subject= Name); + + if (ParamStatus =3D=3D 1) { + DEBUG ((DEBUG_VERBOSE, + "%a:%a: verifying peer certificate with numerical IP address \"%= a\"\n", + gEfiCallerBaseName, __FUNCTION__, SubjectName)); + } else { + ParamStatus =3D X509_VERIFY_PARAM_set1_host (VerifyParams, Subject= Name, 0); + + DEBUG ((DEBUG_VERBOSE, + "%a:%a: verifying peer certificate with DNS hostname \"%a\"\n", + gEfiCallerBaseName, __FUNCTION__, SubjectName)); + } + } + + if (ParamStatus =3D=3D 0) { + DEBUG ((DEBUG_ERROR, + "%a:%a: unexpected failure to set verification parameters\n", + gEfiCallerBaseName, __FUNCTION__)); + // + // Reject the certificate. + // + return 0; + } + + VerifyStatus =3D 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/TlsLi= b/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; =20 // // Performs initialization of crypto and ssl library, and loads requir= ed // algorithms. // Ret =3D OPENSSL_init_ssl ( OPENSSL_INIT_LOAD_SSL_STRINGS | OPENSSL_INIT_LOAD_CRYPTO_STRIN= GS, NULL ); if (Ret !=3D 1) { return FALSE; } + // + // OPENSSL_init_ssl() cannot, and need not, be rolled back, if the res= t of + // this function fails. + // + + mPeerSubjectNameKey =3D SSL_get_ex_new_index ( + 0, // "argl": unneed= ed + NULL, // "argp": unneed= ed + NULL, // "new_func": un= needed + TlsPeerSubjectNameDuplicate, // "dup_func" + TlsPeerSubjectNameFree // "free_func" + ); + if (mPeerSubjectNameKey =3D=3D -1) { + return FALSE; + } =20 // // Initialize the pseudorandom number generator. // - return RandomSeed (NULL, 0); + RandomIsSeeded =3D RandomSeed (NULL, 0); + if (!RandomIsSeeded) { + goto DeregisterPeerSubjectName; + } + return TRUE; + +DeregisterPeerSubjectName: + CRYPTO_free_ex_index (CRYPTO_EX_INDEX_SSL, mPeerSubjectNameKey); + return FALSE; } =20 /** Free an allocated SSL_CTX object. =20 @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); =20 + // + // Set peer certificate verification procedure. + // + SSL_CTX_set_cert_verify_callback ( + TlsCtx, + TlsCertVerify, + NULL // "arg": unneeded + ); + return (VOID *) TlsCtx; } =20 /** Free an allocated TLS object. =20 --=20 2.19.1.3.g30247aa5d201