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
Received on 2017-09-09