Remove calls to OpenSSL when building with --disable-ssl

The attached patch removes deprecated(?) calls to OpenSSL functions from crypro.c,
which are called when USE_SSL is not defined.

I'm not so deep into OpenVPN, so maybe these functions are needed, but i thought that all crypto-lib
dependent functions should be moved to the corresponding crypto_LIB.c files.

If they are needed, we should #ifdef them, so that PolarSSL-based build won't break on it.

------------------------------------------------------------------------------
Try before you buy = See our experts in action!
The most comprehensive online learning library for Microsoft developers is just $99.99! Visual Studio, SharePoint, SQL - plus HTML5, CSS3, MVC3, Metro Style Apps, more. Free future releases when you subscribe now!
http://p.sf.net/sfu/learndevnow-dev2
_______________________________________________
Openvpn-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

These are needed for 0.9.8 as far as I remember.

Quoted 22 lines · Igor Novgorodov

The attached patch removes deprecated(?) calls to OpenSSL functions from crypro.c,
which are called when USE_SSL is not defined.

I'm not so deep into OpenVPN, so maybe these functions are needed, but i thought that all crypto-lib
dependent functions should be moved to the corresponding crypto_LIB.c files.

If they are needed, we should #ifdef them, so that PolarSSL-based build won't break on it.

------------------------------------------------------------------------------
Try before you buy = See our experts in action!
The most comprehensive online learning library for Microsoft developers is just $99.99! Visual Studio, SharePoint, SQL - plus HTML5, CSS3, MVC3, Metro Style Apps, more. Free future releases when you subscribe now!
http://p.sf.net/sfu/learndevnow-dev2
_______________________________________________
Openvpn-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Then maybe we should move these calls to crypto_openssl.c into crypto_init_lib() function to make crypto.c library-independent? And why OpenSSL_add_all_algorithms() and stuff is called only when USE_SSL is not defined?

And if these calls are for 0.9.8, maybe add a check for OpenSSL version?

Quoted 28 lines · Alon Bar-Lev

These are needed for 0.9.8 as far as I remember.

Quoted 25 lines · Igor Novgorodov

The attached patch removes deprecated(?) calls to OpenSSL functions from crypro.c,
which are called when USE_SSL is not defined.

I'm not so deep into OpenVPN, so maybe these functions are needed, but i thought that all crypto-lib
dependent functions should be moved to the corresponding crypto_LIB.c files.

If they are needed, we should #ifdef them, so that PolarSSL-based build won't break on it.

------------------------------------------------------------------------------
Try before you buy = See our experts in action!
The most comprehensive online learning library for Microsoft developers is just $99.99! Visual Studio, SharePoint, SQL - plus HTML5, CSS3, MVC3, Metro Style Apps, more. Free future releases when you subscribe now!
http://p.sf.net/sfu/learndevnow-dev2
_______________________________________________
Openvpn-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Then maybe we should move these calls to crypto_openssl.c into crypto_init_lib() function to make crypto.c library-independent? And why OpenSSL_add_all_algorithms() and stuff is called only when USE_SSL is not defined?

And if these calls are for 0.9.8, maybe add a check for OpenSSL version?

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 5 lines · Igor Novgorodov

Then maybe we should move these calls to crypto_openssl.c into crypto_init_lib() function to make crypto.c library-independent? And why OpenSSL_add_all_algorithms() and stuff is called only when USE_SSL is not defined?

Quoted 2 lines

And if these calls are for 0.9.8, maybe add a check for OpenSSL version?

Remember that OpenSSL covers two parts. One part is the SSL stuff, the other part is the crypto layer. So even if the SSL stuff isn't used, the crypto stuff most likely is. In the crypto stuff, also all the hashing algorithms are included. However, using SSL without crypto doesn't make sense. If it's not needed any more by OpenSSL 1.0.0, then make it version dependent. Can probably be done at compile time.

Removing the ERR_load_crypto_strings() call will most likely break the error logging too, which is used by the msg() function. It will not make the crypto/SSL errors more understandable, how I understand it.

May I suggest that both ERR_load_crypto_strings() and SSL_load_error_strings() (gotta love the consistency of function naming) is loaded by default, unless ENABLE_SMALL is defined?

Right now, this patch makes me really concerned and scared. For this to be accepted, a lot of testing must be done - and most likely by people understanding the darker sides of crypto far better than I. We can't risk that we're regressing on a well proved and tested encryption layer.
There are people located in not so democratic countries who use OpenVPN
to access a not-restricted/censored Internet - and their safety may rely on the security OpenVPN provides.

kind regards,

David Sommerseth
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk9MrvEACgkQDC186MBRfroaSgCdGkPFPLK7D9XKiJa30lkfWmaV BJkAnAyAg+GbYmA3OrQ3HmNL+4AQTisq
=kilG
-----END PGP SIGNATURE-----

Quoted 8 lines · David Sommerseth

Quoted 7 lines · Igor Novgorodov

Then maybe we should move these calls to crypto_openssl.c into crypto_init_lib() function to make crypto.c library-independent? And why OpenSSL_add_all_algorithms() and stuff is called only when USE_SSL is not defined?

And if these calls are for 0.9.8, maybe add a check for OpenSSL version?

Quoted 7 lines

Remember that OpenSSL covers two parts. One part is the SSL stuff, the other part is the crypto layer. So even if the SSL stuff isn't used, the crypto stuff most likely is. In the crypto stuff, also all the hashing algorithms are included. However, using SSL without crypto doesn't make sense. If it's not needed any more by OpenSSL 1.0.0, then make it version dependent. Can probably be done at compile time.

Well, i'm no expert in OpenSSL programming, but looking through internet, i haven't found an evidence that this stuff should not be called during initialization in OpenSSL 1.0.x

So, just to make OpenVPN possible to build with --ssl-type=polarssl and --disable-ssl, i propose the attached patch that moves calls to these functions into crypto_openssl.c

Quoted 8 lines

Removing the ERR_load_crypto_strings() call will most likely break the error logging too, which is used by the msg() function. It will not make the crypto/SSL errors more understandable, how I understand it.

May I suggest that both ERR_load_crypto_strings() and SSL_load_error_strings() (gotta love the consistency of function naming) is loaded by default, unless ENABLE_SMALL is defined?

I agree, added the check for ENABLE_SMALL in ssl_openssl.c and crypto_openssl.c to the attached patch.

Quoted 8 lines

Right now, this patch makes me really concerned and scared. For this to be accepted, a lot of testing must be done - and most likely by people understanding the darker sides of crypto far better than I. We can't risk that we're regressing on a well proved and tested encryption layer. There are people located in not so democratic countries who use OpenVPN to access a not-restricted/censored Internet - and their safety may rely on the security OpenVPN provides.

I agree fully. So if we just move these calls into crypto_openssl.c, no regression would occur i think.

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 43 lines · Igor Novgorodov

Quoted 8 lines · David Sommerseth

Quoted 7 lines · Igor Novgorodov

Then maybe we should move these calls to crypto_openssl.c into crypto_init_lib() function to make crypto.c library-independent? And why OpenSSL_add_all_algorithms() and stuff is called only when USE_SSL is not defined?

And if these calls are for 0.9.8, maybe add a check for OpenSSL version?

Quoted 7 lines

Remember that OpenSSL covers two parts. One part is the SSL stuff, the other part is the crypto layer. So even if the SSL stuff isn't used, the crypto stuff most likely is. In the crypto stuff, also all the hashing algorithms are included. However, using SSL without crypto doesn't make sense. If it's not needed any more by OpenSSL 1.0.0, then make it version dependent. Can probably be done at compile time.

Well, i'm no expert in OpenSSL programming, but looking through internet, i haven't found an evidence that this stuff should not be called during initialization in OpenSSL 1.0.x

So, just to make OpenVPN possible to build with --ssl-type=polarssl and --disable-ssl, i propose the attached patch that moves calls to these functions into crypto_openssl.c

Quoted 8 lines

Removing the ERR_load_crypto_strings() call will most likely break the error logging too, which is used by the msg() function. It will not make the crypto/SSL errors more understandable, how I understand it.

May I suggest that both ERR_load_crypto_strings() and SSL_load_error_strings() (gotta love the consistency of function naming) is loaded by default, unless ENABLE_SMALL is defined?

I agree, added the check for ENABLE_SMALL in ssl_openssl.c and crypto_openssl.c to the attached patch.

Quoted 8 lines

Right now, this patch makes me really concerned and scared. For this to be accepted, a lot of testing must be done - and most likely by people understanding the darker sides of crypto far better than I. We can't risk that we're regressing on a well proved and tested encryption layer. There are people located in not so democratic countries who use OpenVPN to access a not-restricted/censored Internet - and their safety may rely on the security OpenVPN provides.

Quoted 2 lines

I agree fully. So if we just move these calls into crypto_openssl.c, no regression would occur i think.

Agreed, I think it makes sense to move all native OpenSSL calls into *_openssl.[ch] files.

I'm still not convinced about this part though.

crypto_openssl.c +6 -0

+#ifndef USE_SSL
+#ifndef ENABLE_SMALL
+  ERR_load_crypto_strings ();
+#endif
+  OpenSSL_add_all_algorithms ();
+#endif

OpenSSL_add_algorithms() is also needed for *non-SSL* stuff. It is populates the internal OpenSSL lookup tables, so you can lookup strings like "MD5", "SHA512", "AES256", etc, etc via EVP_get_digestbyname() and EVP_get_cipherbyname() which will return the proper EVP_* objects back.
And neither of these are strictly SSL, they are all crypto related. SSL
depends on the crypto part, but the crypto doesn't need SSL.

Adriaan, what do you think?

kind regards,

David Sommerseth
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk9Mu80ACgkQDC186MBRfrpj2QCfYPpJa8CbFNZwJvGbyAHIpLBI dgwAn2P2QD6YKq2qU9N6MaKhTl2OX94M
=ticZ
-----END PGP SIGNATURE-----

Quoted 3 lines · David Sommerseth

Quoted 2 lines · Igor Novgorodov

Quoted 1 line · David Sommerseth

On 28/02/12 06:54, Igor Novgorodov wrote:

Quoted 18 lines

Quoted 10 lines

Quoted 8 lines

Right now, this patch makes me really concerned and scared. For this to be accepted, a lot of testing must be done - and most likely by people understanding the darker sides of crypto far better than I. We can't risk that we're regressing on a well proved and tested encryption layer. There are people located in not so democratic countries who use OpenVPN to access a not-restricted/censored Internet - and their safety may rely on the security OpenVPN provides.

I agree fully. So if we just move these calls into crypto_openssl.c, no regression would occur i think.

Agreed, I think it makes sense to move all native OpenSSL calls into *_openssl.[ch] files.

I'm still not convinced about this part though.

+#ifndef USE_SSL +#ifndef ENABLE_SMALL + ERR_load_crypto_strings (); +#endif + OpenSSL_add_all_algorithms (); +#endif

Quoted 7 lines

OpenSSL_add_algorithms() is also needed for *non-SSL* stuff. It is populates the internal OpenSSL lookup tables, so you can lookup strings like "MD5", "SHA512", "AES256", etc, etc via EVP_get_digestbyname() and EVP_get_cipherbyname() which will return the proper EVP_* objects back. And neither of these are strictly SSL, they are all crypto related. SSL depends on the crypto part, but the crypto doesn't need SSL.

Well, it's the #ifNdef directive used, so when building *without* SSL support, the OpenSSL_add_all_algorithms() will be called here, in crypto_openssl.c

And when building with SSL support, it won't be called here, but in ssl_openssl.c in tls_init_lib() instead.

Quoted 1 line · Igor Novgorodov

On 28.02.2012 15:34, David Sommerseth wrote:

Quoted 2 lines

And when building with SSL support, it won't be called here, but in ssl_openssl.c in tls_init_lib() instead.

Indeed. This looks good. So unless Adriaan see some other concerns.

Again, sorry about the noise!

Quoted 8 lines · Igor Novgorodov

Quoted 7 lines

Then maybe we should move these calls to crypto_openssl.c into crypto_init_lib() function to make crypto.c library-independent? And why OpenSSL_add_all_algorithms() and stuff is called only when USE_SSL is not defined?

And if these calls are for 0.9.8, maybe add a check for OpenSSL version?

Quoted 7 lines

Remember that OpenSSL covers two parts. One part is the SSL stuff, the other part is the crypto layer. So even if the SSL stuff isn't used, the crypto stuff most likely is. In the crypto stuff, also all the hashing algorithms are included. However, using SSL without crypto doesn't make sense. If it's not needed any more by OpenSSL 1.0.0, then make it version dependent. Can probably be done at compile time.

Well, i'm no expert in OpenSSL programming, but looking through internet, i haven't found an evidence that this stuff should not be called during initialization in OpenSSL 1.0.x

So, just to make OpenVPN possible to build with --ssl-type=polarssl and --disable-ssl, i propose the attached patch that moves calls to these functions into crypto_openssl.c

Quoted 8 lines

Removing the ERR_load_crypto_strings() call will most likely break the error logging too, which is used by the msg() function. It will not make the crypto/SSL errors more understandable, how I understand it.

May I suggest that both ERR_load_crypto_strings() and SSL_load_error_strings() (gotta love the consistency of function naming) is loaded by default, unless ENABLE_SMALL is defined?

I agree, added the check for ENABLE_SMALL in ssl_openssl.c and crypto_openssl.c to the attached patch.

Quoted 8 lines

Right now, this patch makes me really concerned and scared. For this to be accepted, a lot of testing must be done - and most likely by people understanding the darker sides of crypto far better than I. We can't risk that we're regressing on a well proved and tested encryption layer. There are people located in not so democratic countries who use OpenVPN to access a not-restricted/censored Internet - and their safety may rely on the security OpenVPN provides.

I agree fully. So if we just move these calls into crypto_openssl.c, no regression would occur i think.