On Fri, Nov 25, 2022 at 04:52:24PM +0000, Job Snijders wrote: [...]
> My goal was to focus on verifying the signature; less so on additionally > being a RFC 8805 parser. That's fair and completely fine with me. > How about the below? > > * zaps trailing comments > * use() stravis to store the location for prettyprinting No objection to stravis, although I'm not sure it's needed. We don't do this for other files. > * enforces line length limits in the Signature section > * uses the tal.c style of iterating through the file Looks great. One more round of simple comments on geofeed.c inline, then we should be good to go. > Index: geofeed.c > =================================================================== > RCS file: geofeed.c > diff -N geofeed.c > --- /dev/null 1 Jan 1970 00:00:00 -0000 > +++ geofeed.c 25 Nov 2022 16:49:57 -0000 > @@ -0,0 +1,278 @@ > +/* $OpenBSD: geofeed.c,v 1.18 2022/11/02 12:46:49 job Exp $ */ > +/* > + * Copyright (c) 2022 Job Snijders <j...@fastly.com> > + * Copyright (c) 2019 Kristaps Dzonsons <krist...@bsd.lv> > + * > + * Permission to use, copy, modify, and distribute this software for any > + * purpose with or without fee is hereby granted, provided that the above > + * copyright notice and this permission notice appear in all copies. > + * > + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES > + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF > + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR > + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES > + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN > + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF > + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. > + */ > + > +#include <assert.h> > +#include <err.h> > +#include <stdlib.h> > +#include <string.h> > +#include <vis.h> > + > +#include <arpa/inet.h> > +#include <sys/socket.h> > +#include <openssl/bio.h> > +#include <openssl/x509.h> > + > +#include "extern.h" > + > +/* > + * Parse results and data of the Signed Checklist file. > + */ > +struct parse { > + const char *fn; > + struct geofeed *res; > +}; > + > +extern ASN1_OBJECT *geofeed_oid; > + > +/* > + * Take a CIDR prefix (in presentation format) and add it to parse results. > + * Returns 1 on success. > + */ > +static int > +geofeed_parse_geoip(struct geofeed *res, char *cidr, char *loc) > +{ > + struct geoip *geoip; > + struct ip_addr *ipaddr; > + enum afi afi; > + int plen; > + > + if ((ipaddr = calloc(1, sizeof(struct ip_addr))) == NULL) > + err(1, NULL); > + > + if ((plen = inet_net_pton(AF_INET, cidr, ipaddr->addr, > + sizeof(ipaddr->addr))) != -1) > + afi = AFI_IPV4; > + else if ((plen = inet_net_pton(AF_INET6, cidr, ipaddr->addr, > + sizeof(ipaddr->addr))) != -1) > + afi = AFI_IPV6; > + else { > + static char buf[80]; add empty line. > + if (strnvis(buf, cidr, sizeof(buf), VIS_SAFE) > + >= (int)sizeof(buf)) { > + memcpy(buf + sizeof(buf) - 4, "...", 4); > + } > + warnx("invalid address: %s", buf); Leak: free(ipaddr); > + return 0; > + } > + > + ipaddr->prefixlen = plen; > + > + res->geoips = recallocarray(res->geoips, res->geoipsz, > + res->geoipsz + 1, sizeof(struct geoip)); > + if (res->geoips == NULL) > + err(1, NULL); > + geoip = &res->geoips[res->geoipsz++]; > + > + if ((geoip->ip = calloc(1, sizeof(struct cert_ip))) == NULL) > + err(1, NULL); > + > + geoip->ip->type = CERT_IP_ADDR; > + geoip->ip->ip = *ipaddr; > + geoip->ip->afi = afi; > + > + if (stravis(&geoip->loc, loc, VIS_SAFE) == -1) > + err(1, "stravis"); > + > + if (!ip_cert_compose_ranges(geoip->ip)) > + return 0; > + > + return 1; > +} > + > +/* > + * Parse a full RFC 9092 file. > + * Returns the Geofeed, or NULL if the object was malformed. > + */ > +struct geofeed * > +geofeed_parse(X509 **x509, const char *fn, char *buf, size_t len) > +{ > + struct parse p; > + char *delim, *line, *nl; > + BIO *bio; > + char *b64; > + size_t b64sz; > + unsigned char *der; > + size_t dersz; > + const ASN1_TIME *at; > + struct cert *cert = NULL; > + int rpki_signature_seen = 0, end_signature_seen = 0; > + int rc = 0; > + > + bio = BIO_new(BIO_s_mem()); > + assert(bio != NULL); > + > + memset(&p, 0, sizeof(struct parse)); > + p.fn = fn; > + > + if ((p.res = calloc(1, sizeof(struct geofeed))) == NULL) > + err(1, NULL); > + > + if ((b64 = calloc(1, len)) == NULL) > + err(1, NULL); > + b64sz = len; > + > + while ((nl = memchr(buf, '\n', len)) != NULL) { > + line = buf; > + > + /* advance buffer to next line */ > + len -= nl + 1 - buf; > + buf = nl + 1; > + > + /* replace LF and CR with NUL, point nl at first NUL */ > + *nl = '\0'; > + if (nl > line && nl[-1] == '\r') { > + nl[-1] = '\0'; > + nl--; Since you know the start and the end of the line, you can take note of it: linelen = nl - line; Each time you call strlen, the entire string is traversed, so you can avoid that. > + } else { > + warnx("%s: malformed file, expected CRLF line" > + " endings", fn); > + goto out; > + } > + > + if (end_signature_seen) { > + warnx("%s: trailing data after signature section", fn); > + goto out; > + } > + > + if (strncmp(line, "# End Signature:", > + strlen("# End Signature:")) == 0) { > + end_signature_seen = 1; > + continue; > + } > + > + if (rpki_signature_seen) { > + if (strlen(line) > 72) { Should be 74: 2 for "# " and <= 72 bytes of base64. If you want to keep the line[0] and line[1] check below, you should also check that the line isn't too short: if (linelen < 2 || linelen > 74) { warnx("%s: signature line too long or " "too short", fn); The other option is to keep only the linelen > 74 check and to do if (strncmp(line, "# ", strlen("# ")) != 0) { below. > + warnx("%s: overly long line in signature " > + "section", fn); > + goto out; > + } > + if (line[0] != '#' || line[1] != ' ') { > + warnx("%s: missing '# ' prefix in signature " > + "section", fn); > + goto out; > + } > + > + /* skip over '# ' */ Nit: I'd use double quotes. > + line += 2; > + strlcat(b64, line, b64sz); > + continue; > + } > + > + if (strncmp(line, "# RPKI Signature:", > + strlen("# RPKI Signature:")) == 0) { > + rpki_signature_seen = 1; > + continue; > + } > + > + /* > + * Read the Geofeed CSV records into a BIO to later on > + * calculate the message digest and compare with the one > + * in the detached CMS signature. > + */ > + if (BIO_puts(bio, line) <= 0) > + goto out; > + if (BIO_puts(bio, "\r\n") <= 0) > + goto out; We need to do this here: /* Skip empty lines or commented lines. */ if (linelen == 0 || line[0] == '#') continue; Otherwise geofeed_parse_geoip() will most likely throw an error on a commented line (it'd still accept "#127.0.0.1," or similar) > + > + /* zap comments */ > + delim = memchr(line, '#', strlen(line)); delim = memchr(line, '#', linelen); > + if (delim != NULL) > + *delim = '\0'; > + > + /* Split prefix and location info */ > + delim = memchr(line, ',', strlen(line)); again use linelen. > + if (delim == NULL) > + goto out; > + *delim = '\0'; No comma is not an error. It might be missing if there's no geolocation info for an address. I'd introduce a 'char *loc;' at the top and do: if (delim != NULL) { *delim = '\0'; loc = delim + 1; } else loc = ""; > + > + /* read each prefix */ > + if (!geofeed_parse_geoip(p.res, line, delim + 1)) then if (!geofeed_parse_geoip(p.res, line, loc)) > + goto out; > + } Probably best to stop immediately if we haven't seen both signature markers: if (!rpki_signature_seen || !end_signature_seen) { warnx("%s: absent or invalid signature", fn); goto out; } > + > + if ((base64_decode(b64, strlen(b64), &der, &dersz)) == -1) { > + warnx("base64_decode failed"); warnx("%s: ...", fn); > + goto out; > + } > + > + if (!cms_parse_validate_detached(x509, fn, der, dersz, geofeed_oid, > + bio)) > + goto out; > + > + if (!x509_get_aia(*x509, fn, &p.res->aia)) > + goto out; > + if (!x509_get_aki(*x509, fn, &p.res->aki)) > + goto out; > + if (!x509_get_ski(*x509, fn, &p.res->ski)) > + goto out; > + > + if (p.res->aia == NULL || p.res->aki == NULL || p.res->ski == NULL) { > + warnx("%s: missing AIA, AKI, SIA, or SKI X509 extension", fn); > + goto out; > + } 8 spaces instead of tab > + > + at = X509_get0_notAfter(*x509); > + if (at == NULL) { > + warnx("%s: X509_get0_notAfter failed", fn); > + goto out; > + } > + if (!x509_get_time(at, &p.res->expires)) { > + warnx("%s: ASN1_time_parse failed", fn); > + goto out; > + } > + > + if ((cert = cert_parse_ee_cert(fn, *x509)) == NULL) > + goto out; > + > + if (cert->asz > 0) { > + warnx("%s: superfluous AS Resources extension present", fn); > + goto out; > + } > + > + p.res->valid = valid_geofeed(fn, cert, p.res); > + > + rc = 1; > + out: > + if (rc == 0) { > + geofeed_free(p.res); > + p.res = NULL; > + X509_free(*x509); > + *x509 = NULL; > + } > + cert_free(cert); > + BIO_free(bio); > + > + return p.res; > +} > + > +/* > + * Free what follows a pointer to a geofeed structure. > + * Safe to call with NULL. > + */ > +void > +geofeed_free(struct geofeed *p) > +{ > + if (p == NULL) > + return; You need to walk the list of geoips and free the locs (they were allocated by stravis()): for (i = 0; i < p->geoipsz; i++) free(p->geoips[i]->loc); > + > + free(p->geoips); > + free(p->aia); > + free(p->aki); > + free(p->ski); > + free(p); > +}