diff --git a/NEWS b/NEWS index 51f1f057793efb64ea2a93f204e2d6b662dfaa3d..565b15455e5ae79a617cb8414f02d5bec88f236c 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 d54ddc56dc3a210132686fefad90a572957ebe1d..33f19e7890dee699b669c1eb6b1f36de44751512 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/fuzz/gnutls_x509_parser_fuzzer.c b/fuzz/gnutls_x509_parser_fuzzer.c index 9e50d94e1afb913f9c3cba75491940099a450b66..87b09c4f718627c61434c96ec3e6c86679b949af 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; diff --git a/lib/errors.c b/lib/errors.c index 0ba7106cfa0705ee9f094982de2705662cf00c78..842afe0e246d5295a1cb9a28573cabe5c6102e79 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 d8464c94da80f0fe9fa91d9fd3335e444d4f001d..fb617f99634c81e1f55193fdf3636e50081976eb 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 62e7abe8e9a7721224ec13563df80b063d886666..57c718289474edd97b88725ee4292530ce35aba7 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; } /** @@ -2641,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 diff --git a/tests/cert-tests/Makefile.am b/tests/cert-tests/Makefile.am index 38dd2fb53f5916165dbd0041c3ee6ac8e9a68f4f..861e52d634578827ea7b866dec0dbfbfe834bc03 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 0000000000000000000000000000000000000000..9dcf74c9bbd30758eedb71d71e212b676281ddf9 --- /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 0000000000000000000000000000000000000000..534a534a69af1622a03e7d1fcc0a73de4076a922 --- /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