MySQL: Add tls version pinning for MySQL #2675

RobinGeuze · 2017-08-07T08:22:45Z

Add MYSQL_OPT_TLS_VERSION to mysqlnd
Add support for MYSQL_OPT_TLS_VERSION to mysqli and pdo_mysql if using
mysqlnd or mysql client library >= 5.7.10

Relevant mysql documentation: https://dev.mysql.com/doc/refman/5.7/en/mysql-options.html

Allowing TLS version pinning prevents downgrade attacks to lower TLS versions.

andreyhristov

LGTM

andreyhristov · 2017-08-08T11:59:15Z

ext/mysqlnd/mysqlnd_enum_n_def.h

@@ -234,6 +234,7 @@ typedef enum mysqlnd_client_option
 	MYSQL_REPORT_DATA_TRUNCATION,
 	MYSQL_OPT_RECONNECT,
 	MYSQL_OPT_SSL_VERIFY_SERVER_CERT,
+	MYSQL_OPT_TLS_VERSION,


Hi, I just pushed a change to this file with this and other enums added (coming from MySQL 5.7)

Should I rebase on that?

Add MYSQL_OPT_TLS_VERSION to mysqlnd Add support for MYSQL_OPT_TLS_VERSION to mysqli and pdo_mysql if using mysqlnd or mysql client library >= 5.7.10

RobinGeuze · 2017-08-10T06:33:42Z

I rebased it and removed the resulting double MYSQL_OPT_TLS_VERSION constant.

andrewnester · 2017-08-11T14:07:02Z

ext/mysqli/tests/mysqli_constants.phpt

@@ -210,6 +210,10 @@ require_once('skipifconnectfailure.inc');
 		$expected_constants["MYSQLI_TYPE_JSON"]	= true;
 	}

+	if ($IS_MYSQLND || $version >= 50710) {
+		$expected_constants['MYSQLI_OPT_TLS_VERSION']	= true;


redundant whitespace before = sign

andrewnester · 2017-08-11T14:08:27Z

ext/mysqlnd/mysqlnd_net.c

@@ -817,6 +817,29 @@ MYSQLND_METHOD(mysqlnd_net, set_client_option)(MYSQLND_NET * const net, enum mys
 			net->data->options.ssl_verify_peer = val;
 			break;
 		}
+		case MYSQL_OPT_TLS_VERSION:
+			if (strcmp(value, "TLSv1") == 0) {


maybe strncmp is better?

andrewnester · 2017-08-11T14:14:17Z

ext/mysqlnd/mysqlnd_vio.c

@@ -414,6 +414,29 @@ MYSQLND_METHOD(mysqlnd_vio, set_client_option)(MYSQLND_VIO * const net, enum_mys
 			net->data->options.ssl_verify_peer = val;
 			break;
 		}
+		case MYSQL_OPT_TLS_VERSION:
+			if (strcmp(value, "TLSv1") == 0) {


looks like code duplication https://github.com/php/php-src/pull/2675/files#diff-07a72755040c3a5d02660c0559d48a3bR820
I would suggest to move it to separate function/macro

Actually that entire function seems to be duplicated between mysqlnd_vio.c and mysqlnd_net.c. It seems that the _net version is no longer used, but I wasn't sure, so I just copied was what there.

RobinGeuze · 2017-08-17T07:17:09Z

Since this is my first contribution to the PHP project, is there anything else I need to do to get this merged?

andrewnester · 2017-08-17T07:20:29Z

@RobinGeuze First of all, thank you for your work!
Have you created any ticket in PHP bug tracker or started internal discussion about your improvement in PHP internals? It might help to speed up process of accepting your changes

KalleZ · 2017-08-17T08:00:47Z

@andrewnester as @andreyhristov said good for it (mysql maintainer), it should just be merged to master at least :)

KiNgMaR · 2017-08-18T15:09:41Z

Shouldn't TLSv1.x with x > 2 fall back to the latest supported version (currently 1.2) instead of "any"? If somebody requests "TLSv1.3" (in user land), but the PHP version doesn't know anything about it yet, I think quietely allowing TLS 1.0 (which is included in STREAM_CRYPTO_METHOD_TLS_ANY_CLIENT) is not the safest approach.

RobinGeuze · 2017-08-18T15:56:33Z

Another option would be to throw an error for an unrecognized option. Maybe that is a nicer solution?

It uses the same set of rules (and is based on) the MySQL client library process_tls_version function to ensure consistent behaviour

RobinGeuze · 2017-08-21T08:28:48Z

So I reworked it a little bit to throw an error when an unrecognised option is supplied. This is consistent with the behaviour of the mysql client library, which seemed like the right thing to do. You can now also specify multiple tls versions in a comma separated manner.

RobinGeuze · 2017-08-25T06:39:17Z

@KiNgMaR is this a satisfying solution for your concerns? Also @andrewnester these changes also fix the last comment you had about code duplication.

KiNgMaR · 2017-08-25T06:53:13Z

@RobinGeuze: yes, I think that throwing an error is a better solution than a silent fallback 👍

RobinGeuze · 2017-08-29T11:53:57Z

So should I make a post to PHP internals or can this be merged as is?

dennisdegreef · 2017-12-05T12:05:07Z

Bump?

bukka · 2017-12-06T13:41:00Z

I don't think it is a good idea to do single version constant and support list of versions (if I understand the patch correctly). It creates potential for holes (e.g. TLSv1,TLSv1.2 which is not working well and might have some security implications). It should be min and max version IMO.

Currently we don't have min and max version support in streams but I hope to propose it before 7.3 freeze and then it should be quite simple to change it in here. It might be worth to wait with merging though.

RobinGeuze · 2017-12-06T14:18:34Z

@bukka This behaviour is the same as the normal libmysql behavior (based on the source). I personally thought it would be a bad idea to diverge the workings between mysqlnd and libmysql.

bukka · 2017-12-10T19:27:44Z

@RobinGeuze As I said this is problematic and we shouldn't add any option that allows creating protocol holes. This is explicitly documented in OpenSSL docs:

https://www.openssl.org/docs/man1.1.0/ssl/SSL_CTX_new.html (Notes section)

Considering that libmysql uses OpenSSL they probably do something wrong unless it explicitly checks for the holes.

Anyway it shouldn't matter from the user point of view as that selection can be converted to the list of versions if needed and we can still expose constants for min and max.

RobinGeuze · 2017-12-11T08:09:27Z

@bukka Looking at the OpenSSL documentation you are indeed right. Looking at the libmysql code here: https://github.com/mysql/mysql-server/blob/5.7/vio/viosslfactories.c#L433 it doesn't seem they do any hole detection, so that could be considered a bug on their side (mariadb doesn't seem to support the feature in general). I've field a bug report here: https://bugs.mysql.com/bug.php?id=88851

If there can't be any holes in the version list I would prefer only allowing a minimal version, since I don't really see a reason to limit the maximum version.

Also if we shouldn't do this in general, why do SSL streams allow it.

Tbh I also find the OpenSSL behavior in this case weird, what if there is a vulnerability that exists solely in TLSv1.1 (unlikely I know, this is just an example), there now is no way to disable just that version without just downgrading all connections to TLSv1.0.

…d maximum version

RobinGeuze · 2017-12-13T09:35:53Z

MySQL has accepted my bug request. Accordingly I have adapted the MR. It now looks for the highest and lowest version listed in the list and builds the set between those versions. @bukka do you agree this is the right way to go for now (pending whatever MySQL is going to do obviously).

bukka · 2017-12-17T19:18:05Z

@RobinGeuze I still think it would be better to introduce min and max as it's more flexible and doesn't require hidden adding of missing protocols. There is no point to mimic exactly what libmysql does IMHO. Basically it seems better to use 2 options rather than one like TLSv1.0,TLSv1.2. Technically most users will just need a single options (min). The max makes sense only if the server cannot handle higher protocol which is not so usual - it happened with an old OpenSSL version that failed if the TLS version was higher than 1.0 but that was fixed later. It's just for broken servers though.

RobinGeuze · 2017-12-18T07:37:54Z

@bukka The reason I want to "mimic" what MySQL does is because you can also build against libmysql instead of mysqlnd. With the current way it works the code is completely portable between the two (although they work slightly differently). If we change it to two separate settings that means we need to add wrapper code in both MySQLi and PDO_MySQL making it much harder to maintain.

Maybe its a good idea to hold of on putting this in a release till the Oracle guys decide whtat they want to do with it, but that might take considerable time.

Tbh the most common use case I see for this (and what I use it for myself) is pinning the TLS version to the highest version supported by both sides of the application.

krakjoe added Feature Waiting on Review labels Aug 7, 2017

RobinGeuze force-pushed the addTlsVersionPinningForMySQL branch from 3b62bcb to 53f653f Compare August 7, 2017 10:57

andreyhristov approved these changes Aug 8, 2017

View reviewed changes

Robin Geuze added 3 commits August 10, 2017 08:28

MySQL: implement TLS version pinning for MySQL

4456859

Add MYSQL_OPT_TLS_VERSION to mysqlnd Add support for MYSQL_OPT_TLS_VERSION to mysqli and pdo_mysql if using mysqlnd or mysql client library >= 5.7.10

MySQLi: add MYSQLI_OPT_TLS_VERSION to class constants test

ca9351c

Rebase and remove resulting double MYSQL_OPT_TLS_VERSION enum value

4b2939f

RobinGeuze force-pushed the addTlsVersionPinningForMySQL branch from 95f00e2 to 4b2939f Compare August 10, 2017 06:33

andrewnester reviewed Aug 11, 2017

View reviewed changes

Processed review feedback from andrewnester

c93941b

andrewnester approved these changes Aug 14, 2017

View reviewed changes

Robin Geuze added 2 commits August 21, 2017 10:04

Add mysqlnd_process_tls_version to parse the supplied TLS version

36f1125

It uses the same set of rules (and is based on) the MySQL client library process_tls_version function to ensure consistent behaviour

Remove unnecessary whitespace and unused variable

b26edaa

Robin Geuze added 2 commits December 13, 2017 09:58

Rework code to interpret the specified TLS versions as the minimum an…

a3ddec5

…d maximum version

Update comment to make clear the reason for our min and max structure

e717f1d

RobinGeuze closed this Oct 30, 2020

MySQL: Add tls version pinning for MySQL #2675

MySQL: Add tls version pinning for MySQL #2675

Uh oh!

Conversation

RobinGeuze commented Aug 7, 2017

Uh oh!

andreyhristov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RobinGeuze commented Aug 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RobinGeuze commented Aug 17, 2017

Uh oh!

andrewnester commented Aug 17, 2017

Uh oh!

KalleZ commented Aug 17, 2017

Uh oh!

KiNgMaR commented Aug 18, 2017

Uh oh!

RobinGeuze commented Aug 18, 2017

Uh oh!

RobinGeuze commented Aug 21, 2017

Uh oh!

RobinGeuze commented Aug 25, 2017

Uh oh!

KiNgMaR commented Aug 25, 2017

Uh oh!

RobinGeuze commented Aug 29, 2017

Uh oh!

dennisdegreef commented Dec 5, 2017

Uh oh!

bukka commented Dec 6, 2017

Uh oh!

RobinGeuze commented Dec 6, 2017

Uh oh!

bukka commented Dec 10, 2017

Uh oh!

RobinGeuze commented Dec 11, 2017

Uh oh!

RobinGeuze commented Dec 13, 2017

Uh oh!

bukka commented Dec 17, 2017

Uh oh!

RobinGeuze commented Dec 18, 2017

Uh oh!

Uh oh!