From 6328a070ddaab16faaf008cb9a8a62439c30f2a8 Mon Sep 17 00:00:00 2001 From: Martin Hecht Date: Fri, 21 Feb 2020 21:37:06 +0100 Subject: [PATCH] fix TLS Certificate CommonName NULL Byte Vulnerability CVE-2020-7043 TLS Certificate CommonName NULL Byte Vulnerability is fixed with this commit with #8 hostname validation for the certificate was introduced but unfortunately strncasecmp() was used to compare the byte array against the expected hostname. This does not correctly treat a CN which contains a NULL byte. In order to fix this vulnerability the reference implementation from iSECPartners has been included into the code. --- Makefile.am | 4 +- src/openssl_hostname_validation.c | 165 ++++++++++++++++++++++++++++++ src/openssl_hostname_validation.h | 53 ++++++++++ src/tunnel.c | 16 ++- tests/lint/run.sh | 6 +- 5 files changed, 231 insertions(+), 13 deletions(-) create mode 100644 src/openssl_hostname_validation.c create mode 100644 src/openssl_hostname_validation.h diff --git a/Makefile.am b/Makefile.am index 9c74e502..c3515108 100644 --- a/Makefile.am +++ b/Makefile.am @@ -5,7 +5,9 @@ openfortivpn_SOURCES = src/config.c src/config.h src/hdlc.c src/hdlc.h \ src/http.c src/http.h src/io.c src/io.h src/ipv4.c \ src/ipv4.h src/log.c src/log.h src/tunnel.c \ src/tunnel.h src/main.c src/ssl.h src/xml.c \ - src/xml.h src/userinput.c src/userinput.h + src/xml.h src/userinput.c src/userinput.h \ + src/openssl_hostname_validation.c \ + src/openssl_hostname_validation.h openfortivpn_CFLAGS = -Wall -pedantic -std=gnu99 openfortivpn_CPPFLAGS = -DSYSCONFDIR=\"$(sysconfdir)\" \ -DPPP_PATH=\"@PPP_PATH@\" \ diff --git a/src/openssl_hostname_validation.c b/src/openssl_hostname_validation.c new file mode 100644 index 00000000..a654e4c7 --- /dev/null +++ b/src/openssl_hostname_validation.c @@ -0,0 +1,165 @@ +/* Obtained from: https://github.com/iSECPartners/ssl-conservatory */ + +/* + * Helper functions to perform basic hostname validation using OpenSSL. + * + * Author: Alban Diquet + * + * Copyright (C) 2012, iSEC Partners. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to + * use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies + * of the Software, and to permit persons to whom the Software is furnished to do + * so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + * + */ + + +#include +#include +#include + +#define HOSTNAME_MAX_SIZE 255 + +#ifndef HAVE_X509_CHECK_HOST + +#include "openssl_hostname_validation.h" + + +/** +* Tries to find a match for hostname in the certificate's Common Name field. +* +* Returns MatchFound if a match was found. +* Returns MatchNotFound if no matches were found. +* Returns MalformedCertificate if the Common Name had a NUL character embedded in it. +* Returns Error if the Common Name could not be extracted. +*/ +static HostnameValidationResult matches_common_name(const char *hostname, const X509 *server_cert) { + int common_name_loc = -1; + X509_NAME_ENTRY *common_name_entry = NULL; + ASN1_STRING *common_name_asn1 = NULL; + char *common_name_str = NULL; + + // Find the position of the CN field in the Subject field of the certificate + common_name_loc = X509_NAME_get_index_by_NID(X509_get_subject_name((X509 *) server_cert), NID_commonName, -1); + if (common_name_loc < 0) { + return Error; + } + + // Extract the CN field + common_name_entry = X509_NAME_get_entry(X509_get_subject_name((X509 *) server_cert), common_name_loc); + if (common_name_entry == NULL) { + return Error; + } + + // Convert the CN field to a C string + common_name_asn1 = X509_NAME_ENTRY_get_data(common_name_entry); + if (common_name_asn1 == NULL) { + return Error; + } + common_name_str = (char *) ASN1_STRING_data(common_name_asn1); + + // Make sure there isn't an embedded NUL character in the CN + if (ASN1_STRING_length(common_name_asn1) != strlen(common_name_str)) { + return MalformedCertificate; + } + + // Compare expected hostname with the CN + if (strcasecmp(hostname, common_name_str) == 0) { + return MatchFound; + } + else { + return MatchNotFound; + } +} + + +/** +* Tries to find a match for hostname in the certificate's Subject Alternative Name extension. +* +* Returns MatchFound if a match was found. +* Returns MatchNotFound if no matches were found. +* Returns MalformedCertificate if any of the hostnames had a NUL character embedded in it. +* Returns NoSANPresent if the SAN extension was not present in the certificate. +*/ +static HostnameValidationResult matches_subject_alternative_name(const char *hostname, const X509 *server_cert) { + HostnameValidationResult result = MatchNotFound; + int i; + int san_names_nb = -1; + STACK_OF(GENERAL_NAME) *san_names = NULL; + + // Try to extract the names within the SAN extension from the certificate + san_names = X509_get_ext_d2i((X509 *) server_cert, NID_subject_alt_name, NULL, NULL); + if (san_names == NULL) { + return NoSANPresent; + } + san_names_nb = sk_GENERAL_NAME_num(san_names); + + // Check each name within the extension + for (i=0; itype == GEN_DNS) { + // Current name is a DNS name, let's check it + char *dns_name = (char *) ASN1_STRING_data(current_name->d.dNSName); + + // Make sure there isn't an embedded NUL character in the DNS name + if (ASN1_STRING_length(current_name->d.dNSName) != strlen(dns_name)) { + result = MalformedCertificate; + break; + } + else { // Compare expected hostname with the DNS name + if (strcasecmp(hostname, dns_name) == 0) { + result = MatchFound; + break; + } + } + } + } + sk_GENERAL_NAME_pop_free(san_names, GENERAL_NAME_free); + + return result; +} + + +/** +* Validates the server's identity by looking for the expected hostname in the +* server's certificate. As described in RFC 6125, it first tries to find a match +* in the Subject Alternative Name extension. If the extension is not present in +* the certificate, it checks the Common Name instead. +* +* Returns MatchFound if a match was found. +* Returns MatchNotFound if no matches were found. +* Returns MalformedCertificate if any of the hostnames had a NUL character embedded in it. +* Returns Error if there was an error. +*/ +HostnameValidationResult validate_hostname(const char *hostname, const X509 *server_cert) { + HostnameValidationResult result; + + if((hostname == NULL) || (server_cert == NULL)) + return Error; + + // First try the Subject Alternative Names extension + result = matches_subject_alternative_name(hostname, server_cert); + if (result == NoSANPresent) { + // Extension was not found: try the Common Name + result = matches_common_name(hostname, server_cert); + } + + return result; +} + +#endif diff --git a/src/openssl_hostname_validation.h b/src/openssl_hostname_validation.h new file mode 100644 index 00000000..07e37a70 --- /dev/null +++ b/src/openssl_hostname_validation.h @@ -0,0 +1,53 @@ +/* Obtained from: https://github.com/iSECPartners/ssl-conservatory */ + +/* + * Helper functions to perform basic hostname validation using OpenSSL. + * + * Author: Alban Diquet + * + * Copyright (C) 2012, iSEC Partners. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of + * this software and associated documentation files (the "Software"), to deal in + * the Software without restriction, including without limitation the rights to + * use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies + * of the Software, and to permit persons to whom the Software is furnished to do + * so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all + * copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + * + */ + +#ifndef HAVE_X509_CHECK_HOST + +typedef enum { + MatchFound, + MatchNotFound, + NoSANPresent, + MalformedCertificate, + Error +} HostnameValidationResult; + +/** +* Validates the server's identity by looking for the expected hostname in the +* server's certificate. As described in RFC 6125, it first tries to find a match +* in the Subject Alternative Name extension. If the extension is not present in +* the certificate, it checks the Common Name instead. +* +* Returns MatchFound if a match was found. +* Returns MatchNotFound if no matches were found. +* Returns MalformedCertificate if any of the hostnames had a NUL character embedded in it. +* Returns Error if there was an error. +*/ +HostnameValidationResult validate_hostname(const char *hostname, const X509 *server_cert); + +#endif diff --git a/src/tunnel.c b/src/tunnel.c index f0466a6b..ffd83a62 100644 --- a/src/tunnel.c +++ b/src/tunnel.c @@ -29,6 +29,9 @@ #include "tunnel.h" #include "http.h" #include "log.h" +#ifndef HAVE_X509_CHECK_HOST +#include "openssl_hostname_validation.h" +#endif #include #include @@ -671,15 +674,10 @@ static int ssl_verify_cert(struct tunnel *tunnel) 0, 0, NULL) == 1) cert_valid = 1; #else - char common_name[FIELD_SIZE + 1]; - // Use explicit Common Name check if native validation not available. - // Note: this will ignore Subject Alternative Name fields. - if (subj - && X509_NAME_get_text_by_NID(subj, NID_commonName, common_name, - FIELD_SIZE) > 0 - && strncasecmp(common_name, tunnel->config->gateway_host, - FIELD_SIZE) == 0) - cert_valid = 1; + // Use validate_hostname form iSECPartners if native validation not available + // in order to avoid TLS Certificate CommonName NULL Byte Vulnerability + if (validate_hostname(tunnel->config->gateway_host, cert) == MatchFound) + cert_valid = 1; #endif // Try to validate certificate using local PKI diff --git a/tests/lint/run.sh b/tests/lint/run.sh index a8e56565..fe1aa509 100755 --- a/tests/lint/run.sh +++ b/tests/lint/run.sh @@ -3,10 +3,10 @@ rc=0 -./tests/lint/eol-at-eof.sh $(git ls-files) || rc=1 +./tests/lint/eol-at-eof.sh $(git ls-files | grep -v openssl_hostname_validation) || rc=1 -./tests/lint/line_length.py $(git ls-files '*.[ch]') || rc=1 +./tests/lint/line_length.py $(git ls-files '*.[ch]' | grep -v openssl_hostname_validation) || rc=1 -./tests/lint/astyle.sh $(git ls-files '*.[ch]') || rc=1 +./tests/lint/astyle.sh $(git ls-files '*.[ch]' | grep -v openssl_hostname_validation) || rc=1 exit $rc