New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

current CURL_GLOBAL_SSL treatment breaks some applications #2276

Closed
benjsc opened this Issue Jan 30, 2018 · 9 comments

Comments

Projects
None yet
5 participants
@benjsc
Copy link

benjsc commented Jan 30, 2018

Prior to the changes in d661b0a
PR: #2107
the initialization flag:

CURL_GLOBAL_SSL

existed. This allowed external initialization of the ssl subsystem. This flag was not removed but made a NOOP in curl 7.57.0. I argue this is worse than removing it since functionality was changed, removing it would have clearly shown something needed to be done. Sadly this brakes the use of boost asio ssl (or anything that wants to use openssl independantly) if also using libcurl with at least openssl 1.0.2.

In our software we are initing curl as part of an independently loaded subsystem with: curl_global_init(CURL_GLOBAL_NOTHING)

Prior to this we have used been using ssl in boost. Hence both winsock and openssl have already been inited. The use of CURL_GLOBAL_NOTHING allowed us to init curl without winsock/openssl being reinitted.

Now the problem that exists is with the new NOOP of CURL_GLOBAL_SSL. The NOOP will init openssl a second time and this appears to 'mostly' work. That's the big issue here is doesn't. Curl will be able to establish ssl connections via openssl depending on the remote cipher used but the second init to functions like OpenSSL_add_all_algorithms(), SSL_load_error_strings() via Curl_ssl_init() breaks openssl. So some functionality of openssl becomes busted. Things such as getting a meaningful error string change from something legible like (prefabbed examples below but shows the point):

OpenSSL Error: error:140940E5:SSL routines:ssl3_read_bytes:ssl handshake failure

to the meaningless:

OpenSSL error:140940E5: unknown error

since the error string table is all screwed up by the second call to SSL_load_error_strings().
Other openssl functions also break. Ie using d2i_PKCS12_bio when parsing a DES encrypted (yes I know it's insecure - tell Microsoft) ASN1 key into a PKCS12 key will not be able to find the relevant algorithms and will fail due to the second call to OpenSSL_add_all_algorithms() being called.

This sort of stuff starts failing everywhere and it's silent failures all due to the secondary init of openssl. If it wasn't for the unit tests we had in place the first time we found this (we worked out why d2i_PKCS12_bio failed and added the NOTHING flag to global_init) the change in the original PR would have silently let these changes into our product.

I note there was some consultation via the mailing list about the feature. I appreciate the consultation
as it gives history, but companies take time to update versions and sometimes only do it based on CVE requirements. Hence why I'm hitting this now not in November 2017.

My biggest concern is how many other products will be affected by the change (and it will be windows products). I request the reinstating of the CURL_GLOBAL_SSL flag otherwise we'll have no choice but to maintain a fork of libcurl since there is no other way to NOT init the ssl library in curl.

curl/libcurl version

7.57.0 and above

operating system

Windows

@benjsc

This comment has been minimized.

Copy link

benjsc commented Jan 30, 2018

@jay Thanks for your objection to the original change, you were right there are users affected by the documented functionality change.

@bagder bagder added the SSL/TLS label Jan 30, 2018

@benjsc benjsc changed the title The removal of CURL_GLOBAL_SSL breaks libcurl when used with boost under windows The removal of CURL_GLOBAL_SSL breaks libcurl when used with boost or another openssl 1.0.2 consumer Jan 30, 2018

@bagder

This comment has been minimized.

Copy link
Member

bagder commented Jan 31, 2018

  1. This is a good example showing why people/companies invested in curl/libcurl should subscribe to the mailing list or keep up with development. To voice their support pro or against suggested changes.
  2. You're the first to file a problem with this (two months since the change was released in a public release), I guess the risk for many other products to suffer isn't super high.
  3. Just reverting the change is not possible, as was widely debated at the time. We'd need to define better what the bit means and how libcurl should work when that bit is set. The point being that while it might've worked for your use case, CURL_GLOBAL_SSL was not really functional before this change either.

@bagder bagder changed the title The removal of CURL_GLOBAL_SSL breaks libcurl when used with boost or another openssl 1.0.2 consumer current CURL_GLOBAL_SSL treatment breaks some applications Jan 31, 2018

@benjsc

This comment has been minimized.

Copy link

benjsc commented Jan 31, 2018

Companies don't keep up with bleeding edge changes they tend to update based on bugs affecting them or more so these days CVE's affecting them. The recent CVE is why this one crossed my desk.
From the start of the consultation process: (https://curl.haxx.se/mail/lib-2017-11/0072.html 17/11/2017) until the commit was made d661b0a 24/11/2017) was 7 days (including 2 days which are a weekend). Good luck finding many company that will react to this change let alone monitor the mailing list daily to see that request. I work with companies where there change control cycles are well over 6 months.

@benjsc

This comment has been minimized.

Copy link

benjsc commented Jan 31, 2018

From: https://curl.haxx.se/mail/lib-2017-11/0096.html

The backup approach: should the risk turn out real and my estimate be crap (it
has happened before! :-O), the plan is to then backpedal and go with #2112 [*]
instead.

It appears there is an alternative approach which solves both problems in: #2112

@bagder

This comment has been minimized.

Copy link
Member

bagder commented Jan 31, 2018

Good luck finding many company that will react to this change

(There are more than 1600 subscribers on the mailing list. Experience tells us that if we ask something on the list, people either respond within a few days or they don't respond at all. This is true for this case as well.)

But yes, the #2112 is the designated backup method that we should pick up and explore.

@jay

This comment has been minimized.

Copy link
Member

jay commented Feb 13, 2018

But yes, the #2112 is the designated backup method that we should pick up and explore.

I'm not as committed since the documentation was changed to say the flag doesn't matter, which puts us in an awkward position because if we reverse course we end up breaking things for anyone not using it now. If we offer the users a way to skip openssl initialization then things libcurl needs for openssl to function correctly may not be initialized (as it was before). That could even happen in 1.1.0 since OPENSSL_load_builtin_modules and CONF_modules_load_file wouldn't be called without the flag. There's no great solution here, I'm on the fence.

@copelnug

This comment has been minimized.

Copy link

copelnug commented Feb 20, 2018

I've also encountered this problem sadly cannot manage to find a workaround as OpenSSL is initialized by another library.
The behaviour observed range from crash on Windows to a freeze on Linux x86.
Other than either never updating libcurl past 7.56 or creating a custom patch, #2112 seems to be the way to go for me.

While I also agree we jay that his patch would break user expecting the 7.57 behaviour, I'm not sure there will be many projects coded with this new behaviour yet. In my opinion, there is currently many more applications that depend on the old behaviour that will be broken when trying to update libcurl.

@azrle

This comment has been minimized.

Copy link

azrle commented Feb 28, 2018

Sometimes the cleanup function can break current OpenSSL setups. A real case is shown above GoogleCloudPlatform/compute-image-packages#562.

In fact, I am confused with the motivation of removing CURL_GLOBAL_SSL. IMHO, if someone clear CURL_GLOBAL_SSL bit explicitly, I do not think he/she expects communication via TLS/SSL for some reasons. And when an unexpected TLS/SSL communication happens, an error/exception should be returned. It should not be concealed by libcurl.

@bagder

This comment has been minimized.

Copy link
Member

bagder commented Apr 29, 2018

Reverting the change is not a quality fix for this issue. I'd prefer that someone actually describes for me what they think the option should and shouldn't do that will actually work for them and has a likeliness of working for others too, including those that run multissl.

Right now I'm leaning towards adding this issue to KNOWN_BUGS since there doesn't seem to be anyone very eager to work on it for the moment.

@bagder bagder closed this in 8a6a01c May 31, 2018

@lock lock bot locked as resolved and limited conversation to collaborators Aug 29, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.