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

net_ssl: SSL config tweaks for compatibility and security #5509

Merged
merged 1 commit into from May 7, 2019

Conversation

Projects
None yet
6 participants
@moneromooo-monero
Copy link
Contributor

commented May 1, 2019

When built with the depends system, openssl does not include any
cipher on the current whitelist, so add this one, which fixes the
problem, and does seem sensible.

@vtnerd
Copy link
Contributor

left a comment

The exact problem is that RSA is used by default for certificates, but no RSA authentication available in OpenSSL 1.0 is whitelisted. There's nothing wrong with this new cipher, except perhaps the lack of forward-secrecy, which every existing whitelisted cipher supports.

I recommend ECDHE-RSA-AES256-SHA384 and ECDHE-RSA-AES128-SHA256 instead. These match two existing whitelisted ciphers, but use RSA instead of ECDSA for endpoint authentication. They both support forward-secrecy too. SSL_CTX_set_ecdh_auto(ctx, 1); also needs to be added since this is not the default in 1.0.

There's also ECDHE-RSA-AES256-GCM-SHA384 and ECDHE-RSA-AES128-GCM-SHA256 which support forward secrecy with a GCM MAC. GCM should be faster but key/iv is catastrophic. The latter should be minimized/never since key-exchanges are being used. @omartijn did you specifically avoid GCM?

EDIT: I tested the mentioned ciphers with the "auto" parameter using OpenSSL 1.0 which was previously not working with RSA certificates.

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2019

I don't know what I'm doing here, but: those two are apparently CBC, which testssl complains about (they're potentially vulnerable to some timing attack). I replaced with the GCM versions, which fixes that, but testssl also complains about "server cipher order" not being set, and I have no idea what this is about. I tried sorting the ciphers in the same order "openssl ciphers" reports, but that doesn't help (not too surprisingly since I don't know what it is about and the internet doesn't help here).

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2019

I found how to fix the cipher order thing (some option on the SSL context).
That leaves the GCM or CBC thing (or something else).

@omartijn

This comment has been minimized.

Copy link

commented May 2, 2019

I didn't specially avoid GCM ciphers. However, when possible, you'd want to use the newer ciphers supported by TLS 1.2/1.3, like chacha. If that's not possible when building in depends, then I don't have any reason not to accept GCM.

I would, however, like to avoid using CBC ciphers if at all possible.

@moneromooo-monero moneromooo-monero force-pushed the moneromooo-monero:sslw branch from 4317790 to 21c022d May 2, 2019

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2019

Here's a version using the GCM variants then. testssl likes that config.

@vtnerd

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

The two existing AES ciphers (that use ECDSA auth) are CBC implementations too. There's a GCM (i.e. with ECDHE-ECDSA) equivalent to those.

Every cipher currently in the code and these just mentioned are TLS 1.2 ciphers. In TLS 1.3 CBC ciphers have been dropped and the ECDHE-ECDSA/ECDHE-RSA distinction has been dropped.

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2019

Maybe those ones are not vulnerable then, since testssl does not moan about those. Or are you saying it's probably missing those ?

@moneromooo-monero moneromooo-monero changed the title net_ssl: add AES256-GCM-SHA384 for Windows/depends net_ssl: SSL config tweaks for compatibility and security May 2, 2019

@iDunk5400

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

This is what I got on Windows with a x86_64-w64-mingw32 depends build using openssl 1.0.2q.

Hexcode  Cipher Suite Name (OpenSSL)       KeyExch.   Encryption  Bits     Cipher Suite Name (IANA/RFC)
-----------------------------------------------------------------------------------------------------------------------------
 xc028   ECDHE-RSA-AES256-SHA384           ECDH 256   AES         256      TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384
 xc027   ECDHE-RSA-AES128-SHA256           ECDH 256   AES         128      TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256
@moneromooo-monero

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2019

Same source gives me:

 xc030   ECDHE-RSA-AES256-GCM-SHA384       ECDH 253   AESGCM      256      TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384              
 xcca8   ECDHE-RSA-CHACHA20-POLY1305       ECDH 253   ChaCha20    256      TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256        
 xc02f   ECDHE-RSA-AES128-GCM-SHA256       ECDH 253   AESGCM      128      TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256              

on Linux, openssl 1.1.0i-fips

@vtnerd

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

Use a ECDSA certificate and then run the tool. Ciphers not supporting RSA authentication appear to be filtered out.

@vtnerd

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

Also @iDunk5400 did you use an older patch? The latest one is not using those.

@iDunk5400

This comment has been minimized.

Copy link
Contributor

commented May 2, 2019

I wasn't using this patch that time. With this patch, and the cipher list below (one of several I tried during testing) on Win64 builds from depends (openssl 1.0.2q) and MSYS2 (openssl 1.1.1b), the results were consistently as shown below. I haven't yet tested any combination using Linux daemons or wallets from Ubuntu 16.04 and 18.04 available to me. The following tests were using
ECDHE-ECDSA-CHACHA20-POLY1305-SHA256:ECDHE-ECDSA-CHACHA20-POLY1305:ECDHE-RSA-CHACHA20-POLY1305:ECDHE-ECDSA-AES256-GCM-SHA384:ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-RSA-AES256-GCM-SHA384:ECDHE-RSA-AES128-GCM-SHA256

depends x86_64-w64-mingw32, openssl 1.0.2q

Hexcode  Cipher Suite Name (OpenSSL)       KeyExch.   Encryption  Bits     Cipher Suite Name (IANA/RFC)
-----------------------------------------------------------------------------------------------------------------------------
 xc030   ECDHE-RSA-AES256-GCM-SHA384       ECDH 256   AESGCM      256      TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384
 xc02f   ECDHE-RSA-AES128-GCM-SHA256       ECDH 256   AESGCM      128      TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256

MSYS2, mingw64, openssl 1.1.1b

Hexcode  Cipher Suite Name (OpenSSL)       KeyExch.   Encryption  Bits     Cipher Suite Name (IANA/RFC)
-----------------------------------------------------------------------------------------------------------------------------
 xcca8   ECDHE-RSA-CHACHA20-POLY1305       ECDH 253   ChaCha20    256      TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256

As a result, wallet built with depends does not use SSL with daemon built in MSYS2, and vice versa.

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

I'm trying to change the key gen from RSA to ECDSA, and I'm seeing just curves I have no idea about:

2019-05-03 11:28:20.830	I   713: NIST/SECG curve over a 224 bit prime field
2019-05-03 11:28:20.830	I   714: SECG curve over a 256 bit prime field
2019-05-03 11:28:20.830	I   715: NIST/SECG curve over a 384 bit prime field
2019-05-03 11:28:20.830	I   716: NIST/SECG curve over a 521 bit prime field
2019-05-03 11:28:20.830	I   415: X9.62/SECG curve over a 256 bit prime field

This is all my openssl knows about AFAICT. Using one of these is needed to create a EC_KEY.

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

The same list also gets output from openssl ecparam -list_curves, but with actual names:

$ openssl ecparam -list_curves
  secp224r1 : NIST/SECG curve over a 224 bit prime field
  secp256k1 : SECG curve over a 256 bit prime field
  secp384r1 : NIST/SECG curve over a 384 bit prime field
  secp521r1 : NIST/SECG curve over a 521 bit prime field
  prime256v1: X9.62/SECG curve over a 256 bit prime field

No 25519 in sight. I have no idea which one of those is any good...

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

I picked the last one, I now get:

 xc024   ECDHE-ECDSA-AES256-SHA384         ECDH 253   AES         256      TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384            
 xcca9   ECDHE-ECDSA-CHACHA20-POLY1305     ECDH 253   ChaCha20    256      TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256      
 xc023   ECDHE-ECDSA-AES128-SHA256         ECDH 253   AES         128      TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256            
@moneromooo-monero

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

It appears we can generate two certificates at once. I pushed another commit which does that, generating an RSA one and an ECDSA one. I then get:

 xc030   ECDHE-RSA-AES256-GCM-SHA384       ECDH 253   AESGCM      256      TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384              
 xc024   ECDHE-ECDSA-AES256-SHA384         ECDH 253   AES         256      TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384            
 xcca9   ECDHE-ECDSA-CHACHA20-POLY1305     ECDH 253   ChaCha20    256      TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256      
 xcca8   ECDHE-RSA-CHACHA20-POLY1305       ECDH 253   ChaCha20    256      TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256        
 xc02f   ECDHE-RSA-AES128-GCM-SHA256       ECDH 253   AESGCM      128      TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256              
 xc023   ECDHE-ECDSA-AES128-SHA256         ECDH 253   AES         128      TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256            
@moneromooo-monero

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

And by changing the whitelist to use the GCM versions of CBC ciphers:

 xc030   ECDHE-RSA-AES256-GCM-SHA384       ECDH 253   AESGCM      256      TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384              
 xc02c   ECDHE-ECDSA-AES256-GCM-SHA384     ECDH 253   AESGCM      256      TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384            
 xcca9   ECDHE-ECDSA-CHACHA20-POLY1305     ECDH 253   ChaCha20    256      TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256      
 xcca8   ECDHE-RSA-CHACHA20-POLY1305       ECDH 253   ChaCha20    256      TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256        
 xc02f   ECDHE-RSA-AES128-GCM-SHA256       ECDH 253   AESGCM      128      TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256              
 xc02b   ECDHE-ECDSA-AES128-GCM-SHA256     ECDH 253   AESGCM      128      TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256            

All code updated in tmp-ec.

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2019

Better now. Works with all of iDunk's versions. The server generates both RSA and ECDSA certs.

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2019

Looking through docs, I see SSL_OP_SINGLE_DH_USE which is said to be needed in some cases for RSA usage. It also says it's a very heavy process. Any opinion whether this should be set ?

@vtnerd

This comment has been minimized.

Copy link
Contributor

commented May 5, 2019

I don't understand why the ECC certificate was added to the code. Was the thought to be compatible with more clients?

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2019

Yes. You had pointed out the ECDSA ciphers were filtered out earlier.

@vtnerd

vtnerd approved these changes May 6, 2019

Copy link
Contributor

left a comment

I wasn't expecting an ECC key to be generated by default too - I don't think its necessary. The default behavior would be to filter out ECDSA ciphers unless the user specified a custom ECC certificate. In that case, the ECDSA cipher would be used, and the RSA ciphers may be filtered if no custom RSA certificate was being used.

I'm not aware of any problems of using both like this though.

EC_GROUP_free(group);
return false;
}
if (EC_KEY_set_group(ec_key.get(), group) != 1)

This comment has been minimized.

Copy link
@vtnerd

vtnerd May 6, 2019

Contributor

Does this function take ownership of the group? The documentation doesn't make this clear and some quick Googling didn't help.

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero May 6, 2019

Author Contributor

It takes a ref to it. I added a free call.

if (EC_KEY_set_group(ec_key.get(), group) != 1)
{
MERROR("Error setting EC group");
EC_GROUP_free(group);

This comment has been minimized.

Copy link
@vtnerd

vtnerd May 6, 2019

Contributor

There's actually several places that could leak in the rare case an exception is thrown. This is an issue with existing code too, something to cleanup eventually.

Also, worth mentioning that the only potentially throwing call in some of these functions is the log output.

CHECK_AND_ASSERT_THROW_MES(create_ssl_certificate(pkey, cert), "Failed to create certificate");
bool ok = false;

CHECK_AND_ASSERT_THROW_MES(create_ec_ssl_certificate(pkey, cert, NID_X9_62_prime256v1), "Failed to create certificate");

This comment has been minimized.

Copy link
@vtnerd

vtnerd May 6, 2019

Contributor

Is this curve recommended somewhere? Or is it just the most widely supported?

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero May 6, 2019

Author Contributor

It is recommended in various places as good and the most compatible. It's also known as P-256 (and also secp256r1):

crypto/ecdh/ecdhtest.c: (NID_X9_62_prime256v1, "NIST Prime-Curve P-256", ctx, out))

https://security.stackexchange.com/questions/78621

My (Fedora) openssl has just a few curves available:

$ openssl ecparam -list_curves
  secp224r1 : NIST/SECG curve over a 224 bit prime field
  secp256k1 : SECG curve over a 256 bit prime field
  secp384r1 : NIST/SECG curve over a 384 bit prime field
  secp521r1 : NIST/SECG curve over a 521 bit prime field
  prime256v1: X9.62/SECG curve over a 256 bit prime field
@vtnerd

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

Tested this patch with OSX (OpenSSL 1.0) wallet -> Linux (OpenSSL 1.0) daemon. No noticeable issues.

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

Having both was necessary for some combination of wallets/daemons to talk SSL to each other (iDunk can give details on what combination).

@moneromooo-monero moneromooo-monero force-pushed the moneromooo-monero:sslw branch from dad1832 to dab9237 May 6, 2019

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

It seems I was misremembering. RSA was sufficient. Then all this two certs was because of "Use a ECDSA certificate and then run the tool. Ciphers not supporting RSA authentication appear to be filtered out.". Is this not actually needed ?

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

(since it makes the whitelisted ECDSA ciphers never actually used)

@MaxXor

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

Please note that the NIST had standardized shady things in the past (see this for reference: predictable PRNG). So I would suggest the use of secp256k1 curve, which is also widely supported. Unlike the popular NIST curves, secp256k1's constants were selected in a predictable way, which significantly reduces the possibility that the curve's creator inserted any sort of backdoor into the curve.

@vtnerd

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

It seems I was misremembering. RSA was sufficient. Then all this two certs was because of "Use a ECDSA certificate and then run the tool. Ciphers not supporting RSA authentication appear to be filtered out.". Is this not actually needed ?

(since it makes the whitelisted ECDSA ciphers never actually used)

Yes, adding the ECC certificate is probably not needed as RSA is (almost certainly) the most widely supported authentication algorithm in use. Assuming these numbers are accurate, OpenSSL ECC is much faster signature (server) than RSA. Specifying both would allow the client to select ECC, allowing for more throughput on the server, while still having the most widely adopted signature algorithm available as backup.

I doubt many use cases (except for maybe public nodes) will "need" the performance boost from ECC, so its primarily a question of what we want to maintain. Someone running public nodes should arguable do a custom certificate that gets cross-signed anyway.

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

OK, I'll leave it that way then, at least for now. Any comment on the curve being NIST, which was a target for subversion before ?

@vtnerd

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

I'm not aware of a definitive answer about the NIST curves. A NSA employee came up with the parameters for the curve, alarming some after the random number generator disaster, but the NSA would need secret weaknesses on the specific curve types to take advantage in this situation.

DJB (of Ed25519 fame) is critical that the parameters selected were not the most efficient (the stated goal by the authors of the curves), and the difficulty of implementing the curves in constant time. Some other well known cryptographers (on StackOverflow) seem to doubt the existence of special NSA knowledge, but argue that there are better curves with more justifiable parameter choices.

The curve that @MaxXor recommended is the "Bitcoin" curve, so it has that going for it. Ed25519 is not available in OpenSSL 1.0. I wouldn't select the NIST curve based on wide deployment; that is the purpose of the RSA cert.

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2019

This is so damn confusing. If I use secp256k1, testssl just sees the RSA cert, but somehow still does not filter out the ECDSA ciphers, but I end up connected with RSA. The NIST curve still is in the offered curves. But secp256k1 isn't. I think it's all a big plot to make things so complicated there are holes everywhere.

net_ssl: SSL config tweaks for compatibility and security
add two RSA based ciphers for Windows/depends compatibility
also enforce server cipher ordering
also set ECDH to auto because vtnerd says it is good :)

When built with the depends system, openssl does not include any
cipher on the current whitelist, so add this one, which fixes the
problem, and does seem sensible.

@moneromooo-monero moneromooo-monero force-pushed the moneromooo-monero:sslw branch from dab9237 to a62e072 May 7, 2019

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2019

I've disabled the EC cert for now, since RSA provides compatibility we can look at this later.

@fluffypony
Copy link
Collaborator

left a comment

Reviewed

@fluffypony fluffypony merged commit a62e072 into monero-project:master May 7, 2019

5 of 10 checks passed

buildbot/monero-static-osx-10.11 Build done.
Details
buildbot/monero-static-osx-10.12 Build done.
Details
buildbot/monero-static-win32 Build done.
Details
buildbot/monero-static-win64 Build done.
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
buildbot/monero-linux-armv7 Build done.
Details
buildbot/monero-linux-armv8 Build done.
Details
buildbot/monero-static-osx-10.13 Build done.
Details
buildbot/monero-static-ubuntu-amd64 Build done.
Details
buildbot/monero-static-ubuntu-i686 Build done.
Details

fluffypony added a commit that referenced this pull request May 7, 2019

Merge pull request #5509
a62e072 net_ssl: SSL config tweaks for compatibility and security (moneromooo-monero)
@vtnerd

This comment has been minimized.

Copy link
Contributor

commented May 7, 2019

This is so damn confusing. If I use secp256k1, testssl just sees the RSA cert, but somehow still does not filter out the ECDSA ciphers, but I end up connected with RSA. The NIST curve still is in the offered curves. But secp256k1 isn't. I think it's all a big plot to make things so complicated there are holes everywhere.

Don't worry, TLS 1.3 will fix everything! And if it doesn't then its a reason to create TLS 1.4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.