From 8e43f9059094dc0584e594e16501f0c16937dcf2 Mon Sep 17 00:00:00 2001 From: akallabeth Date: Thu, 27 May 2021 08:38:59 +0200 Subject: [PATCH] Fixed #7045: allow NULL isser and subjects in certificates --- libfreerdp/crypto/certificate.c | 81 +++++++++++-- libfreerdp/crypto/crypto.c | 3 + libfreerdp/crypto/test/TestKnownHosts.c | 150 +++++++++++++++++++++++- libfreerdp/crypto/tls.c | 2 + 4 files changed, 219 insertions(+), 17 deletions(-) diff --git a/libfreerdp/crypto/certificate.c b/libfreerdp/crypto/certificate.c index 5992ce9f4..23a3125cf 100644 --- a/libfreerdp/crypto/certificate.c +++ b/libfreerdp/crypto/certificate.c @@ -22,6 +22,7 @@ #include "config.h" #endif +#include #include #include #include @@ -490,10 +491,41 @@ static int certificate_match_data_file(rdpCertificateStore* certificate_store, return rc; } +static BOOL useKnownHosts(rdpCertificateStore* certificate_store) +{ + BOOL use; + assert(certificate_store); + + use = freerdp_settings_get_bool(certificate_store->settings, FreeRDP_CertificateUseKnownHosts); + WLog_INFO(TAG, "known_hosts=%d", use); + return use; +} + +#define check_certificate_store_and_data(store, data) \ + check_certificate_store_and_data_((store), (data), __FILE__, __FUNCTION__, __LINE__) +static BOOL check_certificate_store_and_data_(const rdpCertificateStore* certificate_store, + const rdpCertificateData* certificate_data, + const char* file, const char* fkt, size_t line) +{ + if (!certificate_store) + { + WLog_ERR(TAG, "[%s, %s:%" PRIuz "] certificate_store=NULL", file, fkt, line); + return FALSE; + } + if (!certificate_data) + { + WLog_ERR(TAG, "[%s, %s:%" PRIuz "] certificate_data=NULL", file, fkt, line); + return FALSE; + } + return TRUE; +} + int certificate_store_contains_data(rdpCertificateStore* certificate_store, const rdpCertificateData* certificate_data) { - if (freerdp_settings_get_bool(certificate_store->settings, FreeRDP_CertificateUseKnownHosts)) + if (!check_certificate_store_and_data(certificate_store, certificate_data)) + return -1; + if (useKnownHosts(certificate_store)) { char* pem = NULL; int rc = @@ -570,9 +602,15 @@ static char* certificate_data_get_host_file_entry(const rdpCertificateData* data const char* fingerprint = certificate_data_get_fingerprint(data); char* pem = encode(certificate_data_get_pem(data)); - if (!data || !hostname || !fingerprint || !subject || !issuer) + /* Issuer and subject may be NULL */ + if (!data || !hostname || !fingerprint) goto fail; + if (!subject) + subject = _strdup(""); + if (!issuer) + issuer = _strdup(""); + if (pem) buffer = allocated_printf("%s %" PRIu16 " %s %s %s %s\n", hostname, port, fingerprint, subject, issuer, pem); @@ -609,6 +647,7 @@ static BOOL certificate_data_replace_hosts_file(rdpCertificateStore* certificate const rdpCertificateData* certificate_data, BOOL remove, BOOL append) { + BOOL newfile = TRUE; HANDLE fp = INVALID_HANDLE_VALUE; BOOL rc = FALSE; size_t length; @@ -621,11 +660,14 @@ static BOOL certificate_data_replace_hosts_file(rdpCertificateStore* certificate DWORD lowSize, highSize; rdpCertificateData* copy = NULL; - fp = open_file(certificate_store->file, GENERIC_READ | GENERIC_WRITE, 0, OPEN_EXISTING, + fp = open_file(certificate_store->file, GENERIC_READ | GENERIC_WRITE, 0, OPEN_ALWAYS, FILE_ATTRIBUTE_NORMAL); if (fp == INVALID_HANDLE_VALUE) + { + WLog_ERR(TAG, "failed to open known_hosts file %s, aborting", certificate_store->file); return FALSE; + } /* Create a copy, if a PEM was provided it will replace subject, issuer, fingerprint */ copy = certificate_data_new(certificate_data->hostname, certificate_data->port); @@ -654,7 +696,11 @@ static BOOL certificate_data_replace_hosts_file(rdpCertificateStore* certificate size = (UINT64)lowSize | ((UINT64)highSize << 32); - if (size < 1) + /* Newly created file, just write the new entry */ + if (size > 0) + newfile = FALSE; + + if (newfile) goto fail; data = (char*)malloc(size + 2); @@ -731,8 +777,12 @@ static BOOL certificate_data_replace_hosts_file(rdpCertificateStore* certificate fail: if (!rc && append) { - char* line = certificate_data_get_host_file_entry(copy); - rc = write_line_and_free(certificate_store->file, fp, line); + if (fp != INVALID_HANDLE_VALUE) + { + char* line = certificate_data_get_host_file_entry(copy); + if (line) + rc = write_line_and_free(certificate_store->file, fp, line); + } } CloseHandle(fp); @@ -781,7 +831,9 @@ static BOOL certificate_data_remove_file(rdpCertificateStore* certificate_store, BOOL certificate_store_remove_data(rdpCertificateStore* certificate_store, const rdpCertificateData* certificate_data) { - if (freerdp_settings_get_bool(certificate_store->settings, FreeRDP_CertificateUseKnownHosts)) + if (!check_certificate_store_and_data(certificate_store, certificate_data)) + return FALSE; + if (useKnownHosts(certificate_store)) { /* Ignore return, if the entry was invalid just continue */ certificate_data_replace_hosts_file(certificate_store, certificate_data, TRUE, FALSE); @@ -895,12 +947,11 @@ static BOOL update_from_pem(rdpCertificateData* data) x1 = crypto_cert_from_pem(data->pem, strlen(data->pem), FALSE); if (!x1) goto fail; + + /* Subject and issuer might be NULL */ subject = crypto_cert_subject(x1); - if (!subject) - goto fail; issuer = crypto_cert_issuer(x1); - if (!issuer) - goto fail; + fingerprint = crypto_cert_fingerprint(x1); if (!fingerprint) goto fail; @@ -919,7 +970,9 @@ fail: BOOL certificate_store_save_data(rdpCertificateStore* certificate_store, const rdpCertificateData* certificate_data) { - if (freerdp_settings_get_bool(certificate_store->settings, FreeRDP_CertificateUseKnownHosts)) + if (!check_certificate_store_and_data(certificate_store, certificate_data)) + return FALSE; + if (useKnownHosts(certificate_store)) return certificate_data_replace_hosts_file(certificate_store, certificate_data, TRUE, TRUE); else return certificate_data_write_to_file(certificate_store, certificate_data); @@ -928,7 +981,7 @@ BOOL certificate_store_save_data(rdpCertificateStore* certificate_store, rdpCertificateData* certificate_store_load_data(rdpCertificateStore* certificate_store, const char* host, UINT16 port) { - if (freerdp_settings_get_bool(certificate_store->settings, FreeRDP_CertificateUseKnownHosts)) + if (useKnownHosts(certificate_store)) { int rc; rdpCertificateData* data = certificate_data_new(host, port); @@ -1004,7 +1057,9 @@ UINT16 certificate_data_get_port(const rdpCertificateData* cert) BOOL certificate_data_set_pem(rdpCertificateData* cert, const char* pem) { if (!cert) + { return FALSE; + } if (!duplicate(&cert->pem, pem)) return FALSE; if (!pem) diff --git a/libfreerdp/crypto/crypto.c b/libfreerdp/crypto/crypto.c index 1d2aaea18..67ebb6c02 100644 --- a/libfreerdp/crypto/crypto.c +++ b/libfreerdp/crypto/crypto.c @@ -939,6 +939,8 @@ rdpCertificateData* crypto_get_certificate_data(X509* xcert, const char* hostnam free(pem); return certdata; fail: + WLog_WARN(TAG, "Failed to extract PEM from X509=%p for host %s:%" PRIu16, xcert, hostname, + port); certificate_data_free(certdata); free(pem); return NULL; @@ -1074,6 +1076,7 @@ fail: if (!rc) { + WLog_ERR(TAG, "Failed to extract PEM from certificate %p", xcert); free(pemCert); pemCert = NULL; } diff --git a/libfreerdp/crypto/test/TestKnownHosts.c b/libfreerdp/crypto/test/TestKnownHosts.c index 5f6dea3e3..3c6d6ea5b 100644 --- a/libfreerdp/crypto/test/TestKnownHosts.c +++ b/libfreerdp/crypto/test/TestKnownHosts.c @@ -308,6 +308,49 @@ finish: return rc; } +/* Test host add NULL subject, issuer current file. */ +static BOOL test_known_hosts_host_add_remove_null(rdpCertificateStore* store) +{ + BOOL rc = FALSE; + rdpCertificateData* data; + + printf("%s\n", __FUNCTION__); + + data = certificate_data_new("somehost", 1234); + + if (!data) + { + fprintf(stderr, "Could not create certificate data!\n"); + goto finish; + } + if (!certificate_data_set_subject(data, NULL) || !certificate_data_set_issuer(data, NULL) || + !certificate_data_set_fingerprint(data, "ff:aa:bb:cc")) + goto finish; + + if (!certificate_store_save_data(store, data)) + { + fprintf(stderr, "Could not add host to file!\n"); + goto finish; + } + + if (0 != certificate_store_contains_data(store, data)) + { + fprintf(stderr, "Could not find host written in v2 file!\n"); + goto finish; + } + + if (!certificate_store_remove_data(store, data)) + { + fprintf(stderr, "Could not remove host written in v2 file!\n"); + goto finish; + } + rc = TRUE; +finish: + printf("certificate_data_free %d\n", rc); + certificate_data_free(data); + return rc; +} + /* Test host replace current file. */ static BOOL test_known_hosts_host_replace(rdpCertificateStore* store) { @@ -353,14 +396,14 @@ static BOOL test_known_hosts_host_replace_invalid(rdpCertificateStore* store) rdpCertificateData* data; printf("%s\n", __FUNCTION__); - data = certificate_data_new("somehostXXXX", 1234); + data = certificate_data_new(NULL, 1234); - if (!data) + if (data) { - fprintf(stderr, "Could not create certificate data!\n"); + fprintf(stderr, "Could create invalid certificate data!\n"); goto finish; } - if (!certificate_data_set_fingerprint(data, "ff:aa:bb:dd:ee")) + if (certificate_data_set_fingerprint(data, "ff:aa:bb:dd:ee")) goto finish; if (certificate_store_save_data(store, data)) @@ -381,6 +424,99 @@ finish: return rc; } +static BOOL test_known_hosts_file_emtpy_single(BOOL (*fkt)(rdpCertificateStore* store)) +{ + BOOL rc = FALSE; + rdpSettings* settings = NULL; + rdpCertificateStore* store = NULL; + char* currentFileV2 = NULL; + + printf("%s", __FUNCTION__); + if (!fkt) + return FALSE; + + if (!setup_config(&settings)) + goto finish; + if (!freerdp_settings_set_bool(settings, FreeRDP_CertificateUseKnownHosts, TRUE)) + goto finish; + + currentFileV2 = + GetCombinedPath(freerdp_settings_get_string(settings, FreeRDP_ConfigPath), "known_hosts2"); + + if (!currentFileV2) + { + fprintf(stderr, "Could not get file path!\n"); + goto finish; + } + + printf("certificate_store_new\n"); + store = certificate_store_new(settings); + + if (!store) + { + fprintf(stderr, "Could not create certificate store!\n"); + goto finish; + } + + rc = fkt(store); + +finish: + freerdp_settings_free(settings); + + printf("certificate_store_free\n"); + certificate_store_free(store); + + DeleteFileA(currentFileV2); + free(currentFileV2); + return rc; +} + +static BOOL test_known_hosts_file_empty(void) +{ + BOOL rc = FALSE; + + if (test_known_hosts_file_emtpy_single(test_known_hosts_host_found)) + { + fprintf(stderr, "[%s] test_known_hosts_file_emtpy_single failed\n", __FUNCTION__); + goto finish; + } + + if (!test_known_hosts_file_emtpy_single(test_known_hosts_host_not_found)) + { + fprintf(stderr, "[%s] test_known_hosts_file_emtpy_single failed\n", __FUNCTION__); + goto finish; + } + + if (!test_known_hosts_file_emtpy_single(test_known_hosts_host_add)) + { + fprintf(stderr, "[%s] test_known_hosts_file_emtpy_single failed\n", __FUNCTION__); + goto finish; + } + + if (!test_known_hosts_file_emtpy_single(test_known_hosts_host_add_remove_null)) + { + fprintf(stderr, "[%s] test_known_hosts_file_emtpy_single failed\n", __FUNCTION__); + goto finish; + } + + if (!test_known_hosts_file_emtpy_single(test_known_hosts_host_replace)) + { + fprintf(stderr, "[%s] test_known_hosts_file_emtpy_single failed\n", __FUNCTION__); + goto finish; + } + + if (!test_known_hosts_file_emtpy_single(test_known_hosts_host_replace_invalid)) + { + fprintf(stderr, "[%s] test_known_hosts_file_emtpy_single failed\n", __FUNCTION__); + goto finish; + } + + rc = TRUE; +finish: + + return rc; +} + static BOOL test_known_hosts_file(void) { BOOL rc = FALSE; @@ -424,6 +560,9 @@ static BOOL test_known_hosts_file(void) if (!test_known_hosts_host_add(store)) goto finish; + if (!test_known_hosts_host_add_remove_null(store)) + goto finish; + if (!test_known_hosts_host_replace(store)) goto finish; @@ -680,6 +819,9 @@ int TestKnownHosts(int argc, char* argv[]) { WINPR_UNUSED(argc); WINPR_UNUSED(argv); + if (!test_known_hosts_file_empty()) + return -1; + if (!test_known_hosts_file()) return -1; if (!test_certs_dir(FALSE)) diff --git a/libfreerdp/crypto/tls.c b/libfreerdp/crypto/tls.c index 5e2373476..d3efc4571 100644 --- a/libfreerdp/crypto/tls.c +++ b/libfreerdp/crypto/tls.c @@ -1334,6 +1334,8 @@ int tls_verify_certificate(rdpTls* tls, CryptoCert cert, const char* hostname, U x509_verify_certificate(cert, certificate_store_get_certs_path(tls->certificate_store)); /* verify certificate name match */ certificate_data = crypto_get_certificate_data(cert->px509, hostname, port); + if (!certificate_data) + goto end; /* extra common name and alternative names */ common_name = crypto_cert_subject_common_name(cert->px509, &common_name_length); dns_names = crypto_cert_get_dns_names(cert->px509, &dns_names_count, &dns_names_lengths);