On Wed, Jan 19, 2022 at 02:57:44PM +0100, Claudio Jeker wrote:
> > > + return;
> > > + }
> > > +
> > > + /* TA found play back the stack and add all certs */
> > > + do {
> > > + if (parser_cert_validate(nfile, cert) == NULL)
> > > + break;
> >
> > I think this also needs a cert_free(cert).
>
> Certs are special and never released once valid. The are added to the
> auths tree. If parser_cert_validate() fails then cert is already freed so
> that should also be fine.
Indeed. I got it now.
>
> > > + free(nfile);
> > > + cert = stack[stacksz - 1];
> > > + nfile = filestack[stacksz - 1];
> >
> > I'm probably missing something, but I don't see why we don't hit this
> > in the last iteration where stacksz == 0 (which would be bad).
> >
> > > + } while (stacksz-- > 0);
> >
> > It's probably related to my above confusion: We don't need to free cert
> > if parser_cert_validate() fails but do need to free it if we exit the
> > while loop because stacksz == 0.
>
> Yeah, I'm also confused. I think the code currently underflows the stack
> which seems to result in a NULL cert.
>
> Anyway I totally rewrote that bit without recursion. Have a look below.
> Only think I dislike is the fact that parse_load_certchain() is using the
> URI instead of the filename (but should be easy enough to convert between
> the two).
This is much clearer to me and I like the non-recursive version better.
ok tb
Some tiny nits for your consideration. Apart from the %i do as you
prefer :)
> Index: parser.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/parser.c,v
> retrieving revision 1.44
> diff -u -p -r1.44 parser.c
> --- parser.c 18 Jan 2022 18:19:47 -0000 1.44
> +++ parser.c 19 Jan 2022 13:54:06 -0000
> @@ -383,24 +383,13 @@ proc_parser_mft(char *file, const unsign
> }
>
> /*
> - * Certificates are from manifests (has a digest and is signed with
> - * another certificate) Parse the certificate, make sure its
> - * signatures are valid (with CRLs), then validate the RPKI content.
> - * This returns a certificate (which must not be freed) or NULL on
> - * parse failure.
> + * Validate a certificate, if invalid free the resouces and return NULL.
> */
> static struct cert *
> -proc_parser_cert(char *file, const unsigned char *der, size_t len)
> +parser_cert_validate(char *file, struct cert *cert)
This is the only function with a parser_ prefix.
Should this be proc_parser_cert_validate()?
(parse_cert_validate would be a bad name).
[...]
> +/*
> + * Build the certificate chain by using the Authority Information Access.
> + * This requires that the TA are already validated and added to the auths
> + * tree. Once the TA is located in the chain the chain is validated in
> + * reverse order.
> + */
> +static void
> +parse_load_certchain(char *uri)
> +{
> + struct cert *stack[MAX_CERT_DEPTH];
> + char *filestack[MAX_CERT_DEPTH];
> + struct cert *cert;
> + int i, failed;
> +
> + for (i = 0; i < MAX_CERT_DEPTH; i++) {
> + cert = parse_load_cert(uri);
> + if (cert == NULL) {
> + warnx("failed to build authority chain");
> + return;
> + }
> + stack[i] = cert;
> + filestack[i] = uri;
> + if (auth_find(&auths, cert->aki) != NULL) {
no need for braces
> + break; /* found the TA */
> + }
> + uri = cert->aia;
> + }
> +
> + if (i >= MAX_CERT_DEPTH) {
> + warnx("authority chain exceeds max depth of %i",
use %d instead of %i
> + MAX_CERT_DEPTH);
> + for (i = 0; i < MAX_CERT_DEPTH; i++) {
no need for braces
> + cert_free(stack[i]);
> + }
> + return;
> + }
