On Sat, Dec 25, 2021 at 05:48:53PM +0100, Claudio Jeker wrote: [...] > I would love to get rid of X509_V_FLAG_IGNORE_CRITICAL and use a callback > to ensure the right extensions are critical but I never managed to > understand how the X509_verify_cert() callback actually works. > Documentation seems to be non-existent.
My understanding is that X509_V_FLAG_IGNORE_CRITICAL is a big hammer that was necessary as long as libcrypto didn't know about the RFC 3779 extensions. Without it X509_verify_cert() would error on encountering them. This changed shortly after Gouveia when we enabled the RFC 3779 code in libcrypto. I think setting the flag is no longer appropriate in -current. Below is a diff that removes X509_V_FLAG_IGNORE_CRITICAL and adds a debug verify callback that intercepts unhandled critical extensions. It lets through the RFC 3779 extensions and errors on everything else. If you run with this on -current, the callback doesn't print anything. On -stable you'll see that it logs at least one of the two extensions per cert while X509_verify_cert() is walking through the chains. If we want to drop X509_V_FLAG_IGNORE_CRITICAL, something like this will be needed as long as rpki-client wants to support LibreSSL < 3.5. The idea is that we handle RFC 3779 extensions ourselves, so we can safely ignore them in the verifier. All other critical extensions that neither rpki-client nor libcrypto know about should be verification failures. You can play with this on -current by disabling RFC 3779 again: remove or comment the #ifndef LIBRESSL_CRYPTO_INTERNAL around OPENSSL_NO_RFC3779 in lib/libcrypto/opensslfeatures.h then make clean, make includes, make and make install. Index: parser.c =================================================================== RCS file: /cvs/src/usr.sbin/rpki-client/parser.c,v retrieving revision 1.28 diff -u -p -r1.28 parser.c --- parser.c 4 Nov 2021 18:26:48 -0000 1.28 +++ parser.c 27 Dec 2021 08:37:53 -0000 @@ -34,6 +34,7 @@ #include <openssl/err.h> #include <openssl/evp.h> #include <openssl/x509.h> +#include <openssl/x509v3.h> #include "extern.h" @@ -45,6 +46,102 @@ static X509_STORE_CTX *ctx; static struct auth_tree auths = RB_INITIALIZER(&auths); static struct crl_tree crlt = RB_INITIALIZER(&crlt); +static int +generic_verify(int ok, X509_STORE_CTX *store_ctx, const char *descr) +{ + X509 *cert; + const STACK_OF(X509_EXTENSION) *exts; + X509_EXTENSION *ext; + ASN1_OBJECT *obj; + int depth, error, i, nid; + int saw_ipAddrBlock = 0; + int saw_autonomousSysNum = 0; + int saw_unknown = 0; + + error = X509_STORE_CTX_get_error(store_ctx); + depth = X509_STORE_CTX_get_error_depth(store_ctx); + + switch (error) { + case X509_V_ERR_UNHANDLED_CRITICAL_EXTENSION: + case X509_V_ERR_UNHANDLED_CRITICAL_CRL_EXTENSION: + break; + default: + return ok; + } + + fprintf(stderr, "%s cb: ok %d, %s, depth %d\n", + descr, ok, X509_verify_cert_error_string(error), depth); + + if ((cert = X509_STORE_CTX_get_current_cert(store_ctx)) == NULL) { + fprintf(stderr, "%s cb: got no current cert\n", descr); + return 0; + } + if ((exts = X509_get0_extensions(cert)) == NULL) { + fprintf(stderr, "%s cb: got no extensions\n", descr); + return 0; + } + + for (i = 0; i < sk_X509_EXTENSION_num(exts); i++) { + ext = sk_X509_EXTENSION_value(exts, i); + + /* skip over non-critical and known extensions */ + if (!X509_EXTENSION_get_critical(ext)) + continue; + if (X509_supported_extension(ext)) + continue; + + if ((obj = X509_EXTENSION_get_object(ext)) == NULL) { + fprintf(stderr, "%s cb: got no object\n", descr); + return 0; + } + + nid = OBJ_obj2nid(obj); + switch (nid) { + case NID_sbgp_ipAddrBlock: + saw_ipAddrBlock = 1; + break; + case NID_sbgp_autonomousSysNum: + saw_autonomousSysNum = 1; + break; + default: + fprintf(stderr, "unknown extension: nid %d\n", nid); + X509V3_EXT_print_fp(stderr, ext, X509V3_EXT_DUMP_UNKNOWN, 8); + saw_unknown = 1; + break; + } + } + + fprintf(stderr, "%s cb: addrBlock %d, autonomousSysNum %d\n", + descr, saw_ipAddrBlock, saw_autonomousSysNum); + + /* Fail if we saw an unknown extension. */ + return !saw_unknown; +} + +static int +roa_verify_cb(int ok, X509_STORE_CTX *store_ctx) +{ + return generic_verify(ok, store_ctx, "roa"); +} + +static int +mft_verify_cb(int ok, X509_STORE_CTX *store_ctx) +{ + return generic_verify(ok, store_ctx, "mft"); +} + +static int +cert_verify_cb(int ok, X509_STORE_CTX *store_ctx) +{ + return generic_verify(ok, store_ctx, "crt"); +} + +static int +gbr_verify_cb(int ok, X509_STORE_CTX *store_ctx) +{ + return generic_verify(ok, store_ctx, "gbr"); +} + /* * Parse and validate a ROA. * This is standard stuff. @@ -72,8 +169,8 @@ proc_parser_roa(struct entity *entp, con assert(x509 != NULL); if (!X509_STORE_CTX_init(ctx, NULL, x509, NULL)) cryptoerrx("X509_STORE_CTX_init"); - X509_STORE_CTX_set_flags(ctx, - X509_V_FLAG_IGNORE_CRITICAL | X509_V_FLAG_CRL_CHECK); + X509_STORE_CTX_set_verify_cb(ctx, roa_verify_cb); + X509_STORE_CTX_set_flags(ctx, X509_V_FLAG_CRL_CHECK); X509_STORE_CTX_set_depth(ctx, MAX_CERT_DEPTH); X509_STORE_CTX_set0_trusted_stack(ctx, chain); X509_STORE_CTX_set0_crls(ctx, crls); @@ -151,7 +248,7 @@ proc_parser_mft(struct entity *entp, con cryptoerrx("X509_STORE_CTX_init"); /* CRL checked disabled here because CRL is referenced from mft */ - X509_STORE_CTX_set_flags(ctx, X509_V_FLAG_IGNORE_CRITICAL); + X509_STORE_CTX_set_verify_cb(ctx, mft_verify_cb); X509_STORE_CTX_set_depth(ctx, MAX_CERT_DEPTH); X509_STORE_CTX_set0_trusted_stack(ctx, chain); @@ -211,8 +308,8 @@ proc_parser_cert(const struct entity *en if (!X509_STORE_CTX_init(ctx, NULL, x509, NULL)) cryptoerrx("X509_STORE_CTX_init"); - X509_STORE_CTX_set_flags(ctx, - X509_V_FLAG_IGNORE_CRITICAL | X509_V_FLAG_CRL_CHECK); + X509_STORE_CTX_set_verify_cb(ctx, cert_verify_cb); + X509_STORE_CTX_set_flags(ctx, X509_V_FLAG_CRL_CHECK); X509_STORE_CTX_set_depth(ctx, MAX_CERT_DEPTH); X509_STORE_CTX_set0_trusted_stack(ctx, chain); X509_STORE_CTX_set0_crls(ctx, crls); @@ -412,8 +509,8 @@ proc_parser_gbr(struct entity *entp, con assert(x509 != NULL); if (!X509_STORE_CTX_init(ctx, NULL, x509, NULL)) cryptoerrx("X509_STORE_CTX_init"); - X509_STORE_CTX_set_flags(ctx, - X509_V_FLAG_IGNORE_CRITICAL | X509_V_FLAG_CRL_CHECK); + X509_STORE_CTX_set_verify_cb(ctx, gbr_verify_cb); + X509_STORE_CTX_set_flags(ctx, X509_V_FLAG_CRL_CHECK); X509_STORE_CTX_set_depth(ctx, MAX_CERT_DEPTH); X509_STORE_CTX_set0_trusted_stack(ctx, chain); X509_STORE_CTX_set0_crls(ctx, crls);