Re: [edk2-devel] [PATCH v1 0/4] Support HTTPS HostName validation feature(CVE-2019-14553)

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

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, based
>> on your comment
>>
>>   https://bugzilla.tianocore.org/show_bug.cgi?id=960#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:/openssl.c
> 
> 
> 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 <https://bugzilla.tianocore.org/show_bug.cgi?id=960#c32>:

> 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().
> 
> 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.
> 
> 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, …). 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 — which *can* reach the
> underlying X509_VERIFY_PARAM — 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.
> 
> 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/TlsLib/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 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 = X509_STORE_CTX_get_ex_data(ctx, SSL_get_ex_data_X509_STORE_CTX_idx());
> +	char *hostname = SSL_get_ex_data(ssl, ssl_target_idx);
> +	X509_VERIFY_PARAM *vpm = 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.
>  
> @@ -40,6 +83,9 @@ TlsInitialize (
>      return FALSE;
>    }
>  
> +  ssl_target_idx = 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);
>  
> +  /* 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