On Wed, Dec 29, 2021 at 01:06:30AM +0100, Theo Buehler wrote:
> On Tue, Dec 28, 2021 at 05:08:46PM +0100, Claudio Jeker wrote:
> > 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.
>
> cool
>
> > > + 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?
>
> Only that I didn't really intend this for commit and was therefore a bit
> lazy. I should have made that clearer.
>
> > 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.
>
> Good idea, yes, that works.
>
> > > +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?
>
> It was the first thing that came to mind so I could tell where the calls
> originated.
>
> > See above I guess using X509_STORE_CTX_set_app_data() and
> > X509_STORE_CTX_get_app_data() these wrappers become unnecessary.
>
> I think the suffix of the file gives enough of a clue, so that's
> definitely no longer needed.
Absolutly. Idealy we can pass the path to rpki-client -f in the future to
get a verbose output of the object in question.
> This should be closer to commit now. I removed the switch over the error
> since I guess we would need to handle unhandled critical CRL extensions
> differently. I switched the fprintf to warnx, although some more of
> them should perhaps be fatal since it's most likely an allocation
> failure.
Not too worried about those errors, I doubt the will be seen often.
Lets decide if someone actually runs into those.
> Since this is super noisy (multiple lines per processed file), I reduced
> to printing one line in the ordinary path and only on verbosity > 1. We
> might even want to crank that higher or ditch this last warnx altogether.
I think the verbose > 1 warnx could indeed be removed. With modern libs it
will be never hit and for old libs the code takes care of this.
Would simplify the code a bit more.
> I also suppressed printing the cert error and the ok variable since I
> don't think this is actually interesting, whereas the depth should give
> a valuable clue where to look next, so I kept it.
>
> ok?
I really need to try this on a system with old libressl. Will hopefully
manage to do that today. Apart from that I'm OK with this going in.
We can cleanup the last bits in tree.
> 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 28 Dec 2021 23:34:41 -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,75 @@ static X509_STORE_CTX *ctx;
> static struct auth_tree auths = RB_INITIALIZER(&auths);
> static struct crl_tree crlt = RB_INITIALIZER(&crlt);
>
> +static int
> +verify_cb(int ok, X509_STORE_CTX *store_ctx)
> +{
> + X509 *cert;
> + const STACK_OF(X509_EXTENSION) *exts;
> + X509_EXTENSION *ext;
> + ASN1_OBJECT *obj;
> + char *file;
> + 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);
> +
> + if (error != X509_V_ERR_UNHANDLED_CRITICAL_EXTENSION)
> + return ok;
> +
> + if ((file = X509_STORE_CTX_get_app_data(store_ctx)) == NULL)
> + cryptoerrx("X509_STORE_CTX_get_app_data");
> +
> + if ((cert = X509_STORE_CTX_get_current_cert(store_ctx)) == NULL) {
> + warnx("%s: got no current cert", file);
> + return 0;
> + }
> + if ((exts = X509_get0_extensions(cert)) == NULL) {
> + warnx("%s: got no cert extensions", file);
> + 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) {
> + warnx("%s: got no extension object", file);
> + 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:
> + warnx("%s: depth %d: unknown extension: nid %d",
> + file, depth, nid);
> + saw_unknown = 1;
> + break;
> + }
> + }
> +
> + if (verbose > 1)
> + warnx("%s: depth %d, ipAddrBlock %d, autonomousSysNum %d",
> + file, depth, saw_ipAddrBlock, saw_autonomousSysNum);
> +
> + /* Fail if we saw an unknown extension. */
> + return !saw_unknown;
> +}
> +
> /*
> * Parse and validate a ROA.
> * This is standard stuff.
> @@ -72,8 +142,10 @@ 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, verify_cb);
> + if (!X509_STORE_CTX_set_app_data(ctx, entp->file))
> + cryptoerrx("X509_STORE_CTX_set_app_data");
> + 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);
> @@ -150,8 +222,10 @@ proc_parser_mft(struct entity *entp, con
> if (!X509_STORE_CTX_init(ctx, NULL, x509, NULL))
> 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);
> + /* CRL checks disabled here because CRL is referenced from mft */
> + X509_STORE_CTX_set_verify_cb(ctx, verify_cb);
> + if (!X509_STORE_CTX_set_app_data(ctx, entp->file))
> + cryptoerrx("X509_STORE_CTX_set_app_data");
> X509_STORE_CTX_set_depth(ctx, MAX_CERT_DEPTH);
> X509_STORE_CTX_set0_trusted_stack(ctx, chain);
>
> @@ -211,8 +285,10 @@ 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, verify_cb);
> + if (!X509_STORE_CTX_set_app_data(ctx, entp->file))
> + cryptoerrx("X509_STORE_CTX_set_app_data");
> + 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 +488,10 @@ 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, verify_cb);
> + if (!X509_STORE_CTX_set_app_data(ctx, entp->file))
> + cryptoerrx("X509_STORE_CTX_set_app_data");
> + 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
