From af83068ffc2b3533d9195b1f59132551f6027976 Mon Sep 17 00:00:00 2001 From: Nikos Mavrogiannopoulos Date: Sun, 29 Dec 2019 22:33:07 +0100 Subject: [PATCH 1/3] x509: reject certificates having duplicate extensions According to RFC5280 a certificate must not include more than one instance of a particular extension. We were previously printing warnings when such extensions were found, but that is insufficient to flag such certificates. Instead, refuse to import them. Resolves: #887 Signed-off-by: Nikos Mavrogiannopoulos --- NEWS | 5 ++ bootstrap.conf | 2 +- lib/errors.c | 2 + lib/includes/gnutls/gnutls.h.in | 1 + lib/x509/x509.c | 104 +++++++++++++++++++++------- tests/cert-tests/Makefile.am | 4 +- tests/cert-tests/data/dup-exts.pem | 32 +++++++++ tests/cert-tests/x509-duplicate-ext | 46 ++++++++++++ 8 files changed, 168 insertions(+), 28 deletions(-) create mode 100644 tests/cert-tests/data/dup-exts.pem create mode 100755 tests/cert-tests/x509-duplicate-ext diff --git a/NEWS b/NEWS index 51f1f05779..565b15455e 100644 --- a/NEWS +++ b/NEWS @@ -27,6 +27,11 @@ See the end for copying conditions. for all certificate verifications, not only under TLS. The configuration can be overriden using the GNUTLS_SYSTEM_PRIORITY_FILE environment variable. +** libgnutls: Reject certificates which contain duplicate extensions. We were + previously printing warnings when printing such a certificate, but that is + insufficient to flag such certificates as invalid. Instead we now refuse to + import them (#887). + ** libgnutls: If a CA is found in the trusted list, check in addition to time validity, whether the algorithms comply to the expected level prior to accepting it. This addresses the problem of accepting CAs which would diff --git a/bootstrap.conf b/bootstrap.conf index d54ddc56dc..33f19e7890 100644 --- a/bootstrap.conf +++ b/bootstrap.conf @@ -28,7 +28,7 @@ required_submodules="tests/suite/tls-fuzzer/python-ecdsa tests/suite/tls-fuzzer/ # Reproduce by: gnulib-tool --import --local-dir=gl/override --lib=libgnu --source-base=gl --m4-base=gl/m4 --doc-base=doc --tests-base=gl/tests --aux-dir=build-aux --with-tests --avoid=alignof-tests --avoid=lock-tests --avoid=lseek-tests --lgpl=2 --no-conditional-dependencies --libtool --macro-prefix=gl --no-vc-files alloca byteswap c-ctype extensions func gendocs getline gettext-h gettimeofday hash-pjw-bare havelib intprops lib-msvc-compat lib-symbol-versions maintainer-makefile manywarnings memmem-simple minmax netdb netinet_in pmccabe2html read-file secure_getenv snprintf stdint strcase strndup strtok_r strverscmp sys_socket sys_stat time_r unistd vasprintf vsnprintf warnings gnulib_modules=" -alloca byteswap c-ctype c-strcase extensions func gendocs getline gettext-h gettimeofday hash-pjw-bare havelib arpa_inet inet_ntop inet_pton intprops lib-msvc-compat lib-symbol-versions maintainer-makefile manywarnings memmem-simple minmax netdb netinet_in pmccabe2html read-file secure_getenv setsockopt snprintf stdint strcase strdup-posix strndup strtok_r strverscmp sys_socket sys_stat sys_types time_r unistd valgrind-tests vasprintf vsnprintf warnings +alloca byteswap c-ctype c-strcase extensions func gendocs getline gettext-h gettimeofday hash hash-pjw-bare havelib arpa_inet inet_ntop inet_pton intprops lib-msvc-compat lib-symbol-versions maintainer-makefile manywarnings memmem-simple minmax netdb netinet_in pmccabe2html read-file secure_getenv setsockopt snprintf stdint strcase strdup-posix strndup strtok_r strverscmp sys_socket sys_stat sys_types time_r unistd valgrind-tests vasprintf vsnprintf warnings " unistring_modules=" diff --git a/lib/errors.c b/lib/errors.c index 0ba7106cfa..842afe0e24 100644 --- a/lib/errors.c +++ b/lib/errors.c @@ -186,6 +186,8 @@ static const gnutls_error_entry error_entries[] = { GNUTLS_E_X509_UNSUPPORTED_CRITICAL_EXTENSION), ERROR_ENTRY(N_("Unsupported extension in X.509 certificate."), GNUTLS_E_X509_UNSUPPORTED_EXTENSION), + ERROR_ENTRY(N_("Duplicate extension in X.509 certificate."), + GNUTLS_E_X509_DUPLICATE_EXTENSION), ERROR_ENTRY(N_ ("Key usage violation in certificate has been detected."), GNUTLS_E_KEY_USAGE_VIOLATION), diff --git a/lib/includes/gnutls/gnutls.h.in b/lib/includes/gnutls/gnutls.h.in index d8464c94da..fb617f9963 100644 --- a/lib/includes/gnutls/gnutls.h.in +++ b/lib/includes/gnutls/gnutls.h.in @@ -3386,6 +3386,7 @@ void gnutls_fips140_set_mode(gnutls_fips_mode_t mode, unsigned flags); #define GNUTLS_E_MISSING_EXTENSION -427 #define GNUTLS_E_DB_ENTRY_EXISTS -428 #define GNUTLS_E_EARLY_DATA_REJECTED -429 +#define GNUTLS_E_X509_DUPLICATE_EXTENSION -430 #define GNUTLS_E_UNIMPLEMENTED_FEATURE -1250 diff --git a/lib/x509/x509.c b/lib/x509/x509.c index 62e7abe8e9..bf3e1e8f16 100644 --- a/lib/x509/x509.c +++ b/lib/x509/x509.c @@ -38,6 +38,8 @@ #include #include "urls.h" #include "system-keys.h" +#include "hash.h" +#include "hash-pjw-bare.h" static int crt_reinit(gnutls_x509_crt_t crt) { @@ -404,41 +406,94 @@ static int cache_alt_names(gnutls_x509_crt_t cert) return 0; } +static bool hcomparator(const void *v1, const void *v2) +{ + return (strcmp(v1, v2)==0); +} + +static size_t hhasher(const void *entry, size_t n) +{ + const char *e = entry; + if (e == NULL || e[0] == 0) + return 0; + + return hash_pjw_bare(e, strlen(e)) % n; +} + int _gnutls_check_cert_sanity(gnutls_x509_crt_t cert) { - int result = 0, version; + int ret = 0, version; gnutls_datum_t exts; + Hash_table *htable = NULL; if (cert->flags & GNUTLS_X509_CRT_FLAG_IGNORE_SANITY) return 0; /* enforce the rule that only version 3 certificates carry extensions */ - result = gnutls_x509_crt_get_version(cert); - if (result < 0) { - gnutls_assert(); - goto cleanup; + ret = gnutls_x509_crt_get_version(cert); + if (ret < 0) { + return gnutls_assert_val(ret); } - version = result; + version = ret; if (version < 3) { if (!cert->modified) { - result = _gnutls_x509_get_raw_field2(cert->cert, &cert->der, + ret = _gnutls_x509_get_raw_field2(cert->cert, &cert->der, "tbsCertificate.extensions", &exts); - if (result >= 0 && exts.size > 0) { - gnutls_assert(); + if (ret >= 0 && exts.size > 0) { _gnutls_debug_log("error: extensions present in certificate with version %d\n", version); - result = GNUTLS_E_X509_CERTIFICATE_ERROR; - goto cleanup; + return gnutls_assert_val(GNUTLS_E_X509_CERTIFICATE_ERROR); } } else { if (cert->use_extensions) { - gnutls_assert(); _gnutls_debug_log("error: extensions set in certificate with version %d\n", version); - result = GNUTLS_E_X509_CERTIFICATE_ERROR; + return gnutls_assert_val(GNUTLS_E_X509_CERTIFICATE_ERROR); + } + } + } else { + /* Version is >= 3; ensure no duplicate extensions are + * present. */ + unsigned i; + char oid[MAX_OID_SIZE]; + size_t oid_size; + char *o; + + htable = hash_initialize(16, NULL, hhasher, hcomparator, gnutls_free); + if (htable == NULL) + return gnutls_assert_val(GNUTLS_E_MEMORY_ERROR); + + for (i=0;;i++) { + oid_size = sizeof(oid); + ret = gnutls_x509_crt_get_extension_info(cert, i, oid, &oid_size, NULL); + if (ret < 0) { + if (ret == GNUTLS_E_REQUESTED_DATA_NOT_AVAILABLE) + break; + gnutls_assert(); + goto cleanup; + } + o = gnutls_strdup(oid); + if (o == NULL) { + ret = gnutls_assert_val(GNUTLS_E_MEMORY_ERROR); + goto cleanup; + } + + ret = hash_insert_if_absent(htable, o, NULL); + if (ret == -1) { + gnutls_free(o); + ret = gnutls_assert_val(GNUTLS_E_MEMORY_ERROR); + goto cleanup; + } else if (ret == 0) { + /* duplicate */ + gnutls_free(o); + _gnutls_debug_log("error: duplicate extension (%s) detected\n", oid); + ret = gnutls_assert_val(GNUTLS_E_X509_DUPLICATE_EXTENSION); goto cleanup; } } + + hash_free(htable); + htable = NULL; } if (version < 2) { @@ -446,36 +501,35 @@ int _gnutls_check_cert_sanity(gnutls_x509_crt_t cert) size_t id_size; id_size = sizeof(id); - result = gnutls_x509_crt_get_subject_unique_id(cert, id, &id_size); - if (result >= 0 || result == GNUTLS_E_SHORT_MEMORY_BUFFER) { - gnutls_assert(); + ret = gnutls_x509_crt_get_subject_unique_id(cert, id, &id_size); + if (ret >= 0 || ret == GNUTLS_E_SHORT_MEMORY_BUFFER) { _gnutls_debug_log("error: subjectUniqueID present in certificate with version %d\n", version); - result = GNUTLS_E_X509_CERTIFICATE_ERROR; + ret = gnutls_assert_val(GNUTLS_E_X509_CERTIFICATE_ERROR); goto cleanup; } id_size = sizeof(id); - result = gnutls_x509_crt_get_issuer_unique_id(cert, id, &id_size); - if (result >= 0 || result == GNUTLS_E_SHORT_MEMORY_BUFFER) { - gnutls_assert(); + ret = gnutls_x509_crt_get_issuer_unique_id(cert, id, &id_size); + if (ret >= 0 || ret == GNUTLS_E_SHORT_MEMORY_BUFFER) { _gnutls_debug_log("error: subjectUniqueID present in certificate with version %d\n", version); - result = GNUTLS_E_X509_CERTIFICATE_ERROR; + ret = gnutls_assert_val(GNUTLS_E_X509_CERTIFICATE_ERROR); goto cleanup; } } if (gnutls_x509_crt_get_expiration_time(cert) == -1 || gnutls_x509_crt_get_activation_time(cert) == -1) { - gnutls_assert(); _gnutls_debug_log("error: invalid expiration or activation time in certificate\n"); - result = GNUTLS_E_CERTIFICATE_TIME_ERROR; + ret = gnutls_assert_val(GNUTLS_E_CERTIFICATE_TIME_ERROR); goto cleanup; } - result = 0; + ret = 0; cleanup: - return result; + if (htable) + hash_free(htable); + return ret; } /** diff --git a/tests/cert-tests/Makefile.am b/tests/cert-tests/Makefile.am index 38dd2fb53f..861e52d634 100644 --- a/tests/cert-tests/Makefile.am +++ b/tests/cert-tests/Makefile.am @@ -82,7 +82,7 @@ EXTRA_DIST = data/ca-no-pathlen.pem data/no-ca-or-pathlen.pem data/aki-cert.pem data/ca-gnutls-keyid.pem data/ca-no-keyid.pem data/ca-weird-keyid.pem \ data/key-ca-1234.p8 data/key-ca-empty.p8 data/key-ca-null.p8 \ data/openssl-key-ecc.p8 data/key-ecc.p8 data/key-ecc.pem suppressions.valgrind \ - data/encpkcs8.pem data/unencpkcs8.pem data/enc2pkcs8.pem \ + data/encpkcs8.pem data/unencpkcs8.pem data/enc2pkcs8.pem data/dup-exts.pem \ data/openssl-3des.p8 data/openssl-3des.p8.txt data/openssl-aes128.p8 \ data/openssl-aes128.p8.txt data/openssl-aes256.p8 data/openssl-aes256.p8.txt \ data/cert.dsa.1024.pem data/cert.dsa.2048.pem data/cert.dsa.3072.pem \ @@ -111,7 +111,7 @@ dist_check_SCRIPTS = pathlen aki invalid-sig email \ pkcs12 certtool-crl-decoding pkcs12-encode pkcs12-corner-cases inhibit-anypolicy \ smime cert-time alt-chain pkcs7-list-sign pkcs7-eddsa certtool-ecdsa \ key-id pkcs8 pkcs8-decode ecdsa illegal-rsa pkcs8-invalid key-invalid \ - pkcs8-eddsa certtool-subca certtool-verify-profiles + pkcs8-eddsa certtool-subca certtool-verify-profiles x509-duplicate-ext dist_check_SCRIPTS += key-id ecdsa pkcs8-invalid key-invalid pkcs8-decode pkcs8 pkcs8-eddsa \ certtool-utf8 crq diff --git a/tests/cert-tests/data/dup-exts.pem b/tests/cert-tests/data/dup-exts.pem new file mode 100644 index 0000000000..9dcf74c9bb --- /dev/null +++ b/tests/cert-tests/data/dup-exts.pem @@ -0,0 +1,32 @@ +-----BEGIN CERTIFICATE----- +MIIFiDCCA3CgAwIBAgIRAPABuQ6DmexEq0k9QQaewLcwDQYJKoZIhvcNAQELBQAw +bzELMAkGA1UEBhMCQ04xDDAKBgNVBAgMA1RKNTEMMAoGA1UECgwDVEpVMRQwEgYD +VQQLDAtiZWl5YW5neXVhbjENMAsGA1UEAwwEYjMyNjEfMB0GCSqGSIb3DQEJARYQ +bGpmcG93ZXJAMTYzLmNvbTAeFw0xOTA1MjkxMTU2NDBaFw0yOTA0MDYxMTU2NDBa +MHsxCzAJBgNVBAYTAkNOMQwwCgYDVQQIDANUSjUxCzAJBgNVBAcMAlRKMQwwCgYD +VQQKDANUSlUxFDASBgNVBAsMC2JlaXlhbmd5dWFuMQwwCgYDVQQDDANMUUwxHzAd +BgkqhkiG9w0BCQEWEGxqZnBvd2VyQDE2My5jb20wggEiMA0GCSqGSIb3DQEBAQUA +A4IBDwAwggEKAoIBAQDNKbU4xRcAGOyzHWgEQw0/smt+BJaLtbIvKdPKPTDzDxSl +Rud0rf1GWzG5vKhEzn3ruNwFs23JTu4OcXlkqp4sGqC5SQ06qVhe+eWhK+pjsCll +AG9ZQ40kNdsE5Bt9gbl38tdykM/a5bU4+h8S9P5XP+Vr/xGuB1aqw07NqaUsOs3+ +McH/ZFZQgSv8NDXl9eok5XEfaDZoRf29nAH/I+Ottbw37oW7omvMaC39CVKKmYMA +rdRJR/JrICsOKKnmEf6oLNErBGs3TLXo9/CiQJz/KeV9mHT/BfPumAbSlIXo6en8 +AVyA0V+N1bwUiBu58m9B+z0GlaxeQlxSvTn2wUx5AgMBAAGjggERMIIBDTAJBgNV +HRMEAjAAMB0GA1UdDgQWBBR/7mRMJ+8WoDdxiWO1eCLw0xH+0DAdBgNVHQ4EFgQU +f+5kTCfvFqA3cYljtXgi8NMR/tAwHwYDVR0jBBgwFoAU7S2I/yNy3nSqhHIFpnM6 +2/XWHHgwDgYDVR0PAQH/BAQDAgWgMB0GA1UdJQQWMBQGCCsGAQUFBwMBBggrBgEF +BQcDBDAYBgNVHREEETAPgQ1oaEBiYi5hZGRyZXNzMBgGA1UdEgQRMA+CDWFiY2V3 +d3J3dC5jb20wMAYIKwYBBQUHAQEEJDAiMCAGCCsGAQUFBzAChhRodHRwOi8vbXku +Y2EvY2EuaHRtbDAMBgNVHSQEBTADgAEDMA0GCSqGSIb3DQEBCwUAA4ICAQCopPaM +SMElD42TZYn1+SnACRnH4YWH/gfG3utPeGVPkBmvV5Je7/gNMlhAQJL5YKdDYa4o +S1zjkNrRSlamH6akX4KyOm19tKRkU7dvtcTRF5CwXGcE2Yte6hc1gWeGzsx5taZL +y2yan7jhCHMqtN5R8AMTDdK4ORPu+sSrghAwkS6KSR0VlVmgbrJQ0WAxRk5bKm7v +R402pLhH2MjsJV48XqvaRTjyT96nbAZ4tdSoyJoHXRvUv9QpFtHSddlnPbEgxJWT +3OLbr+kIpWuaaZNjntLOqe9aPkLEhpw07sGLpT23dYqdehZd12O5+3olULXVBOgg +h8uF4Q9kRtJDpLCd70hUoiyovCxgPbFYUjvmtpCtmNkSCq/txWc3YqOwR+HPe83j +aAsIDnEO6cY6M3uqM1xradU5jzDeMKHJV7XDdXsq9nyQoZ8ytKlKcgM5kNoaqAkT +zeutyjGtQCkJr5V+5Te0JJinVL+xafpwP6749VRUaEWHWk2crkTKxu7/lUK6lgnS +70gLDO1QEJ/edPDC143eRP+dF/d7bN2UF1l+G0F4AcW7kB5mKgOBIWTZSnTmByz5 ++HI1touSh9dDcDDuZ7z6k2Obl0fuPY7ROLZQT3BaYGU4M2FGT4sJa6P6VtfufzEB +MHcS14u+3EvHBxhcI8N4WTrBE36FBzPk6R0g+A== +-----END CERTIFICATE----- diff --git a/tests/cert-tests/x509-duplicate-ext b/tests/cert-tests/x509-duplicate-ext new file mode 100755 index 0000000000..534a534a69 --- /dev/null +++ b/tests/cert-tests/x509-duplicate-ext @@ -0,0 +1,46 @@ +#!/bin/sh + +# Copyright (C) 2019 Nikos Mavrogiannopoulos +# +# This file is part of GnuTLS. +# +# GnuTLS is free software; you can redistribute it and/or modify it +# under the terms of the GNU General Public License as published by the +# Free Software Foundation; either version 3 of the License, or (at +# your option) any later version. +# +# GnuTLS is distributed in the hope that it will be useful, but +# WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +# General Public License for more details. +# +# You should have received a copy of the GNU Lesser General Public License +# along with this program. If not, see + +srcdir="${srcdir:-.}" +CERTTOOL="${CERTTOOL:-../../src/certtool${EXEEXT}}" +OUTFILE=out.$$.tmp + +if ! test -x "${CERTTOOL}"; then + exit 77 +fi + +"${CERTTOOL}" --certificate-info --infile "${srcdir}/data/dup-exts.pem" >${OUTFILE} 2>&1 +RET=$? +if test ${RET} = 0; then + echo "Successfully loaded a certificate with duplicate extensions" + cat ${OUTFILE} + exit 1 +fi + +grep "Duplicate extension in" ${OUTFILE} 2>/dev/null +if test $? != 0; then + echo "Could not find the expected error value" + cat ${OUTFILE} + exit 1 +fi + + +rm -f ${OUTFILE} + +exit 0 -- GitLab From 91e811383c4b1849852ebcfc14fde382b083a779 Mon Sep 17 00:00:00 2001 From: Nikos Mavrogiannopoulos Date: Mon, 30 Dec 2019 05:35:45 +0100 Subject: [PATCH 2/3] fuzz: import certificate with and without sanity checks Signed-off-by: Nikos Mavrogiannopoulos --- fuzz/gnutls_x509_parser_fuzzer.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/fuzz/gnutls_x509_parser_fuzzer.c b/fuzz/gnutls_x509_parser_fuzzer.c index 9e50d94e1a..87b09c4f71 100644 --- a/fuzz/gnutls_x509_parser_fuzzer.c +++ b/fuzz/gnutls_x509_parser_fuzzer.c @@ -43,6 +43,13 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) assert(ret >= 0); gnutls_free(out.data); } + gnutls_x509_crt_deinit(crt); + + ret = gnutls_x509_crt_init(&crt); + assert(ret >= 0); + + gnutls_x509_crt_set_flags(crt, GNUTLS_X509_CRT_FLAG_IGNORE_SANITY); + gnutls_x509_crt_import(crt, &raw, GNUTLS_X509_FMT_DER); gnutls_x509_crt_deinit(crt); return 0; -- GitLab From dad163998fb38a27197b43bdd3e805f4a6817251 Mon Sep 17 00:00:00 2001 From: Nikos Mavrogiannopoulos Date: Sun, 29 Dec 2019 21:53:32 +0100 Subject: [PATCH 3/3] gnutls_x509_crt_get_extension_info: optimize when critical equals NULL That is, do not perform the look ups necessary to calculate the value when it will not be used. Signed-off-by: Nikos Mavrogiannopoulos --- lib/x509/x509.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/lib/x509/x509.c b/lib/x509/x509.c index bf3e1e8f16..57c7182894 100644 --- a/lib/x509/x509.c +++ b/lib/x509/x509.c @@ -2695,16 +2695,16 @@ gnutls_x509_crt_get_extension_info(gnutls_x509_crt_t cert, unsigned indx, if (oid && len > 0 && ((uint8_t*)oid)[len-1] == 0) (*oid_size)--; - snprintf(name, sizeof(name), - "tbsCertificate.extensions.?%u.critical", indx + 1); - len = sizeof(str_critical); - result = asn1_read_value(cert->cert, name, str_critical, &len); - if (result != ASN1_SUCCESS) { - gnutls_assert(); - return _gnutls_asn2err(result); - } - if (critical) { + snprintf(name, sizeof(name), + "tbsCertificate.extensions.?%u.critical", indx + 1); + len = sizeof(str_critical); + result = asn1_read_value(cert->cert, name, str_critical, &len); + if (result != ASN1_SUCCESS) { + gnutls_assert(); + return _gnutls_asn2err(result); + } + if (str_critical[0] == 'T') *critical = 1; else -- GitLab