Trade unveil(2) for chroot(2).

This uses less code and unveil(2) seems to be the better tool here.
The directory one chroots into needs to be carefully setup (they are
not) and comon wisedom is that root can break out of chroots.
There is probably nothing wrong with the chroot code because of pledge
but it still makes me feel uneasy.
input & OK on previous version mestre
OK on previous version deraadt
bug found, input & OK benno

5 files changed, 70 insertions, 91 deletions

diff --git a/usr.sbin/acme-client/chngproc.c b/usr.sbin/acme-client/chngproc.c
index 218b608000d..8b255647bed 100644
--- a/usr.sbin/acme-client/chngproc.c
+++ b/usr.sbin/acme-client/chngproc.c
@@ -1,4 +1,4 @@
-/*	$Id: chngproc.c,v 1.13 2019/04/01 04:18:54 naddy Exp $ */
+/*	$Id: chngproc.c,v 1.14 2019/06/16 19:49:13 florian Exp $ */
 /*
  * Copyright (c) 2016 Kristaps Dzonsons <kristaps@bsd.lv>
  *
@@ -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, "wc") == -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 a/usr.sbin/acme-client/extern.h b/usr.sbin/acme-client/extern.h
index d533466fbe6..b2d2e47f1d7 100644
--- a/usr.sbin/acme-client/extern.h
+++ b/usr.sbin/acme-client/extern.h
@@ -1,4 +1,4 @@
-/*	$Id: extern.h,v 1.14 2019/06/14 19:55:08 florian Exp $ */
+/*	$Id: extern.h,v 1.15 2019/06/16 19:49:13 florian Exp $ */
 /*
  * Copyright (c) 2016 Kristaps Dzonsons <kristaps@bsd.lv>
  *
@@ -203,8 +203,8 @@ int		 acctproc(int, const char *);
 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 a/usr.sbin/acme-client/fileproc.c b/usr.sbin/acme-client/fileproc.c
index 00ce339670a..b7cdff5525d 100644
--- a/usr.sbin/acme-client/fileproc.c
+++ b/usr.sbin/acme-client/fileproc.c
@@ -1,4 +1,4 @@
-/*	$Id: fileproc.c,v 1.15 2018/07/29 20:15:23 benno Exp $ */
+/*	$Id: fileproc.c,v 1.16 2019/06/16 19:49:13 florian Exp $ */
 /*
  * Copyright (c) 2016 Kristaps Dzonsons <kristaps@bsd.lv>
  *
@@ -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 a/usr.sbin/acme-client/main.c b/usr.sbin/acme-client/main.c
index 1352ad0a1da..a409e84fc9a 100644
--- a/usr.sbin/acme-client/main.c
+++ b/usr.sbin/acme-client/main.c
@@ -1,4 +1,4 @@
-/*	$Id: main.c,v 1.50 2019/06/16 07:24:28 florian Exp $ */
+/*	$Id: main.c,v 1.51 2019/06/16 19:49:13 florian Exp $ */
 /*
  * Copyright (c) 2016 Kristaps Dzonsons <kristaps@bsd.lv>
  *
@@ -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,31 +110,21 @@ 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");
+	/* chain or fullchain can be relative paths according */
+	if (domain->chain && domain->chain[0] != '/') {
+		if (asprintf(&tmps, "%s/%s", certdir, domain->chain) == -1)
+			err(EXIT_FAILURE, "asprintf");
+		free(domain->chain);
+		domain->chain = tmps;
+		tmps = NULL;
 	}
-
-	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 (domain->fullchain && domain->fullchain[0] != '/') {
+		if (asprintf(&tmps, "%s/%s", certdir, domain->fullchain) == -1)
+			err(EXIT_FAILURE, "asprintf");
+		free(domain->fullchain);
+		domain->fullchain = tmps;
+		tmps = NULL;
 	}
 
 	if ((auth = domain->auth) == NULL) {
@@ -320,8 +312,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.
@@ -352,9 +344,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 a/usr.sbin/acme-client/revokeproc.c b/usr.sbin/acme-client/revokeproc.c
index 273496c6c74..ce67174b5a5 100644
--- a/usr.sbin/acme-client/revokeproc.c
+++ b/usr.sbin/acme-client/revokeproc.c
@@ -1,4 +1,4 @@
-/*	$Id: revokeproc.c,v 1.14 2018/07/28 15:25:23 tb Exp $ */
+/*	$Id: revokeproc.c,v 1.15 2019/06/16 19:49:13 florian Exp $ */
 /*
  * Copyright (c) 2016 Kristaps Dzonsons <kristaps@bsd.lv>
  *
@@ -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);