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.89.1571158616204526986 for ; Tue, 15 Oct 2019 09:56:56 -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-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id BD7B918C427F; Tue, 15 Oct 2019 16:56:55 +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 3CC1960BE2; Tue, 15 Oct 2019 16:56:54 +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 References: <20190927034441.3096-1-Jiaxin.wu@intel.com> <8106467c9f4132c831d0aa604e897fe9d4dda12a.camel@infradead.org> <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> Message-ID: <7356801d-0b64-6059-a737-aa5ddd2d297b@redhat.com> Date: Tue, 15 Oct 2019 18:56:53 +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: <72205ed7-e848-62a0-11d2-408d5b070cc7@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.6.2 (mx1.redhat.com [10.5.110.62]); Tue, 15 Oct 2019 16:56:55 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 10/15/19 15:54, Laszlo Ersek wrote: > On 10/15/19 13:03, David Woodhouse wrote: >> 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 >> =20 >> #include "InternalTlsLib.h" >> =20 >> +/* 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, >> + int idx, long argl, void *argp) >> +{ >> + /* Free it */ >> +} >> + >> +int ssl_target_dup(CRYPTO_EX_DATA *to, const CRYPTO_EX_DATA *from, >> + 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)) >> + X509_VERIFY_PARAM_set1_host(vpm, hostname, 0); >> + >> + return X509_verify_cert(ctx); >> +} >> + >> /** >> Initializes the OpenSSL library. >> =20 >> @@ -40,6 +83,9 @@ TlsInitialize ( >> return FALSE; >> } >> =20 >> + ssl_target_idx =3D SSL_get_ex_new_index(0, "TLS target hosthame/IP"= , NULL, >> + ssl_target_dup, ssl_target_free); >> + >> // >> // Initialize the pseudorandom number generator. >> // >> @@ -106,6 +152,10 @@ TlsCtxNew ( >> // >> SSL_CTX_set_min_proto_version (TlsCtx, ProtoVersion); >> =20 >> + /* 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; >> } > (4) What happens if we call SSL_set_ex_data(), but a non-NULL value has > already been stored for the same index? >=20 > 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"? >=20 > (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.) A similar question: is it possible that app_verify_callback() is called more frequently than SSL_set_ex_data() (in TlsSetVerify())? 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? If it is, is it OK? To me the ownership of these strings (i.e., what component is responsible for freeing the strings) is impenetrable. :( 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. :/ Laszlo