Skip to content
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

Fix libressl support #207

Merged
merged 4 commits into from
Aug 26, 2018
Merged

Fix libressl support #207

merged 4 commits into from
Aug 26, 2018

Conversation

sonertari
Copy link
Collaborator

This pull request provides the correct fix for signal 6 and 10 crashes we discussed in pull req #203. The main issue was that LibreSSL versions up to v2.7.4 behave like OPENSSL_VERSION_NUMBER < 0x1000200fL, beware not just OPENSSL_VERSION_NUMBER < 0x10100000L.

SSLproxy with a similar patch is running for days without any crash on OpenBSD. Looking at the code enabled/disabled by these modified directives for libressl I think that they are quite related with the crashes we discussed in pull req #203.

What convinces me even further that this patch fixes the crashes I was having in the last couple of weeks is that I had most (not all) of those directives in the sources up until August 3rd and never had any crash for a year until that date. I had them since OpenBSD 5.9, but decided to remove them because 6.3 did not produce any errors or warnings without them. Obviously, I was wrong.

I should note that these crashes were not about the previous LibreSSL version 2.7.3, or having LibreSSL and OpenSSL installed on the same system, or an issue/corruption/async in/between the systems. I have rebuilt the systems, have only LibreSSL, and initially was able to reproduce the crashes with LibreSSL 2.7.4 too.

I have also tested adding the following lines in ssl.h, instead of adding new macros to many directives:

#if defined(LIBRESSL_VERSION_NUMBER)
#undef OPENSSL_VERSION_NUMBER
#define OPENSSL_VERSION_NUMBER 0x10002000L
#endif

I'd prefer this solution, because it is quite simple and achieves the same result. But with this solution, I also have to hush OpenSSL version mismatch warnings for LibreSSL, which may not be desirable, unless I can fix those warnings for LibreSSL instead of silencing them.

Beside my practical tests, the version choice of 0x10002000L makes theoretical sense too, because according to libressl-portable README "LibreSSL is API compatible with OpenSSL 1.0.1, but does not yet include all new APIs from OpenSSL 1.0.2 and later. LibreSSL also includes APIs not yet present in OpenSSL. The current common API subset is OpenSSL 1.0.1".

Since there is no difference between sslsplit and sslproxy code affected by this patch, imo the same issue must exist in sslsplit (hence this pull request). Note again that without this patch, sslproxy builds fine and also runs fine for a while, but it eventually crashes on OpenBSD (LibreSSL). So I think that if sslsplit is left running long enough, it too will crash on OpenBSD.

My only concern with this patch is that this is OpenBSD and/or LibreSSL specific. SSLproxy on UTMFW cannot run without it, so I'll always have it in my project. But since this patch clutters the sources even further (different OpenSSL versions do that already), perhaps it should be part of the OpenBSD sslsplit port instead. Btw, the patches in the OpenBSD port for sslsplit 5.2 add libressl macros to a couple of directives, but they are not enough to fix the crashes I'm having (which I have tested with sslproxy). On the other hand, perhaps some of the libressl macros added in this patch are redundant, not needed to fix those crashes. But as it is hard to test it, I don't know which ones they are, if any.

As for the weird stack traces with core dumps, I think it was because I was using gdb with sslproxy binaries compiled using clang. Since release(8) on OpenBSD does not build lldb by default (but I can see that its code is available in the sources), i.e. lldb is not available on OpenBSD 6.3 release, I couldn't try and see if lldb gives meaningful traces. I should try to build it myself in the future (so yes, I still don't know what triggers the crashes).

So, @droe I trust your judgment to decide whether to merge this pull request or not.

…tives This provides correct fix for signal 6 and 10 crashes discussed in pull req #203. LibreSSL versions up to v2.7.4 behave like OPENSSL_VERSION_NUMBER < 0x1000200fL, beware not just OPENSSL_VERSION_NUMBER < 0x10100000L
@sonertari sonertari requested a review from droe August 25, 2018 20:42
@sonertari sonertari self-assigned this Aug 25, 2018
Copy link
Owner

@droe droe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with this patch and approach. The decentralized checks clutter the source code, but that's the only way to be able to later support different quirks in different versions of LibreSSL too.

@sonertari sonertari merged commit 8e7e2df into develop Aug 26, 2018
@sonertari sonertari deleted the fix_libressl_support branch August 26, 2018 00:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants