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);
