subject:"syslogd fork\+exec"

Re: syslogd fork+exec

2016-10-05 Thread Alexander Bluhm

On Sun, Oct 02, 2016 at 11:07:21PM +0200, Alexander Bluhm wrote:
> On Sat, Oct 01, 2016 at 07:41:13PM +0200, Rafael Zalamena wrote:
> > This could be replaced with "closefrom(4);".
> 
> Updated diff:
> - use closefrom(2)
> - use execvp(3) to allow starting syslogd(8) without full path
> - add a debug message to make testing easier

Merged with -current.  Any ok?

bluhm

Index: privsep.c
===
RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/syslogd/privsep.c,v
retrieving revision 1.61
diff -u -p -r1.61 privsep.c
--- privsep.c   28 Jun 2016 18:22:50 -  1.61
+++ privsep.c   5 Oct 2016 17:11:32 -
@@ -2,6 +2,7 @@
 
 /*
  * Copyright (c) 2003 Anil Madhavapeddy 
+ * Copyright (c) 2016 Alexander Bluhm 
  *
  * Permission to use, copy, modify, and distribute this software for any
  * purpose with or without fee is hereby granted, provided that the above
@@ -71,9 +72,6 @@ enum cmd_types {
 
 static int priv_fd = -1;
 static volatile pid_t child_pid = -1;
-static char config_file[PATH_MAX];
-static struct stat cf_info;
-static int allow_getnameinfo = 0;
 static volatile sig_atomic_t cur_state = STATE_INIT;
 
 /* Queue for the allowed logfiles */
@@ -94,25 +92,12 @@ static void must_read(int, void *, size_
 static void must_write(int, void *, size_t);
 static int  may_read(int, void *, size_t);
 
-int
-priv_init(char *conf, int numeric, int lockfd, int nullfd, char *argv[])
+void
+priv_init(int lockfd, int nullfd, int argc, char *argv[])
 {
-   int i, fd, socks[2], cmd, addr_len, result, restart;
-   size_t path_len, protoname_len, hostname_len, servname_len;
-   char path[PATH_MAX], protoname[5];
-   char hostname[NI_MAXHOST], servname[NI_MAXSERV];
-   struct sockaddr_storage addr;
-   struct stat cf_stat;
+   int i, socks[2];
struct passwd *pw;
-   struct addrinfo hints, *res0;
-   struct sigaction sa;
-
-   memset(, 0, sizeof(sa));
-   sigemptyset(_mask);
-   sa.sa_flags = SA_RESTART;
-   sa.sa_handler = SIG_DFL;
-   for (i = 1; i < _NSIG; i++)
-   sigaction(i, , NULL);
+   char childnum[11], **privargv;
 
/* Create sockets */
if (socketpair(AF_LOCAL, SOCK_STREAM, PF_UNSPEC, socks) == -1)
@@ -141,12 +126,9 @@ priv_init(char *conf, int numeric, int l
err(1, "setresuid() failed");
close(socks[0]);
priv_fd = socks[1];
-   return 0;
+   return;
}
-
-   if (pledge("stdio rpath wpath cpath dns getpw sendfd id proc exec",
-   NULL) == -1)
-   err(1, "pledge priv");
+   close(socks[1]);
 
if (!Debug) {
close(lockfd);
@@ -157,20 +139,6 @@ priv_init(char *conf, int numeric, int l
if (nullfd > 2)
close(nullfd);
 
-   /* Father */
-   /* Pass TERM/HUP/INT/QUIT through to child, and accept CHLD */
-   sa.sa_handler = sig_pass_to_chld;
-   sigaction(SIGTERM, , NULL);
-   sigaction(SIGHUP, , NULL);
-   sigaction(SIGINT, , NULL);
-   sigaction(SIGQUIT, , NULL);
-   sa.sa_handler = sig_got_chld;
-   sa.sa_flags |= SA_NOCLDSTOP;
-   sigaction(SIGCHLD, , NULL);
-
-   setproctitle("[priv]");
-   close(socks[1]);
-
/* Close descriptors that only the unpriv child needs */
if (fd_ctlconn != -1)
close(fd_ctlconn);
@@ -194,38 +162,87 @@ priv_init(char *conf, int numeric, int l
if (fd_unix[i] != -1)
close(fd_unix[i]);
 
-   /* Save the config file specified by the child process */
-   if (strlcpy(config_file, conf, sizeof config_file) >= 
sizeof(config_file))
-   errx(1, "config_file truncation");
+   if (dup3(socks[0], 3, 0) == -1)
+   err(1, "dup3 priv sock failed");
+   snprintf(childnum, sizeof(childnum), "%d", child_pid);
+   if ((privargv = reallocarray(NULL, argc + 3, sizeof(char *))) == NULL)
+   err(1, "alloc priv argv failed");
+   for (i = 0; i < argc; i++)
+   privargv[i] = argv[i];
+   privargv[i++] = "-P";
+   privargv[i++] = childnum;
+   privargv[i++] = NULL;
+   execvp(privargv[0], privargv);
+   err(1, "exec priv '%s' failed", privargv[0]);
+}
 
-   if (stat(config_file, _info) < 0)
-   err(1, "stat config file failed");
+__dead void
+priv_exec(char *conf, int numeric, int child, int argc, char *argv[])
+{
+   int i, fd, sock, cmd, addr_len, result, restart;
+   size_t path_len, protoname_len, hostname_len, servname_len;
+   char path[PATH_MAX], protoname[5];
+   char hostname[NI_MAXHOST], servname[NI_MAXSERV];
+   struct sockaddr_storage addr;
+   struct stat cf_info, cf_stat;
+   struct addrinfo hints, *res0;
+   struct sigaction sa;
+
+   if (pledge("stdio rpath wpath cpath dns getpw sendfd id

Re: syslogd fork+exec

2016-10-02 Thread Alexander Bluhm

On Sat, Oct 01, 2016 at 07:41:13PM +0200, Rafael Zalamena wrote:
> This could be replaced with "closefrom(4);".

Updated diff:
- use closefrom(2)
- use execvp(3) to allow starting syslogd(8) without full path
- add a debug message to make testing easier

ok?

bluhm

Index: usr.sbin/syslogd/privsep.c
===
RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/syslogd/privsep.c,v
retrieving revision 1.61
diff -u -p -u -p -r1.61 privsep.c
--- usr.sbin/syslogd/privsep.c  28 Jun 2016 18:22:50 -  1.61
+++ usr.sbin/syslogd/privsep.c  2 Oct 2016 20:57:10 -
@@ -2,6 +2,7 @@
 
 /*
  * Copyright (c) 2003 Anil Madhavapeddy 
+ * Copyright (c) 2016 Alexander Bluhm 
  *
  * Permission to use, copy, modify, and distribute this software for any
  * purpose with or without fee is hereby granted, provided that the above
@@ -71,9 +72,6 @@ enum cmd_types {
 
 static int priv_fd = -1;
 static volatile pid_t child_pid = -1;
-static char config_file[PATH_MAX];
-static struct stat cf_info;
-static int allow_getnameinfo = 0;
 static volatile sig_atomic_t cur_state = STATE_INIT;
 
 /* Queue for the allowed logfiles */
@@ -94,25 +92,12 @@ static void must_read(int, void *, size_
 static void must_write(int, void *, size_t);
 static int  may_read(int, void *, size_t);
 
-int
-priv_init(char *conf, int numeric, int lockfd, int nullfd, char *argv[])
+void
+priv_init(int lockfd, int nullfd, int argc, char *argv[])
 {
-   int i, fd, socks[2], cmd, addr_len, result, restart;
-   size_t path_len, protoname_len, hostname_len, servname_len;
-   char path[PATH_MAX], protoname[5];
-   char hostname[NI_MAXHOST], servname[NI_MAXSERV];
-   struct sockaddr_storage addr;
-   struct stat cf_stat;
+   int i, socks[2];
struct passwd *pw;
-   struct addrinfo hints, *res0;
-   struct sigaction sa;
-
-   memset(, 0, sizeof(sa));
-   sigemptyset(_mask);
-   sa.sa_flags = SA_RESTART;
-   sa.sa_handler = SIG_DFL;
-   for (i = 1; i < _NSIG; i++)
-   sigaction(i, , NULL);
+   char childnum[11], **privargv;
 
/* Create sockets */
if (socketpair(AF_LOCAL, SOCK_STREAM, PF_UNSPEC, socks) == -1)
@@ -141,12 +126,9 @@ priv_init(char *conf, int numeric, int l
err(1, "setresuid() failed");
close(socks[0]);
priv_fd = socks[1];
-   return 0;
+   return;
}
-
-   if (pledge("stdio rpath wpath cpath dns getpw sendfd id proc exec",
-   NULL) == -1)
-   err(1, "pledge priv");
+   close(socks[1]);
 
if (!Debug) {
close(lockfd);
@@ -157,20 +139,6 @@ priv_init(char *conf, int numeric, int l
if (nullfd > 2)
close(nullfd);
 
-   /* Father */
-   /* Pass TERM/HUP/INT/QUIT through to child, and accept CHLD */
-   sa.sa_handler = sig_pass_to_chld;
-   sigaction(SIGTERM, , NULL);
-   sigaction(SIGHUP, , NULL);
-   sigaction(SIGINT, , NULL);
-   sigaction(SIGQUIT, , NULL);
-   sa.sa_handler = sig_got_chld;
-   sa.sa_flags |= SA_NOCLDSTOP;
-   sigaction(SIGCHLD, , NULL);
-
-   setproctitle("[priv]");
-   close(socks[1]);
-
/* Close descriptors that only the unpriv child needs */
if (fd_ctlconn != -1)
close(fd_ctlconn);
@@ -194,38 +162,87 @@ priv_init(char *conf, int numeric, int l
if (fd_unix[i] != -1)
close(fd_unix[i]);
 
-   /* Save the config file specified by the child process */
-   if (strlcpy(config_file, conf, sizeof config_file) >= 
sizeof(config_file))
-   errx(1, "config_file truncation");
+   if (dup3(socks[0], 3, 0) == -1)
+   err(1, "dup3 priv sock failed");
+   snprintf(childnum, sizeof(childnum), "%d", child_pid);
+   if ((privargv = reallocarray(NULL, argc + 3, sizeof(char *))) == NULL)
+   err(1, "alloc priv argv failed");
+   for (i = 0; i < argc; i++)
+   privargv[i] = argv[i];
+   privargv[i++] = "-P";
+   privargv[i++] = childnum;
+   privargv[i++] = NULL;
+   execvp(privargv[0], privargv);
+   err(1, "exec priv '%s' failed", privargv[0]);
+}
 
-   if (stat(config_file, _info) < 0)
-   err(1, "stat config file failed");
+__dead void
+priv_exec(char *conf, int numeric, int child, int argc, char *argv[])
+{
+   int i, fd, sock, cmd, addr_len, result, restart;
+   size_t path_len, protoname_len, hostname_len, servname_len;
+   char path[PATH_MAX], protoname[5];
+   char hostname[NI_MAXHOST], servname[NI_MAXSERV];
+   struct sockaddr_storage addr;
+   struct stat cf_info, cf_stat;
+   struct addrinfo hints, *res0;
+   struct sigaction sa;
+
+   if (pledge("stdio rpath wpath cpath dns getpw sendfd id proc exec",
+   NULL) == -1)
+

Re: syslogd fork+exec

2016-10-01 Thread Rafael Zalamena

On Thu, Sep 29, 2016 at 08:09:23PM +0200, Alexander Bluhm wrote:
> Hi,
> 
> With this diff syslogd(8) does an exec on itself in the privileged
> parent process to reshuffle its memory layout.
> 
> As syslogd only forks once, it does not really matter wether we
> fork+exec in the child or in the parent.  To do it in the parent
> is easier as it has much less state.
> 
> ok?
> 
> bluhm

Your diffs looks good and you made me realize that I should use dup3()
instead of dup2() to create children socket.

Short explanation for outsiders: dup2(fd1, fd2) duplicates fd1 onto fd2
removing CLOEXEC flags, except if fd1 == fd2, then in that case the fd
will remain with CLOEXEC and things will not work. This is not the case
with httpd(8), relayd(8), ntpd(8) and switchd(8), but since code might
be copied around it would be good to fix this there.

I'm using this diff and it works in my default configuration, but since
I'm not familiar with syslogd I don't feel confortable giving oks here.

I made one comment inline in the snipped diff below.

> 
> Index: usr.sbin/syslogd/privsep.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/syslogd/privsep.c,v
> retrieving revision 1.61
> diff -u -p -r1.61 privsep.c
> --- usr.sbin/syslogd/privsep.c28 Jun 2016 18:22:50 -  1.61
> +++ usr.sbin/syslogd/privsep.c29 Sep 2016 17:55:03 -
> @@ -194,38 +162,87 @@ priv_init(char *conf, int numeric, int l
>   if (fd_unix[i] != -1)
>   close(fd_unix[i]);
>  
> - /* Save the config file specified by the child process */
> - if (strlcpy(config_file, conf, sizeof config_file) >= 
> sizeof(config_file))
> - errx(1, "config_file truncation");
> + if (dup3(socks[0], 3, 0) == -1)
> + err(1, "dup3 priv sock failed");
> + snprintf(childnum, sizeof(childnum), "%d", child_pid);
> + if ((privargv = reallocarray(NULL, argc + 3, sizeof(char *))) == NULL)
> + err(1, "alloc priv argv failed");
> + for (i = 0; i < argc; i++)
> + privargv[i] = argv[i];
> + privargv[i++] = "-P";
> + privargv[i++] = childnum;
> + privargv[i++] = NULL;
> + execv(privargv[0], privargv);
> + err(1, "exec priv '%s' failed", privargv[0]);
> +}
>  
> - if (stat(config_file, _info) < 0)
> - err(1, "stat config file failed");
> +__dead void
> +priv_exec(char *conf, int numeric, int child, int argc, char *argv[])
> +{
> + int i, fd, sock, cmd, addr_len, result, restart;
> + size_t path_len, protoname_len, hostname_len, servname_len;
> + char path[PATH_MAX], protoname[5];
> + char hostname[NI_MAXHOST], servname[NI_MAXSERV];
> + struct sockaddr_storage addr;
> + struct stat cf_info, cf_stat;
> + struct addrinfo hints, *res0;
> + struct sigaction sa;
>  
> - /* Save whether or not the child can have access to getnameinfo(3) */
> - if (numeric > 0)
> - allow_getnameinfo = 0;
> - else
> - allow_getnameinfo = 1;
> + if (pledge("stdio rpath wpath cpath dns getpw sendfd id proc exec",
> + NULL) == -1)
> + err(1, "pledge priv");
> +
> + if (argc <= 2 || strcmp("-P", argv[argc - 2]) != 0)
> + errx(1, "exec without priv");
> + argv[argc -= 2] = NULL;
> +
> + sock = 3;
> + for (fd = 4; fd < 1024; fd++)
> + close(fd);

This could be replaced with "closefrom(4);".

> +
> + child_pid = child;
> +
> + memset(, 0, sizeof(sa));
> + sigemptyset(_mask);
> + sa.sa_flags = SA_RESTART;
> + sa.sa_handler = SIG_DFL;
> + for (i = 1; i < _NSIG; i++)
> + sigaction(i, , NULL);
> +
> + /* Pass TERM/HUP/INT/QUIT through to child, and accept CHLD */
> + sa.sa_handler = sig_pass_to_chld;
> + sigaction(SIGTERM, , NULL);
> + sigaction(SIGHUP, , NULL);
> + sigaction(SIGINT, , NULL);
> + sigaction(SIGQUIT, , NULL);
> + sa.sa_handler = sig_got_chld;
> + sa.sa_flags |= SA_NOCLDSTOP;
> + sigaction(SIGCHLD, , NULL);
> +
> + setproctitle("[priv]");
> +
> + if (stat(conf, _info) < 0)
> + err(1, "stat config file failed");
>  
>   TAILQ_INIT();
>   increase_state(STATE_CONFIG);
>   restart = 0;
>  
>   while (cur_state < STATE_QUIT) {
> - if (may_read(socks[0], , sizeof(int)))
> + if (may_read(sock, , sizeof(int)))
>   break;
>   switch (cmd) {
>   case PRIV_OPEN_TTY:
>   logdebug("[priv]: msg PRIV_OPEN_TTY received\n");
>   /* Expecting: length, path */
> - must_read(socks[0], _len, sizeof(size_t));
> + must_read(sock, _len, sizeof(size_t));
>   if (path_len == 0 || path_len > sizeof(path))
>   _exit(1);
> - must_read(socks[0], , path_len);
> +

syslogd fork+exec

2016-09-29 Thread Alexander Bluhm

Hi,

With this diff syslogd(8) does an exec on itself in the privileged
parent process to reshuffle its memory layout.

As syslogd only forks once, it does not really matter wether we
fork+exec in the child or in the parent.  To do it in the parent
is easier as it has much less state.

ok?

bluhm

Index: usr.sbin/syslogd/privsep.c
===
RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/syslogd/privsep.c,v
retrieving revision 1.61
diff -u -p -r1.61 privsep.c
--- usr.sbin/syslogd/privsep.c  28 Jun 2016 18:22:50 -  1.61
+++ usr.sbin/syslogd/privsep.c  29 Sep 2016 17:55:03 -
@@ -2,6 +2,7 @@
 
 /*
  * Copyright (c) 2003 Anil Madhavapeddy 
+ * Copyright (c) 2016 Alexander Bluhm 
  *
  * Permission to use, copy, modify, and distribute this software for any
  * purpose with or without fee is hereby granted, provided that the above
@@ -71,9 +72,6 @@ enum cmd_types {
 
 static int priv_fd = -1;
 static volatile pid_t child_pid = -1;
-static char config_file[PATH_MAX];
-static struct stat cf_info;
-static int allow_getnameinfo = 0;
 static volatile sig_atomic_t cur_state = STATE_INIT;
 
 /* Queue for the allowed logfiles */
@@ -94,25 +92,12 @@ static void must_read(int, void *, size_
 static void must_write(int, void *, size_t);
 static int  may_read(int, void *, size_t);
 
-int
-priv_init(char *conf, int numeric, int lockfd, int nullfd, char *argv[])
+void
+priv_init(int lockfd, int nullfd, int argc, char *argv[])
 {
-   int i, fd, socks[2], cmd, addr_len, result, restart;
-   size_t path_len, protoname_len, hostname_len, servname_len;
-   char path[PATH_MAX], protoname[5];
-   char hostname[NI_MAXHOST], servname[NI_MAXSERV];
-   struct sockaddr_storage addr;
-   struct stat cf_stat;
+   int i, socks[2];
struct passwd *pw;
-   struct addrinfo hints, *res0;
-   struct sigaction sa;
-
-   memset(, 0, sizeof(sa));
-   sigemptyset(_mask);
-   sa.sa_flags = SA_RESTART;
-   sa.sa_handler = SIG_DFL;
-   for (i = 1; i < _NSIG; i++)
-   sigaction(i, , NULL);
+   char childnum[11], **privargv;
 
/* Create sockets */
if (socketpair(AF_LOCAL, SOCK_STREAM, PF_UNSPEC, socks) == -1)
@@ -141,12 +126,9 @@ priv_init(char *conf, int numeric, int l
err(1, "setresuid() failed");
close(socks[0]);
priv_fd = socks[1];
-   return 0;
+   return;
}
-
-   if (pledge("stdio rpath wpath cpath dns getpw sendfd id proc exec",
-   NULL) == -1)
-   err(1, "pledge priv");
+   close(socks[1]);
 
if (!Debug) {
close(lockfd);
@@ -157,20 +139,6 @@ priv_init(char *conf, int numeric, int l
if (nullfd > 2)
close(nullfd);
 
-   /* Father */
-   /* Pass TERM/HUP/INT/QUIT through to child, and accept CHLD */
-   sa.sa_handler = sig_pass_to_chld;
-   sigaction(SIGTERM, , NULL);
-   sigaction(SIGHUP, , NULL);
-   sigaction(SIGINT, , NULL);
-   sigaction(SIGQUIT, , NULL);
-   sa.sa_handler = sig_got_chld;
-   sa.sa_flags |= SA_NOCLDSTOP;
-   sigaction(SIGCHLD, , NULL);
-
-   setproctitle("[priv]");
-   close(socks[1]);
-
/* Close descriptors that only the unpriv child needs */
if (fd_ctlconn != -1)
close(fd_ctlconn);
@@ -194,38 +162,87 @@ priv_init(char *conf, int numeric, int l
if (fd_unix[i] != -1)
close(fd_unix[i]);
 
-   /* Save the config file specified by the child process */
-   if (strlcpy(config_file, conf, sizeof config_file) >= 
sizeof(config_file))
-   errx(1, "config_file truncation");
+   if (dup3(socks[0], 3, 0) == -1)
+   err(1, "dup3 priv sock failed");
+   snprintf(childnum, sizeof(childnum), "%d", child_pid);
+   if ((privargv = reallocarray(NULL, argc + 3, sizeof(char *))) == NULL)
+   err(1, "alloc priv argv failed");
+   for (i = 0; i < argc; i++)
+   privargv[i] = argv[i];
+   privargv[i++] = "-P";
+   privargv[i++] = childnum;
+   privargv[i++] = NULL;
+   execv(privargv[0], privargv);
+   err(1, "exec priv '%s' failed", privargv[0]);
+}
 
-   if (stat(config_file, _info) < 0)
-   err(1, "stat config file failed");
+__dead void
+priv_exec(char *conf, int numeric, int child, int argc, char *argv[])
+{
+   int i, fd, sock, cmd, addr_len, result, restart;
+   size_t path_len, protoname_len, hostname_len, servname_len;
+   char path[PATH_MAX], protoname[5];
+   char hostname[NI_MAXHOST], servname[NI_MAXSERV];
+   struct sockaddr_storage addr;
+   struct stat cf_info, cf_stat;
+   struct addrinfo hints, *res0;
+   struct sigaction sa;
 
-   /* Save whether or not the child can have access to getnameinfo(3) */
-   if

Re: syslogd fork+exec

Re: syslogd fork+exec

Re: syslogd fork+exec

syslogd fork+exec

4 matches

Site Navigation

Mail list logo

Footer information