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

Handle SSL/TLS correctly #110

Closed
pali opened this issue Mar 23, 2017 · 11 comments

Comments

Projects
None yet
3 participants
@pali
Copy link
Contributor

commented Mar 23, 2017

Currently DBD::mysql has these SSL/TLS connections parameters:

mysql_ssl
mysql_ssl_client_key
mysql_ssl_client_cert
mysql_ssl_ca_file
mysql_ssl_ca_path
mysql_ssl_cipher
mysql_ssl_verify_server_cert

After discussion in #82 and #108 I'm proposing following change how DBD::mysql should process SSL/TLS settings:

  1. Parameter mysql_ssl=1 would change its meaning and after this change SSL/TLS will be required and enforced. If server does not support it then client must not connect to MySQL server and must reject connection. mysql_ssl=0 would mean that SSL/TLS is disabled (default value).

  2. Introduce new mysql_ssl_optional=1 parameter (default false; SSL/TLS is required and enforced) which could allow to connect to MySQL server without SSL/TLS support when mysql_ssl=1. This is dangerous as BACKRONYM (http://backronym.fail/) or Riddle (http://riddle.link/) can take effect, but in some cases it could be useful (e.g. when it can be ensured that it is not possible to modify any packet between client and server, just there can be passive monitoring of network). Documentation for mysql_ssl_optional=1 must describe this problem and suggest users to not enable this option if they are unsure.

  3. When specified mysql_ssl_ca_file or mysql_ssl_ca_path then DBD::mysql must check and verify CA certificate. If validation fails then connection must be rejected. Certificate hostname is not validated until mysql_ssl_verify_server_cert=1 is set.

  4. If DBD::mysql (or underlaying libmysqlclient.so library) decide to reject connection with MySQL server then data (login credentials, authentication negotiation or SQL statements) must not be sent to MySQL server.

  5. If underlaying libmysqlclient.so library is not able to enforce current configuration specified by mysql_ssl* parameters then it must reject connection.

  6. If it is possible try to support different versions of libmysqlclient.so library (MySQL, MariaDB, 5.5, 5.6, 5.7, 8.0, ...). If not, rather drop SSL support for particular libmysqlclient.so version as providing incorrect/broken/vulnerable SSL encryption.

  7. If some combination of SSL parameters is not supported by current version of libmysqlclient.so or combination does not make any sense (e.g mysql_ssl=0 and mysql_ssl_verify_server_cert are specified), then DBD::mysql must throw error and refuse connection.

CC: @dveeden @mbeijen @CaptTofu
Please review my changes and if something is missing or incorrect let me know.

@dveeden

This comment has been minimized.

Copy link
Collaborator

commented Mar 28, 2017

You forgot about mysql_ssl_verify_server_cert

re 1. Making mysql_ssl=1 enforce TLS feels right. Should mysql_ssl=0 then mean TLS is fully disabled? And not specifying this would make TLS preferred?

re 2. Sounds right.

re 3. Completely agree

re 4. Yes, but how would that be implemented?

re 5. Agree

re 6. Not sure I like the solution, but I don't know of anything better.

Also:

  • If TLS is somehow not possible: die
  • What about hostname validation? By default mysql_ssl_rsa_setup generated a certificates which doesn't match the hostname and SubjectAltName support is missing..
@pali

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2017

You forgot about mysql_ssl_verify_server_cert

It is needed? I think it is already handled by 3.?

Should mysql_ssl=0 then mean TLS is fully disabled?

Yes.

And not specifying this would make TLS preferred?

No, same as mysql_ssl=0 -- disabled. As wrote in 2. I do not think that it make sense to have default option which allows man in the middle attack.

re 4. Yes, but how would that be implemented?

By underlying libmysqlclient.so library. If current version does not support it then 5. will be applied.

What about hostname validation?

We could introduce new option e.g. mysql_ssl_disable_hostname_validation (or better shorter name) which can turn off host name validation.

@dveeden

This comment has been minimized.

Copy link
Collaborator

commented Mar 29, 2017

mysql_ssl_verify_server_cert is already present. It turns on hostname verification.

I do think having TLS preferred should be the default. Yes this is not perfect, but I think it's better than not using TLS at all.

Having hostname validation on by default would be nice. Just change the default for mysql_ssl_verify_server_cert to be 1.

What about backwards compatibility? I think changing this should result in a new major version. I wouldn't like to force the new behaviour on someone who just wants to update DBD::mysql because of an unrelated security issue.

@pali

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2017

mysql_ssl_verify_server_cert is already present. It turns on hostname verification.

It is not so good name when "verify_server_cert" is configuring verification of hostname...

I do think having TLS preferred should be the default. Yes this is not perfect, but I think it's better than not using TLS at all.

My idea to have TLS turned off by default is because:

  1. Do not have enabled vulnerable TLS by default
  2. In most cases TLS is not needed when MySQL and application are running on same server node. Using TLS here is contra-productive, only slow down communication
  3. Those who running MySQL server and client application on different nodes, then already need to use some really secured connection. So they either need to ensure enforced TLS or create own secure tunnel (e.g. stunnel, vpn, ipsec...) and TLS from MySQL is useless and again only contra-productive.

I think that TLS should be by default used correctly and behavior "try tls, if failed then fallback to plain text" is not correct as default. I understand that in some situations it can be useful, but it depends on other settings of connection and it is still better to not provide false-security in default configuration.

@mbeijen @CaptTofu what is your opinion for this?

@CaptTofu

This comment has been minimized.

Copy link
Member

commented Mar 29, 2017

@pali

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2017

mysql_ssl_verify_server_cert is already present. It turns on hostname verification.

It is not so good name when "verify_server_cert" is configuring verification of hostname...

Looks like this "verify_server_cert" is already defined by MySQL API and used also for mysql command line client. So stay with current name and just properly write into documentation that mysql_ssl_verify_server_cert enable certificate validation together with hostname. Without mysql_ssl_verify_server_cert is certificate also validated (when mysql_ssl_ca_file or mysql_ssl_ca_path are specified), but hostname of certificate is not. I think this is not a security problem as enabling mysql_ssl_verify_server_cert ensure verification of certificate.

I updated my first comment.

@pali

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2017

I finished pull request #114. Please look and review it.

Basically everything from my first comment is implemented.

@dveeden

This comment has been minimized.

Copy link
Collaborator

commented Apr 16, 2017

I had a quick look at the code and did a very rudimentary test.
LGTM

@pali

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2017

Implemented and merged in 23e8012

@pali

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2017

Fix was reverted in 4.043, therefore reopening.

@dveeden

This comment has been minimized.

Copy link
Collaborator

commented Sep 15, 2018

This has been fixed in 4.044

@dveeden dveeden closed this Sep 15, 2018

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.