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

enable automatic ECDH and DHE when possible (openssl 1.0.2) #1024

Merged
merged 2 commits into from Mar 1, 2018

Conversation

Projects
None yet
3 participants
@enricotagliavini
Contributor

enricotagliavini commented Feb 2, 2018

Openssl 1.1.0 and later are enabling ECDH automatically, but for older
version it must be enabled explicitly or all Perfect Forward Secrecy
ciphers will be silently ignored. See also [1]. This commit applies the
same fix as found in CnetOS 7 httpd package to enable automatic ECDH as
found in [2].

Should fix issue #1023

[1] https://wiki.openssl.org/index.php/Diffie-Hellman_parameters
[2] https://git.centos.org/blob/rpms!httpd.git/c7/SOURCES!httpd-2.4.6-ssl-ecdh-auto.patch

enable automatic ECDH when possible (openssl 1.0.2)
Openssl 1.1.0 and later are enabling ECDH automatically, but for older
version it must be enabled explicitly or all Perfect Forward Secrecy
ciphers will be silently ignored. See also [1]. This commit applies the
same fix as found in CnetOS 7 httpd package to enable automatic ECDH as
found in [2].

[1] https://wiki.openssl.org/index.php/Diffie-Hellman_parameters
[2] https://git.centos.org/blob/rpms!httpd.git/c7/SOURCES!httpd-2.4.6-ssl-ecdh-auto.patch
@enricotagliavini

This comment has been minimized.

Contributor

enricotagliavini commented Feb 2, 2018

Please note I tested this only on CentOS 7 and I was able to connect with cipher ECDHE-RSA-AES256-GCM-SHA384 from xfreerdp client on Fedora 27.

@speidy

This comment has been minimized.

Contributor

speidy commented Feb 3, 2018

@enricotagliavini

This comment has been minimized.

Contributor

enricotagliavini commented Feb 3, 2018

I understand your point. I'll have a look if I can manage. Although keep in mind DHE and co. are not recommended any longer for modern setups. For example, Mozilla recommends ECDH only [1].

I have a significant maintenance going on until Tuesday, included, so I will start looking into this after that.

[1] https://mozilla.github.io/server-side-tls/ssl-config-generator/?server=apache-2.4.6&openssl=1.0.2k&hsts=yes&profile=modern

@metalefty

This comment has been minimized.

Member

metalefty commented Feb 5, 2018

LGTM at least about ECDH.
I confirmed that this patch enables xrdp to use ECDHE-* also on FreeBSD 11-STABLE.
FreeBSD's base system OpenSSL is still 1.0.2k with some FreeBSD patches.

v0.9.5 + FreeBSD 11-STABLE (OpenSSL 1.0.2k)

  • Windows 10 IP build 17083: AES256-GCM-SHA384
  • Windows 7 SP1: AES256-GCM-SHA384
  • FreeRDP 1.1.0 on macOS High Sierra (LibreSSL 2.2.7):: AES256-SHA
  • FreeBSD 2.0.0 on macOS High Sierra (LibreSSL 2.2.7): AES256-GCM-SHA384

this PR + FreeBSD 11-STABLE (OpenSSL 1.0.2k)

  • Windows 10 IP build 17083: ECDHE-RSA-AES256-GCM-SHA384
  • Windows 7 SP1: ECDHE-RSA-AES256-SHA384
  • FreeRDP 1.1.0 on macOS High Sierra (LibreSSL 2.2.7):: ECDHE-RSA-AES256-SHA
  • FreeBSD 2.0.0 on macOS High Sierra (LibreSSL 2.2.7): ECDHE-RSA-AES256-GCM-SHA384
add support for DHE ciphers via compiled in dhparam
make it possible to use regular (non EC) EDH ciphers. To make this
possible a Diffie-Hellman parameter must be passed to the openssl
library. There are a few options possible as described in the manuals at
[1] and [2]. Simplest approach is to generate a DH parameter using
openssl dhparam -C <lenght> and include the code into the application.
The lenght used for this commit is 2236 bits long, which is the longest
possible without risking backward incompatibilities with old systems as
stated in [1]. Newer systems should use ECDH anyway, so it makes sense
to keep this method as compatible with older system as possible.
Paramters longer than 2048 should still be secure enough at the time of
writing.

[1] https://wiki.openssl.org/index.php/Diffie-Hellman_parameters
[2] https://wiki.openssl.org/index.php/Manual:SSL_CTX_set_tmp_dh_callback(3)

@enricotagliavini enricotagliavini changed the title from enable automatic ECDH when possible (openssl 1.0.2) to enable automatic ECDH and DHE when possible (openssl 1.0.2) Feb 7, 2018

@enricotagliavini

This comment has been minimized.

Contributor

enricotagliavini commented Feb 7, 2018

Thank you for testing the change on so many platforms!

So I added the DHE (non EC based) support. I'll be honest and say am a bit confused by the docs on it, so I hope what I did is not a giant mistake. I forced the cipher list to be only DHE-RSA-AES256-GCM-SHA384 and I was able to connect after I applied this changes. I was not with the stock version.

There are at least 3 options for the DHE support:

  • 1 generate a random DH parameter and compile it in the application
  • 2 make the user / admin generate one and load it at runtime
  • 3 use a callback function

I choose option 1. because it's the simplest for developers, for users and for admins. It just works out of the box without any additional human intervention and you don't risk forgetting a file and having a potential problem hard to sort out (as logs will not mention, it will be completely silent).

I don't like 2. for the reasons explained. I think this kind of stuff should be automatic and require no user intervention like in the case of ECDH, but it's easy to conditionally load the dhparam from the file if the user specify one in the config

Option 3. is the most elaborate solution and that's what apache httpd's mod_ssl uses as far as I understand. This adds a callback function the openssl library will call when it needs a dhparam and they call the standard openssl functions like get_rfc3526_prime_2048 (see [1] and [2]).

What do you think? Would that be good enough?

Also feel free to re-generate the key, I get you might not want to trust the first random person on the internet to generate a random safe key. No offense taken if you do :)

[1] https://github.com/openssl/openssl/blob/28c0a61b3084ca05d1c590113332501e96b6175d/include/openssl/bn.h#L465
[2] https://github.com/apache/httpd/blob/8d52110d191880fd066b0af35e092a4f3320f8f9/modules/ssl/ssl_engine_init.c#L78

@@ -32,6 +32,7 @@
#include <openssl/hmac.h>
#include <openssl/bn.h>
#include <openssl/rsa.h>
# include <openssl/dh.h>

This comment has been minimized.

@metalefty

metalefty Mar 1, 2018

Member

remove the whitespace after #

see also
* https://wiki.openssl.org/index.php/Diffie-Hellman_parameters
* https://wiki.openssl.org/index.php/Manual:SSL_CTX_set_tmp_dh_callback(3)
*/

This comment has been minimized.

@metalefty

metalefty Mar 1, 2018

Member

This part doesn't meet our coding standard but I know this is autogenerated code by OpenSSL so it's OK with this.

@metalefty

This comment has been minimized.

Member

metalefty commented Mar 1, 2018

I also tested DHE. LGTM. I'll change trivial coding styles by myself.

@metalefty metalefty merged commit 70b5adb into neutrinolabs:devel Mar 1, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@enricotagliavini

This comment has been minimized.

Contributor

enricotagliavini commented Mar 1, 2018

Oh sorry about the space and coding style, I didn't noticed. Thank you very much!

@speidy

Sorry for the later review, but I think we should be 100% sure about this

g_writeln("SSL_CTX_set_tmp_dh failed");
return 1;
}
DH_free (dh);

This comment has been minimized.

@speidy

speidy Mar 17, 2018

Contributor

Aren't we freeing dh too early?
Did you test it really works with this particular dh groups?

This comment has been minimized.

@enricotagliavini

enricotagliavini Mar 19, 2018

Contributor

I didn't checked, have no idea how to and I also didn't checked the internal code for the function itself, I cannot figure out where is it. I just followed the example on the website wiki at https://wiki.openssl.org/index.php/Diffie-Hellman_parameters#Diffie-Hellman

@@ -592,6 +647,15 @@ ssl_tls_accept(struct ssl_tls *self, long ssl_protocols,
SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER |
SSL_MODE_ENABLE_PARTIAL_WRITE);
SSL_CTX_set_options(self->ctx, options);
DH *dh = get_dh2236 ();

This comment has been minimized.

@speidy

speidy Mar 17, 2018

Contributor

Check return value is not NULL?

This comment has been minimized.

@enricotagliavini

enricotagliavini Mar 19, 2018

Contributor

Actually yes we should check it's not NULL, it could happen if DH_new() returns NULL

DH *dh = get_dh2236 ();
if (SSL_CTX_set_tmp_dh(self->ctx, dh) != 1) {
g_writeln("SSL_CTX_set_tmp_dh failed");
return 1;

This comment has been minimized.

@speidy

speidy Mar 17, 2018

Contributor

Potential memory leak here, the SSL_CTX object is never freed afterwards.

This comment has been minimized.

@speidy

speidy Mar 17, 2018

Contributor

ok, my bad, its is freed afterwards by ssl_tls_delete() actually.

This comment has been minimized.

@speidy

speidy Mar 17, 2018

Contributor

can we use SSL_set_dh_auto() here like in ecdh?

This comment has been minimized.

@enricotagliavini

enricotagliavini Mar 19, 2018

Contributor

SSL_set_dh_auto() can be used with openssl 1.1.0 and newer. But there is the cons of having two code paths, which makes testing and reproducing issues harder.I'm 50-50 on this choice to be honest

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