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

["David Woodhouse" <dwmw2@infradead.org>]

[-- Attachment #1: Type: text/plain, Size: 8795 bytes --]

On Tue, 2019-10-15 at 15:54 +0200, Laszlo Ersek wrote:
> 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>:

Yeah, I was wrong.

You were the one who called me an expert; I didn't :)

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

Yep. In the case where there's no such ex_data attached to the SSL
object, our own app_verify_callback() function would do nothing
special, just call through to the standard X509_verify_cert() function
that would have been called if we hadn't registered our own override.

You may call this "correct". I think you're right, in the context you
meant it.

But I suggest there is a conversation to be had about whether it's ever
"correct" for a crypto library to silently accept any bloody
certificate it sees, regardless of what the Subject of that certificate
is.

Or whether perhaps it ought to fail safe, and require you to jump
through extra hoops if you *really* want to accept any certificate.

In a world where crypto libraries made it hard for the users to Do The
Wrong Thing, CVE-2019-14553 would never have happened.

But we digress. Is it late enough in the day for me to start drinking
yet?

> (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?

I was just looking at the code. I happened to be looking at OpenSSL
HEAD but the sconnect demo I showed in my second email works with
1.1.1.

> 
> (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?

Yeah, I had just cargo-culted that from elsewhere, and was assuming it
was performing some kind of de-duplication or alternative lookup
functionality. It does look like you can leave it NULL and it doesn't
matter.

> 
> (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"?

It will be automatically freed with free_func. Note that this doesn't
work in the sconnect.c patch I sent, but only because I'd forgotten to
actually add the call to SSL_get_ex_new_index(). After I fixed that and
wasn't just using an index of zero, I saw the printf() that I'd added
in my 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.)

I believe new_func is only for when a new SSL object is created. We
don't need that.

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


Yes, I think dup_func wants to do something like:

int ssl_target_dup(CRYPTO_EX_DATA *to, const CRYPTO_EX_DATA *from,
		   void *from_d, int idx, long argl, void *argp)
{
	char **p = from_d;
	if (*p) {
		*p = strdup(*p);
		return !!*p;
	}
	return 1;
}

> 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? :)

I confess I hadn't realised that we needed to jump through ex_data
hoops to even get the hostname information to the right place in the
callback. But I don't think I actually said "easy", did I? Only that it
would take less typing than we'd already expended in trying to make
excuses for the regression.

I also said you could manage it, which you seem to have proved by
working out the details I had... erm... left as an exercise for you :)

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

Yes, that makes sense.


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5174 bytes --]