On Sat, Jan 22, 2022 at 07:08:12PM +0100, Claudio Jeker wrote:
> On Sat, Jan 22, 2022 at 02:21:23PM +0100, Theo Buehler wrote:
> > On Sat, Jan 22, 2022 at 12:42:30PM +0100, Theo Buehler wrote:
> > > On Sat, Jan 22, 2022 at 11:47:17AM +0100, Claudio Jeker wrote:
> > > > The valid_cert() and valid_roa() functions both redo the valid_aki_ski()
> > > > call that the callee already did. Adjust the functions and skip this
> > > > redundant call. Also move the place where we set the talid for roa to a
> > > > better place.
> > > >
> > > > With RFC3779 support in LibreSSL these functions no longer trigger since
> > > > the range checks for IP and ASnums happen in X509_verify_cert().
> > > > The fucntions are still needed for libcrypto libs compiled without
> > > > RFC3779
> > > > support. Also they print a less generic error message and change the
> > > > stats
> > > > from failed parse to invalid (in case of roas).
> > > >
> > > > Lets start with this diff and then see how error reporting could be
> > > > improved.
> > >
> > > I'm fine with this in principle, but I have one question.
> > >
> > > > --
> > > > :wq Claudio
> > > >
> > > > Index: extern.h
> > > > ===================================================================
> > > > RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
> > > > retrieving revision 1.112
> > > > diff -u -p -r1.112 extern.h
> > > > --- extern.h 22 Jan 2022 09:18:48 -0000 1.112
> > > > +++ extern.h 22 Jan 2022 10:33:45 -0000
> > > > @@ -446,9 +446,8 @@ struct auth *valid_ski_aki(const char *,
> > > > const char *, const char *);
> > > > int valid_ta(const char *, struct auth_tree *,
> > > > const struct cert *);
> > > > -int valid_cert(const char *, struct auth_tree *,
> > > > - const struct cert *);
> > > > -int valid_roa(const char *, struct auth_tree *, struct roa
> > > > *);
> > > > +int valid_cert(const char *, struct auth *, const struct
> > > > cert *);
> > > > +int valid_roa(const char *, struct auth *, struct roa *);
> > > > int valid_filehash(int, const char *, size_t);
> > > > int valid_uri(const char *, size_t, const char *);
> > > > int valid_origin(const char *, const char *);
> > > > Index: parser.c
> > > > ===================================================================
> > > > RCS file: /cvs/src/usr.sbin/rpki-client/parser.c,v
> > > > retrieving revision 1.50
> > > > diff -u -p -r1.50 parser.c
> > > > --- parser.c 22 Jan 2022 09:18:48 -0000 1.50
> > > > +++ parser.c 22 Jan 2022 10:36:43 -0000
> > > > @@ -268,6 +268,8 @@ proc_parser_roa(char *file, const unsign
> > > > }
> > > > X509_free(x509);
> > > >
> > > > + roa->talid = a->cert->talid;
> > >
> > > Previously there was a NULL check for a before doing this. As far as I
> > > can tell, we can now get here with a == NULL. This also makes me wonder
> > > if we should keep the NULL check for a in valid_roa() and valid_cert()
> > > and do this assignment only after a successful call to valid_roa().
>
> We can't really end up here with a == NULL because valid_x509() above will
> fail for a == NULL (X509_verify_cert() with a empty trusted stack will
> fail. Now maybe that is a bit too indirect but we depend on the same fact
> in proc_parser_cert_validate().
Ah, that's what I was missing. I don't think it's too indirect. I should
have seen that. Thanks.
> > > > +
> > > > /*
> > > > * Check CRL to figure out the soonest transitive expiry moment
> > > > */
> > > > @@ -288,7 +290,7 @@ proc_parser_roa(char *file, const unsign
> > > > * the code around roa_read() to check the "valid" field itself.
> > > > */
> > > >
> > > > - if (valid_roa(file, &auths, roa))
> > > > + if (valid_roa(file, a, roa))
> >
> > There is a second issue: a is not the return value of valid_ski_aki()
> > here, but rather the auth struct of the trust anchor due to the for loop
> > before this call.
>
> This is indeed a good point. I moved the code up, so we do the valid_roa()
> check before figuring out the expire time.
>
> Adjusted diff below
ok
> --
> :wq Claudio
>
> Index: extern.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
> retrieving revision 1.112
> diff -u -p -r1.112 extern.h
> --- extern.h 22 Jan 2022 09:18:48 -0000 1.112
> +++ extern.h 22 Jan 2022 10:33:45 -0000
> @@ -446,9 +446,8 @@ struct auth *valid_ski_aki(const char *,
> const char *, const char *);
> int valid_ta(const char *, struct auth_tree *,
> const struct cert *);
> -int valid_cert(const char *, struct auth_tree *,
> - const struct cert *);
> -int valid_roa(const char *, struct auth_tree *, struct roa *);
> +int valid_cert(const char *, struct auth *, const struct cert *);
> +int valid_roa(const char *, struct auth *, struct roa *);
> int valid_filehash(int, const char *, size_t);
> int valid_uri(const char *, size_t, const char *);
> int valid_origin(const char *, const char *);
> Index: parser.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/parser.c,v
> retrieving revision 1.50
> diff -u -p -r1.50 parser.c
> --- parser.c 22 Jan 2022 09:18:48 -0000 1.50
> +++ parser.c 22 Jan 2022 18:00:01 -0000
> @@ -268,6 +268,16 @@ proc_parser_roa(char *file, const unsign
> }
> X509_free(x509);
>
> + roa->talid = a->cert->talid;
> +
> + /*
> + * If the ROA isn't valid, we accept it anyway and depend upon
> + * the code around roa_read() to check the "valid" field itself.
> + */
> +
> + if (valid_roa(file, a, roa))
> + roa->valid = 1;
> +
> /*
> * Check CRL to figure out the soonest transitive expiry moment
> */
> @@ -283,14 +293,6 @@ proc_parser_roa(char *file, const unsign
> roa->expires = a->cert->expires;
> }
>
> - /*
> - * If the ROA isn't valid, we accept it anyway and depend upon
> - * the code around roa_read() to check the "valid" field itself.
> - */
> -
> - if (valid_roa(file, &auths, roa))
> - roa->valid = 1;
> -
> return roa;
> }
>
> @@ -401,8 +403,8 @@ proc_parser_cert_validate(char *file, st
>
> cert->talid = a->cert->talid;
>
> - /* Validate the cert to get the parent */
> - if (!valid_cert(file, &auths, cert)) {
> + /* Validate the cert */
> + if (!valid_cert(file, a, cert)) {
> cert_free(cert);
> return NULL;
> }
> Index: validate.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/validate.c,v
> retrieving revision 1.26
> diff -u -p -r1.26 validate.c
> --- validate.c 22 Jan 2022 09:18:48 -0000 1.26
> +++ validate.c 22 Jan 2022 10:34:31 -0000
> @@ -143,17 +143,12 @@ valid_ta(const char *fn, struct auth_tre
> * Returns 1 if valid, 0 otherwise.
> */
> int
> -valid_cert(const char *fn, struct auth_tree *auths, const struct cert *cert)
> +valid_cert(const char *fn, struct auth *a, const struct cert *cert)
> {
> - struct auth *a;
> size_t i;
> uint32_t min, max;
> char buf1[64], buf2[64];
>
> - a = valid_ski_aki(fn, auths, cert->ski, cert->aki);
> - if (a == NULL)
> - return 0;
> -
> for (i = 0; i < cert->asz; i++) {
> if (cert->as[i].type == CERT_AS_INHERIT) {
> if (cert->purpose == CERT_PURPOSE_BGPSEC_ROUTER)
> @@ -207,17 +202,11 @@ valid_cert(const char *fn, struct auth_t
> * Returns 1 if valid, 0 otherwise.
> */
> int
> -valid_roa(const char *fn, struct auth_tree *auths, struct roa *roa)
> +valid_roa(const char *fn, struct auth *a, struct roa *roa)
> {
> - struct auth *a;
> size_t i;
> char buf[64];
>
> - a = valid_ski_aki(fn, auths, roa->ski, roa->aki);
> - if (a == NULL)
> - return 0;
> -
> - roa->talid = a->cert->talid;
>
> for (i = 0; i < roa->ipsz; i++) {
> if (valid_ip(a, roa->ips[i].afi, roa->ips[i].min,
