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