Description: Properly store certificate exceptions in native and java VNC viewer. They stored the certificates as authorities, meaning that the owner of a certificate could impersonate any server after a client had added an exception. Author: Pierre Ossman and Brian P. Hinz Abstract: Properly store certificate exceptions . . git commit b30f10c681ec87720cff85d490f67098568a9cba . 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. . . git commit f029745f63ac7d22fb91639b2cb5b3ab56134d6e . Like the native viewer, the Java viewer didn't store certificate exceptions properly. Whilst not as bad as the native viewer, it still failed to check that a stored certificate wouldn't be maliciously used for another server. In practice this can in most cases be used to impersonate another server. . Handle this like the native viewer by storing exceptions for a specific hostname/certificate combination. . This issue is CVE-2020-26117. Index: pkg-tigervnc/common/rfb/CSecurityTLS.cxx =================================================================== --- pkg-tigervnc.orig/common/rfb/CSecurityTLS.cxx +++ pkg-tigervnc/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) { @@ -366,87 +353,80 @@ void CSecurityTLS::checkSession() vlog.debug("Saved server certificates don't match"); - 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"); + } + + CharArray dbPath(strlen(homeDir) + 16 + 1); + sprintf(dbPath.buf, "%sx509_known_hosts", homeDir); + delete [] homeDir; + + err = gnutls_verify_stored_pubkey(dbPath.buf, NULL, + client->getServerName(), NULL, + GNUTLS_CRT_X509, &cert_list[0], 0); + + /* Previously known? */ + if (err == GNUTLS_E_SUCCESS) { + vlog.debug("Server certificate found in known hosts file"); + gnutls_x509_crt_deinit(crt); + return; } - 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]; - if (certinfo == NULL) - throw AuthFailureException("Out of memory"); - - 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); - - for (int i = 0; i < len - 1; i++) - if (certinfo[i] == ',' && certinfo[i + 1] == ' ') - certinfo[i] = '\n'; - - if (!msg->showMsgBox(UserMsgBox::M_YESNO, "certificate issuer unknown", - certinfo)) { - delete [] certinfo; - throw AuthFailureException("certificate issuer unknown"); - } - - delete [] certinfo; - - if (gnutls_x509_crt_export(crt, GNUTLS_X509_FMT_PEM, NULL, &out_size) - == GNUTLS_E_SHORT_MEMORY_BUFFER) - throw AuthFailureException("Out of memory"); - - // Save cert - out_buf = new char[out_size]; - if (out_buf == NULL) - throw AuthFailureException("Out of memory"); - - if (gnutls_x509_crt_export(crt, GNUTLS_X509_FMT_PEM, out_buf, &out_size) < 0) - throw AuthFailureException("certificate issuer unknown, and certificate " - "export failed"); - - 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); - } + if ((err != GNUTLS_E_NO_CERTIFICATE_FOUND) && + (err != GNUTLS_E_CERTIFICATE_KEY_MISMATCH)) { + throw AuthFailureException("Could not load known hosts database"); } - delete [] out_buf; + if (gnutls_x509_crt_print(crt, GNUTLS_CRT_PRINT_ONELINE, &info)) + throw AuthFailureException("Could not find certificate to display"); + + 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'; + } + + /* 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"); + } + + 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); /* Index: pkg-tigervnc/java/com/tigervnc/rfb/CSecurityTLS.java =================================================================== --- pkg-tigervnc.orig/java/com/tigervnc/rfb/CSecurityTLS.java +++ pkg-tigervnc/java/com/tigervnc/rfb/CSecurityTLS.java @@ -107,12 +107,6 @@ public class CSecurityTLS extends CSecur X509CRL.setDefaultStr(getDefaultCRL()); } -// FIXME: -// Need to shutdown the connection cleanly - -// FIXME? -// add a finalizer method that calls shutdown - public boolean processMsg(CConnection cc) { is = (FdInStream)cc.getInStream(); os = (FdOutStream)cc.getOutStream(); @@ -269,8 +263,13 @@ public class CSecurityTLS extends CSecur { Collection certs = null; X509Certificate cert = chain[0]; + String pk = + Base64.getEncoder().encodeToString(cert.getPublicKey().getEncoded()); try { cert.checkValidity(); + verifyHostname(cert); + } catch(CertificateParsingException e) { + throw new SystemException(e.getMessage()); } catch(CertificateNotYetValidException e) { throw new AuthFailureException("server certificate has not been activated"); } catch(CertificateExpiredException e) { @@ -279,73 +278,111 @@ public class CSecurityTLS extends CSecur "do you want to continue?")) throw new AuthFailureException("server certificate has expired"); } - String thumbprint = getThumbprint(cert); File vncDir = new File(FileUtils.getVncHomeDir()); - File certFile = new File(vncDir, "x509_savedcerts.pem"); - CertificateFactory cf = CertificateFactory.getInstance("X.509"); - if (vncDir.exists() && certFile.exists() && certFile.canRead()) { - InputStream certStream = new MyFileInputStream(certFile); - certs = cf.generateCertificates(certStream); - for (Certificate c : certs) - if (thumbprint.equals(getThumbprint((X509Certificate)c))) - return; - } + if (!vncDir.exists()) + throw new AuthFailureException("Could not obtain VNC home directory "+ + "path for known hosts storage"); + File dbPath = new File(vncDir, "x509_known_hosts"); + String info = + " Subject: "+cert.getSubjectX500Principal().getName()+"\n"+ + " Issuer: "+cert.getIssuerX500Principal().getName()+"\n"+ + " Serial Number: "+cert.getSerialNumber()+"\n"+ + " Version: "+cert.getVersion()+"\n"+ + " Signature Algorithm: "+cert.getPublicKey().getAlgorithm()+"\n"+ + " Not Valid Before: "+cert.getNotBefore()+"\n"+ + " Not Valid After: "+cert.getNotAfter()+"\n"+ + " SHA-1 Fingerprint: "+getThumbprint(cert)+"\n"; try { - verifyHostname(cert); + if (dbPath.exists()) { + FileReader db = new FileReader(dbPath); + BufferedReader dbBuf = new BufferedReader(db); + String line; + String server = client.getServerName().toLowerCase(); + while ((line = dbBuf.readLine())!=null) { + String fields[] = line.split("\\|"); + if (fields.length==6) { + if (server.equals(fields[2]) && pk.equals(fields[5])) { + vlog.debug("Server certificate found in known hosts file"); + dbBuf.close(); + return; + } else if (server.equals(fields[2]) && !pk.equals(fields[5]) || + !server.equals(fields[2]) && pk.equals(fields[5])) { + throw new CertStoreException(); + } + } + } + dbBuf.close(); + } tm.checkServerTrusted(chain, authType); + } catch (IOException e) { + throw new AuthFailureException("Could not load known hosts database"); + } catch (CertStoreException e) { + vlog.debug("Server host key mismatch"); + vlog.debug(info); + String text = + "This host is previously known with a different "+ + "certificate, and the new certificate has been "+ + "signed by an unknown authority\n"+ + "\n"+info+"\n"+ + "Someone could be trying to impersonate the site and you should not continue.\n"+ + "\n"+ + "Do you want to make an exception for this server?"; + if (!msg.showMsgBox(YES_NO_OPTION, "Unexpected certificate issuer", text)) + throw new AuthFailureException("Unexpected certificate issuer"); + store_pubkey(dbPath, client.getServerName().toLowerCase(), pk); } catch (java.lang.Exception e) { if (e.getCause() instanceof CertPathBuilderException) { - String certinfo = + vlog.debug("Server host not previously known"); + vlog.debug(info); + String text = "This certificate has been signed by an unknown authority\n"+ + "\n"+info+"\n"+ + "Someone could be trying to impersonate the site and you should not continue.\n"+ "\n"+ - " Subject: "+cert.getSubjectX500Principal().getName()+"\n"+ - " Issuer: "+cert.getIssuerX500Principal().getName()+"\n"+ - " Serial Number: "+cert.getSerialNumber()+"\n"+ - " Version: "+cert.getVersion()+"\n"+ - " Signature Algorithm: "+cert.getPublicKey().getAlgorithm()+"\n"+ - " Not Valid Before: "+cert.getNotBefore()+"\n"+ - " Not Valid After: "+cert.getNotAfter()+"\n"+ - " SHA1 Fingerprint: "+getThumbprint(cert)+"\n"+ - "\n"+ - "Do you want to save it and continue?"; - if (!msg.showMsgBox(YES_NO_OPTION, "certificate issuer unknown", - certinfo)) { - throw new AuthFailureException("certificate issuer unknown"); - } - if (certs == null || !certs.contains(cert)) { - byte[] der = cert.getEncoded(); - String pem = Base64.getEncoder().encodeToString(der); - pem = pem.replaceAll("(.{64})", "$1\n"); - FileWriter fw = null; - try { - if (!vncDir.exists()) - vncDir.mkdir(); - if (!certFile.exists() && !certFile.createNewFile()) { - vlog.error("Certificate save failed."); - } else { - fw = new FileWriter(certFile.getAbsolutePath(), true); - fw.write("-----BEGIN CERTIFICATE-----\n"); - fw.write(pem+"\n"); - fw.write("-----END CERTIFICATE-----\n"); - } - } catch (IOException ioe) { - msg.showMsgBox(OK_OPTION, "certificate save failed", - "Could not save the certificate"); - } finally { - try { - if (fw != null) - fw.close(); - } catch(IOException ioe2) { - throw new Exception(ioe2.getMessage()); - } - } - } + "Do you want to make an exception for this server?"; + if (!msg.showMsgBox(YES_NO_OPTION, "Unknown certificate issuer", text)) + throw new AuthFailureException("Unknown certificate issuer"); + store_pubkey(dbPath, client.getServerName().toLowerCase(), pk); } else { throw new SystemException(e.getMessage()); } } } + private void store_pubkey(File dbPath, String serverName, String pk) + { + ArrayList lines = new ArrayList(); + File vncDir = new File(FileUtils.getVncHomeDir()); + try { + if (dbPath.exists()) { + FileReader db = new FileReader(dbPath); + BufferedReader dbBuf = new BufferedReader(db); + String line; + while ((line = dbBuf.readLine())!=null) { + String fields[] = line.split("\\|"); + if (fields.length==6) + if (!serverName.equals(fields[2]) && !pk.equals(fields[5])) + lines.add(line); + } + dbBuf.close(); + } + } catch (IOException e) { + throw new AuthFailureException("Could not load known hosts database"); + } + try { + if (!dbPath.exists()) + dbPath.createNewFile(); + FileWriter fw = new FileWriter(dbPath.getAbsolutePath(), false); + Iterator i = lines.iterator(); + while (i.hasNext()) + fw.write((String)i.next()+"\n"); + fw.write("|g0|"+serverName+"|*|0|"+pk+"\n"); + fw.close(); + } catch (IOException e) { + vlog.error("Failed to store server certificate to known hosts database"); + } + } + public X509Certificate[] getAcceptedIssuers () { return tm.getAcceptedIssuers(); @@ -399,12 +436,13 @@ public class CSecurityTLS extends CSecur } Object[] answer = {"YES", "NO"}; int ret = JOptionPane.showOptionDialog(null, - "Hostname verification failed. Do you want to continue?", - "Hostname Verification Failure", + "Hostname ("+client.getServerName()+") does not match the"+ + " server certificate, do you want to continue?", + "Certificate hostname mismatch", JOptionPane.YES_NO_OPTION, JOptionPane.WARNING_MESSAGE, null, answer, answer[0]); if (ret != JOptionPane.YES_OPTION) - throw new WarningException("Hostname verification failed."); + throw new WarningException("Certificate hostname mismatch."); } catch (CertificateParsingException e) { throw new SystemException(e.getMessage()); } catch (InvalidNameException e) {