On Mon, Oct 21, 2019 at 02:14:15PM +0200, Claudio Jeker wrote:
> On Sun, Oct 20, 2019 at 12:46:44PM -0600, Theo de Raadt wrote:
> > There has been an update to
> >
> > https://www.ietf.org/rfcdiff?url2=draft-ietf-sidrops-https-tal-05
> >
> > Which permits comments in the tal files. I proposed this at nanog yvr
> > as something which might help the arin tal hangup.
> >
> > queue_add_tal(int fd, struct entityq *q, const char *file, size_t *eid)
> > {
> > + static unsigned char buf[4096];
> > char *nfile;
> > + ssize_t n, i;
> > + int tfd;
> > +
> > + if ((tfd = open(file, O_RDONLY)) == -1)
> > + err(EXIT_FAILURE, "open: %s", file);
> > + n = read(tfd, buf, sizeof(buf));
> > + if (n == -1)
> > + err(EXIT_FAILURE, "read: %s", file);
> > + if (n == sizeof(buf))
> > + errx(EXIT_FAILURE, "read: %s: file too big", file);
> > + for (i = 0; i < n; i++)
> >
> > A commented file might not fit in your buffer. I suggest you minimally
> > parse the file, discarding the comment lines. The remaining tal contents
> > will be small enough to pass to the other process.
>
> I would have preferred to not parse anything in the parent but skipping
> comments makes sense since it keeps the passed buffer small.
>
> Here is an updated diff.
If nobody objects I will commit this later today.
--
:wq Claudio
> Index: extern.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
> retrieving revision 1.9
> diff -u -p -r1.9 extern.h
> --- extern.h 16 Oct 2019 17:43:29 -0000 1.9
> +++ extern.h 20 Oct 2019 10:17:11 -0000
> @@ -229,7 +229,7 @@ extern int verbose;
>
> void tal_buffer(char **, size_t *, size_t *, const struct tal *);
> void tal_free(struct tal *);
> -struct tal *tal_parse(const char *);
> +struct tal *tal_parse(const char *, char *);
> struct tal *tal_read(int);
>
> void cert_buffer(char **, size_t *, size_t *, const struct cert *);
> Index: main.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v
> retrieving revision 1.20
> diff -u -p -r1.20 main.c
> --- main.c 16 Oct 2019 21:43:41 -0000 1.20
> +++ main.c 21 Oct 2019 12:10:15 -0000
> @@ -22,6 +22,7 @@
> #include <sys/wait.h>
>
> #include <assert.h>
> +#include <ctype.h>
> #include <err.h>
> #include <dirent.h>
> #include <fcntl.h>
> @@ -462,14 +463,64 @@ queue_add_from_mft_set(int fd, struct en
> static void
> queue_add_tal(int fd, struct entityq *q, const char *file, size_t *eid)
> {
> - char *nfile;
> + char *nfile, *nbuf, *line = NULL, *buf = NULL;
> + FILE *in;
> + ssize_t n, i;
> + size_t sz = 0, bsz = 0;
> + int optcomment = 1;
> +
> + if ((in = fopen(file, "r")) == NULL)
> + err(EXIT_FAILURE, "fopen: %s", file);
> +
> + while ((n = getline(&line, &sz, in)) != -1) {
> + /* replace CRLF with just LF */
> + if (n > 1 && line[n - 1] == '\n' && line[n - 2] == '\r') {
> + line[n - 2] = '\n';
> + line[n - 1] = '\0';
> + n--;
> + }
> + if (optcomment) {
> + /* if this is comment, just eat the line */
> + if (line[0] == '#')
> + continue;
> + optcomment = 0;
> + /*
> + * Empty line is end of section and needs
> + * to be eaten as well.
> + */
> + if (line[0] == '\n')
> + continue;
> + }
> +
> + /* make sure every line is valid ascii */
> + for (i = 0; i < n; i++)
> + if (!isprint(line[i]) && !isspace(line[i]))
> + errx(EXIT_FAILURE, "getline: %s: "
> + "invalid content", file);
> +
> + /* concat line to buf */
> + if ((nbuf = realloc(buf, bsz + n + 1)) == NULL)
> + err(EXIT_FAILURE, NULL);
> + buf = nbuf;
> + bsz += n + 1;
> + strlcat(buf, line, bsz);
> + /* limit the buffer size */
> + if (bsz > 4096)
> + errx(EXIT_FAILURE, "%s: file too big", file);
> + }
> +
> + free(line);
> + if (ferror(in))
> + err(EXIT_FAILURE, "getline: %s", file);
> + fclose(in);
>
> if ((nfile = strdup(file)) == NULL)
> err(EXIT_FAILURE, "strdup");
>
> /* Not in a repository, so directly add to queue. */
> -
> - entityq_add(fd, q, nfile, RTYPE_TAL, NULL, NULL, NULL, 0, NULL, eid);
> + entityq_add(fd, q, nfile, RTYPE_TAL, NULL, NULL, NULL, 0, buf, eid);
> + /* entityq_add makes a copy of buf */
> + free(buf);
> }
>
> /*
> @@ -1020,7 +1071,6 @@ proc_parser(int fd, int force, int norev
> X509_STORE *store;
> X509_STORE_CTX *ctx;
> struct auth *auths = NULL;
> - int first_tals = 1;
>
> ERR_load_crypto_strings();
> OpenSSL_add_all_ciphers();
> @@ -1102,31 +1152,12 @@ proc_parser(int fd, int force, int norev
> entp = TAILQ_FIRST(&q);
> assert(entp != NULL);
>
> - /*
> - * Extra security.
> - * Our TAL files may be anywhere, but the repository
> - * resources may only be in BASE_DIR.
> - * When we've finished processing TAL files, make sure
> - * that we can only see what's under that.
> - */
> -
> - if (entp->type != RTYPE_TAL && first_tals) {
> - if (unveil(BASE_DIR, "r") == -1)
> - err(EXIT_FAILURE, "%s: unveil", BASE_DIR);
> - if (unveil(NULL, NULL) == -1)
> - err(EXIT_FAILURE, "unveil");
> - first_tals = 0;
> - } else if (entp->type != RTYPE_TAL) {
> - assert(!first_tals);
> - } else if (entp->type == RTYPE_TAL)
> - assert(first_tals);
> -
> entity_buffer_resp(&b, &bsz, &bmax, entp);
>
> switch (entp->type) {
> case RTYPE_TAL:
> assert(!entp->has_dgst);
> - if ((tal = tal_parse(entp->uri)) == NULL)
> + if ((tal = tal_parse(entp->uri, entp->descr)) == NULL)
> goto out;
> tal_buffer(&b, &bsz, &bmax, tal);
> tal_free(tal);
> @@ -1420,7 +1451,12 @@ main(int argc, char *argv[])
>
> if (procpid == 0) {
> close(fd[1]);
> - if (pledge("stdio rpath unveil", NULL) == -1)
> + /* Only allow access to BASE_DIR. */
> + if (unveil(BASE_DIR, "r") == -1)
> + err(EXIT_FAILURE, "%s: unveil", BASE_DIR);
> + if (unveil(NULL, NULL) == -1)
> + err(EXIT_FAILURE, "unveil");
> + if (pledge("stdio rpath", NULL) == -1)
> err(EXIT_FAILURE, "pledge");
> proc_parser(fd[0], force, norev);
> /* NOTREACHED */
> @@ -1460,13 +1496,7 @@ main(int argc, char *argv[])
>
> assert(rsync != proc);
>
> - /*
> - * The main process drives the top-down scan to leaf ROAs using
> - * data downloaded by the rsync process and parsed by the
> - * parsing process.
> - */
> -
> - if (pledge("stdio", NULL) == -1)
> + if (pledge("stdio rpath", NULL) == -1)
> err(EXIT_FAILURE, "pledge");
>
> /*
> @@ -1477,6 +1507,15 @@ main(int argc, char *argv[])
>
> for (i = 0; i < talsz; i++)
> queue_add_tal(proc, &q, tals[i], &eid);
> +
> + /*
> + * The main process drives the top-down scan to leaf ROAs using
> + * data downloaded by the rsync process and parsed by the
> + * parsing process.
> + */
> +
> + if (pledge("stdio", NULL) == -1)
> + err(EXIT_FAILURE, "pledge");
>
> pfd[0].fd = rsync;
> pfd[1].fd = proc;
> Index: rsync.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/rsync.c,v
> retrieving revision 1.6
> diff -u -p -r1.6 rsync.c
> --- rsync.c 19 Jun 2019 16:30:37 -0000 1.6
> +++ rsync.c 20 Oct 2019 16:34:19 -0000
> @@ -129,8 +129,6 @@ rsync_uri_parse(const char **hostp, size
> *rtypep = RTYPE_CER;
> else if (strcasecmp(path + sz - 4, ".crl") == 0)
> *rtypep = RTYPE_CRL;
> - else if (strcasecmp(path + sz - 4, ".tal") == 0)
> - *rtypep = RTYPE_TAL;
> }
>
> return 1;
> Index: tal.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/tal.c,v
> retrieving revision 1.7
> diff -u -p -r1.7 tal.c
> --- tal.c 8 Oct 2019 10:04:36 -0000 1.7
> +++ tal.c 21 Oct 2019 10:09:27 -0000
> @@ -29,18 +29,18 @@
> #include "extern.h"
>
> /*
> - * Inner function for parsing RFC 7730 from a file stream.
> + * Inner function for parsing RFC 7730 from a buffer.
> * Returns a valid pointer on success, NULL otherwise.
> * The pointer must be freed with tal_free().
> */
> static struct tal *
> -tal_parse_stream(const char *fn, FILE *f)
> +tal_parse_buffer(const char *fn, char *buf)
> {
> - char *line = NULL;
> + char *nl, *line;
> unsigned char *b64 = NULL;
> - size_t sz, b64sz = 0, linesize = 0, lineno = 0;
> - ssize_t linelen, ssz;
> - int rc = 0;
> + size_t sz;
> + ssize_t linelen;
> + int rc = 0, b64sz;
> struct tal *tal = NULL;
> enum rtype rp;
> EVP_PKEY *pkey = NULL;
> @@ -48,27 +48,19 @@ tal_parse_stream(const char *fn, FILE *f
> if ((tal = calloc(1, sizeof(struct tal))) == NULL)
> err(EXIT_FAILURE, NULL);
>
> - /* Begin with the URI section. */
> + /* Begin with the URI section, comment section already removed. */
> + while ((nl = strchr(buf, '\n')) != NULL) {
> + line = buf;
> + *nl = '\0';
>
> - while ((linelen = getline(&line, &linesize, f)) != -1) {
> - lineno++;
> - assert(linelen);
> - if (line[linelen - 1] != '\n') {
> - warnx("%s: RFC 7730 section 2.1: "
> - "failed to parse URL", fn);
> - goto out;
> - }
> - line[--linelen] = '\0';
> - if (linelen && line[linelen - 1] == '\r')
> - line[--linelen] = '\0';
> + /* advance buffer to next line */
> + buf = nl + 1;
>
> /* Zero-length line is end of section. */
> -
> - if (linelen == 0)
> + if (*line == '\0')
> break;
>
> /* Append to list of URIs. */
> -
> tal->uri = reallocarray(tal->uri,
> tal->urisz + 1, sizeof(char *));
> if (tal->uri == NULL)
> @@ -77,85 +69,48 @@ tal_parse_stream(const char *fn, FILE *f
> tal->uri[tal->urisz] = strdup(line);
> if (tal->uri[tal->urisz] == NULL)
> err(EXIT_FAILURE, NULL);
> -
> tal->urisz++;
>
> /* Make sure we're a proper rsync URI. */
> -
> if (!rsync_uri_parse(NULL, NULL,
> NULL, NULL, NULL, NULL, &rp, line)) {
> warnx("%s: RFC 7730 section 2.1: "
> "failed to parse URL: %s", fn, line);
> goto out;
> - } else if (rp != RTYPE_CER) {
> + }
> + if (rp != RTYPE_CER) {
> warnx("%s: RFC 7730 section 2.1: "
> "not a certificate URL: %s", fn, line);
> goto out;
> }
> - }
>
> - if (ferror(f))
> - err(EXIT_FAILURE, "%s: getline", fn);
> + }
>
> if (tal->urisz == 0) {
> warnx("%s: no URIs in manifest part", fn);
> goto out;
> - } else if (tal->urisz > 1) {
> + } else if (tal->urisz > 1)
> warnx("%s: multiple URIs: using the first", fn);
> - goto out;
> - }
> -
> - /* Now the BASE64-encoded public key. */
> -
> - while ((linelen = getline(&line, &linesize, f)) != -1) {
> - lineno++;
> - assert(linelen);
> - if (line[linelen - 1] != '\n') {
> - warnx("%s: RFC 7730 section 2.1: "
> - "failed to parse public key", fn);
> - goto out;
> - }
> - line[--linelen] = '\0';
> - if (linelen && line[linelen - 1] == '\r')
> - line[--linelen] = '\0';
> -
> - /* Zero-length line can be ignored... ? */
> -
> - if (linelen == 0)
> - continue;
> -
> - /* Do our base64 decoding in-band. */
> -
> - sz = ((linelen + 2) / 3) * 4 + 1;
> - if ((b64 = realloc(b64, b64sz + sz)) == NULL)
> - err(EXIT_FAILURE, NULL);
> - if ((ssz = b64_pton(line, b64 + b64sz, sz)) < 0)
> - errx(EXIT_FAILURE, "b64_pton");
> -
> - /*
> - * This might be different from our allocation size, but
> - * that doesn't matter: the slop here is minimal.
> - */
>
> - b64sz += ssz;
> - }
> -
> - if (ferror(f))
> - err(EXIT_FAILURE, "%s: getline", fn);
> -
> - if (b64sz == 0) {
> + sz = strlen(buf);
> + if (sz == 0) {
> warnx("%s: RFC 7730 section 2.1: subjectPublicKeyInfo: "
> "zero-length public key", fn);
> goto out;
> }
>
> + /* Now the BASE64-encoded public key. */
> + sz = ((sz + 2) / 3) * 4 + 1;
> + if ((b64 = malloc(sz)) == NULL)
> + err(EXIT_FAILURE, NULL);
> + if ((b64sz = b64_pton(buf, b64, sz)) < 0)
> + errx(EXIT_FAILURE, "b64_pton");
> +
> tal->pkey = b64;
> tal->pkeysz = b64sz;
>
> /* Make sure it's a valid public key. */
> -
> pkey = d2i_PUBKEY(NULL, (const unsigned char **)&b64, b64sz);
> - b64 = NULL;
> if (pkey == NULL) {
> cryptowarnx("%s: RFC 7730 section 2.1: subjectPublicKeyInfo: "
> "failed public key parse", fn);
> @@ -163,8 +118,6 @@ tal_parse_stream(const char *fn, FILE *f
> }
> rc = 1;
> out:
> - free(line);
> - free(b64);
> if (rc == 0) {
> tal_free(tal);
> tal = NULL;
> @@ -180,18 +133,13 @@ out:
> * memory, bad syntax, etc.
> */
> struct tal *
> -tal_parse(const char *fn)
> +tal_parse(const char *fn, char *buf)
> {
> - FILE *f;
> struct tal *p;
> char *d;
> size_t dlen;
>
> - if ((f = fopen(fn, "r")) == NULL)
> - err(EXIT_FAILURE, "%s: open", fn);
> -
> - p = tal_parse_stream(fn, f);
> - fclose(f);
> + p = tal_parse_buffer(fn, buf);
>
> /* extract the TAL basename (without .tal suffix) */
> d = basename(fn);
>
