From b30f10c681ec87720cff85d490f67098568a9cba Mon Sep 17 00:00:00 2001 From: Pierre Ossman Date: Thu, 21 May 2020 21:10:38 +0200 Subject: [PATCH] Properly store certificate exceptions The previous method stored the certificates as authorities, meaning that the owner of that certificate could impersonate any server it wanted after a client had added an exception. Handle this more properly by only storing exceptions for specific hostname/certificate combinations, the same way browsers or SSH does things. --- common/rfb/CSecurityTLS.cxx | 163 ++++++++++++++++++++------------------------ 1 file changed, 73 insertions(+), 90 deletions(-) diff --git a/common/rfb/CSecurityTLS.cxx b/common/rfb/CSecurityTLS.cxx index 5c303a37..99008378 100644 --- a/common/rfb/CSecurityTLS.cxx +++ b/common/rfb/CSecurityTLS.cxx @@ -250,22 +250,6 @@ void CSecurityTLS::setParam() if (*cafile && gnutls_certificate_set_x509_trust_file(cert_cred,cafile,GNUTLS_X509_FMT_PEM) < 0) throw AuthFailureException("load of CA cert failed"); - /* Load previously saved certs */ - char *homeDir = NULL; - int err; - if (getvnchomedir(&homeDir) == -1) - vlog.error("Could not obtain VNC home directory path"); - else { - CharArray caSave(strlen(homeDir) + 19 + 1); - sprintf(caSave.buf, "%sx509_savedcerts.pem", homeDir); - delete [] homeDir; - - err = gnutls_certificate_set_x509_trust_file(cert_cred, caSave.buf, - GNUTLS_X509_FMT_PEM); - if (err < 0) - vlog.debug("Failed to load saved server certificates from %s", caSave.buf); - } - if (*crlfile && gnutls_certificate_set_x509_crl_file(cert_cred,crlfile,GNUTLS_X509_FMT_PEM) < 0) throw AuthFailureException("load of CRL failed"); @@ -290,7 +274,10 @@ void CSecurityTLS::checkSession() const gnutls_datum_t *cert_list; unsigned int cert_list_size = 0; int err; + + char *homeDir; gnutls_datum_t info; + size_t len; if (anon) return; @@ -333,13 +320,13 @@ void CSecurityTLS::checkSession() throw AuthFailureException("decoding of certificate failed"); if (gnutls_x509_crt_check_hostname(crt, client->getServerName()) == 0) { - char buf[255]; + CharArray text; vlog.debug("hostname mismatch"); - snprintf(buf, sizeof(buf), "Hostname (%s) does not match any certificate, " - "do you want to continue?", client->getServerName()); - buf[sizeof(buf) - 1] = '\0'; - if (!msg->showMsgBox(UserMsgBox::M_YESNO, "hostname mismatch", buf)) - throw AuthFailureException("hostname mismatch"); + text.format("Hostname (%s) does not match the server certificate, " + "do you want to continue?", client->getServerName()); + if (!msg->showMsgBox(UserMsgBox::M_YESNO, + "Certificate hostname mismatch", text.buf)) + throw AuthFailureException("Certificate hostname mismatch"); } if (status == 0) { @@ -364,86 +351,82 @@ void CSecurityTLS::checkSession() throw AuthFailureException("Invalid status of server certificate verification"); } - vlog.debug("Saved server certificates don't match"); + /* Certificate is fine, except we don't know the issuer, so TOFU time */ - if (gnutls_x509_crt_print(crt, GNUTLS_CRT_PRINT_ONELINE, &info)) { - /* - * GNUTLS doesn't correctly export gnutls_free symbol which is - * a function pointer. Linking with Visual Studio 2008 Express will - * fail when you call gnutls_free(). - */ -#if WIN32 - free(info.data); -#else - gnutls_free(info.data); -#endif - throw AuthFailureException("Could not find certificate to display"); + homeDir = NULL; + if (getvnchomedir(&homeDir) == -1) { + throw AuthFailureException("Could not obtain VNC home directory " + "path for known hosts storage"); } - size_t out_size = 0; - char *out_buf = NULL; - char *certinfo = NULL; - int len = 0; - - vlog.debug("certificate issuer unknown"); - - len = snprintf(NULL, 0, "This certificate has been signed by an unknown " - "authority:\n\n%s\n\nDo you want to save it and " - "continue?\n ", info.data); - if (len < 0) - throw AuthFailureException("certificate decoding error"); - - vlog.debug("%s", info.data); - - certinfo = new char[len]; - - snprintf(certinfo, len, "This certificate has been signed by an unknown " - "authority:\n\n%s\n\nDo you want to save it and " - "continue? ", info.data); + CharArray dbPath(strlen(homeDir) + 16 + 1); + sprintf(dbPath.buf, "%sx509_known_hosts", homeDir); + delete [] homeDir; - for (int i = 0; i < len - 1; i++) - if (certinfo[i] == ',' && certinfo[i + 1] == ' ') - certinfo[i] = '\n'; + err = gnutls_verify_stored_pubkey(dbPath.buf, NULL, + client->getServerName(), NULL, + GNUTLS_CRT_X509, &cert_list[0], 0); - if (!msg->showMsgBox(UserMsgBox::M_YESNO, "certificate issuer unknown", - certinfo)) { - delete [] certinfo; - throw AuthFailureException("certificate issuer unknown"); + /* Previously known? */ + if (err == GNUTLS_E_SUCCESS) { + vlog.debug("Server certificate found in known hosts file"); + gnutls_x509_crt_deinit(crt); + return; } - delete [] certinfo; - - if (gnutls_x509_crt_export(crt, GNUTLS_X509_FMT_PEM, NULL, &out_size) - != GNUTLS_E_SHORT_MEMORY_BUFFER) - throw AuthFailureException("certificate issuer unknown, and certificate " - "export failed"); + if ((err != GNUTLS_E_NO_CERTIFICATE_FOUND) && + (err != GNUTLS_E_CERTIFICATE_KEY_MISMATCH)) { + throw AuthFailureException("Could not load known hosts database"); + } - // Save cert - out_buf = new char[out_size]; + if (gnutls_x509_crt_print(crt, GNUTLS_CRT_PRINT_ONELINE, &info)) + throw AuthFailureException("Could not find certificate to display"); - if (gnutls_x509_crt_export(crt, GNUTLS_X509_FMT_PEM, out_buf, &out_size) < 0) - throw AuthFailureException("certificate issuer unknown, and certificate " - "export failed"); + len = strlen((char*)info.data); + for (size_t i = 0; i < len - 1; i++) { + if (info.data[i] == ',' && info.data[i + 1] == ' ') + info.data[i] = '\n'; + } - char *homeDir = NULL; - if (getvnchomedir(&homeDir) == -1) - vlog.error("Could not obtain VNC home directory path"); - else { - FILE *f; - CharArray caSave(strlen(homeDir) + 1 + 19); - sprintf(caSave.buf, "%sx509_savedcerts.pem", homeDir); - delete [] homeDir; - f = fopen(caSave.buf, "a+"); - if (!f) - msg->showMsgBox(UserMsgBox::M_OK, "certificate save failed", - "Could not save the certificate"); - else { - fprintf(f, "%s\n", out_buf); - fclose(f); - } + /* New host */ + if (err == GNUTLS_E_NO_CERTIFICATE_FOUND) { + CharArray text; + + vlog.debug("Server host not previously known"); + vlog.debug("%s", info.data); + + text.format("This certificate has been signed by an unknown " + "authority:\n\n%s\n\nSomeone could be trying to " + "impersonate the site and you should not " + "continue.\n\nDo you want to make an exception " + "for this server?", info.data); + + if (!msg->showMsgBox(UserMsgBox::M_YESNO, + "Unknown certificate issuer", + text.buf)) + throw AuthFailureException("Unknown certificate issuer"); + } else if (err == GNUTLS_E_CERTIFICATE_KEY_MISMATCH) { + CharArray text; + + vlog.debug("Server host key mismatch"); + vlog.debug("%s", info.data); + + text.format("This host is previously known with a different " + "certificate, and the new certificate has been " + "signed by an unknown authority:\n\n%s\n\nSomeone " + "could be trying to impersonate the site and you " + "should not continue.\n\nDo you want to make an " + "exception for this server?", info.data); + + if (!msg->showMsgBox(UserMsgBox::M_YESNO, + "Unexpected server certificate", + text.buf)) + throw AuthFailureException("Unexpected server certificate"); } - delete [] out_buf; + if (gnutls_store_pubkey(dbPath.buf, NULL, client->getServerName(), + NULL, GNUTLS_CRT_X509, &cert_list[0], 0, 0)) + vlog.error("Failed to store server certificate to known hosts database"); gnutls_x509_crt_deinit(crt); /* -- 2.16.4