From: Arran Cudbard-Bell Date: Fri, 8 Oct 2021 19:31:35 +0000 (-0500) Subject: Always use a separate vertificate store, and print out the certs in it when we fail... X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=fd9a1060fbba03a720fcf259c617dd0e8d5ec94b;p=thirdparty%2Ffreeradius-server.git Always use a separate vertificate store, and print out the certs in it when we fail to validate a client cert --- diff --git a/src/lib/tls/ctx.c b/src/lib/tls/ctx.c index 3ca02f0d89f..9978cd301ef 100644 --- a/src/lib/tls/ctx.c +++ b/src/lib/tls/ctx.c @@ -38,6 +38,7 @@ USES_APPLE_DEPRECATED_API /* OpenSSL API has been deprecated by Apple */ #include #include +#include #if OPENSSL_VERSION_NUMBER >= 0x30000000L # include #endif @@ -240,7 +241,7 @@ static int tls_ctx_verify_chain_member(fr_unix_time_t *expires_first, X509 **sel return 0; } -static int tls_ctx_load_cert_chain(SSL_CTX *ctx, fr_tls_chain_conf_t *chain) +static int tls_ctx_load_cert_chain(SSL_CTX *ctx, fr_tls_chain_conf_t *chain, bool allow_multi_self_signed) { char *password; @@ -371,6 +372,8 @@ static int tls_ctx_load_cert_chain(SSL_CTX *ctx, fr_tls_chain_conf_t *chain) return -1; } + if (allow_multi_self_signed) self_signed = NULL; + DIAG_OFF(DIAG_UNKNOWN_PRAGMAS) DIAG_OFF(used-but-marked-unused) /* fix spurious warnings for sk macros */ for (i = sk_X509_num(our_chain); i > 0 ; i--) { @@ -382,6 +385,8 @@ DIAG_OFF(used-but-marked-unused) /* fix spurious warnings for sk macros */ if (tls_ctx_verify_chain_member(&expires_first, &self_signed, ctx, sk_X509_value(our_chain, i - 1), chain->verify_mode) < 0) return -1; + + if (allow_multi_self_signed) self_signed = NULL; } DIAG_ON(used-but-marked-unused) /* fix spurious warnings for sk macros */ DIAG_ON(DIAG_UNKNOWN_PRAGMAS) @@ -591,16 +596,10 @@ SSL_CTX *fr_tls_ctx_alloc(fr_tls_conf_t const *conf, bool client) { SSL_CTX *ctx; X509_STORE *cert_vpstore; - X509_STORE *chain_store; - X509_STORE *verify_store; + X509_STORE *verify_store; int verify_mode = SSL_VERIFY_NONE; int ctx_options = 0; - /* - * This addresses memory leaks in OpenSSL 1.0.2 - * at the cost of the server occasionally - * crashing on exit. - */ ctx = SSL_CTX_new(SSLv23_method()); if (!ctx) { fr_tls_log_error(NULL, "Failed creating TLS context"); @@ -732,35 +731,54 @@ SSL_CTX *fr_tls_ctx_alloc(fr_tls_conf_t const *conf, bool client) } /* - * If we're using a sufficiently new version of - * OpenSSL, initialise different stores for creating - * the certificate chains we present, and for - * holding certificates to verify the chain presented - * by the peer. - * - * If we don't do this, a single store is used for - * both functions, which is confusing and annoying. + * Initialise a separate store for verifying user + * certificates. * - * We use the set0 variant so that the stores are - * freed at the same time as the SSL_CTX. + * This makes the configuration cleaner as there's + * no mixing of chain certs and user certs. */ - if (!conf->auto_chain) { - MEM(chain_store = X509_STORE_new()); - SSL_CTX_set0_chain_cert_store(ctx, chain_store); + MEM(verify_store = X509_STORE_new()); - MEM(verify_store = X509_STORE_new()); - SSL_CTX_set0_verify_cert_store(ctx, verify_store); - } + /* Sets OpenSSL's (CERT *)->verify_store, overring (SSL_CTX *)->cert_store */ + SSL_CTX_set0_verify_cert_store(ctx, verify_store); + + /* This isn't accessible to use later, i.e. there's no SSL_CTX_get0_verify_cert_store */ + SSL_CTX_set_ex_data(ctx, FR_TLS_EX_CTX_INDEX_VERIFY_STORE, verify_store); /* * Load the CAs we trust */ if (conf->ca_file || conf->ca_path) { - if (!SSL_CTX_load_verify_locations(ctx, conf->ca_file, conf->ca_path)) { + /* + * This adds all the certificates to the store for conf->ca_file + * and adds a dynamic lookup for conf->ca_path. + * + * It's also possible to add extra virtual server lookups + */ + if (!X509_STORE_load_locations(verify_store, conf->ca_file, conf->ca_path)) { fr_tls_log_error(NULL, "Failed reading Trusted root CA list \"%s\"", conf->ca_file); goto error; } + + /* + * These set the default parameters of the store when the + * store is involved in building chains. + * + * - X509_PURPOSE_SSL_CLIENT ensure the purpose of the + * client certificate is for peer authentication as + * a client. + */ + X509_STORE_set_purpose(verify_store, X509_PURPOSE_SSL_CLIENT); + + /* + * Sets the list of CAs we send to the peer if we're + * requesting a certificate. + * + * This does not change the trusted certificate authorities, + * those are set above with SSL_CTX_load_verify_locations. + */ + if (conf->ca_file) SSL_CTX_set_client_CA_list(ctx, SSL_load_client_CA_file(conf->ca_file)); } /* @@ -782,7 +800,7 @@ SSL_CTX *fr_tls_ctx_alloc(fr_tls_conf_t const *conf, bool client) size_t i; for (i = 0; i < chains_conf; i++) { - if (tls_ctx_load_cert_chain(ctx, conf->chains[i]) < 0) goto error; + if (tls_ctx_load_cert_chain(ctx, conf->chains[i], false) < 0) goto error; } } @@ -843,15 +861,6 @@ SSL_CTX *fr_tls_ctx_alloc(fr_tls_conf_t const *conf, bool client) } } - /* - * Sets the list of CAs we send to the peer if we're - * requesting a certificate. - * - * This does not change the trusted certificate authorities, - * those are set above with SSL_CTX_load_verify_locations. - */ - if (conf->ca_file && *conf->ca_file) SSL_CTX_set_client_CA_list(ctx, SSL_load_client_CA_file(conf->ca_file)); - #ifdef PSK_MAX_IDENTITY_LEN post_ca: #endif diff --git a/src/lib/tls/index.h b/src/lib/tls/index.h index 9b22e7bd774..1c768219328 100644 --- a/src/lib/tls/index.h +++ b/src/lib/tls/index.h @@ -29,14 +29,15 @@ RCSIDH(index_h, "$Id$") extern "C" { #endif -#define FR_TLS_EX_INDEX_EAP_SESSION (10) -#define FR_TLS_EX_INDEX_CONF (11) -#define FR_TLS_EX_INDEX_REQUEST (12) -#define FR_TLS_EX_INDEX_IDENTITY (13) -#define FR_TLS_EX_INDEX_STORE (14) -#define FR_TLS_EX_INDEX_TLS_SESSION (15) -#define FR_TLS_EX_INDEX_TALLOC (16) +#define FR_TLS_EX_INDEX_EAP_SESSION (10) +#define FR_TLS_EX_INDEX_CONF (11) +#define FR_TLS_EX_INDEX_REQUEST (12) +#define FR_TLS_EX_INDEX_IDENTITY (13) +#define FR_TLS_EX_INDEX_OCSP_STORE (14) +#define FR_TLS_EX_INDEX_TLS_SESSION (16) +#define FR_TLS_EX_INDEX_TALLOC (17) +#define FR_TLS_EX_CTX_INDEX_VERIFY_STORE (20) #ifdef __cplusplus } #endif diff --git a/src/lib/tls/log.c b/src/lib/tls/log.c index 4b82ae2eb94..9a8049035a9 100644 --- a/src/lib/tls/log.c +++ b/src/lib/tls/log.c @@ -92,7 +92,6 @@ static void _tls_ctx_print_cert_line(char const *file, int line, } } - static void _tls_ctx_print_cert_line_marker(char const *file, int line, request_t *request, fr_log_type_t log_type, int idx, X509 *cert, bool marker) @@ -113,6 +112,23 @@ static void _tls_ctx_print_cert_line_marker(char const *file, int line, } } +static void _tls_ctx_print_cert_line_no_idx(char const *file, int line, + request_t *request, fr_log_type_t log_type, X509 *cert) +{ + char subject[1024]; + + X509_NAME_oneline(X509_get_subject_name(cert), subject, sizeof(subject)); + subject[sizeof(subject) - 1] = '\0'; + + if (request) { + log_request(log_type, fr_debug_lvl, request, file, line, + "%s %s", fr_tls_utils_x509_pkey_type(cert), subject); + } else { + fr_log(LOG_DST, log_type, file, line, + "%s %s", fr_tls_utils_x509_pkey_type(cert), subject); + } +} + DIAG_OFF(DIAG_UNKNOWN_PRAGMAS) DIAG_OFF(used-but-marked-unused) /* fix spurious warnings for sk macros */ /** Print out the current stack of certs @@ -132,7 +148,7 @@ void _fr_tls_log_certificate_chain(char const *file, int line, for (i = sk_X509_num(chain); i > 0 ; i--) { _tls_ctx_print_cert_line(file, line, request, log_type, i, sk_X509_value(chain, i - 1)); } - _tls_ctx_print_cert_line(file, line, request, log_type, i, cert); + if (cert) _tls_ctx_print_cert_line(file, line, request, log_type, i, cert); } /** Print out the current stack of certs @@ -155,7 +171,38 @@ void _fr_tls_log_certificate_chain_marker(char const *file, int line, X509 *selected = sk_X509_value(chain, i - 1); _tls_ctx_print_cert_line_marker(file, line, request, log_type, i, selected, (selected == marker)); } - _tls_ctx_print_cert_line_marker(file, line, request, log_type, i, cert, (cert == marker)); + if (cert) _tls_ctx_print_cert_line_marker(file, line, request, log_type, i, cert, (cert == marker)); +} + +/** Print out the current stack of X509 objects (certificates only) + * + * @param[in] file File where this function is being called. + * @param[in] line Line where this function is being called. + * @param[in] request Current request, may be NULL. + * @param[in] log_type The type of log message to produce L_INFO, L_ERR, L_DBG etc... + * @param[in] objects A stack of X509 objects + */ +void _fr_tls_log_x509_objects(char const *file, int line, + request_t *request, fr_log_type_t log_type, + STACK_OF(X509_OBJECT) *objects) +{ + int i; + + for (i = sk_X509_OBJECT_num(objects); i > 0 ; i--) { + X509_OBJECT *obj = sk_X509_OBJECT_value(objects, i - 1); + + switch (X509_OBJECT_get_type(obj)) { + case X509_LU_X509: /* X509 certificate */ + _tls_ctx_print_cert_line_no_idx(file, line, request, log_type, X509_OBJECT_get0_X509(obj)); + break; + + case X509_LU_CRL: /* Certificate revocation list */ + continue; + + default: + continue; + } + } } DIAG_ON(used-but-marked-unused) DIAG_ON(DIAG_UNKNOWN_PRAGMAS) diff --git a/src/lib/tls/log.h b/src/lib/tls/log.h index 8a0f540ee50..9ba21018960 100644 --- a/src/lib/tls/log.h +++ b/src/lib/tls/log.h @@ -34,17 +34,22 @@ RCSIDH(tls_log_h, "$Id$") #include "base.h" -#define fr_tls_log_certificate_chain(_request, _log_type, _chain, _leaf) \ - _fr_tls_log_certificate_chain( __FILE__, __LINE__, _request, _log_type, _chain, _leaf) +#define fr_tls_log_certificate_chain(...) \ + _fr_tls_log_certificate_chain( __FILE__, __LINE__, ## __VA_ARGS__) void _fr_tls_log_certificate_chain(char const *file, int line, request_t *request, fr_log_type_t log_type, STACK_OF(X509) *chain, X509 *leaf); +#define fr_tls_log_certificate_chain_marker(...) \ + _fr_tls_log_certificate_chain_marker( __FILE__, __LINE__, ## __VA_ARGS__) void _fr_tls_log_certificate_chain_marker(char const *file, int line, request_t *request, fr_log_type_t log_type, STACK_OF(X509) *chain, X509 *leaf, X509 *marker); -#define fr_tls_log_certificate_chain_marker(_request, _log_type, _chain, _leaf, _marker) \ - _fr_tls_log_certificate_chain_marker( __FILE__, __LINE__, _request, _log_type, _chain, _leaf, _marker) +#define fr_tls_log_x509_objects(...) \ + _fr_tls_log_x509_objects( __FILE__, __LINE__, ## __VA_ARGS__) +void _fr_tls_log_x509_objects(char const *file, int line, + request_t *request, fr_log_type_t log_type, + STACK_OF(X509_OBJECT) *objects); int fr_tls_log_io_error(request_t *request, int err, char const *msg, ...) CC_HINT(format (printf, 3, 4)); diff --git a/src/lib/tls/verify.c b/src/lib/tls/verify.c index 81272784531..bc93b5b78d8 100644 --- a/src/lib/tls/verify.c +++ b/src/lib/tls/verify.c @@ -30,6 +30,7 @@ #include #include +#include #include #include #include @@ -65,6 +66,41 @@ bool verify_applies(fr_tls_verify_mode_t mode, int depth, int untrusted) DIAG_OFF(DIAG_UNKNOWN_PRAGMAS) DIAG_OFF(used-but-marked-unused) /* fix spurious warnings for sk macros */ + +/** Print verbose humanly readable messages about why certificate validation failed + * + */ +static void tls_verify_error_detail(request_t *request, SSL_CTX *ctx, int err) +{ + X509_STORE *store = SSL_CTX_get_ex_data(ctx, FR_TLS_EX_CTX_INDEX_VERIFY_STORE); + + switch (err) { + /* + * We linked the provided cert to at least one + * other in a chain, but the chain doesn't terminate + * in a root CA. + */ + case X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT: + FALL_THROUGH; + + /* + * We failed to link the provided cert to any + * other local certificates in the chain. + */ + case X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY: + RDEBUG2("Static certificates in verification store are"); + if (RDEBUG_ENABLED2) { + RINDENT(); + fr_tls_log_x509_objects(request, L_DBG, X509_STORE_get0_objects(store)); + REXDENT(); + } + break; + + default: + break; + } +} + /** Validates a certificate using custom logic * * Before trusting a certificate, we make sure that the certificate is @@ -94,6 +130,8 @@ DIAG_OFF(used-but-marked-unused) /* fix spurious warnings for sk macros */ int fr_tls_verify_cert_cb(int ok, X509_STORE_CTX *x509_ctx) { X509 *cert; + + SSL_CTX *ssl_ctx; SSL *ssl; fr_tls_session_t *tls_session; int err, depth; @@ -114,6 +152,7 @@ int fr_tls_verify_cert_cb(int ok, X509_STORE_CTX *x509_ctx) * and the application specific data stored into the SSL object. */ ssl = X509_STORE_CTX_get_ex_data(x509_ctx, SSL_get_ex_data_X509_STORE_CTX_idx()); + ssl_ctx = SSL_get_SSL_CTX(ssl); conf = fr_tls_session_conf(ssl); tls_session = talloc_get_type_abort(SSL_get_ex_data(ssl, FR_TLS_EX_INDEX_TLS_SESSION), fr_tls_session_t); request = fr_tls_session_request(tls_session->ssl); @@ -159,10 +198,13 @@ int fr_tls_verify_cert_cb(int ok, X509_STORE_CTX *x509_ctx) if (!verify_applies(conf->verify.mode, depth, untrusted) || ((conf->verify.allow_expired_crl) && (err == X509_V_ERR_CRL_HAS_EXPIRED))) { RDEBUG2("Ignoring verification error - %s (%i)", p, err); + tls_verify_error_detail(request, ssl_ctx, err); + my_ok = 1; X509_STORE_CTX_set_error(x509_ctx, 0); } else { RERROR("Verification error - %s (%i)", p, err); + tls_verify_error_detail(request, ssl_ctx, err); goto done; } } @@ -283,6 +325,7 @@ int fr_tls_verify_cert_chain(request_t *request, SSL *ssl) int verify; int ret = 1; + SSL_CTX *ssl_ctx; STACK_OF(X509) *chain; X509 *cert; X509_STORE *store; @@ -298,10 +341,27 @@ int fr_tls_verify_cert_chain(request_t *request, SSL *ssl) #endif if (!cert) return 1; + ssl_ctx = SSL_get_SSL_CTX(ssl); store_ctx = X509_STORE_CTX_new(); chain = SSL_get_peer_cert_chain(ssl); /* Does not increase ref count */ - store = SSL_CTX_get_cert_store(SSL_get_SSL_CTX(ssl)); /* Does not increase ref count */ + store = SSL_CTX_get_ex_data(ssl_ctx, FR_TLS_EX_CTX_INDEX_VERIFY_STORE); /* Gets the verification store */ + /* + * This sets up a store_ctx for doing peer certificate verification. + * + * store_ctx - Is the ctx to initialise + * store - Is an X509_STORE of implicitly + * trusted certificates. Here we're using + * the verify store that was created when we + * allocated the SSL_CTX. + * cert - Is the certificate to validate. + * chain - Is any other certificates the peer provided + * us in order to build a chain from a trusted + * root or intermediary to its leaf (cert). + * + * Note: SSL_CTX_get_cert_store() returns the ctx->cert_store, which + * is not the same as the verification cert store. + */ X509_STORE_CTX_init(store_ctx, store, cert, chain); X509_STORE_CTX_set_ex_data(store_ctx, SSL_get_ex_data_X509_STORE_CTX_idx(), ssl); X509_STORE_CTX_set_verify_cb(store_ctx, fr_tls_verify_cert_cb); diff --git a/src/modules/rlm_ocsp/conf.c b/src/modules/rlm_ocsp/conf.c index 6970b680810..e531cb7d191 100644 --- a/src/modules/rlm_ocsp/conf.c +++ b/src/modules/rlm_ocsp/conf.c @@ -90,7 +90,7 @@ static int _conf_server_free( /* Session init */ #ifdef HAVE_OPENSSL_OCSP_H - SSL_set_ex_data(tls_session->ssl, FR_TLS_EX_INDEX_STORE, (void *)tls_conf->ocsp.store); + SSL_set_ex_data(tls_session->ssl, FR_TLS_EX_INDEX_OCSP_STORE, (void *)tls_conf->ocsp.store); #endif /* Validation checks */