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

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

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, 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;
>>  }

> (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.)

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