acme-client(1): use unveil(2) instead of chroot(2)

classic Classic list List threaded Threaded
1 message Options
Reply | Threaded
Open this post in threaded view
|

acme-client(1): use unveil(2) instead of chroot(2)

Florian Obser-2
This uses less code and unveil(2) seems to be the better tool here.

There is probably nothing wrong with the chroot code because of pledge
but it still makes me feel uneasy.

The directory one chroots into needs to be carefully setup (they are
not) and comon wisedom is that root can break out of chroots.

Again, pledge probably takes away all the necessary tools to mess with
the chroot...

OK?

diff --git chngproc.c chngproc.c
index 218b608000d..bb3cea6710e 100644
--- chngproc.c
+++ chngproc.c
@@ -36,14 +36,12 @@ chngproc(int netsock, const char *root)
  enum chngop  op;
  void *pp;
 
- if (chroot(root) == -1) {
- warn("chroot");
- goto out;
- }
- if (chdir("/") == -1) {
- warn("chdir");
+
+ if (unveil(root, "rwc") == -1) {
+ warn("unveil");
  goto out;
  }
+
  if (pledge("stdio cpath wpath", NULL) == -1) {
  warn("pledge");
  goto out;
@@ -80,6 +78,11 @@ chngproc(int netsock, const char *root)
  else if ((tok = readstr(netsock, COMM_TOK)) == NULL)
  goto out;
 
+ if (asprintf(&fmt, "%s.%s", tok, th) == -1) {
+ warn("asprintf");
+ goto out;
+ }
+
  /* Vector appending... */
 
  pp = reallocarray(fs, (fsz + 1), sizeof(char *));
@@ -88,14 +91,13 @@ chngproc(int netsock, const char *root)
  goto out;
  }
  fs = pp;
- fs[fsz] = tok;
- tok = NULL;
- fsz++;
-
- if (asprintf(&fmt, "%s.%s", fs[fsz - 1], th) == -1) {
+ if (asprintf(&fs[fsz], "%s/%s", root, tok) == -1) {
  warn("asprintf");
  goto out;
  }
+ fsz++;
+ free(tok);
+ tok = NULL;
 
  /*
  * Create and write to our challenge file.
@@ -121,7 +123,7 @@ chngproc(int netsock, const char *root)
  free(fmt);
  th = fmt = NULL;
 
- dodbg("%s/%s: created", root, fs[fsz - 1]);
+ dodbg("%s: created", fs[fsz - 1]);
 
  /*
  * Write our acknowledgement.
diff --git extern.h extern.h
index 7fcbf77d88f..ae18b133b29 100644
--- extern.h
+++ extern.h
@@ -203,8 +203,8 @@ int acctproc(int, const char *, enum keytype);
 int certproc(int, int);
 int chngproc(int, const char *);
 int dnsproc(int);
-int revokeproc(int, const char *, const char *,
- int, int, const char *const *, size_t);
+int revokeproc(int, const char *, int, int, const char *const *,
+ size_t);
 int fileproc(int, const char *, const char *, const char *,
  const char *);
 int keyproc(int, const char *, const char **, size_t,
diff --git fileproc.c fileproc.c
index 00ce339670a..c09da6244ac 100644
--- fileproc.c
+++ fileproc.c
@@ -86,12 +86,8 @@ fileproc(int certsock, const char *certdir, const char *certfile, const char
  long lval;
  enum fileop op;
 
- if (chroot(certdir) == -1) {
- warn("chroot");
- goto out;
- }
- if (chdir("/") == -1) {
- warn("chdir");
+ if (unveil(certdir, "rwc") == -1) {
+ warn("unveil");
  goto out;
  }
 
@@ -129,27 +125,26 @@ fileproc(int certsock, const char *certdir, const char *certfile, const char
  if (FILE_REMOVE == op) {
  if (certfile) {
  if (unlink(certfile) == -1 && errno != ENOENT) {
- warn("%s/%s", certdir, certfile);
+ warn("%s", certfile);
  goto out;
  } else
- dodbg("%s/%s: unlinked", certdir, certfile);
+ dodbg("%s: unlinked", certfile);
  }
 
  if (chainfile) {
  if (unlink(chainfile) == -1 && errno != ENOENT) {
- warn("%s/%s", certdir, chainfile);
+ warn("%s", chainfile);
  goto out;
  } else
- dodbg("%s/%s: unlinked", certdir, chainfile);
+ dodbg("%s: unlinked", chainfile);
  }
 
  if (fullchainfile) {
  if (unlink(fullchainfile) == -1 && errno != ENOENT) {
- warn("%s/%s", certdir, fullchainfile);
+ warn("%s", fullchainfile);
  goto out;
  } else
- dodbg("%s/%s: unlinked", certdir,
-    fullchainfile);
+ dodbg("%s: unlinked", fullchainfile);
  }
 
  rc = 2;
@@ -168,7 +163,7 @@ fileproc(int certsock, const char *certdir, const char *certfile, const char
  if (!serialise(chainfile, ch, chsz, NULL, 0))
  goto out;
 
- dodbg("%s/%s: created", certdir, chainfile);
+ dodbg("%s: created", chainfile);
  }
 
  /*
@@ -185,7 +180,7 @@ fileproc(int certsock, const char *certdir, const char *certfile, const char
  if (!serialise(certfile, csr, csz, NULL, 0))
  goto out;
 
- dodbg("%s/%s: created", certdir, certfile);
+ dodbg("%s: created", certfile);
  }
 
  /*
@@ -199,7 +194,7 @@ fileproc(int certsock, const char *certdir, const char *certfile, const char
     chsz))
  goto out;
 
- dodbg("%s/%s: created", certdir, fullchainfile);
+ dodbg("%s: created", fullchainfile);
  }
 
  rc = 2;
diff --git main.c main.c
index a49415e1610..c2abd1c417f 100644
--- main.c
+++ main.c
@@ -36,8 +36,7 @@ int
 main(int argc, char *argv[])
 {
  const char **alts = NULL;
- char *certdir = NULL, *certfile = NULL;
- char *chainfile = NULL, *fullchainfile = NULL;
+ char *certdir = NULL;
  char *chngdir = NULL, *auth = NULL;
  char *conffile = CONF_FILE;
  char *tmps, *tmpsd;
@@ -97,7 +96,10 @@ main(int argc, char *argv[])
  argc--;
  argv++;
 
- /* the parser enforces that at least cert or fullchain is set */
+ /*
+ * The parser enforces that at least cert or fullchain is set.
+ * XXX Test if cert, chain and fullchain have the same dirname?
+ */
  tmps = domain->cert ? domain->cert : domain->fullchain;
  if ((tmps = strdup(tmps)) == NULL)
  err(EXIT_FAILURE, "strdup");
@@ -108,33 +110,6 @@ main(int argc, char *argv[])
  free(tmps);
  tmps = tmpsd = NULL;
 
- if (domain->cert != NULL) {
- if ((tmps = strdup(domain->cert)) == NULL)
- err(EXIT_FAILURE, "strdup");
- if ((certfile = basename(tmps)) == NULL)
- err(EXIT_FAILURE, "basename");
- if ((certfile = strdup(certfile)) == NULL)
- err(EXIT_FAILURE, "strdup");
- }
-
- if (domain->chain != NULL) {
- if ((tmps = strdup(domain->chain)) == NULL)
- err(EXIT_FAILURE, "strdup");
- if ((chainfile = basename(tmps)) == NULL)
- err(EXIT_FAILURE, "basename");
- if ((chainfile = strdup(chainfile)) == NULL)
- err(EXIT_FAILURE, "strdup");
- }
-
- if (domain->fullchain != NULL) {
- if ((tmps = strdup(domain->fullchain)) == NULL)
- err(EXIT_FAILURE, "strdup");
- if ((fullchainfile = basename(tmps)) == NULL)
- err(EXIT_FAILURE, "basename");
- if ((fullchainfile = strdup(fullchainfile)) == NULL)
- err(EXIT_FAILURE, "strdup");
- }
-
  if ((auth = domain->auth) == NULL) {
  /* use the first authority from the config as default XXX */
  authority = authority_find0(conf);
@@ -321,8 +296,8 @@ main(int argc, char *argv[])
  proccomp = COMP_FILE;
  close(dns_fds[0]);
  close(rvk_fds[0]);
- c = fileproc(file_fds[1], certdir, certfile, chainfile,
-    fullchainfile);
+ c = fileproc(file_fds[1], certdir, domain->cert, domain->chain,
+    domain->fullchain);
  /*
  * This is different from the other processes in that it
  * can return 2 if the certificates were updated.
@@ -353,9 +328,8 @@ main(int argc, char *argv[])
 
  if (pids[COMP_REVOKE] == 0) {
  proccomp = COMP_REVOKE;
- c = revokeproc(rvk_fds[0], certdir,
-    certfile != NULL ? certfile : fullchainfile,
-    force, revocate,
+ c = revokeproc(rvk_fds[0], domain->cert != NULL ? domain->cert :
+    domain->fullchain, force, revocate,
     (const char *const *)alts, altsz);
  exit(c ? EXIT_SUCCESS : EXIT_FAILURE);
  }
diff --git revokeproc.c revokeproc.c
index 273496c6c74..c0d2e1640c2 100644
--- revokeproc.c
+++ revokeproc.c
@@ -91,10 +91,10 @@ X509expires(X509 *x)
 }
 
 int
-revokeproc(int fd, const char *certdir, const char *certfile, int force,
+revokeproc(int fd, const char *certfile, int force,
     int revocate, const char *const *alts, size_t altsz)
 {
- char *path = NULL, *der = NULL, *dercp, *der64 = NULL;
+ char *der = NULL, *dercp, *der64 = NULL;
  char *san = NULL, *str, *tok;
  int rc = 0, cc, i, extsz, ssz, len;
  size_t *found = NULL;
@@ -114,11 +114,8 @@ revokeproc(int fd, const char *certdir, const char *certfile, int force,
  * We allow "f" to be NULL IFF the cert doesn't exist yet.
  */
 
- if (asprintf(&path, "%s/%s", certdir, certfile) == -1) {
- warn("asprintf");
- goto out;
- } else if ((f = fopen(path, "r")) == NULL && errno != ENOENT) {
- warn("%s", path);
+ if ((f = fopen(certfile, "r")) == NULL && errno != ENOENT) {
+ warn("%s", certfile);
  goto out;
  }
 
@@ -140,7 +137,7 @@ revokeproc(int fd, const char *certdir, const char *certfile, int force,
  */
 
  if (f == NULL && revocate) {
- warnx("%s/%s: no certificate found", certdir, certfile);
+ warnx("%s: no certificate found", certfile);
  (void)writeop(fd, COMM_REVOKE_RESP, REVOKE_OK);
  goto out;
  } else if (f == NULL && !revocate) {
@@ -181,7 +178,7 @@ revokeproc(int fd, const char *certdir, const char *certfile, int force,
  continue;
 
  if (san != NULL) {
- warnx("%s/%s: two SAN entries", certdir, certfile);
+ warnx("%s: two SAN entries", certfile);
  goto out;
  }
 
@@ -204,7 +201,7 @@ revokeproc(int fd, const char *certdir, const char *certfile, int force,
  }
 
  if (san == NULL) {
- warnx("%s/%s: does not have a SAN entry", certdir, certfile);
+ warnx("%s: does not have a SAN entry", certfile);
  goto out;
  }
 
@@ -233,13 +230,11 @@ revokeproc(int fd, const char *certdir, const char *certfile, int force,
  if (strcmp(tok, alts[j]) == 0)
  break;
  if (j == altsz) {
- warnx("%s/%s: unknown SAN entry: %s",
-    certdir, certfile, tok);
+ warnx("%s: unknown SAN entry: %s", certfile, tok);
  goto out;
  }
  if (found[j]++) {
- warnx("%s/%s: duplicate SAN entry: %s",
-    certdir, certfile, tok);
+ warnx("%s: duplicate SAN entry: %s", certfile, tok);
  goto out;
  }
  }
@@ -247,8 +242,7 @@ revokeproc(int fd, const char *certdir, const char *certfile, int force,
  for (j = 0; j < altsz; j++) {
  if (found[j])
  continue;
- warnx("%s/%s: domain not listed: %s",
-    certdir, certfile, alts[j]);
+ warnx("%s: domain not listed: %s", certfile, alts[j]);
  goto out;
  }
 
@@ -259,7 +253,7 @@ revokeproc(int fd, const char *certdir, const char *certfile, int force,
  */
 
  if (revocate) {
- dodbg("%s/%s: revocation", certdir, certfile);
+ dodbg("%s: revocation", certfile);
 
  /*
  * First, tell netproc we're online.
@@ -293,16 +287,14 @@ revokeproc(int fd, const char *certdir, const char *certfile, int force,
  rop = time(NULL) >= (t - RENEW_ALLOW) ? REVOKE_EXP : REVOKE_OK;
 
  if (rop == REVOKE_EXP)
- dodbg("%s/%s: certificate renewable: %lld days left",
-    certdir, certfile,
-    (long long)(t - time(NULL)) / 24 / 60 / 60);
+ dodbg("%s: certificate renewable: %lld days left",
+    certfile, (long long)(t - time(NULL)) / 24 / 60 / 60);
  else
- dodbg("%s/%s: certificate valid: %lld days left",
-    certdir, certfile,
-    (long long)(t - time(NULL)) / 24 / 60 / 60);
+ dodbg("%s: certificate valid: %lld days left",
+    certfile, (long long)(t - time(NULL)) / 24 / 60 / 60);
 
  if (rop == REVOKE_OK && force) {
- warnx("%s/%s: forcing renewal", certdir, certfile);
+ warnx("%s: forcing renewal", certfile);
  rop = REVOKE_EXP;
  }
 
@@ -338,7 +330,6 @@ out:
  X509_free(x);
  BIO_free(bio);
  free(san);
- free(path);
  free(der);
  free(found);
  free(der64);


--
I'm not entirely sure you are real.