From feb20edda79ed1184b1fb508b03a08712014b0f8 Mon Sep 17 00:00:00 2001 From: Pawel Jakub Dawidek Date: Fri, 3 Feb 2012 09:00:39 +0100 Subject: [PATCH 1/7] Plug memory leak in case of an empty file and terminate string with '\0'. --- libfreerdp-core/certificate.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/libfreerdp-core/certificate.c b/libfreerdp-core/certificate.c index ec50b4ed0..5eb877146 100644 --- a/libfreerdp-core/certificate.c +++ b/libfreerdp-core/certificate.c @@ -610,13 +610,17 @@ int certificate_data_match(rdpCertificateStore* certificate_store, rdpCertificat size = ftell(fp); fseek(fp, 0, SEEK_SET); - data = (char*) xmalloc(size + 1); - length = fread(data, size, 1, fp); - if (size < 1) return match; + data = (char*) xmalloc(size + 2); + if (fread(data, size, 1, fp) != 1) { + xfree(data); + return match; + } + data[size] = '\n'; + data[size + 1] = '\0'; pline = strtok(data, "\n"); while (pline != NULL) From 1619b4378179906777feae489e1f6db58801e73a Mon Sep 17 00:00:00 2001 From: Pawel Jakub Dawidek Date: Fri, 3 Feb 2012 14:44:45 +0100 Subject: [PATCH 2/7] Fix problem where we check errno even for status == 0. This way when connection was properly terminated, but errno had EAGAIN value from before, we looped idenfiniately. --- libfreerdp-core/tcp.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/libfreerdp-core/tcp.c b/libfreerdp-core/tcp.c index 281ddae23..095f369af 100644 --- a/libfreerdp-core/tcp.c +++ b/libfreerdp-core/tcp.c @@ -185,7 +185,12 @@ int tcp_read(rdpTcp* tcp, uint8* data, int length) status = recv(tcp->sockfd, data, length, 0); - if (status <= 0) + if (status == 0) + { + /* Peer disconnected. */ + return -1; + } + else if (status < 0) { #ifdef _WIN32 int wsa_error = WSAGetLastError(); @@ -194,17 +199,13 @@ int tcp_read(rdpTcp* tcp, uint8* data, int length) if (wsa_error == WSAEWOULDBLOCK) return 0; - /* When peer disconnects we get status 0 with no error. */ - if (status < 0) - printf("recv() error: %d\n", wsa_error); + printf("recv() error: %d\n", wsa_error); #else /* No data available */ if (errno == EAGAIN || errno == EWOULDBLOCK) return 0; - /* When peer disconnects we get status 0 with no error. */ - if (status < 0) - perror("recv"); + perror("recv"); #endif return -1; } From 5ca9a37f68bfe182ff667c4fabeb7e8a2c4b5a3a Mon Sep 17 00:00:00 2001 From: Alexis Moinet Date: Fri, 3 Feb 2012 15:27:04 +0100 Subject: [PATCH 3/7] remove redundant if(NULL) checks (already checked inside of cache_free and rail_free) Besides "ptr=NULL; free(ptr);" does nothing so no need to check for NULL in xfree() --- client/X11/xfreerdp.c | 10 ++-------- libfreerdp-utils/memory.c | 1 - 2 files changed, 2 insertions(+), 9 deletions(-) diff --git a/client/X11/xfreerdp.c b/client/X11/xfreerdp.c index 30a231071..28bc33134 100644 --- a/client/X11/xfreerdp.c +++ b/client/X11/xfreerdp.c @@ -888,16 +888,11 @@ void xf_window_free(xfInfo* xfi) if (context != NULL) { - if (context->cache != NULL) - { cache_free(context->cache); context->cache = NULL; - } - if (context->rail != NULL) - { + rail_free(context->rail); context->rail = NULL; - } } if (xfi->rfx_context) @@ -917,8 +912,7 @@ void xf_free(xfInfo* xfi) { xf_window_free(xfi); - if (xfi->bmp_codec_none != NULL) - xfree(xfi->bmp_codec_none); + xfree(xfi->bmp_codec_none); XCloseDisplay(xfi->display); diff --git a/libfreerdp-utils/memory.c b/libfreerdp-utils/memory.c index 1a4ff6c57..2e2a11a17 100644 --- a/libfreerdp-utils/memory.c +++ b/libfreerdp-utils/memory.c @@ -103,7 +103,6 @@ void* xrealloc(void* ptr, size_t size) void xfree(void* ptr) { - if (ptr != NULL) free(ptr); } From 3c2997b35236b9e4f6875c63045a39611e0bcc07 Mon Sep 17 00:00:00 2001 From: Christian Nilsson Date: Sat, 4 Feb 2012 00:04:41 +0100 Subject: [PATCH 4/7] Fix duplicate xfree on format_name->name, shuld resolve Issue #387 and #395 ? --- channels/cliprdr/cliprdr_format.c | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) diff --git a/channels/cliprdr/cliprdr_format.c b/channels/cliprdr/cliprdr_format.c index f5223b369..19099b080 100644 --- a/channels/cliprdr/cliprdr_format.c +++ b/channels/cliprdr/cliprdr_format.c @@ -272,19 +272,8 @@ void cliprdr_process_format_list(cliprdrPlugin* cliprdr, STREAM* s, uint32 dataL xfree(format_name->name); } - if (cliprdr->format_names != NULL) - { - for (i = 0; i < cliprdr->num_format_names; i++) - { - format_name = &cliprdr->format_names[i]; - - if (format_name->length > 0) - xfree(format_name->name); - } - - xfree(cliprdr->format_names); - cliprdr->format_names = NULL; - } + xfree(cliprdr->format_names); + cliprdr->format_names = NULL; cliprdr->num_format_names = 0; From 4695faae38ef5cf7a867578c23632ea23565635d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Moreau?= Date: Sat, 4 Feb 2012 02:21:39 -0500 Subject: [PATCH 5/7] libfreerdp-core: added check for certificate name against hostname --- client/X11/xfreerdp.c | 4 ++- libfreerdp-core/certificate.c | 7 +++-- libfreerdp-core/crypto.c | 54 ++++++++++++++++++++++++++++++++++- libfreerdp-core/crypto.h | 2 ++ libfreerdp-core/tls.c | 51 ++++++++++++++++++++++++++++++++- libfreerdp-core/tls.h | 3 +- 6 files changed, 115 insertions(+), 6 deletions(-) diff --git a/client/X11/xfreerdp.c b/client/X11/xfreerdp.c index 28bc33134..c15a501af 100644 --- a/client/X11/xfreerdp.c +++ b/client/X11/xfreerdp.c @@ -735,6 +735,8 @@ boolean xf_authenticate(freerdp* instance, char** username, char** password, cha boolean xf_verify_certificate(freerdp* instance, char* subject, char* issuer, char* fingerprint) { + char answer; + printf("Certificate details:\n"); printf("\tSubject: %s\n", subject); printf("\tIssuer: %s\n", issuer); @@ -743,7 +745,6 @@ boolean xf_verify_certificate(freerdp* instance, char* subject, char* issuer, ch "the CA certificate in your certificate store, or the certificate has expired. " "Please look at the documentation on how to create local certificate store for a private CA.\n"); - char answer; while (1) { printf("Do you trust the above certificate? (Y/N) "); @@ -757,6 +758,7 @@ boolean xf_verify_certificate(freerdp* instance, char* subject, char* issuer, ch { break; } + printf("\n"); } return false; diff --git a/libfreerdp-core/certificate.c b/libfreerdp-core/certificate.c index 5eb877146..69fcf1da6 100644 --- a/libfreerdp-core/certificate.c +++ b/libfreerdp-core/certificate.c @@ -375,7 +375,8 @@ boolean certificate_read_server_proprietary_certificate(rdpCertificate* certific return false; } stream_read_uint16(s, wSignatureBlobLen); - if (wSignatureBlobLen != 72) { + if (wSignatureBlobLen != 72) + { printf("certificate_process_server_public_signature: invalid signature length (got %d, expected %d)\n", wSignatureBlobLen, 64); return false; } @@ -614,7 +615,9 @@ int certificate_data_match(rdpCertificateStore* certificate_store, rdpCertificat return match; data = (char*) xmalloc(size + 2); - if (fread(data, size, 1, fp) != 1) { + + if (fread(data, size, 1, fp) != 1) + { xfree(data); return match; } diff --git a/libfreerdp-core/crypto.c b/libfreerdp-core/crypto.c index 1b1ada5fb..3fbe34ffb 100644 --- a/libfreerdp-core/crypto.c +++ b/libfreerdp-core/crypto.c @@ -355,7 +355,7 @@ char* crypto_print_name(X509_NAME* name) char* buffer = NULL; BIO* outBIO = BIO_new(BIO_s_mem()); - if(X509_NAME_print_ex(outBIO, name, 0, XN_FLAG_ONELINE) > 0) + if (X509_NAME_print_ex(outBIO, name, 0, XN_FLAG_ONELINE) > 0) { unsigned long size = BIO_number_written(outBIO); buffer = xzalloc(size + 1); @@ -373,6 +373,58 @@ char* crypto_cert_subject(X509* xcert) return crypto_print_name(X509_get_subject_name(xcert)); } +char* crypto_cert_subject_common_name(X509* xcert) +{ + int index; + int length; + uint8* common_name; + X509_NAME* subject_name; + X509_NAME_ENTRY* entry; + ASN1_STRING* entry_data; + + subject_name = X509_get_subject_name(xcert); + index = X509_NAME_get_index_by_NID(subject_name, NID_commonName, -1); + + entry = X509_NAME_get_entry(subject_name, index); + entry_data = X509_NAME_ENTRY_get_data(entry); + + length = ASN1_STRING_to_UTF8(&common_name, entry_data); + + return (char*) common_name; +} + +char** crypto_cert_subject_alt_name(X509* xcert, int* count) +{ + int index; + char** strings; + uint8* string; + int num_subject_alt_names; + GENERAL_NAMES* subject_alt_names; + GENERAL_NAME* subject_alt_name; + + *count = 0; + subject_alt_names = X509_get_ext_d2i(xcert, NID_subject_alt_name, 0, 0); + + if (!subject_alt_names) + return NULL; + + num_subject_alt_names = sk_GENERAL_NAME_num(subject_alt_names); + strings = malloc(sizeof(char*) * num_subject_alt_names); + + for (index = 0; index < num_subject_alt_names; ++index) + { + subject_alt_name = sk_GENERAL_NAME_value(subject_alt_names, index); + + if (subject_alt_name->type == GEN_DNS) + { + ASN1_STRING_to_UTF8(&string, subject_alt_name->d.dNSName); + strings[(*count)++] = (char*) string; + } + } + + return strings; +} + char* crypto_cert_issuer(X509* xcert) { return crypto_print_name(X509_get_issuer_name(xcert)); diff --git a/libfreerdp-core/crypto.h b/libfreerdp-core/crypto.h index 9253305ae..515399f0c 100644 --- a/libfreerdp-core/crypto.h +++ b/libfreerdp-core/crypto.h @@ -115,6 +115,8 @@ typedef struct crypto_cert_struct* CryptoCert; CryptoCert crypto_cert_read(uint8* data, uint32 length); char* crypto_cert_fingerprint(X509* xcert); char* crypto_cert_subject(X509* xcert); +char* crypto_cert_subject_common_name(X509* xcert); +char** crypto_cert_subject_alt_name(X509* xcert, int* count); char* crypto_cert_issuer(X509* xcert); void crypto_cert_print_info(X509* xcert); void crypto_cert_free(CryptoCert cert); diff --git a/libfreerdp-core/tls.c b/libfreerdp-core/tls.c index 22137b2de..882243733 100644 --- a/libfreerdp-core/tls.c +++ b/libfreerdp-core/tls.c @@ -230,7 +230,12 @@ CryptoCert tls_get_certificate(rdpTls* tls) int tls_verify_certificate(rdpTls* tls, CryptoCert cert, char* hostname) { int match; + int index; boolean status; + char* common_name; + char** alt_names; + int alt_names_count; + boolean hostname_match = false; status = x509_verify_certificate(cert, tls->certificate_store->path); @@ -245,8 +250,22 @@ int tls_verify_certificate(rdpTls* tls, CryptoCert cert, char* hostname) match = certificate_data_match(tls->certificate_store, certificate_data); - if (match == 0) + common_name = crypto_cert_subject_common_name(cert->px509); + alt_names = crypto_cert_subject_alt_name(cert->px509, &alt_names_count); + + if (strcmp(hostname, common_name) == 0) + hostname_match = true; + + for (index = 0; index < alt_names_count; index++) + { + if (strcmp(hostname, alt_names[index]) == 0) + hostname_match = true; + } + + if ((match == 0) && hostname_match) return 0; + else + tls_print_certificate_name_mismatch_error(hostname, common_name, alt_names, alt_names_count); issuer = crypto_cert_issuer(cert->px509); subject = crypto_cert_subject(cert->px509); @@ -299,6 +318,36 @@ void tls_print_certificate_error(char* hostname, char* fingerprint) printf("Host key verification failed.\n"); } +void tls_print_certificate_name_mismatch_error(char* hostname, char* common_name, char** alt_names, int alt_names_count) +{ + int index; + + printf("@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@\n"); + printf("@ WARNING: CERTIFICATE NAME MISMATCH! @\n"); + printf("@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@\n"); + printf("The hostname used for this connection (%s) \n", hostname); + + if (alt_names_count < 1) + { + printf("does not match the name given in the certificate:\n"); + printf("%s\n", common_name); + } + else + { + printf("does not match the names given in the certificate:\n"); + printf("%s", common_name); + + for (index = 0; index < alt_names_count; index++) + { + printf(", %s", alt_names[index]); + } + + printf("\n"); + } + + printf("A valid certificate for the wrong name should NOT be trusted!\n"); +} + rdpTls* tls_new(rdpSettings* settings) { rdpTls* tls; diff --git a/libfreerdp-core/tls.h b/libfreerdp-core/tls.h index 06e565971..9657b3a27 100644 --- a/libfreerdp-core/tls.h +++ b/libfreerdp-core/tls.h @@ -49,7 +49,8 @@ int tls_write(rdpTls* tls, uint8* data, int length); CryptoCert tls_get_certificate(rdpTls* tls); int tls_verify_certificate(rdpTls* tls, CryptoCert cert, char* hostname); -void tls_print_certificate_error(); +void tls_print_certificate_error(char* hostname, char* fingerprint); +void tls_print_certificate_name_mismatch_error(char* hostname, char* common_name, char** alt_names, int alt_names_count); boolean tls_print_error(char* func, SSL* connection, int value); From ffec1c20626146c9a79695737e71de9b0b4dfeaf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Moreau?= Date: Sat, 4 Feb 2012 15:04:03 -0500 Subject: [PATCH 6/7] libfreerdp-core: improve and clarify certificate checking --- libfreerdp-core/credssp.c | 2 +- libfreerdp-core/tls.c | 115 ++++++++++++++++++++++++-------------- libfreerdp-core/tls.h | 2 +- 3 files changed, 75 insertions(+), 44 deletions(-) diff --git a/libfreerdp-core/credssp.c b/libfreerdp-core/credssp.c index 64aa1f0b2..e269a21ce 100644 --- a/libfreerdp-core/credssp.c +++ b/libfreerdp-core/credssp.c @@ -136,7 +136,7 @@ int credssp_get_public_key(rdpCredssp* credssp) return 0; } - if (tls_verify_certificate(credssp->transport->tls, cert, credssp->transport->settings->hostname)) + if (!tls_verify_certificate(credssp->transport->tls, cert, credssp->transport->settings->hostname)) tls_disconnect(credssp->transport->tls); status = crypto_cert_get_public_key(cert, &credssp->public_key); diff --git a/libfreerdp-core/tls.c b/libfreerdp-core/tls.c index 882243733..8344832b0 100644 --- a/libfreerdp-core/tls.c +++ b/libfreerdp-core/tls.c @@ -227,79 +227,110 @@ CryptoCert tls_get_certificate(rdpTls* tls) return cert; } -int tls_verify_certificate(rdpTls* tls, CryptoCert cert, char* hostname) +boolean tls_verify_certificate(rdpTls* tls, CryptoCert cert, char* hostname) { int match; int index; - boolean status; char* common_name; char** alt_names; int alt_names_count; + boolean certificate_status; boolean hostname_match = false; + rdpCertificateData* certificate_data; - status = x509_verify_certificate(cert, tls->certificate_store->path); + /* ignore certificate verification if user explicitly required it (discouraged) */ + if (tls->settings->ignore_certificate) + return true; /* success! */ - if (status != true) + /* attempt verification using OpenSSL and the ~/.freerdp/certs certificate store */ + certificate_status = x509_verify_certificate(cert, tls->certificate_store->path); + + /* verify certificate name match */ + certificate_data = crypto_get_certificate_data(cert->px509, hostname); + + /* extra common name and alternative names */ + common_name = crypto_cert_subject_common_name(cert->px509); + alt_names = crypto_cert_subject_alt_name(cert->px509, &alt_names_count); + + /* compare against common name */ + if (strcmp(hostname, common_name) == 0) + hostname_match = true; + + /* compare against alternative names */ + for (index = 0; index < alt_names_count; index++) + { + if (strcmp(hostname, alt_names[index]) == 0) + hostname_match = true; + } + + /* if the certificate is valid and the certificate name matches, verification succeeds */ + if (certificate_status && hostname_match) + return true; /* success! */ + + /* if the certificate is valid but the certificate name does not match, warn user, do not accept */ + if (certificate_status && !hostname_match) + tls_print_certificate_name_mismatch_error(hostname, common_name, alt_names, alt_names_count); + + /* verification could not succeed with OpenSSL, use known_hosts file and prompt user for manual verification */ + + if (!certificate_status) { char* issuer; char* subject; char* fingerprint; - rdpCertificateData* certificate_data; - - certificate_data = crypto_get_certificate_data(cert->px509, hostname); - - match = certificate_data_match(tls->certificate_store, certificate_data); - - common_name = crypto_cert_subject_common_name(cert->px509); - alt_names = crypto_cert_subject_alt_name(cert->px509, &alt_names_count); - - if (strcmp(hostname, common_name) == 0) - hostname_match = true; - - for (index = 0; index < alt_names_count; index++) - { - if (strcmp(hostname, alt_names[index]) == 0) - hostname_match = true; - } - - if ((match == 0) && hostname_match) - return 0; - else - tls_print_certificate_name_mismatch_error(hostname, common_name, alt_names, alt_names_count); + boolean accept_certificate = false; + boolean verification_status = false; issuer = crypto_cert_issuer(cert->px509); subject = crypto_cert_subject(cert->px509); fingerprint = crypto_cert_fingerprint(cert->px509); + /* search for matching entry in known_hosts file */ + match = certificate_data_match(tls->certificate_store, certificate_data); + if (match == 1) { - boolean accept_certificate = tls->settings->ignore_certificate; + /* no entry was found in known_hosts file, prompt user for manual verification */ + + freerdp* instance = (freerdp*) tls->settings->instance; + + if (!hostname_match) + tls_print_certificate_name_mismatch_error(hostname, common_name, alt_names, alt_names_count); + + if (instance->VerifyCertificate) + accept_certificate = instance->VerifyCertificate(instance, subject, issuer, fingerprint); if (!accept_certificate) { - freerdp* instance = (freerdp*) tls->settings->instance; - - if (instance->VerifyCertificate) - accept_certificate = instance->VerifyCertificate(instance, subject, issuer, fingerprint); - - xfree(issuer); - xfree(subject); - xfree(fingerprint); + /* user did not accept, abort and do not add entry in known_hosts file */ + verification_status = false; /* failure! */ + } + else + { + /* user accepted certificate, add entry in known_hosts file */ + certificate_data_print(tls->certificate_store, certificate_data); + verification_status = true; /* success! */ } - - if (!accept_certificate) - return 1; - - certificate_data_print(tls->certificate_store, certificate_data); } else if (match == -1) { + /* entry was found in known_hosts file, but fingerprint does not match */ tls_print_certificate_error(hostname, fingerprint); - return 1; + verification_status = false; /* failure! */ } + else if (match == 0) + { + verification_status = true; /* success! */ + } + + xfree(issuer); + xfree(subject); + xfree(fingerprint); + + return verification_status; } - return 0; + return false; } void tls_print_certificate_error(char* hostname, char* fingerprint) diff --git a/libfreerdp-core/tls.h b/libfreerdp-core/tls.h index 9657b3a27..c3f2f5999 100644 --- a/libfreerdp-core/tls.h +++ b/libfreerdp-core/tls.h @@ -48,7 +48,7 @@ int tls_read(rdpTls* tls, uint8* data, int length); int tls_write(rdpTls* tls, uint8* data, int length); CryptoCert tls_get_certificate(rdpTls* tls); -int tls_verify_certificate(rdpTls* tls, CryptoCert cert, char* hostname); +boolean tls_verify_certificate(rdpTls* tls, CryptoCert cert, char* hostname); void tls_print_certificate_error(char* hostname, char* fingerprint); void tls_print_certificate_name_mismatch_error(char* hostname, char* common_name, char** alt_names, int alt_names_count); From 47de99062dfe31266dbe61fd2042c211aca683cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Moreau?= Date: Sat, 4 Feb 2012 15:16:41 -0500 Subject: [PATCH 7/7] libfreerdp-core: added --certificate-name option for explicitly specifying a certificate name --- include/freerdp/settings.h | 3 ++- libfreerdp-core/tls.c | 4 ++++ libfreerdp-utils/args.c | 11 +++++++++++ 3 files changed, 17 insertions(+), 1 deletion(-) diff --git a/include/freerdp/settings.h b/include/freerdp/settings.h index 3c2be0198..5a9acd8a3 100644 --- a/include/freerdp/settings.h +++ b/include/freerdp/settings.h @@ -358,7 +358,8 @@ struct rdp_settings rdpCertificate* server_cert; /* 269 */ char* rdp_key_file; /* 270 */ rdpKey* server_key; /* 271 */ - uint32 paddingL[280 - 272]; /* 272 */ + char* certificate_name; /* 272 */ + uint32 paddingL[280 - 273]; /* 273 */ /* Codecs */ boolean rfx_codec; /* 280 */ diff --git a/libfreerdp-core/tls.c b/libfreerdp-core/tls.c index 8344832b0..4f7e1476d 100644 --- a/libfreerdp-core/tls.c +++ b/libfreerdp-core/tls.c @@ -242,6 +242,10 @@ boolean tls_verify_certificate(rdpTls* tls, CryptoCert cert, char* hostname) if (tls->settings->ignore_certificate) return true; /* success! */ + /* if user explicitly specified a certificate name, use it instead of the hostname */ + if (tls->settings->certificate_name) + hostname = tls->settings->certificate_name; + /* attempt verification using OpenSSL and the ~/.freerdp/certs certificate store */ certificate_status = x509_verify_certificate(cert, tls->certificate_store->path); diff --git a/libfreerdp-utils/args.c b/libfreerdp-utils/args.c index ee00d7528..cf10519d6 100644 --- a/libfreerdp-utils/args.c +++ b/libfreerdp-utils/args.c @@ -306,6 +306,17 @@ int freerdp_parse_args(rdpSettings* settings, int argc, char** argv, { settings->ignore_certificate = true; } + else if (strcmp("--certificate-name", argv[index]) == 0) + { + index++; + if (index == argc) + { + printf("missing certificate name\n"); + return FREERDP_ARGS_PARSE_FAILURE; + } + + settings->certificate_name = xstrdup(argv[index]); + } else if (strcmp("--no-fastpath", argv[index]) == 0) { settings->fastpath_input = false;