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.web12.3219.1571147702702699707 for ; Tue, 15 Oct 2019 06:55:02 -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-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 1550D18C8936; Tue, 15 Oct 2019 13:55:02 +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 53EB95DA8C; Tue, 15 Oct 2019 13:55:00 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553) 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> <69774fe6-ea00-44b9-5468-c092dea6cd36@redhat.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> From: "Laszlo Ersek" Message-ID: <72205ed7-e848-62a0-11d2-408d5b070cc7@redhat.com> Date: Tue, 15 Oct 2019 15:54:59 +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: <1400b3e6c04f3422a1ba0bef844664aa84c6ff33.camel@infradead.org> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.6.2 (mx1.redhat.com [10.5.110.70]); Tue, 15 Oct 2019 13:55:02 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 10/15/19 13:03, David Woodhouse wrote: > On Mon, 2019-10-14 at 18:15 +0200, Laszlo Ersek wrote: >> My understanding is that a fix purely in edk2 -- that is, without >> advancing our openssl submodule reference at once -- is possible, base= d >> on your comment >> >> https://bugzilla.tianocore.org/show_bug.cgi?id=3D960#c32 >> >> Namely, edk2 commit 9396cdfeaa7a ("CryptoPkg: Add new TlsLib library", >> 2016-12-22) added a SSL_set_verify() call (in function TlsSetVerify())= . >> The last argument of that call is currently NULL. >> >> We should change that, to a callback function that implements what >> ssl_app_verify_callback() and match_cert_hostname() do, in your source= file >> >> http://git.infradead.org/users/dwmw2/openconnect.git/blob/HEAD:/openss= l.c >=20 >=20 > Hm, you are lost in a twisty maze of verify callbacks, all alike. Definitely. > Actually the one you can set with SSL_set_verify() isn't the one you > want. That's a low-level one, called from within the generic > X509_verify_cert() function. Well, above I referred to SSL_set_verify() only because you had named that function in : > Note: the callback you're after is the final argument to > SSL_set_verify(), which you are calling from TlsSetVerify() with a > NULL argument. You need to point it at your own function of type > SSL_verify_cb which does the checking correctly. Back to the email: 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(). >=20 > It is X509_verify_cert() which can do the hostname/IP checks for us, if > we can only tell it that we want it to. But the X509_VERIFY_PARAM > object is private to the SSL. >=20 > 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 n= o 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). >=20 > 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* rea= ch the > underlying X509_VERIFY_PARAM =E2=80=94 call X509_VERIFY_PARAM_set1_host= () or > X509_VERIFY_PARAM_set1_ip_asc() accordingly, before just calling the > normal X509_verify_cert() function that it has overridden. >=20 > 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)); >=20 > diff --git a/CryptoPkg/Library/TlsLib/TlsInit.c b/CryptoPkg/Library/Tls= Lib/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 address > + * 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_STO= RE_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(), which > + * 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; > } Thank you very much for this code. To me it seems like this patch should be squashed into patch#2 (which modifies TlsLib), after adapting it to the edk2 coding style. I have some comments / questions. (1) In TlsSetVerifyHost(), you mention that SSL_set1_host() should be replaced with SSL_set_ex_data(). OK. In case the client does not use the TlsSetVerifyHost() function, SSL_set_ex_data() would not be called. However, app_verify_callback() would still be called. Is my understanding correct that "hostname" (from SSL_get_ex_data()) would be NULL in that case, and therefore both X509_VERIFY_PARAM_set1_ip_asc() and X509_VERIFY_PARAM_set1_host() would be omitted? (I mean this looks like the correct behavior to me; just asking if that is indeed the intent of the code.) (2) The documentation of the APIs seems a bit strange. I've found the following "directory" web pages: https://www.openssl.org/docs/man1.0.2/man3/ https://www.openssl.org/docs/man1.1.0/man3/ https://www.openssl.org/docs/man1.1.1/man3/ edk2 currently consumes "OpenSSL_1_1_1b", so I thought I should only look at the last page. However, SSL_get_ex_new_index() is only documented in the first page. Is my understanding correct that each OpenSSL API should be looked up in the latest documentation directory that appears to describe it? In other words: assuming I look for SSL_get_ex_new_index() in the 1.1.1 manual, and fail to find it, does that mean that the API is deprecated, or only that I should look at an earlier release of the manual? (3) Anyway, SSL_get_ex_new_index() seems like a thin wrapper around CRYPTO_get_ex_new_index(). https://www.openssl.org/docs/man1.0.2/man3/SSL_get_ex_new_index.html https://www.openssl.org/docs/man1.1.1/man3/CRYPTO_get_ex_new_index.html Based on that, I think the second ("argp") parameter of our SSL_get_ex_new_index() call should be NULL, as the "argp" parameters of the "free" and "dup" functions are unused. Do you agree? (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 will 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.) (5) The most confusing part... If I look at the 1.0.2 documentation, it says about "dup_func": > dup_func() is called when a structure is being copied. [...] The > from_d parameter is passed a pointer to the source application data > when the function is called, when the function returns the value is > copied to the destination: the application can thus modify the data > pointed to by from_d and have different values in the source and > destination This makes no sense: I wouldn't want to modify the source application data in-place, just to give the new parent object a modified instance of = it! Also, how would OpenSSL know how many bytes to copy? So I looked at the code, and it turns out dup_func is called with a (void**), not a (void*): if (!storage[i]->dup_func(to, from, &ptr, i, storage[i]->argl, storage[i]->argp)= ) The 1.1.1 documentation is more accurate; it says: https://www.openssl.org/docs/man1.1.1/man3/CRYPTO_get_ex_new_index.html > The from_d parameter needs to be cast to a void **pptr as the API has > currently the wrong signature; that will be changed in a future > version. The *pptr is a pointer to the source exdata. When the > dup_func() returns, the value in *pptr is copied to the destination > ex_data. If the pointer contained in *pptr is not modified by the > dup_func(), then both to and from will point to the same data So let me summarize the documentation status: - SSL_get_ex_new_index is not documented in 1.1.1, - SSL_get_ex_new_index is not documented in 1.1.0, - SSL_get_ex_new_index has a "shim" documentation in 1.0.2, and it refers the reader to RSA_get_ex_new_index(), - RSA_get_ex_new_index is documented in 1.0.2 *incorrectly* (with regard to dup_func), - the valid documentation is in 1.1.1, under CRYPTO_get_ex_new_index -- but for learning about that function, I had to look at the library source code. When you suggested the IP checking would be easy to imlement, you were joking, right? :) (6) Given that we are introducing callbacks (from CryptoPkg/Library/OpensslLib to CryptoPkg/Library/TlsLib), and OpenSSL does not declare these function prototypes with EFIAPI, the callbacks too must be defined with "native" (not EFIAPI) calling convention. Correct? Thanks Laszlo