Add OpenSSL 1.1 compatibility #83

gjasny · 2017-07-13T20:59:47Z

Hello,

I tried to migrate reSIProcate to the OpenSSL 1.1 API. Those changes have been compile tested with OpenSSL 1.0.2 and OpenSSL 1.1.0.

Unfortunately my (D)TLS and OpenSSL knowledge is quite small. So I consider this PR just a discussion and review starter instead of a polished PR.

Thanks,
Gregor

gjasny · 2017-07-13T21:02:06Z

resip/stack/ssl/DtlsTransport.cxx


   len = SSL_read( ssl, pt, UdpTransport::MaxBufferSize ) ;
   int err = SSL_get_error( ssl, len ) ;

   /* done with the rbio */
-   BIO_free( ssl->rbio ) ;
-   ssl->rbio = mDummyBio ;
+   SSL_set0_rbio( ssl, mDummyBio );


I'm not sure if this could lead to errors. Formerly BIO_free was used. Now BIO_free_all is used instead. IMHO the difference is that the later is freeing recursively. But our rbio seems to be not stacked, so the difference might not matter at all.

gjasny · 2017-08-18T16:12:00Z

In the debian bug tracker is also a patch: https://bugs.debian.org/cgi-bin/bugreport.cgi?att=1;bug=859715;filename=resip.patch;msg=15

gjasny · 2017-08-20T14:18:47Z

Hey @FauxFaux, I merged my patch (which preserves OpenSSL 1.0 compatibility) with yours. Could you please give it a review?

Differences from your patch to this PR are: https://gist.github.com/gjasny/04463e481d9b14e81c08d0fa3b9071af

Lastique · 2017-09-20T15:02:55Z

resip/stack/ssl/Security.hxx

@@ -181,7 +181,7 @@ class BaseSecurity
      // retrieves a list of all certificate names (subjectAltNAme's and CommonName)
      static void getCertNames(X509 *cert, std::list<PeerName> &peerNames, bool useEmailAsSIP = false);

-      static bool isSelfSigned(const X509* cert);
+      static bool isSelfSigned(X509* cert);


This should not be needed. The operation is not supposed to modify the certificate and both X509_get_issuer_name and X509_get_subject_name accept const X509*.

In OpenSSL 1.0.2 the property accessors just take a non-const X509 pointer.
See x509.h

Then, I believe, isSelfSigned should perform the const_cast internally or use the old code for OpenSSL 1.0.x. There is no reason to change the public interface of isSelfSigned.

Lastique · 2017-10-03T21:22:11Z

reflow/dtls_wrapper/bf_dwrap.c

@@ -43,19 +58,28 @@ typedef struct BIO_F_DWRAP_CTX_

 BIO_METHOD *BIO_f_dwrap(void) 
 {
-   return(&methods_dwrap);
+   BIO_METHOD *meth = BIO_meth_new(BIO_TYPE_DWRAP, "dtls_wrapper");


I believe, this memory leaks.

I will try to create an unit test and check for memory leaks.

Lastique · 2017-10-03T21:43:15Z

Also, I believe, dwrap_free in reflow/dtls_wrapper/bf_dwrap.c needs to be updated. It should free the BIO_F_DWRAP_CTX structure allocated in dwrap_new, not the BIO object itself.

Lastique · 2017-10-04T13:37:38Z

In rutil/ssl/OpenSSLInit.cxx, OpenSSLInit::~OpenSSLInit(), the call sk_SSL_COMP_free( SSL_COMP_get_compression_methods()) causes a crash with OpenSSL 1.1.0 because libssl does free the compression methods itself when being unloaded. This line is viable for OpenSSL until 1.0.2, which introduced SSL_COMP_free_compression_methods for this purpose. In OpenSSL 1.1.0 this call is not needed and should be removed.

gjasny · 2017-10-24T11:47:50Z

@Lastique I think I addressed all of your comments. The dtls_wrapper tests no exit without leaked memory. (For enabling and fixing those tests see future PR).

The sk_SSL_COMP_free code seems to be commented out.

Lastique · 2017-10-24T12:11:27Z

reflow/dtls_wrapper/bf_dwrap.c

+BIO_METHOD *BIO_f_dwrap(void)
+{
+   if (!dtls_meth) {
+      dtls_meth = BIO_meth_new(BIO_TYPE_DWRAP, "dtls_wrapper");


This is not thread-safe. Not sure if there are cases when this code can run in different threads, though. I would recommend using pthread_once or atomics instead.

You're right but this codebase is still C++03. How about using a static auto_ptr holding the BIO_method? That would also enable me to get rid of the atexit hack.

A simpler solution is to convert dtls_meth to an object with a constructor and destructor and move the BIO_METHOD initialization to the constructor. This would still be not thread-safe, but in practical sense this would be better because the constructor will be run at library load time, when there are typically no multiple threads.

OTOH who knows if calling into OpenSSL works before initalization ...

How about using a static auto_ptr holding the BIO_method?

This won't help. The race is between loading the pointer for testing against NULL and storing the allocated pointer. auto_ptr doesn't help with it.

Re atomics, I was meaning compiler-specific intrinsics. One would likely have to introduce some abstraction layer for portability. pthread_once is available for C++03 programs but not on Windows. I would probably use Interlocked* intrinsics on Windows and pthread_once everywhere else.

Will work on something like this. But I have to finish some unrelated work first.

OTOH who knows if calling into OpenSSL works before initalization

I would assume it should work for the methods we need - those are just memory allocation and accessors, no apparent reason for them to not work.

@Lastique I changed the code as you suggested. Could you pleas give it another review?

Thanks a lot for your time!

Those changes have been compile tested with OpenSSL 1.0.2 and OpenSSL 1.1.0.

easassone

We have compiled these changes with OpenSSL 1.1.0f successfully. Testing with TLS in our application using reSIProcate was successful also. thanks!

gjasny · 2018-03-03T11:50:56Z

@sgodin Are you ok with merging this?

sgodin · 2018-03-05T16:46:13Z

Hi Gregor, I haven't had a chance to try it out, but I trust you have, and I see a number of names on this pull request that have, so I'm good with it. If you haven't tried a Windows build and there are issues after you merge, I can try to resolve those ASAP. Thanks for your efforts with this!! Scott

…

On Sat, Mar 3, 2018 at 6:50 AM, Gregor Jasny ***@***.***> wrote: @sgodin <https://github.com/sgodin> Are you ok with merging this? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#83 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACtNUaDpmKhBPn3QX-N_igA2p_zv5QTPks5taoOhgaJpZM4OXiZH> .

gjasny requested review from derekmacdonald and dpocock July 13, 2017 20:59

gjasny commented Jul 13, 2017

View reviewed changes

gjasny force-pushed the openssl-1.1 branch from c96713e to 1476276 Compare July 13, 2017 21:16

gjasny force-pushed the openssl-1.1 branch from 1476276 to 06d6602 Compare August 20, 2017 14:13

gjasny force-pushed the openssl-1.1 branch 3 times, most recently from e88b3b5 to c971c02 Compare August 20, 2017 18:13

Lastique reviewed Sep 20, 2017

View reviewed changes

Lastique reviewed Oct 3, 2017

View reviewed changes

gjasny force-pushed the openssl-1.1 branch from c971c02 to 9ee9143 Compare October 15, 2017 09:08

gjasny force-pushed the openssl-1.1 branch from 7cc4d8e to f07716c Compare October 24, 2017 11:44

Lastique reviewed Oct 24, 2017

View reviewed changes

gjasny and others added 3 commits October 31, 2017 17:34

Add OpenSSL 1.1 compatibility

b74d77e

Those changes have been compile tested with OpenSSL 1.0.2 and OpenSSL 1.1.0.

fix build on libssl-dev 1.1

7f8a0ba

Provide OpenSSL 1.0 compatibility methods for DTLS BIO

587383c

gjasny force-pushed the openssl-1.1 branch from f07716c to d862fc9 Compare October 31, 2017 19:26

gjasny added 4 commits October 31, 2017 20:28

dtls_wrapper: Use RAII for SRTP session keys

c0a4a3c

dtls_wrapper: free allocated bio

1a272c6

dtls_wrapper: Make bf_dwrap a C++ file to use RAII

c54a5cd

dtls_wrapper: Use RAII to track BIO_meth life cycle

d0fd46c

gjasny force-pushed the openssl-1.1 branch from d862fc9 to d0fd46c Compare October 31, 2017 19:32

easassone reviewed Nov 16, 2017

View reviewed changes

sgodin merged commit 190afbb into resiprocate:master Mar 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add OpenSSL 1.1 compatibility #83

Add OpenSSL 1.1 compatibility #83

gjasny commented Jul 13, 2017

gjasny Jul 13, 2017

gjasny commented Aug 18, 2017

gjasny commented Aug 20, 2017

Lastique Sep 20, 2017

gjasny Sep 24, 2017

Lastique Sep 24, 2017

gjasny Oct 15, 2017

Lastique Oct 3, 2017

gjasny Oct 15, 2017

Lastique commented Oct 3, 2017

Lastique commented Oct 4, 2017

gjasny commented Oct 24, 2017

Lastique Oct 24, 2017

gjasny Oct 24, 2017

Lastique Oct 24, 2017

gjasny Oct 24, 2017

Lastique Oct 24, 2017

gjasny Oct 24, 2017

Lastique Oct 24, 2017

gjasny Oct 31, 2017

easassone left a comment

gjasny commented Mar 3, 2018

sgodin commented Mar 5, 2018 via email

Add OpenSSL 1.1 compatibility #83

Add OpenSSL 1.1 compatibility #83

Conversation

gjasny commented Jul 13, 2017

Choose a reason for hiding this comment

gjasny commented Aug 18, 2017

gjasny commented Aug 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lastique commented Oct 3, 2017

Lastique commented Oct 4, 2017

gjasny commented Oct 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

easassone left a comment

Choose a reason for hiding this comment

gjasny commented Mar 3, 2018

sgodin commented Mar 5, 2018 via email