On Thu, Apr 21, 2022 at 01:14:31PM +0200, Claudio Jeker wrote:
> So here is the cleanup of filemode.c and also a bit of cleanup in parse.c
> This should also fix a few bugs in parse_load_certchain() (mainly
> memleaks).
A couple of suggestions for parse_load_certchain() below.
ok
>
> --
> :wq Claudio
>
> Index: cert.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
> retrieving revision 1.70
> diff -u -p -r1.70 cert.c
> --- cert.c 21 Apr 2022 09:53:07 -0000 1.70
> +++ cert.c 21 Apr 2022 10:45:17 -0000
> @@ -999,6 +999,9 @@ out:
> struct cert *
> cert_parse(const char *fn, struct cert *p)
> {
> + if (p == NULL)
> + return NULL;
> +
> if (p->aki == NULL) {
> warnx("%s: RFC 6487 section 8.4.2: "
> "non-trust anchor missing AKI", fn);
> @@ -1032,6 +1035,9 @@ ta_parse(const char *fn, struct cert *p,
> ASN1_TIME *notBefore, *notAfter;
> EVP_PKEY *pk, *opk;
>
> + if (p == NULL)
> + return NULL;
> +
> /* first check pubkey against the one from the TAL */
> pk = d2i_PUBKEY(NULL, &pkey, pkeysz);
> if (pk == NULL) {
> @@ -1207,7 +1213,7 @@ auth_find(struct auth_tree *auths, const
> return RB_FIND(auth_tree, auths, &a);
> }
>
> -void
> +struct auth *
> auth_insert(struct auth_tree *auths, struct cert *cert, struct auth *parent)
> {
> struct auth *na;
> @@ -1221,6 +1227,8 @@ auth_insert(struct auth_tree *auths, str
>
> if (RB_INSERT(auth_tree, auths, na) != NULL)
> err(1, "auth tree corrupted");
> +
> + return na;
> }
>
> static void
> Index: extern.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
> retrieving revision 1.131
> diff -u -p -r1.131 extern.h
> --- extern.h 21 Apr 2022 09:53:07 -0000 1.131
> +++ extern.h 21 Apr 2022 10:46:09 -0000
> @@ -308,7 +308,7 @@ struct auth {
> RB_HEAD(auth_tree, auth);
>
> struct auth *auth_find(struct auth_tree *, const char *);
> -void auth_insert(struct auth_tree *, struct cert *, struct auth *);
> +struct auth *auth_insert(struct auth_tree *, struct cert *, struct auth *);
>
> enum http_result {
> HTTP_FAILED, /* anything else */
> Index: filemode.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/filemode.c,v
> retrieving revision 1.1
> diff -u -p -r1.1 filemode.c
> --- filemode.c 21 Apr 2022 09:53:07 -0000 1.1
> +++ filemode.c 21 Apr 2022 10:47:24 -0000
> @@ -46,80 +46,6 @@ static struct crl_tree crlt = RB_INITIA
> struct tal *talobj[TALSZ_MAX];
>
> /*
> - * Validate a certificate, if invalid free the resouces and return NULL.
> - */
> -static struct cert *
> -proc_parser_cert_validate(char *file, struct cert *cert)
> -{
> - struct auth *a;
> - struct crl *crl;
> -
> - a = valid_ski_aki(file, &auths, cert->ski, cert->aki);
> - crl = crl_get(&crlt, a);
> -
> - if (!valid_x509(file, ctx, cert->x509, a, crl, 0)) {
> - cert_free(cert);
> - return NULL;
> - }
> -
> - cert->talid = a->cert->talid;
> -
> - /* Validate the cert */
> - if (!valid_cert(file, a, cert)) {
> - cert_free(cert);
> - return NULL;
> - }
> -
> - /*
> - * Add validated CA certs to the RPKI auth tree.
> - */
> - if (cert->purpose == CERT_PURPOSE_CA)
> - auth_insert(&auths, cert, a);
> -
> - return cert;
> -}
> -
> -/*
> - * Root certificates come from TALs (has a pkey and is self-signed).
> - * Parse the certificate, ensure that its public key matches the
> - * known public key from the TAL, and then validate the RPKI
> - * content.
> - *
> - * This returns a certificate (which must not be freed) or NULL on
> - * parse failure.
> - */
> -static struct cert *
> -proc_parser_root_cert(char *file, const unsigned char *der, size_t len,
> - unsigned char *pkey, size_t pkeysz, int talid)
> -{
> - struct cert *cert;
> -
> - /* Extract certificate data. */
> -
> - cert = cert_parse_pre(file, der, len);
> - if (cert == NULL)
> - return NULL;
> - cert = ta_parse(file, cert, pkey, pkeysz);
> - if (cert == NULL)
> - return NULL;
> -
> - if (!valid_ta(file, &auths, cert)) {
> - warnx("%s: certificate not a valid ta", file);
> - cert_free(cert);
> - return NULL;
> - }
> -
> - cert->talid = talid;
> -
> - /*
> - * Add valid roots to the RPKI auth tree.
> - */
> - auth_insert(&auths, cert, NULL);
> -
> - return cert;
> -}
> -
> -/*
> * Use the X509 CRL Distribution Points to locate the CRL needed for
> * verification.
> */
> @@ -207,25 +133,25 @@ parse_load_cert(char *uri)
> static void
> parse_load_certchain(char *uri)
> {
> - struct cert *stack[MAX_CERT_DEPTH];
> + struct cert *stack[MAX_CERT_DEPTH] = { 0 };
> char *filestack[MAX_CERT_DEPTH];
> struct cert *cert;
> - int i, failed;
> + struct crl *crl;
> + struct auth *a;
> + int i;
>
> for (i = 0; i < MAX_CERT_DEPTH; i++) {
> - cert = parse_load_cert(uri);
> - if (cert == NULL) {
> + filestack[i] = uri;
> + stack[i] = cert = parse_load_cert(uri);
> + if (cert == NULL || cert->purpose != CERT_PURPOSE_CA) {
> warnx("failed to build authority chain");
> - return;
> + goto fail;
> }
> if (auth_find(&auths, cert->ski) != NULL) {
> assert(i == 0);
> - cert_free(cert);
> - return; /* cert already added */
> + goto fail;
> }
> - stack[i] = cert;
> - filestack[i] = uri;
> - if (auth_find(&auths, cert->aki) != NULL)
> + if ((a = auth_find(&auths, cert->aki)) != NULL)
> break; /* found chain to TA */
> uri = cert->aia;
> }
> @@ -233,47 +159,65 @@ parse_load_certchain(char *uri)
> if (i >= MAX_CERT_DEPTH) {
> warnx("authority chain exceeds max depth of %d",
> MAX_CERT_DEPTH);
I would prefer to move this cleanup to the end of the function and
have a goto fail here.
> +fail:
> for (i = 0; i < MAX_CERT_DEPTH; i++)
> cert_free(stack[i]);
> return;
> }
>
> /* TA found play back the stack and add all certs */
> - for (failed = 0; i >= 0; i--) {
> + for (; i >= 0; i--) {
> cert = stack[i];
> uri = filestack[i];
>
> - if (failed)
> - cert_free(cert);
> - else if (proc_parser_cert_validate(uri, cert) == NULL)
> - failed = 1;
> + crl = crl_get(&crlt, a);
I think this loop is easier to understand:
for (; i >= 0; i--) {
cert = stack[i];
uri = filestack[i];
crl = crl_get(&crlt, a);
if (!valid_x509(uri, ctx, cert->x509, a, crl, 0) ||
!valid_cert(uri, a, cert))
goto fail;
cert->talid = a->cert->talid;
a = auth_insert(&auths, cert, a);
stack[i] = NULL;
}
> + if (valid_x509(uri, ctx, cert->x509, a, crl, 0) &&
> + valid_cert(uri, a, cert)) {
> + cert->talid = a->cert->talid;
> + a = auth_insert(&auths, cert, a);
> + } else {
> + for (; i >= 0; i--)
> + cert_free(stack[i]);
> + }
> }
> }
>
> static void
> parse_load_ta(struct tal *tal)
> {
> - const char *file;
> - char *nfile, *f;
> + const char *filename;
> + struct cert *cert;
> + unsigned char *f = NULL;
> + char *file;
> size_t flen;
>
> /* does not matter which URI, all end with same filename */
> - file = strrchr(tal->uri[0], '/');
> - assert(file);
> + filename = strrchr(tal->uri[0], '/');
> + assert(filename);
>
> - if (asprintf(&nfile, "ta/%s%s", tal->descr, file) == -1)
> + if (asprintf(&file, "ta/%s%s", tal->descr, filename) == -1)
> err(1, NULL);
>
> - f = load_file(nfile, &flen);
> + f = load_file(file, &flen);
> if (f == NULL) {
> - warn("parse file %s", nfile);
> - free(nfile);
> - return;
> + warn("parse file %s", file);
> + goto out;
> }
>
> - /* if TA is valid it was added as a root which is all we need */
> - proc_parser_root_cert(nfile, f, flen, tal->pkey, tal->pkeysz, tal->id);
> - free(nfile);
> + /* Extract certificate data. */
> + cert = cert_parse_pre(file, f, flen);
> + cert = ta_parse(file, cert, tal->pkey, tal->pkeysz);
> + if (cert == NULL)
> + goto out;
> +
> + cert->talid = tal->id;
> +
> + if (!valid_ta(file, &auths, cert))
> + cert_free(cert);
> + else
> + auth_insert(&auths, cert, NULL);
> +out:
> + free(file);
> free(f);
> }
>
> Index: parser.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/parser.c,v
> retrieving revision 1.72
> diff -u -p -r1.72 parser.c
> --- parser.c 21 Apr 2022 09:53:07 -0000 1.72
> +++ parser.c 21 Apr 2022 10:45:53 -0000
> @@ -389,30 +389,37 @@ proc_parser_mft(struct entity *entp, str
> }
>
> /*
> - * Validate a certificate, if invalid free the resouces and return NULL.
> + * 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.
> */
> static struct cert *
> -proc_parser_cert_validate(char *file, struct cert *cert)
> +proc_parser_cert(char *file, const unsigned char *der, size_t len)
> {
> - struct auth *a;
> + struct cert *cert;
> struct crl *crl;
> + struct auth *a;
> +
> + /* Extract certificate data. */
> +
> + cert = cert_parse_pre(file, der, len);
> + cert = cert_parse(file, cert);
> + if (cert == NULL)
> + return NULL;
>
> a = valid_ski_aki(file, &auths, cert->ski, cert->aki);
> crl = crl_get(&crlt, a);
>
> - if (!valid_x509(file, ctx, cert->x509, a, crl, 0)) {
> + if (!valid_x509(file, ctx, cert->x509, a, crl, 0) ||
> + !valid_cert(file, a, cert)) {
> cert_free(cert);
> return NULL;
> }
>
> cert->talid = a->cert->talid;
>
> - /* Validate the cert */
> - if (!valid_cert(file, a, cert)) {
> - cert_free(cert);
> - return NULL;
> - }
> -
> /*
> * Add validated CA certs to the RPKI auth tree.
> */
> @@ -423,31 +430,6 @@ proc_parser_cert_validate(char *file, st
> }
>
> /*
> - * 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.
> - */
> -static struct cert *
> -proc_parser_cert(char *file, const unsigned char *der, size_t len)
> -{
> - struct cert *cert;
> -
> - /* Extract certificate data. */
> -
> - cert = cert_parse_pre(file, der, len);
> - if (cert == NULL)
> - return NULL;
> - cert = cert_parse(file, cert);
> - if (cert == NULL)
> - return NULL;
> -
> - cert = proc_parser_cert_validate(file, cert);
> - return cert;
> -}
> -
> -/*
> * Root certificates come from TALs (has a pkey and is self-signed).
> * Parse the certificate, ensure that its public key matches the
> * known public key from the TAL, and then validate the RPKI
> @@ -465,8 +447,6 @@ proc_parser_root_cert(char *file, const
> /* Extract certificate data. */
>
> cert = cert_parse_pre(file, der, len);
> - if (cert == NULL)
> - return NULL;
> cert = ta_parse(file, cert, pkey, pkeysz);
> if (cert == NULL)
> return NULL;
>
