On Mon, Dec 27, 2021 at 12:23:32PM +0100, Theo Buehler wrote:
> 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.
I like this. A few questions inline.
> 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);
Is there a reason you decided to use fprintf() over warnx() here?
And especially for the error cases below. AFAIK rpki-client uses warnx()
in almost all such cases.
Also we normally print the path of the file causing the error but I guess
this is tricky to pass in here. I guess one could use
X509_STORE_CTX_set_app_data() for that.
> + 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");
> +}
Is there no other way to avoid this extra wrapping of functions?
See above I guess using X509_STORE_CTX_set_app_data() and
X509_STORE_CTX_get_app_data() these wrappers become unnecessary.
> +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);
>
--
:wq Claudio
