Set ssl_mode for Oracle MySQL if applicable. #108

dveeden · 2017-03-15T18:01:46Z

Supersedes #82

pali · 2017-03-15T21:55:39Z

With some versions of mysql client compilation failed... Looks like these continuous-integration tests are really good for catching problems with code which has lot of #ifdefs...

pali · 2017-03-15T22:04:40Z

Anyway, I do not think that usage of SSL/TLS tunel without verifycation of server certificate (either by CA or certificate chain or directly by fingerprint of server certificate) is a good idea. In some cases it could make sense (e.g. passive scanning), but basically it has similar security as not using SSL/TLS tunel.

Without verification of server certificate MITM is possible in same way as MITM for plain text without SSL/TLS.

I really would suggest that verification of server certificate should be preferred option. We probably want to have backward compatibility, but at least examples and documentation can be updated to show people default usage of certificate verification.

In every case it is better if users of DBD::mysql starts doing certificate verification. And documentation or "example" usage could help...

dveeden · 2017-03-16T08:37:20Z

Anyway, I do not think that usage of SSL/TLS tunel without verifycation of server certificate (either by CA or certificate chain or directly by fingerprint of server certificate) is a good idea. In some cases it could make sense (e.g. passive scanning), but basically it has similar security as not using SSL/TLS tunel.

It is slightly better than not using TLS as it protects against passive attacks.
However it can also give a false sense of security and be a performance overhead.
So no clear winner there.

Without verification of server certificate MITM is possible in same way as MITM for plain text without SSL/TLS.

Yes indeed.

I really would suggest that verification of server certificate should be preferred option. We probably want to have backward compatibility, but at least examples and documentation can be updated to show people default usage of certificate verification.

Yes, VERIFY_CA is reasonably secure. VERIFY_IDENTITY is more secure, but hostname validation is in practice not always possible becuase MySQL and MariaDB don't support SubjectAlternativeName.

In every case it is better if users of DBD::mysql starts doing certificate verification. And documentation or "example" usage could help...

Indeed.

pali · 2017-03-16T21:19:12Z

dbdimp.c

@@ -2083,11 +2081,33 @@ MYSQL *mysql_dr_connect(

 	    mysql_ssl_set(sock, client_key, client_cert, ca_file,
 			  ca_path, cipher);
-#if MYSQL_VERSION_ID >= SSL_VERIFY_VERSION && MYSQL_VERSION_ID <= SSL_LAST_VERIFY_VERSION
+#ifdef MYSQL_SSL_MODE
+            if ssl_verify_true:


syntax error. In C it is if (ssl_verify_true) and no colon at the end.

pali · 2017-03-16T21:20:18Z

dbdimp.h

 /* This is to avoid the ugly #ifdef mess in dbdimp.c */
 #if MYSQL_VERSION_ID < SQL_STATE_VERSION
 #define mysql_sqlstate(svsock) (NULL)
 #endif
-
+#ifdef SSL_MODE_PREFERRED


Looks like SSL_MODE_PREFERRED is not macro so this #ifdef is always false and MYSQL_SSL_MODE is never defined.

pali · 2017-03-17T23:25:04Z

dbdimp.c

+#ifdef MYSQL_SSL_MODE
+            if (ssl_verify_true)
+              ssl_mode = SSL_MODE_VERIFY_IDENTITY;
+            else if (strlen(ca_file) > 0)


ca_file and also ca_path could be NULL which means strlen here can crash

pali · 2017-03-17T23:28:07Z

dbdimp.c

+	    mysql_options(sock, MYSQL_OPT_SSL_ENFORCE, &ssl_verify_true);
+#endif
+#else
+            warn("Can't enable strict certificate checks");


I would say this is a fatal error.

Yes I agree. Note that currently there isn't even a warning in this case.

pali · 2017-03-17T23:31:07Z

Now we have there Riddle vulnerability at http://riddle.link/

And looks like Oracle probably fixes SSL enforcement in 5.5 and 5.6 versions of MySQL as documentation was already updated. Compare:
https://web.archive.org/web/20161219180824/https://dev.mysql.com/doc/refman/5.5/en/mysql-ssl-set.html
https://dev.mysql.com/doc/refman/5.5/en/mysql-ssl-set.html

In MySQL 5.5.55, the MYSQL_OPT_SSL_MODE option for mysql_options() was backported from MySQL 5.7 to MySQL 5.5.

But MySQL 5.5.55 was not released yet...

pali

This code starts to be a big #if mess with long conditions after #if... I do not have idea how to improve it right now.

But it would be a big problem after release of 5.5.55 as specified in documentation...

pali · 2017-03-20T12:10:36Z

dbdimp.c

+	    mysql_options(sock, MYSQL_OPT_SSL_ENFORCE, &ssl_verify_true);
+#endif
+#else
+            die("Can't enable strict certificate checks");


Use croak(...) as you are not passing return value from die(...) somewhere else. Look into perlapi for details.

I've fixed that

pali · 2017-03-20T12:11:45Z

dbdimp.c

+              ssl_mode = SSL_MODE_VERIFY_CA;
+            else if (ca_path && (strlen(ca_path) > 0))
+              ssl_mode = SSL_MODE_VERIFY_CA;
+	    mysql_options(sock, MYSQL_OPT_SSL_MODE, &ssl_mode);


Seems your patch is whitespace inconsistence with code around... See indentation. If it is possible try to fix it...

I've tried to fix it.

pali · 2017-03-20T12:14:41Z

dbdimp.c

+#ifdef MYSQL_SSL_MODE
+            if (ssl_verify_true)
+              ssl_mode = SSL_MODE_VERIFY_IDENTITY;
+            else if (ca_file && (strlen(ca_file) > 0))


Do we need check for strlen()? I have not find any information about it and variables are already passed to mysql_ssl_set() without strlen() check... So I'm not sure there...

Removed strlen.

dveeden · 2017-03-20T12:29:27Z

This code starts to be a big #if mess with long conditions after #if... I do not have idea how to improve it right now.

I'm afraid this will get worse over time because MySQL and MariaDB are (very) slowly diverging. We can try to prevent that as much as possible, but eventually it might make sense to split this into DBD::mysql and DBD::mariadb.

pali · 2017-03-20T13:17:42Z

I'm afraid this will get worse over time because MySQL and MariaDB are (very) slowly diverging.

Probably...

We can try to prevent that as much as possible, but eventually it might make sense to split this into DBD::mysql and DBD::mariadb.

But here we have another problem unrelated to MariaDB... Problem is with different MySQL versions. So splitting into DBD::mysql and DBD::mariadb does not help there. And there are still very large common codebase so I do not think split is good idea.

dveeden · 2017-03-20T13:45:32Z

If mysql_ssl=1 is not present in the dsn but something like mysql_ssl_ca_file is then the latter is ignored. I think we should generate a warning in that case. Not sure what the best way is to do that and I'd like to fix that in a separate pull request.

pali · 2017-04-02T21:28:09Z

I did some SSL changes here: #114 I tried to make that ifdef hell more readable and prepare ground for new MySQL 5.5.55 which should finally have fixed Riddle vulnerability.

dveeden · 2017-04-03T08:54:18Z

@pali should we wait on your ssl branch to be ready to merge? Or should this branch be merged?
I'd like to see something merged to fix the insecure code (not related to Riddle).

pali · 2017-04-03T10:57:02Z

For me this is PR is ready to merge, I already marked it.

pali · 2017-04-03T22:29:57Z

I updated my #114 on top of master (so including this PR).

pali · 2017-04-04T07:31:39Z

@dveeden this PR #108 broke compilation for MySQL C/Connector 6.0:
https://travis-ci.org/perl5-dbi/DBD-mysql/builds/218257554

dveeden force-pushed the mysql_ssl_mode_compat branch from c9995ea to c630ee8 Compare March 15, 2017 18:06

dveeden force-pushed the mysql_ssl_mode_compat branch from c630ee8 to 19ea86f Compare March 16, 2017 08:28

pali requested changes Mar 16, 2017

View reviewed changes

dveeden force-pushed the mysql_ssl_mode_compat branch 2 times, most recently from de57748 to eb88c1b Compare March 17, 2017 12:22

pali requested changes Mar 17, 2017

View reviewed changes

pali reviewed Mar 17, 2017

View reviewed changes

dveeden force-pushed the mysql_ssl_mode_compat branch from eb88c1b to 558959a Compare March 19, 2017 14:53

pali requested changes Mar 20, 2017

View reviewed changes

Set ssl_mode for Oracle MySQL if applicable.

3085995

dveeden force-pushed the mysql_ssl_mode_compat branch from 558959a to 3085995 Compare March 20, 2017 13:42

pali approved these changes Mar 20, 2017

View reviewed changes

pali mentioned this pull request Mar 23, 2017

Handle SSL/TLS correctly #110

Closed

mbeijen merged commit fa5e426 into perl5-dbi:master Apr 3, 2017

dveeden mentioned this pull request Apr 4, 2017

Fix build with Connector/C 6.0.x #116

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set ssl_mode for Oracle MySQL if applicable. #108

Set ssl_mode for Oracle MySQL if applicable. #108

dveeden commented Mar 15, 2017

pali commented Mar 15, 2017

pali commented Mar 15, 2017

dveeden commented Mar 16, 2017 •

edited

pali Mar 16, 2017

pali Mar 16, 2017

pali Mar 17, 2017

dveeden Mar 19, 2017

pali Mar 17, 2017

dveeden Mar 19, 2017

pali commented Mar 17, 2017

pali left a comment

pali Mar 20, 2017

dveeden Mar 20, 2017

pali Mar 20, 2017

dveeden Mar 20, 2017

pali Mar 20, 2017

dveeden Mar 20, 2017

dveeden commented Mar 20, 2017

pali commented Mar 20, 2017

dveeden commented Mar 20, 2017

pali commented Apr 2, 2017

dveeden commented Apr 3, 2017

pali commented Apr 3, 2017

pali commented Apr 3, 2017

pali commented Apr 4, 2017

Set ssl_mode for Oracle MySQL if applicable. #108

Set ssl_mode for Oracle MySQL if applicable. #108

Conversation

dveeden commented Mar 15, 2017

pali commented Mar 15, 2017

pali commented Mar 15, 2017

dveeden commented Mar 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pali commented Mar 17, 2017

pali left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dveeden commented Mar 20, 2017

pali commented Mar 20, 2017

dveeden commented Mar 20, 2017

pali commented Apr 2, 2017

dveeden commented Apr 3, 2017

pali commented Apr 3, 2017

pali commented Apr 3, 2017

pali commented Apr 4, 2017

dveeden commented Mar 16, 2017 •

edited