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

OpenSSL v1.1 fixes for v3.0.x #1850

Merged
merged 16 commits into from Dec 7, 2016

Conversation

spbnick
Copy link
Contributor

@spbnick spbnick commented Nov 18, 2016

This makes v3.0.x build on Fedora Rawhide and concerns #1817.

I tested the build on Fedora Rawhide, Debian Testing, and RHEL6.8.

I didn't run any tests.

There are still a couple OpenSSL-related warnings on Rawhide, which I'm not yet sure how to deal with:

src/main/threads.c: In function ‘request_handler_thread’:
src/main/threads.c:721:2: warning: ‘ERR_remove_state’ is deprecated [-Wdeprecated-declarations]
  ERR_remove_state(0);
  ^~~~~~~~~~~~~~~~
In file included from /usr/include/openssl/opensslconf.h:42:0,
				 from /usr/include/openssl/md4.h:13,
				 from src/freeradius-devel/md4.h:30,
				 from src/freeradius-devel/libradius.h:96,
				 from src/freeradius-devel/radiusd.h:29,
				 from src/main/threads.c:27:
/usr/include/openssl/err.h:247:1: note: declared here
 DEPRECATEDIN_1_0_0(void ERR_remove_state(unsigned long pid))
 ^
At top level:
src/main/threads.c:239:13: warning: ‘ssl_locking_function’ defined but not used [-Wunused-function]
 static void ssl_locking_function(int mode, int n, UNUSED char const *file, UNUSED int line)
			 ^~~~~~~~~~~~~~~~~~~~
src/main/threads.c:225:22: warning: ‘ssl_id_function’ defined but not used [-Wunused-function]
 static unsigned long ssl_id_function(void)
					  ^~~~~~~~~~~~~~~

src/main/tls.c: In function ‘tls_global_cleanup’:
src/main/tls.c:2525:2: warning: ‘ERR_remove_state’ is deprecated [-Wdeprecated-declarations]
  ERR_remove_state(0);
  ^~~~~~~~~~~~~~~~
In file included from /usr/include/openssl/opensslconf.h:42:0,
				 from /usr/include/openssl/md4.h:13,
				 from src/freeradius-devel/md4.h:30,
				 from src/freeradius-devel/libradius.h:96,
				 from src/freeradius-devel/radiusd.h:29,
				 from src/main/tls.c:28:
/usr/include/openssl/err.h:247:1: note: declared here
 DEPRECATEDIN_1_0_0(void ERR_remove_state(unsigned long pid))
 ^

RHEL6.8 also complained a bit:

src/main/tls.c: In function ‘cbtls_verify’:
src/main/tls.c:2131: warning: cast discards qualifiers from pointer target type
src/main/tls.c:2139: warning: cast discards qualifiers from pointer target type
src/main/tls.c:2143: warning: cast discards qualifiers from pointer target type

The above can be fixed by checking that we're below OpenSSL_0_9_8y, and
removing const from ext_list in cbtls_verify in src/main/tls.c, but I'm not
sure if it's worth it, or maybe I just had enough for today.

Some of the fallback functions I defined in missing-h might not be necessary.

Please tell me what you think and if anything needs changing.

Thank you.

@spbnick
Copy link
Contributor Author

spbnick commented Nov 18, 2016

Alright, I see some failures in CI and will try to fix them next week.
It would still be good if you gave me some feedback regardless.

@herwinw
Copy link
Contributor

herwinw commented Nov 20, 2016

Not directly related to this change, but it looks like both 3.0 and 3.1 compile fine on my Debian Sid system (OpenSSL 1.1.0c-1) now. I'm actually not really sure what caused this

@spbnick spbnick closed this Nov 21, 2016
@spbnick spbnick reopened this Nov 21, 2016
@spbnick spbnick force-pushed the v3.0.x_openssl_1.1_fix branch 2 times, most recently from 44db3d0 to 6979958 Compare November 21, 2016 12:32
@spbnick
Copy link
Contributor Author

spbnick commented Nov 21, 2016

The mentioned warnings are still there (except ones seen on RHEL6), but otherwise it builds fine in CI now.

@spbnick
Copy link
Contributor Author

spbnick commented Nov 23, 2016

Alright, now all OpenSSL-related warnings are gone. @alandekok, @arr2036, could you please review? Thank you.

p += SSL3_RANDOM_SIZE;
#ifdef HAVE_SSL_GET_CLIENT_RANDOM
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to just use the local version of SSL_get_server_random() instead of ifdefs. We took a pass through the code to do this, but haven't finished it yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, OK, makes sense.

@alandekok
Copy link
Member

It looks pretty good. I'll take another pass through it later today.

Needed for conditionally avoiding accessing private OpenSSL structures
in a following patch.

Backported from v3.1.x.
Switch to checking for EVP_CIPHER_CTX_new instead of EVP_cleanup to
detect presence of libcrypto, because EVP_cleanup was removed as symbol
from OpenSSL 1.1, and the check would always fail.
Apart from dealing with a FIXME, this is needed for implementing
compatibility fallbacks for some functions introduced in OpenSSL 1.1, in
following commits.
Check for presence of openssl/asn1.h to support definition of fallback
functions in later patches.
Check for presence of openssl/conf.h to support definition of fallback
functions in later patches.
Add four fallback function implementations to use in place of functions
removed/deprecated in OpenSSL 1.1. Those are to be used in the following
patches to make the build work and not produce deprecation warnings.
Some more OpenSSL structures were made private in v1.1 and accessor
functions were added instead. Switch to using accessor functions to fix
the build.
Add the missing mandatory HMAC context initialization to rlm_otp's
otp_gen_state. Otherwise the outcome of the following HMAC operations is
undefined.
Switch to using HMAC_CTX_new in place of HMAC_CTX_init, which was
removed in OpenSSL 1.1, resulting in broken build.
Replace remaining use of HMAC_Init with HMAC_Init_ex to silence
deprecation warnings with OpenSSL 1.1.
Switch to using ASN1_STRING_get0_data instead of ASN1_STRING_data, which
was deprecated in OpenSSL 1.1 and would produce build warnings.
Switch to using CONF_modules_load_file instead of OPENSSL_config, which
was deprecated in OpenSSL 1.1 and would produce build warnings.
Update some declarations to use const to match respective changes in
OpenSSL 1.1 and not produce build warnings.
Use appropriate OpenSSL thread cleanup function or don't use any,
depending on their deprecation status in various OpenSSL versions.
Check if CRYPTO_set_id_callback and CRYPTO_set_locking_callback are
defined as functions (as opposed to stub macros), and if they aren't,
don't call them and don't define the corresponding callbacks.

This avoids the "unused function" warnings with OpenSSL v1.1.
@spbnick
Copy link
Contributor Author

spbnick commented Dec 7, 2016

Alright, I moved SSL_get_client_random, SSL_get_server_random, and SSL_SESSION_get_master_key to missing.c and switched to using them exclusively in mppe_keys.c instead of ifdefs.

@alandekok alandekok merged commit b810473 into FreeRADIUS:v3.0.x Dec 7, 2016
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.

None yet

3 participants