| Bug #17208 | SSL: client does not verify server certificate | ||
|---|---|---|---|
| Submitted: | 7 Feb 2006 23:16 | Modified: | 13 May 2006 7:05 |
| Reporter: | Jorj Bauer | ||
| Status: | Closed | ||
| Category: | Server | Severity: | S1 (Critical) |
| Version: | 5.0.18, 5.1-bk | OS: | |
| Assigned to: | Magnus Svensson | Target Version: | |
[7 Feb 2006 23:21]
Jorj Bauer
In case it's not clear, this is a showstopper. From a security standpoint is means SSL is unusable.
[8 Feb 2006 15:58]
Hartmut Holzgraefe
Could you please attache the patch as a file to this bug report? The word wrapping applied to the text field during transmission corrupted it :(
[9 Feb 2006 10:53]
Jorj Bauer
diff in original report
Attachment: ssl-client-cert-check-patch3.diff (text/x-patch), 6.61 KiB.
[9 Feb 2006 10:54]
Jorj Bauer
added patch as a file.
[18 Feb 2006 15:33]
Nick Drake
At least related, if not the cause: in libmysqld/sql_acl.cpp access is not denied if the
issuer and/or subject of the certificate mismatch.
Starting line 853 of said file, + indicates lines need to be added.
DBUG_PRINT("info",("checkpoint 2"));
/* If X509 issuer is specified, we check it... */
if (acl_user->x509_issuer)
{
DBUG_PRINT("info",("checkpoint 3"));
char *ptr = X509_NAME_oneline(X509_get_issuer_name(cert), 0, 0);
DBUG_PRINT("info",("comparing issuers: '%s' and '%s'",
acl_user->x509_issuer, ptr));
if (strcmp(acl_user->x509_issuer, ptr))
{
if (global_system_variables.log_warnings)
sql_print_information("X509 issuer mismatch: should be '%s' "
"but is '%s'", acl_user->x509_issuer, ptr);
free(ptr);
+ user_access=NO_ACCESS;
break;
}
user_access= acl_user->access;
free(ptr);
}
DBUG_PRINT("info",("checkpoint 4"));
/* X509 subject is specified, we check it .. */
if (acl_user->x509_subject)
{
char *ptr= X509_NAME_oneline(X509_get_subject_name(cert), 0, 0);
DBUG_PRINT("info",("comparing subjects: '%s' and '%s'",
acl_user->x509_subject, ptr));
if (strcmp(acl_user->x509_subject,ptr))
{
if (global_system_variables.log_warnings)
sql_print_information("X509 subject mismatch: '%s' vs '%s'",
acl_user->x509_subject, ptr);
+ user_access=NO_ACCESS;
}
else
user_access= acl_user->access;
free(ptr);
}
[3 Mar 2006 14:01]
Domas Mituzas
Verified at ChangeSet@1.2214, 2006-03-03 11:16:56+01:00 Thanks for submitting these patches, MITM prevention should really exist.
[10 Mar 2006 12:36]
Magnus Svensson
Investigating how the "client" could be made more secure.
[10 Mar 2006 12:38]
Magnus Svensson
Checked the code form "sql_acl.cc" above and it starts the function with setting "user_access= NO_ACCESS" as the default. And unless it find's any valid grant's NO_ACCESS is what the function will return. If you are experiencing any problem, please let me know how to repeat it.
[18 Apr 2006 17:59]
Bugs System
A patch for this bug has been committed. After review, it may be pushed to the relevant source trees for release in the next version. You can access the patch from: http://lists.mysql.com/commits/5091
[18 Apr 2006 18:48]
Chad MILLER
This now checks the Common Name in the cert against the name of the host we intended to connect to, which is good. I'm worried that we don't care about the authority of the certificate, as anyone can author a certificate to claim to be anything. I'd feel better if we also had a failing test for using a cert with the same "localhost" CN/hostname, where the cert is generated by a CA we do not trust.
[19 Apr 2006 16:42]
Chad MILLER
I see no problem with this patch, per se. Magnus is going to begin to get the other half of the security mechanism in place tomorrow.
[9 May 2006 11:41]
Magnus Svensson
The tests for connecting to server with invalid certs etc: http://lists.mysql.com/commits/5876
[10 May 2006 9:52]
Magnus Svensson
Pushed to 5.0.22
[12 May 2006 13:45]
Magnus Svensson
Pushed to 5.1.11
[12 May 2006 13:46]
Magnus Svensson
5.1 is now updated with the latest yaSSL patches.
[13 May 2006 7:05]
Paul DuBois
Noted in 5.0.22, 5.1.11 changelogs. Added the <option>--ssl-verify-server-cert</option> option to MySQL client programs. This option causes the server's Common Name value in its certificate to be verified against the hostname used when connecting to the server, and the connection is rejected if there is a mismatch. Added <literal>OPT_SSL_VERIFY_SERVER_CERT</literal> option for the <literal>mysql_options()</literal> C API function to enable this verification. This feature can be used to prevent man-in-the-middle attacks. Verification is disabled by default. Also updates SSL option section and description for mysql_options().
[13 May 2006 7:13]
Paul DuBois
Correction: The mysql_options() option name is MYSQL_OPT_SSL_VERIFY_SERVER_CERT.
Description: The SSL code (in viosslfactories.c) explicitly sets the certificate validation to NONE. This means that a client may be talking to a server other than the one intended (man in the middle attack). How to repeat: Use an invalid cert on the server. Client will still allow it. Suggested fix: The following patch corrects this behavior. The patch was generated against 4.1.12; the problem also exists in 5.0.18. diff --unified --recursive mysql-4.1.12/include/violite.h mysql-4.1.12.patched/include/violite.h --- mysql-4.1.12/include/violite.h 2005-05-13 07:32:21.000000000 -0400 +++ mysql-4.1.12.patched/include/violite.h 2006-02-01 12:36:16.000000000 -0500 @@ -23,7 +23,9 @@ #define vio_violite_h_ #include "my_net.h" /* needed because of struct in_addr */ - +#include "mysql.h" /* needed to get options, and hostname, + down to the SSL layer for cert + validation -- jorj@isc.upenn.edu */ /* Simple vio interface in C; The functions are implemented in violite.c */ @@ -109,13 +111,15 @@ SSL_CTX *ssl_context; /* function pointers which are only once for SSL client */ SSL_METHOD *ssl_method; + MYSQL *mysql; /* so we can get the hostname later -- jorj@isc.upenn.edu */ }; int sslaccept(struct st_VioSSLAcceptorFd*, Vio *, long timeout); int sslconnect(struct st_VioSSLConnectorFd*, Vio *, long timeout); struct st_VioSSLConnectorFd -*new_VioSSLConnectorFd(const char *key_file, const char *cert_file, +*new_VioSSLConnectorFd(MYSQL *mysql, + const char *key_file, const char *cert_file, const char *ca_file, const char *ca_path, const char *cipher); struct st_VioSSLAcceptorFd diff --unified --recursive mysql-4.1.12/sql-common/client.c mysql-4.1.12.patched/sql-common/client.c --- mysql-4.1.12/sql-common/client.c 2005-05-13 07:32:04.000000000 -0400 +++ mysql-4.1.12.patched/sql-common/client.c 2006-02-01 12:36:16.000000000 -0500 @@ -1991,7 +1991,8 @@ } /* Do the SSL layering. */ if (!(mysql->connector_fd= - (gptr) new_VioSSLConnectorFd(options->ssl_key, + (gptr) new_VioSSLConnectorFd(mysql, + options->ssl_key, options->ssl_cert, options->ssl_ca, options->ssl_capath, diff --unified --recursive mysql-4.1.12/vio/test-ssl.c mysql-4.1.12.patched/vio/test-ssl.c --- mysql-4.1.12/vio/test-ssl.c 2005-05-13 07:32:49.000000000 -0400 +++ mysql-4.1.12.patched/vio/test-ssl.c 2006-02-01 12:36:16.000000000 -0500 @@ -91,7 +91,8 @@ ssl_acceptor = new_VioSSLAcceptorFd(server_key, server_cert, ca_file, ca_path, cipher); - ssl_connector = new_VioSSLConnectorFd(client_key, client_cert, ca_file, + ssl_connector = new_VioSSLConnectorFd(NULL, + client_key, client_cert, ca_file, ca_path, cipher); client_vio = (struct st_vio*)my_malloc(sizeof(struct st_vio),MYF(0)); diff --unified --recursive mysql-4.1.12/vio/test-sslclient.c mysql-4.1.12.patched/vio/test-sslclient.c --- mysql-4.1.12/vio/test-sslclient.c 2005-05-13 07:32:27.000000000 -0400 +++ mysql-4.1.12.patched/vio/test-sslclient.c 2006-02-01 12:36:16.000000000 -0500 @@ -61,7 +61,7 @@ if (ca_path!=0) printf("CApath : %s\n", ca_path); - ssl_connector = new_VioSSLConnectorFd(client_key, client_cert, ca_file, ca_path, cipher); + ssl_connector = new_VioSSLConnectorFd(NULL, client_key, client_cert, ca_file, ca_path, cipher); if(!ssl_connector) { fatal_error("client:new_VioSSLConnectorFd failed"); } diff --unified --recursive mysql-4.1.12/vio/viossl.c mysql-4.1.12.patched/vio/viossl.c --- mysql-4.1.12/vio/viossl.c 2005-05-13 07:32:44.000000000 -0400 +++ mysql-4.1.12.patched/vio/viossl.c 2006-02-01 12:36:38.000000000 -0500 @@ -1,3 +1,4 @@ +#include <netdb.h> /* Copyright (C) 2000 MySQL AB This program is free software; you can redistribute it and/or modify @@ -382,10 +383,12 @@ vio_blocking(vio, net_blocking, &unused); DBUG_RETURN(1); } + + server_cert = SSL_get_peer_certificate ((SSL*) vio->ssl_arg); + #ifndef DBUG_OFF DBUG_PRINT("info",("SSL_get_cipher_name() = '%s'" ,SSL_get_cipher_name((SSL*) vio->ssl_arg))); - server_cert = SSL_get_peer_certificate ((SSL*) vio->ssl_arg); if (server_cert != NULL) { DBUG_PRINT("info",("Server certificate:")); @@ -401,12 +404,72 @@ We could do all sorts of certificate verification stuff here before deallocating the certificate. */ - X509_free (server_cert); } else DBUG_PRINT("info",("Server does not have certificate.")); #endif + + if (server_cert == NULL) + goto failed; + + if (SSL_get_verify_result(vio->ssl_arg) != X509_V_OK) + goto failed; + + // If we don't have a mysql struct, then we can't continue verifying. We'll + // assume everything is okay (generally a bad assumption). + if (ptr->mysql == NULL) + goto success; + + + // We already know that the certificate exchanged was valid; OpenSSL + // handled that. Now we need to verify that the contents of the certificate + // are what we expect. + + { + char *cp1, *cp2; + char inbuf[256]; + + // If we can't look up the server's hostname, or we can't verify that + // hostname against the cert, then we've failed the cert check. + + // We have to dig the hostname out of the options... + if (! ptr->mysql->host) { + DBUG_PRINT("error", ("SSL: no hostname, can't verify cert")); + goto failed; + } + + strncpy(inbuf, + X509_NAME_oneline(X509_get_subject_name(server_cert), NULL, 0), + sizeof(inbuf)); + cp1 = strstr(inbuf, "/CN="); + if (cp1 == NULL) { + DBUG_PRINT("error", ("SSL: can't find cert CN")); + goto failed; + } + + cp1 += 4; // Skip the "/CN=" that we found + cp2 = strchr(cp1, '/'); + if (cp2 != NULL) *cp2 = '\0'; + if (strcmp(cp1, ptr->mysql->host)) { + DBUG_PRINT("error", ("SSL: cert name '%s' doesn't match name '%s'", + cp1, ptr->mysql->host)); + goto failed; + } + } + + success: + X509_free (server_cert); DBUG_RETURN(0); + + failed: + DBUG_PRINT("error", ("SSL certificate validation failure")); + report_errors(); + SSL_free((SSL*) vio->ssl_arg); + vio->ssl_arg = 0; + vio_reset(vio, old_type,vio->sd,0,FALSE); + vio_blocking(vio, net_blocking, &unused); + X509_free (server_cert); + DBUG_RETURN(1); } diff --unified --recursive mysql-4.1.12/vio/viosslfactories.c mysql-4.1.12.patched/vio/viosslfactories.c --- mysql-4.1.12/vio/viosslfactories.c 2005-05-13 07:32:30.000000000 -0400 +++ mysql-4.1.12.patched/vio/viosslfactories.c 2006-02-03 12:13:26.000000000 -0500 @@ -208,13 +208,15 @@ */ struct st_VioSSLConnectorFd * -new_VioSSLConnectorFd(const char* key_file, +new_VioSSLConnectorFd(MYSQL *mysql, + const char* key_file, const char* cert_file, const char* ca_file, const char* ca_path, const char* cipher) { - int verify = SSL_VERIFY_NONE; + int verify = SSL_VERIFY_PEER; +; struct st_VioSSLConnectorFd* ptr; int result; DH *dh; @@ -229,6 +231,7 @@ ptr->ssl_context= 0; ptr->ssl_method= 0; + ptr->mysql = mysql; /* FIXME: constants! */ #ifdef __NETWARE__