Re: rpki-client: check ipAddrBlock and autonomousSysNum for criticality
On Wed, Dec 29, 2021 at 01:12:25PM +0100, Claudio Jeker wrote:
> 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.
I finally tested this on
Re: rpki-client: check ipAddrBlock and autonomousSysNum for criticality
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 - 1.28
> +++ parser.c 28 Dec 2021 23:34:41 -
> @@ -34,6 +34,7 @@
> #incl
Re: rpki-client: check ipAddrBlock and autonomousSysNum for criticality
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.c4 Nov 2021 18:26:48 - 1.28
+++ parser.c28 Dec 2021 23:34:41 -
@@ -34,6 +34,7 @@
#include
#include
#include
+#include
#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 = X5
Re: rpki-client: check ipAddrBlock and autonomousSysNum for criticality
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 - 1.28
> +++ parser.c 27 Dec 2021 08:37:53 -
> @@ -34,6 +34,7 @@
> #include
> #include
> #include
> +#include
>
> #include "extern.h"
>
> @@ -45,6 +46,102 @@ static X509_STORE_CTX *ctx;
> static struct auth_tree auths = RB_INITIALIZER(&auths);
> static struct crl_treecrlt = 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;
>
Re: rpki-client: check ipAddrBlock and autonomousSysNum for criticality
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.c4 Nov 2021 18:26:48 - 1.28
+++ parser.c27 Dec 2021 08:37:53 -
@@ -34,6 +34,7 @@
#include
#include
#include
+#include
#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 *s
Re: rpki-client: check ipAddrBlock and autonomousSysNum for criticality
Hi Claudio, Claudio Jeker wrote on Sat, Dec 25, 2021 at 05:48:53PM +0100: > On Sat, Dec 25, 2021 at 11:36:50AM +0100, Theo Buehler wrote: >> These extensions MUST be marked critical by the sections of the spec >> mentioned in the cryptowarnx(). That's determined by the ASN1_BOOLEAN >> that is extracted and ignored after the FIXME a few lines below each of >> the two hunks. Rather than getting the info from there, it's easier to >> use an API call that checks what was already parsed by d2i_X509(). > I like this a lot. OK claudio@ Merely to avoid a misunderstanding: I have no oponion whatsoever on this rpki-client patch. > 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. Not exactly non-existent, but admittedly very poor indeed. There is X509_STORE_CTX_set_verify_cb(3), mostly written by Dr. Stephen Henson in 2009. But it does not really explain how the callback is used and instead only provides four examples. The reasons for this being so badly documented are as follows: 1. X.509 PKI Path Validation as specified in RFC 5280 is ridiculously complicated no matter the implementation. 2. Our API design and implementation was inherited from OpenSSL, and everybody knows what that means. 3. Our documentation was inherited from OpenSSL, and everybody knows what that means. 4. Beck@ has embarked on an epic quest to mitigate parts of the consequences of item 2. Consequently, the surrounding airs have been saturated with large amounts of dust for quite some time now. 5. While parts of that dust have arguably started to settle, the air still feels somewhat dusty to me, and i have been fearing that spending the several days of work required to throughly improve this particular part of the documentation might end up as working straight for the waste paper basket once beck@ achieves his next major steps forward. So given that there are literally hundreds of other public API functions in libcrypto that are still completely undocumented (as opposed to the verification callback being documented very badly), i so far refrained from even trying to attack that particular problem... Yours, Ingo
Re: rpki-client: check ipAddrBlock and autonomousSysNum for criticality
On Sat, Dec 25, 2021 at 11:36:50AM +0100, Theo Buehler wrote:
> These extensions MUST be marked critical by the sections of the spec
> mentioned in the cryptowarnx(). That's determined by the ASN1_BOOLEAN
> that is extracted and ignored after the FIXME a few lines below each of
> the two hunks. Rather than getting the info from there, it's easier to
> use an API call that checks what was already parsed by d2i_X509().
I like this a lot. OK claudio@
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.
> Index: cert.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
> retrieving revision 1.47
> diff -u -p -r1.47 cert.c
> --- cert.c5 Nov 2021 10:50:41 - 1.47
> +++ cert.c24 Dec 2021 23:40:55 -
> @@ -588,6 +588,12 @@ sbgp_assysnum(struct parse *p, X509_EXTE
> int dsz, rc = 0, i, ptag;
> long plen;
>
> + if (!X509_EXTENSION_get_critical(ext)) {
> + cryptowarnx("%s: RFC 6487 section 4.8.11: autonomousSysNum: "
> + "extension not critical", p->fn);
> + goto out;
> + }
> +
> if ((dsz = i2d_X509_EXTENSION(ext, &sv)) < 0) {
> cryptowarnx("%s: RFC 6487 section 4.8.11: autonomousSysNum: "
> "failed extension parse", p->fn);
> @@ -890,6 +896,12 @@ sbgp_ipaddrblk(struct parse *p, X509_EXT
> ASN1_SEQUENCE_ANY *seq = NULL, *sseq = NULL;
> const ASN1_TYPE *t = NULL;
> int i;
> +
> + if (!X509_EXTENSION_get_critical(ext)) {
> + cryptowarnx("%s: RFC 6487 section 4.8.10: sbgp-ipAddrBlock: "
> + "extension not critical", p->fn);
> + goto out;
> + }
>
> if ((dsz = i2d_X509_EXTENSION(ext, &sv)) < 0) {
> cryptowarnx("%s: RFC 6487 section 4.8.10: sbgp-ipAddrBlock: "
>
--
:wq Claudio
rpki-client: check ipAddrBlock and autonomousSysNum for criticality
These extensions MUST be marked critical by the sections of the spec
mentioned in the cryptowarnx(). That's determined by the ASN1_BOOLEAN
that is extracted and ignored after the FIXME a few lines below each of
the two hunks. Rather than getting the info from there, it's easier to
use an API call that checks what was already parsed by d2i_X509().
Index: cert.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
retrieving revision 1.47
diff -u -p -r1.47 cert.c
--- cert.c 5 Nov 2021 10:50:41 - 1.47
+++ cert.c 24 Dec 2021 23:40:55 -
@@ -588,6 +588,12 @@ sbgp_assysnum(struct parse *p, X509_EXTE
int dsz, rc = 0, i, ptag;
long plen;
+ if (!X509_EXTENSION_get_critical(ext)) {
+ cryptowarnx("%s: RFC 6487 section 4.8.11: autonomousSysNum: "
+ "extension not critical", p->fn);
+ goto out;
+ }
+
if ((dsz = i2d_X509_EXTENSION(ext, &sv)) < 0) {
cryptowarnx("%s: RFC 6487 section 4.8.11: autonomousSysNum: "
"failed extension parse", p->fn);
@@ -890,6 +896,12 @@ sbgp_ipaddrblk(struct parse *p, X509_EXT
ASN1_SEQUENCE_ANY *seq = NULL, *sseq = NULL;
const ASN1_TYPE *t = NULL;
int i;
+
+ if (!X509_EXTENSION_get_critical(ext)) {
+ cryptowarnx("%s: RFC 6487 section 4.8.10: sbgp-ipAddrBlock: "
+ "extension not critical", p->fn);
+ goto out;
+ }
if ((dsz = i2d_X509_EXTENSION(ext, &sv)) < 0) {
cryptowarnx("%s: RFC 6487 section 4.8.10: sbgp-ipAddrBlock: "
