Teach ntpd(8) constraint process to use exec*() instead of just forking,

with this change we get the pledge() ability back to the parent process.
some tweaks from and ok reyk@

src/usr.sbin/ntpd/constraint.c

@@ -1,4 +1,4 @@
-/*	$OpenBSD: constraint.c,v 1.31 2016/09/14 09:26:10 reyk Exp $	*/
+/*	$OpenBSD: constraint.c,v 1.32 2016/09/26 17:17:01 rzalamena Exp $	*/
 
 /*
  * Copyright (c) 2015 Reyk Floeter <reyk@openbsd.org>
@@ -57,8 +57,8 @@ void	 constraint_reset(void);
 int	 constraint_cmp(const void *, const void *);
 
 void	 priv_constraint_close(int, int);
-void	 priv_constraint_child(struct constraint *, struct ntp_addr_msg *,
-	    u_int8_t *, int[2], const char *, uid_t, gid_t);
+void	 priv_constraint_readquery(struct constraint *, struct ntp_addr_msg *,
+	    uint8_t **);
 
 struct httpsdate *
 	 httpsdate_init(const char *, const char *, const char *,
@@ -210,8 +210,8 @@ constraint_query(struct constraint *cstr)
 }
 
 void
-priv_constraint_msg(u_int32_t id, u_int8_t *data, size_t len,
-    const char *pw_dir, uid_t pw_uid, gid_t pw_gid)
+priv_constraint_msg(u_int32_t id, u_int8_t *data, size_t len, int argc,
+    char **argv)
 {
 	struct ntp_addr_msg	 am;
 	struct ntp_addr		*h;
@@ -246,40 +246,80 @@ priv_constraint_msg(u_int32_t id, u_int8_t *data, size_t len,
 	constraint_add(cstr);
 	constraint_cnt++;
 
-	if (socketpair(AF_UNIX, SOCK_DGRAM, AF_UNSPEC, pipes) == -1)
+	if (socketpair(AF_UNIX, SOCK_DGRAM | SOCK_CLOEXEC, AF_UNSPEC,
+	    pipes) == -1)
 		fatal("%s pipes", __func__);
 
+	/* Prepare and send constraint data to child. */
+	cstr->fd = pipes[0];
+	imsg_init(&cstr->ibuf, cstr->fd);
+	if (imsg_compose(&cstr->ibuf, IMSG_CONSTRAINT_QUERY, id, 0, -1,
+	    data, len) == -1)
+		fatal("%s: imsg_compose", __func__);
+	if (imsg_flush(&cstr->ibuf) == -1)
+		fatal("%s: imsg_flush", __func__);
+
 	/*
 	 * Fork child handlers and make sure to do any sensitive work in the
 	 * the (unprivileged) child.  The parent should not do any parsing,
 	 * certificate loading etc.
 	 */
-	switch (cstr->pid = fork()) {
-	case -1:
-		cstr->senderrors++;
-		close(pipes[0]);
-		close(pipes[1]);
-		return;
-	case 0:
-		priv_constraint_child(cstr, &am, data + sizeof(am), pipes,
-		    pw_dir, pw_uid, pw_gid);
+	start_child(CONSTRAINT_PROC_NAME, pipes[1], argc, argv);
+}
 
-		_exit(0);
-		/* NOTREACHED */
-	default:
-		/* Parent */
-		close(pipes[1]);
-		cstr->fd = pipes[0];
+void
+priv_constraint_readquery(struct constraint *cstr, struct ntp_addr_msg *am,
+    uint8_t **data)
+{
+	struct ntp_addr		*h;
+	uint8_t			*dptr;
+	int			 n;
+	struct imsg		 imsg;
+	size_t			 mlen;
 
-		imsg_init(&cstr->ibuf, cstr->fd);
-		break;
-	}
+	/* Read the message our parent left us. */
+	if (((n = imsg_read(&cstr->ibuf)) == -1 && errno != EAGAIN) || n == 0)
+		fatal("%s: imsg_read", __func__);
+	if (((n = imsg_get(&cstr->ibuf, &imsg)) == -1) || n == 0)
+		fatal("%s: imsg_get", __func__);
+	if (imsg.hdr.type != IMSG_CONSTRAINT_QUERY)
+		fatalx("%s: invalid message type", __func__);
+
+	/*
+	 * Copy the message contents just like our father:
+	 * priv_constraint_msg().
+	 */
+	mlen = imsg.hdr.len - IMSG_HEADER_SIZE;
+	if (mlen < sizeof(*am))
+		fatalx("%s: mlen < sizeof(*am)", __func__);
+
+	memcpy(am, imsg.data, sizeof(*am));
+	if (mlen != (sizeof(*am) + am->namelen + am->pathlen))
+		fatalx("%s: mlen < sizeof(*am) + am->namelen + am->pathlen",
+		    __func__);
+
+	if ((h = calloc(1, sizeof(*h))) == NULL ||
+	    (*data = calloc(1, mlen)) == NULL)
+		fatal("%s: calloc", __func__);
+
+	memcpy(h, &am->a, sizeof(*h));
+	h->next = NULL;
+
+	cstr->id = imsg.hdr.peerid;
+	cstr->addr = h;
+	cstr->addr_head.a = h;
+
+	dptr = imsg.data;
+	memcpy(*data, dptr + sizeof(*am), mlen - sizeof(*am));
+	imsg_free(&imsg);
 }
 
 void
-priv_constraint_child(struct constraint *cstr, struct ntp_addr_msg *am,
-    u_int8_t *data, int pipes[2], const char *pw_dir, uid_t pw_uid, gid_t pw_gid)
+priv_constraint_child(const char *pw_dir, uid_t pw_uid, gid_t pw_gid)
 {
+	struct constraint	*cstr;
+	struct ntp_addr_msg	*am;
+	uint8_t			*data;
 	static char		 addr[NI_MAXHOST];
 	struct timeval		 rectv, xmttv;
 	struct sigaction	 sa;
@@ -292,6 +332,10 @@ priv_constraint_child(struct constraint *cstr, struct ntp_addr_msg *am,
 	if (setpriority(PRIO_PROCESS, 0, 0) == -1)
 		log_warn("could not set priority");
 
+	if ((cstr = calloc(1, sizeof(*cstr))) == NULL ||
+	    (am = calloc(1, sizeof(*am))) == NULL)
+		fatal("%s: calloc", __func__);
+
 	/* Init TLS and load CA certs before chroot() */
 	if (tls_init() == -1)
 		fatalx("tls_init");
@@ -320,6 +364,10 @@ priv_constraint_child(struct constraint *cstr, struct ntp_addr_msg *am,
 	if (pledge("stdio inet", NULL) == -1)
 		fatal("pledge");
 
+	cstr->fd = CONSTRAINT_PASSFD;
+	imsg_init(&cstr->ibuf, cstr->fd);
+	priv_constraint_readquery(cstr, am, &data);
+
 	/*
 	 * Get the IP address as name and set the process title accordingly.
 	 * This only converts an address into a string and does not trigger
@@ -334,14 +382,6 @@ priv_constraint_child(struct constraint *cstr, struct ntp_addr_msg *am,
 
 	log_debug("constraint request to %s", addr);
 	setproctitle("constraint from %s", addr);
-
-	/* Set file descriptors */
-	if (dup2(pipes[1], CONSTRAINT_PASSFD) == -1)
-		fatal("%s dup2 CONSTRAINT_PASSFD", __func__);
-	if (pipes[0] != CONSTRAINT_PASSFD)
-		close(pipes[0]);
-	if (pipes[1] != CONSTRAINT_PASSFD)
-		close(pipes[1]);
 	(void)closefrom(CONSTRAINT_PASSFD + 1);
 
 	/*
@@ -353,9 +393,6 @@ priv_constraint_child(struct constraint *cstr, struct ntp_addr_msg *am,
 	if (fcntl(CONSTRAINT_PASSFD, F_SETFD, FD_CLOEXEC) == -1)
 		fatal("%s fcntl F_SETFD", __func__);
 
-	cstr->fd = CONSTRAINT_PASSFD;
-	imsg_init(&cstr->ibuf, cstr->fd);
-
 	/* Get remaining data from imsg in the unpriv child */
 	if (am->namelen) {
 		if ((cstr->addr_head.name =
@@ -387,6 +424,8 @@ priv_constraint_child(struct constraint *cstr, struct ntp_addr_msg *am,
 
 	/* Tear down the TLS connection after sending the result */
 	httpsdate_free(ctx);
+
+	exit(0);
 }
 
 void

src/usr.sbin/ntpd/ntpd.c

@@ -1,4 +1,4 @@
-/*	$OpenBSD: ntpd.c,v 1.110 2016/09/26 16:55:02 rzalamena Exp $ */
+/*	$OpenBSD: ntpd.c,v 1.111 2016/09/26 17:17:01 rzalamena Exp $ */
 
 /*
  * Copyright (c) 2003, 2004 Henning Brauer <henning@openbsd.org>
@@ -42,7 +42,7 @@ void		sighdlr(int);
 __dead void	usage(void);
 int		main(int, char *[]);
 void		check_child(void);
-int		dispatch_imsg(struct ntpd_conf *, const char *, uid_t, gid_t);
+int		dispatch_imsg(struct ntpd_conf *, int, char **);
 int		dispatch_imsg_ctl(struct ntpd_conf *);
 void		reset_adjtime(void);
 int		ntpd_adjtime(double);
@@ -196,6 +196,9 @@ main(int argc, char *argv[])
 			ntp_main(&lconf, pw, argc0, argv0);
 		else if (strcmp(NTPDNS_PROC_NAME, pname) == 0)
 			ntp_dns(&lconf, pw);
+		else if (strcmp(CONSTRAINT_PROC_NAME, pname) == 0)
+			priv_constraint_child(pw->pw_dir, pw->pw_uid,
+			    pw->pw_gid);
 		else
 			fatalx("%s: invalid process name '%s'", __func__,
 			    pname);
@@ -248,10 +251,8 @@ main(int argc, char *argv[])
 	 * Constraint processes are forked with certificates in memory,
 	 * then privdrop into chroot before speaking to the outside world.
 	 */
-#if 0	
-	if (pledge("stdio rpath inet settime proc id", NULL) == -1)
+	if (pledge("stdio rpath inet settime proc exec id", NULL) == -1)
 		err(1, "pledge");
-#endif
 
 	while (quit == 0) {
 		new_cnt = PFD_MAX + constraint_cnt;
@@ -306,7 +307,7 @@ main(int argc, char *argv[])
 
 		if (nfds > 0 && pfd[PFD_PIPE].revents & POLLIN) {
 			nfds--;
-			if (dispatch_imsg(&lconf, pw_dir, pw_uid, pw_gid) == -1)
+			if (dispatch_imsg(&lconf, argc0, argv0) == -1)
 				quit = 1;
 		}
 
@@ -353,8 +354,7 @@ check_child(void)
 }
 
 int
-dispatch_imsg(struct ntpd_conf *lconf, const char *pw_dir,
-    uid_t pw_uid, gid_t pw_gid)
+dispatch_imsg(struct ntpd_conf *lconf, int argc, char **argv)
 {
 	struct imsg		 imsg;
 	int			 n;
@@ -404,7 +404,7 @@ dispatch_imsg(struct ntpd_conf *lconf, const char *pw_dir,
 		case IMSG_CONSTRAINT_QUERY:
 			priv_constraint_msg(imsg.hdr.peerid,
 			    imsg.data, imsg.hdr.len - IMSG_HEADER_SIZE,
-			    pw_dir, pw_uid, pw_gid);
+			    argc, argv);
 			break;
 		case IMSG_CONSTRAINT_KILL:
 			priv_constraint_kill(imsg.hdr.peerid);

src/usr.sbin/ntpd/ntpd.h

@@ -1,4 +1,4 @@
-/*	$OpenBSD: ntpd.h,v 1.132 2016/09/14 13:20:16 rzalamena Exp $ */
+/*	$OpenBSD: ntpd.h,v 1.133 2016/09/26 17:17:01 rzalamena Exp $ */
 
 /*
  * Copyright (c) 2003, 2004 Henning Brauer <henning@openbsd.org>
@@ -82,10 +82,11 @@
 #define CONSTRAINT_PASSFD		(STDERR_FILENO + 1)
 #define CONSTRAINT_CA			"/etc/ssl/cert.pem"
 
-#define PARENT_SOCK_FILENO		3
+#define PARENT_SOCK_FILENO		CONSTRAINT_PASSFD
 
 #define NTP_PROC_NAME			"ntp_main"
 #define NTPDNS_PROC_NAME		"ntp_dns"
+#define CONSTRAINT_PROC_NAME		"constraint"
 
 enum client_state {
 	STATE_NONE,
@@ -357,8 +358,8 @@ int	 constraint_check(double);
 void	 constraint_msg_dns(u_int32_t, u_int8_t *, size_t);
 void	 constraint_msg_result(u_int32_t, u_int8_t *, size_t);
 void	 constraint_msg_close(u_int32_t, u_int8_t *, size_t);
-void	 priv_constraint_msg(u_int32_t, u_int8_t *, size_t,
-	    const char *, uid_t, gid_t);
+void	 priv_constraint_msg(u_int32_t, u_int8_t *, size_t, int, char **);
+void	 priv_constraint_child(const char *, uid_t, gid_t);
 void	 priv_constraint_kill(u_int32_t);
 int	 priv_constraint_dispatch(struct pollfd *);
 void	 priv_constraint_check_child(pid_t, int);