Hi Kamil, On Sat, 9 Sep 2017, Johannes Schindelin wrote:
> On Wed, 6 Sep 2017, Kamil Dudka wrote: > > > On Thursday, August 31, 2017 9:36:10 AM CEST Kamil Dudka via > > curl-library wrote: > > > On Wednesday, August 30, 2017 10:40:14 PM CEST Johannes Schindelin wrote: > > > > > > > > On Wed, 30 Aug 2017, Kamil Dudka wrote: > > > > > On Wednesday, August 30, 2017 10:14:06 AM CEST Daniel Stenberg wrote: > > > > > > On Wed, 30 Aug 2017, Kamil Dudka wrote: > > > > > > > This is caused by using NSS for the crypto operations > > > > > > > despite only OpenSSL was initialized. Should the switch > > > > > > > work for SSL only or should it work for the low-level crypto > > > > > > > operations, too? > > > > > > > > > > > > I don't think it matters, does it? The low level crypto > > > > > > functions should just work no matter which backend that > > > > > > provides them. Swichting or not, I mean. > > > > > > > > > > > > > A lightweight solution would be to fix curl_ntlm_core.c such > > > > > > > that it uses crypto operations from the default SSL/crypto > > > > > > > backend. This would fix the breakage in the most common > > > > > > > case. However, NTLM would still break if the SSL backend > > > > > > > was switched at run-time. > > > > > > > > > > > > We're selecting which SSL backend to use before anything used > > > > > > curl for the first time and then never again for the process > > > > > > life time - at least for the time being. > > > > > > > > > > The point is that it does not work well if you initialize only > > > > > OpenSSL (either be it compile-time default, or run-time selected > > > > > backend) and then you try to use low-level crypto from NSS. > > > > > > > > The current design relies on the fact that OpenSSL is initialized > > > > when OpenSSL is used, and NSS is initialized when NSS is used. The > > > > SSL backend is not (yet) a per-session choice. It is a global > > > > choice before you call curl_global_init(), > > > > > > Sure. I believe that I understand the design pretty well. > > > > > > > to prevent issues as you are concerned about. > > > > > > It apparently does not prevent the issues because otherwise I would > > > not be reporting them ;-) > > > > > > > Do you see a real breakage? > > > > > > Yes, see my initial post in this thread. If you have a system where > > > you can compile libcurl against both OpenSSL and NSS, you can use > > > the following steps to reproduce the bug: > > > > > > $ ./buildconf > > > $ ./configure --with-ssl --with-nss --with-default-ssl-backend=openssl > > > --disable-tls-srp $ make -j16 > > > $ cd tests > > > $ make -j16 > > > $ ./runtests.pl -a -p -v "HTTP NTLM auth" > > > > > > 8 tests fail with the following message in the verbose output: > > > > > > * unable to initialize NSS, curl_global_init() should have been > > > called with CURL_GLOBAL_SSL or CURL_GLOBAL_ALL > > > > If so, it is a bug in the implementation of that design. > > I finally got around to test this here, and it would seem that the tests > fail here, too. Just to make sure: the reported failures are > > TESTFAIL: These test cases failed: 176 2025 2028 2029 2030 2031 2032 2033 > > > > The problem is that the changes made to vtls/* were not reflected in > > > curl_ntlm_core.c, which now uses uninitialized NSS crypto backend > > > for the low-level crypto operation in case both NSS and OpenSSL > > > backends are enabled and OpenSSL is selected. In my example OpenSSL > > > is selected as the default crypto backend but the same will happen > > > if you select OpenSSL by curl_global_sslset(). > > > > > > In other words, only the selected crypto backend is actually > > > initialized but curl_ntlm_core.c does not know which crypto backend > > > is selected and, in some cases, tries to use an unitialized crypto > > > backend, which fails. > > > > Basically the same problem is reported in the following pull request: > > > > https://github.com/curl/curl/pull/1848 > > > > ... although the proposed changes would solve it only partially. > > This PR has been updated, and I offered a further fixup that I hope will > be part of the PR before merging. It is available as `ntlmmulti` branch > at https://github.com/dscho/curl. > > Would you mind testing again with that branch? Actually, there is a remaining problem with that PR (which was sadly closed, but I think we need to get this problem fixed ASAP): when compiling cURL with NSS and, say, Secure Channel support, and Secure Channel is selected at runtime, then Curl_nss_init() will not have been called, ergo nss_initlink won't be initialized, and hence Curl_nss_force_init() will complain thusly: unable to initialize NSS, curl_global_init() should have been called with CURL_GLOBAL_SSL or CURL_GLOBAL_ALL I see that you added this test yourself, in f3b77e561 (http_ntlm: add support for NSS, 2010-06-27). So I see two options to move forward, building on PR 1848: 1) move NSS *waaaay* down in the #if..#elif chain in curl_ntlm_core.c 2) change Curl_nss_force_init() so that it can be called *without* Curl_nss_init() having been called before that. The easier method would be 1), I guess. For 2), we could introduce yet another lock (or a super-ugly workaround by calling Curl_nss_init() directly from Curl_ssl_init() if NSS is not the current backend and neither OpenSSL nor GNU TLS support was compiled in). What do you think? Ciao, Johannes ------------------------------------------------------------------- Unsubscribe: https://cool.haxx.se/list/listinfo/curl-library Etiquette: https://curl.haxx.se/mail/etiquette.html
