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);
> +}
