rpki-client change way TAL are loaded

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

rpki-client change way TAL are loaded

Claudio Jeker
Change the way TAL files are loaded into rpki-client. Instead of passing
the filename to the parser process and have that one open the file. Do it
in the main process and pass the buffer to the parser. The benefit of this
is that TAL files are not read by the parser and therefor the unveil can
be locked to the base directory early on. In my opinion this is a good
thing.

--
: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 20 Oct 2019 11:18:18 -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,28 @@ 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)
 {
+ 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++)
+ if (!isprint(buf[i]) && !isspace(buf[i]))
+ errx(EXIT_FAILURE, "read: %s: invalid content", file);
+ buf[n] = '\0';
 
  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);
 }
 
 /*
@@ -1020,7 +1035,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 +1116,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 +1415,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 +1460,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 +1471,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 20 Oct 2019 10:16:51 -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;
@@ -50,25 +50,20 @@ tal_parse_stream(const char *fn, FILE *f
 
  /* Begin with the URI section. */
 
- 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';
+ while ((nl = strchr(buf, '\n')) != NULL) {
+ line = buf;
+ *nl = '\0';
+ if (*(nl - 1) == '\r')
+ *(nl - 1) = '\0';
 
- /* Zero-length line is end of section. */
+ /* advance buffer to next line */
+ buf = nl + 1;
 
- if (linelen == 0)
+ /* Zero-length line is end of section. */
+ 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 +72,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 +121,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 +136,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);

Reply | Threaded
Open this post in threaded view
|

Re: rpki-client change way TAL are loaded

Theo de Raadt-2
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.

Reply | Threaded
Open this post in threaded view
|

Re: rpki-client change way TAL are loaded

Claudio Jeker
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.
--
: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);

Reply | Threaded
Open this post in threaded view
|

Re: rpki-client change way TAL are loaded

Claudio Jeker
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);
>