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; >