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.104.1571160894284044159 for ; Tue, 15 Oct 2019 10:34:54 -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-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id BFAA9882FB; Tue, 15 Oct 2019 17:34:53 +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 403FE5C1D6; Tue, 15 Oct 2019 17:34:52 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553) From: "Laszlo Ersek" To: David Woodhouse , "Wu, Jiaxin" , "devel@edk2.groups.io" , "Wang, Jian J" , Bret Barkelew Cc: Richard Levitte Reply-To: devel@edk2.groups.io, lersek@redhat.com References: <20190927034441.3096-1-Jiaxin.wu@intel.com> <895558F6EA4E3B41AC93A00D163B727416F5D921@SHSMSX107.ccr.corp.intel.com> <777053db79600eb90a19945700293d14f4978344.camel@infradead.org> <6bb5d2f6-ec6f-1766-e19b-03fd45c1bc12@redhat.com> <9A4966EE-76CD-465C-A6CA-70DD9E38D834@infradead.org> <850a81a8-2cdc-0708-4ff7-db9825fdaedc@redhat.com> <23699ae3-10c2-037c-b3f5-ac8f5bea1fb7@redhat.com> <895558F6EA4E3B41AC93A00D163B727416F7E4AB@SHSMSX107.ccr.corp.intel.com> <6939ba4e-6c77-0769-4ac2-c3ba1ea9a0b7@redhat.com> <44468659be80e9bf1886e7b6f8f3aa77044b5fd6.camel@infradead.org> <5bbadb29-36f2-1054-fd41-5577d59c9290@redhat.com> <5c33b6c2-c8b0-aa64-a85f-06bdc3c69843@redhat.com> <1400b3e6c04f3422a1ba0bef844664aa84c6ff33.camel@infradead.org> <72205ed7-e848-62a0-11d2-408d5b070cc7@redhat.com> <7356801d-0b64-6059-a737-aa5ddd2d297b@redhat.com> Message-ID: <7a3736ba-6bf6-1420-8471-4678e7340580@redhat.com> Date: Tue, 15 Oct 2019 19:34:51 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <7356801d-0b64-6059-a737-aa5ddd2d297b@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Tue, 15 Oct 2019 17:34:53 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 10/15/19 18:56, Laszlo Ersek wrote: > On 10/15/19 15:54, Laszlo Ersek wrote: >> On 10/15/19 13:03, David Woodhouse wrote: >=20 >>> The "app callback" in my OpenConnect example is set on the SSL_CTX not >>> the SSL object, and is called from the top-level >>> ssl_verify_cert_chain() function *instead* of X509_verify_cert(). >>> >>> It is X509_verify_cert() which can do the hostname/IP checks for us, i= f >>> we can only tell it that we want it to. But the X509_VERIFY_PARAM >>> object is private to the SSL. >>> >>> As discussed, we have the SSL_set1_host() accessor function which lets >>> us set the hostname. The implementation really is a simple one-liner, >>> calling X509_VERIFY_PARAM_set1_host(s->param, =E2=80=A6). But there's = no way >>> for use to set the IP address from the outside, without an equivalent >>> accessor function for that (and without SSL_set1_host() spotting that >>> the string it's given is an IP address, and doing so). >>> >>> But what we can do is stash the target string in some ex_data hanging >>> off the SSL object, then have an app callback =E2=80=94 which *can* re= ach the >>> underlying X509_VERIFY_PARAM =E2=80=94 call X509_VERIFY_PARAM_set1_hos= t() or >>> X509_VERIFY_PARAM_set1_ip_asc() accordingly, before just calling the >>> normal X509_verify_cert() function that it has overridden. >>> >>> Something like this... and instead of calling SSL_set1_host(ssl, host) >>> your own code now has to call >>> SSL_set_ex_data(ssl, ssl_target_idx, strdup(host)); >>> >>> diff --git a/CryptoPkg/Library/TlsLib/TlsInit.c b/CryptoPkg/Library/Tl= sLib/TlsInit.c >>> index f9ad6f6b946c..add5810cc4bd 100644 >>> --- a/CryptoPkg/Library/TlsLib/TlsInit.c >>> +++ b/CryptoPkg/Library/TlsLib/TlsInit.c >>> @@ -9,6 +9,49 @@ SPDX-License-Identifier: BSD-2-Clause-Patent >>> >>> #include "InternalTlsLib.h" >>> >>> +/* You are lost in a twisty maze of SSL cert verify callbacks, all >>> + * alike. All we really wanted to do was call SSL_set1_host() and >>> + * have it work for IP addresses too, which OpenSSL PR#9201 will do >>> + * for us. But until we update OpenSSL, that doesn't work. And we >>> + * can't get at the underlying X509_VERIFY_PARAM to set the IP addres= s >>> + * for ourselves. >>> + * >>> + * So we install an app_verify_callback in the SSL_CTX (which is >>> + * different to the per-SSL callback wae can use, because it happens >>> + * sooner. All our callback does it set the hostname or IP address in >>> + * the X509_VERIFY_PARAM like we wanted to in the first place, and >>> + * then call X509_verify_param() which is the default function. >>> + * >>> + * How does it find the hostname/IP string? It's attached to the SSL >>> + * as ex_data, using this index: >>> + */ >>> +static int ssl_target_idx; >>> + >>> +void ssl_target_free(void *parent, void *ptr, CRYPTO_EX_DATA *ad, >>> + =09 int idx, long argl, void *argp) >>> +{ >>> + /* Free it */ >>> +} >>> + >>> +int ssl_target_dup(CRYPTO_EX_DATA *to, const CRYPTO_EX_DATA *from, >>> + =09 void *from_d, int idx, long argl, void *argp) >>> +{ >>> + /* strdup it */ >>> + return 0; >>> +} >>> + >>> +int app_verify_callback(X509_STORE_CTX *ctx, void *dummy) >>> +{ >>> + SSL *ssl =3D X509_STORE_CTX_get_ex_data(ctx, SSL_get_ex_data_X509_ST= ORE_CTX_idx()); >>> + char *hostname =3D SSL_get_ex_data(ssl, ssl_target_idx); >>> + X509_VERIFY_PARAM *vpm =3D X509_STORE_CTX_get0_param(ctx); >>> + >>> + if (hostname && !X509_VERIFY_PARAM_set1_ip_asc(vpm, hostname)) >>> + =09X509_VERIFY_PARAM_set1_host(vpm, hostname, 0); >>> + >>> + return X509_verify_cert(ctx); >>> +} >>> + >>> /** >>> Initializes the OpenSSL library. >>> >>> @@ -40,6 +83,9 @@ TlsInitialize ( >>> return FALSE; >>> } >>> >>> + ssl_target_idx =3D SSL_get_ex_new_index(0, "TLS target hosthame/IP"= , NULL, >>> + =09 =09 ssl_target_dup, ssl_target_free); >>> + >>> // >>> // Initialize the pseudorandom number generator. >>> // >>> @@ -106,6 +152,10 @@ TlsCtxNew ( >>> // >>> SSL_CTX_set_min_proto_version (TlsCtx, ProtoVersion); >>> >>> + /* SSL_CTX_set_cert_verify_callback. Not SSL_CTX_set_verify(), whic= h >>> + * we could have done as SSL_set_verify(). Twisty maze, remember? *= / >>> + SSL_CTX_set_cert_verify_callback(TlsCtx, app_verify_callback, NULL)= ; >>> + >>> return (VOID *) TlsCtx; >>> } >=20 >> (4) What happens if we call SSL_set_ex_data(), but a non-NULL value has >> already been stored for the same index? >> >> Do we have to first fetch it with SSL_get_ex_data() and free it, or wil= l >> it be automatically freed with "free_func"? >> >> (Note: I think that, if we used a "new_func" for allocating anything, >> this question could be relevant the very first time SSL_set_ex_data() >> were called.) >=20 > A similar question: >=20 > is it possible that app_verify_callback() is called more frequently than > SSL_set_ex_data() (in TlsSetVerify())? >=20 > Because that means that the frequency of SSL_set1_host() calls changes. > Previously we'd call SSL_set1_host() once per TlsSetVerify(), but now it > could be called multiple times per TlsSetVerify(). Is that the case? >=20 > If it is, is it OK? Ehh, I failed to ask the actual question. Is it OK to call X509_VERIFY_PARAM_set1*() multiple times -- basically, every time just before we call X509_verify_cert()? My concern is not with the crypto functionality, but whether we could be leaking memory allocations. Thanks Laszlo > To me the ownership of these strings (i.e., what component is > responsible for freeing the strings) is impenetrable. :( >=20 > Even the UEFI 2.8 spec doesn't explain whether EFI_TLS_SET_SESSION_DATA > saves the array of characters pointed-to by > "EFI_TLS_VERIFY_HOST.HostName", or just the pointer itself. :/ >=20 > Laszlo >=20 >=20 >=20