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