From 968b5f0aa2d22d0bcd6c7deb6f422497e61c3881 Mon Sep 17 00:00:00 2001 From: Georg Richter Date: Tue, 24 Sep 2024 12:08:42 +0200 Subject: [PATCH] Fix for CONC-731: wrong error message (incorrect fp) - moved fingerprint verification to ma_tls.c - don't verify cert again if fingerprint check succeeded. - Disable self signed check in fingerprint tests (Schannel only). --- libmariadb/ma_tls.c | 28 ++++++++++- libmariadb/secure/gnutls.c | 9 ---- libmariadb/secure/openssl.c | 14 +----- libmariadb/secure/schannel.c | 9 ---- plugins/auth/my_auth.c | 4 -- unittest/libmariadb/CMakeLists.txt | 9 ++++ unittest/libmariadb/tls.c.in | 81 ++++++++++++++++++++++++++++++ 7 files changed, 118 insertions(+), 36 deletions(-) diff --git a/libmariadb/ma_tls.c b/libmariadb/ma_tls.c index f854b37b..79d373e6 100644 --- a/libmariadb/ma_tls.c +++ b/libmariadb/ma_tls.c @@ -118,9 +118,33 @@ int ma_pvio_tls_verify_server_cert(MARIADB_TLS *ctls, unsigned int flags) getenv("MARIADB_TLS_DISABLE_PEER_VERIFICATION")) && (!mysql->options.extension->tls_fp && !mysql->options.extension->tls_fp_list)) { + /* Since OpenSSL implementation sets status during TLS handshake + we need to clear verification status */ + mysql->net.tls_verify_status= 0; return 0; } + if (flags & MARIADB_TLS_VERIFY_FINGERPRINT) + { + if (ma_pvio_tls_check_fp(ctls, mysql->options.extension->tls_fp, mysql->options.extension->tls_fp_list)) + { + mysql->net.tls_verify_status|= MARIADB_TLS_VERIFY_FINGERPRINT; + mysql->extension->tls_validation= mysql->net.tls_verify_status; + my_set_error(mysql, CR_SSL_CONNECTION_ERROR, SQLSTATE_UNKNOWN, + ER(CR_SSL_CONNECTION_ERROR), + "Fingerprint validation of peer certificate failed"); + return 1; + } +#ifdef HAVE_OPENSSL + /* verification already happened via callback */ + if (!(mysql->net.tls_verify_status & flags)) + { + mysql->extension->tls_validation= mysql->net.tls_verify_status; + mysql->net.tls_verify_status= MARIADB_TLS_VERIFY_OK; + return 0; + } +#endif + } rc= ma_tls_verify_server_cert(ctls, flags); /* Set error messages */ @@ -151,7 +175,9 @@ int ma_pvio_tls_verify_server_cert(MARIADB_TLS *ctls, unsigned int flags) ER(CR_SSL_CONNECTION_ERROR), "Peer certificate is not trusted"); } - + /* Save original validation, since we might unset trust flag in + my_auth */ + mysql->extension->tls_validation= mysql->net.tls_verify_status; return rc; } diff --git a/libmariadb/secure/gnutls.c b/libmariadb/secure/gnutls.c index 97d505d0..9bce65d3 100644 --- a/libmariadb/secure/gnutls.c +++ b/libmariadb/secure/gnutls.c @@ -1445,15 +1445,6 @@ int ma_tls_verify_server_cert(MARIADB_TLS *ctls, unsigned int flags) CLEAR_CLIENT_ERROR(mysql); - if (flags & MARIADB_TLS_VERIFY_FINGERPRINT) - { - if (ma_pvio_tls_check_fp(ctls, mysql->options.extension->tls_fp, mysql->options.extension->tls_fp_list)) - { - mysql->net.tls_verify_status= MARIADB_TLS_VERIFY_FINGERPRINT; - goto end; - } - } - if (gnutls_certificate_verify_peers2(ssl, &status)) return GNUTLS_E_CERTIFICATE_ERROR; diff --git a/libmariadb/secure/openssl.c b/libmariadb/secure/openssl.c index 1e8b6cc6..7ce55a02 100644 --- a/libmariadb/secure/openssl.c +++ b/libmariadb/secure/openssl.c @@ -786,19 +786,7 @@ int ma_tls_verify_server_cert(MARIADB_TLS *ctls, unsigned int verify_flags) if ((mysql->net.tls_verify_status > MARIADB_TLS_VERIFY_FINGERPRINT) || (mysql->net.tls_verify_status & verify_flags)) { - return 1; - } - - if (verify_flags & MARIADB_TLS_VERIFY_FINGERPRINT) - { - if (ma_pvio_tls_check_fp(ctls, mysql->options.extension->tls_fp, mysql->options.extension->tls_fp_list)) - { - mysql->net.tls_verify_status |= MARIADB_TLS_VERIFY_FINGERPRINT; - return 1; - } - - mysql->net.tls_verify_status= MARIADB_TLS_VERIFY_OK; - return 0; + return MARIADB_TLS_VERIFY_ERROR; } if (verify_flags & MARIADB_TLS_VERIFY_HOST) diff --git a/libmariadb/secure/schannel.c b/libmariadb/secure/schannel.c index 098c0e4b..47763c47 100644 --- a/libmariadb/secure/schannel.c +++ b/libmariadb/secure/schannel.c @@ -719,15 +719,6 @@ int ma_tls_verify_server_cert(MARIADB_TLS *ctls, unsigned int verify_flags) return 1; } } - - if (verify_flags & MARIADB_TLS_VERIFY_FINGERPRINT) - { - if (ma_pvio_tls_check_fp(ctls, mysql->options.extension->tls_fp, mysql->options.extension->tls_fp_list)) - { - mysql->net.tls_verify_status |= MARIADB_TLS_VERIFY_FINGERPRINT; - return 1; - } - } return ma_schannel_verify_certs(ctls, verify_flags); } diff --git a/plugins/auth/my_auth.c b/plugins/auth/my_auth.c index 3f466b17..37b60068 100644 --- a/plugins/auth/my_auth.c +++ b/plugins/auth/my_auth.c @@ -435,8 +435,6 @@ static int send_client_reply_packet(MCPVIO_EXT *mpvio, if (mysql->options.extension->tls_verification_callback(mysql->net.pvio->ctls, verify_flags)) { - /* Save original verification result */ - mysql->extension->tls_validation= mysql->net.tls_verify_status; if (mysql->net.tls_verify_status > MARIADB_TLS_VERIFY_TRUST || (mysql->options.ssl_ca || mysql->options.ssl_capath)) goto error; @@ -449,8 +447,6 @@ static int send_client_reply_packet(MCPVIO_EXT *mpvio, else if (!password_and_hashing(mysql, mpvio->plugin)) goto error; } - else - mysql->extension->tls_validation= mysql->net.tls_verify_status; } #endif /* HAVE_TLS */ diff --git a/unittest/libmariadb/CMakeLists.txt b/unittest/libmariadb/CMakeLists.txt index 5442fb5c..c592c781 100644 --- a/unittest/libmariadb/CMakeLists.txt +++ b/unittest/libmariadb/CMakeLists.txt @@ -60,6 +60,15 @@ ENDIF() ADD_LIBRARY(ma_getopt ma_getopt.c) IF(${Python3_FOUND}) + execute_process(COMMAND ${Python3_EXECUTABLE} -m pip show pyopenssl + RESULT_VARIABLE EXIT_CODE + OUTPUT_QUIET) + if (${EXIT_CODE} EQUAL 0) + SET(RUN_TLS 1) + endif() +ENDIF() + +IF(${RUN_TLS}) CONFIGURE_FILE(${CC_SOURCE_DIR}/unittest/libmariadb/tls.c.in ${CC_BINARY_DIR}/unittest/libmariadb/tls.c) diff --git a/unittest/libmariadb/tls.c.in b/unittest/libmariadb/tls.c.in index 220b2979..3ed4d309 100644 --- a/unittest/libmariadb/tls.c.in +++ b/unittest/libmariadb/tls.c.in @@ -672,7 +672,9 @@ static int test_cert_wildcard(MYSQL *my __attribute((unused))) if (!my_test_connect(mysql, tls_dummy_host, "tlsuser", "foo", NULL, tls_dummy_port, NULL, 0, 0)) { CHECK_NO_TLS_FLAG(mysql, MARIADB_TLS_VERIFY_HOST, "Hostname verification didn't pass"); +#ifndef HAVE_SCHANNEL CHECK_TLS_FLAGS(mysql, MARIADB_TLS_VERIFY_TRUST, "Self signed certificate expected"); +#endif mysql_close(mysql); } else { mysql_close(mysql); @@ -699,12 +701,91 @@ static int test_cert_wildcard(MYSQL *my __attribute((unused))) return OK; } +static int test_env_var(MYSQL *my __attribute__((unused))) +{ + MYSQL *mysql= mysql_init(NULL); + int rc= FAIL; + unsigned int status; + +#ifdef _WIN32 + _putenv_s("MARIADB_TLS_DISABLE_PEER_VERIFICATION", "1"); +#else + setenv("MARIADB_TLS_DISABLE_PEER_VERIFICATION", "1", 1); +#endif + + if (!my_test_connect(mysql, hostname, username, password, schema, + port, socketname, 0, 0)) + { + diag("expected to pass, since environment variable was set"); + goto end; + } + + mariadb_get_infov(mysql, MARIADB_TLS_VERIFY_STATUS, &status); + + if (status) + { + diag("expected status=0, since environment variable was set"); + goto end; + } + + rc= OK; + +end: +#ifdef _WIN32 + _putenv_s("MARIADB_TLS_DISABLE_PEER_VERIFICATION", ""); +#else + unsetenv("MARIADB_TLS_DISABLE_PEER_VERIFICATION"); +#endif + mysql_close(mysql); + return rc; +} + +static int test_fp_and_verify(MYSQL *my __attribute__((unused))) +{ + MYSQL *mysql= mysql_init(NULL); + int rc= FAIL; +#ifndef HAVE_SCHANNEL + unsigned int status; +#endif + my_bool verify= 1; + + mysql_options(mysql, MARIADB_OPT_SSL_FP, fingerprint); + mysql_options(mysql, MYSQL_OPT_SSL_VERIFY_SERVER_CERT, &verify); + + if (!my_test_connect(mysql, hostname, username, password, schema, + port, socketname, 0, 0)) + { + diag("expected to pass, since fingerprint was specified"); + diag("error: %s", mysql_error(mysql)); + goto end; + } + +/* Schannel aborts on first error, if fingerprint was specified, + MARIADB_TLS_VERIFY_TRUST is unset */ +#ifndef HAVE_SCHANNEL + mariadb_get_infov(mysql, MARIADB_TLS_VERIFY_STATUS, &status); + + if (!status) + { + diag("expected status flag set (self signed)"); + goto end; + } +#endif + rc= OK; + +end: + mysql_close(mysql); + return rc; +} + struct my_tests_st my_tests[] = { /* Don't add test above, test_init needs to be run first */ {"test_start_tls_server", test_start_tls_server, TEST_CONNECTION_NONE, 0, NULL, NULL}, {"test_init", test_init, TEST_CONNECTION_NONE, 0, NULL, NULL}, /* Here you can add more tests */ + {"test_fp_and_verify", test_fp_and_verify, TEST_CONNECTION_NEW, 0, NULL, NULL}, + {"test_env_var", test_env_var, TEST_CONNECTION_NEW, 0, NULL, NULL}, {"test_cert_wildcard", test_cert_wildcard, TEST_CONNECTION_NEW, 0, NULL, NULL}, {"test_cert_expired", test_cert_expired, TEST_CONNECTION_NEW, 0, NULL, NULL}, {"test_pw_check", test_pw_check, TEST_CONNECTION_NEW, 0, NULL, NULL},