syslogd optarg

[Alexander Bluhm]

Hi,

When syslogd is reloading a modified config, it does a reexec on
itself.  For this it uses the original arguments of main().  The
function loghost_parse() modifies the optarg memory it is operating
on.  To prevent that the exec arguments have been tampered, pass a
copy of optarg to loghost_parse().

ok?

bluhm

Index: usr.sbin/syslogd/syslogd.c
===
RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/syslogd/syslogd.c,v
retrieving revision 1.178
diff -u -p -r1.178 syslogd.c
--- usr.sbin/syslogd/syslogd.c  25 Aug 2015 17:14:16 -  1.178
+++ usr.sbin/syslogd/syslogd.c  27 Aug 2015 13:33:52 -
@@ -346,6 +346,8 @@ main(int argc, char *argv[])
struct timeval   to;
const char  *errstr;
char*p;
+   char udparg[NI_MAXHOST+2+NI_MAXSERV];
+   char tcparg[NI_MAXHOST+2+NI_MAXSERV];
int  ch, i;
int  lockpipe[2] = { -1, -1}, pair[2], nullfd, fd;
 
@@ -393,12 +395,14 @@ main(int argc, char *argv[])
path_ctlsock = optarg;
break;
case 'T':   /* allow tcp and listen on address */
-   if (loghost_parse(optarg, NULL, listen_host,
+   strlcpy(tcparg, optarg, sizeof(tcparg));
+   if (loghost_parse(tcparg, NULL, listen_host,
listen_port) == -1)
errx(1, bad listen address: %s, optarg);
break;
case 'U':   /* allow udp only from address */
-   if (loghost_parse(optarg, NULL, bind_host, bind_port)
+   strlcpy(udparg, optarg, sizeof(udparg));
+   if (loghost_parse(udparg, NULL, bind_host, bind_port)
== -1)
errx(1, bad bind address: %s, optarg);
break;

Re: syslogd optarg

[Alexander Bluhm]

On Thu, Aug 27, 2015 at 10:13:25AM -0600, Theo de Raadt wrote:
 Why not strdup?

And now with strdup() as suggested by Theo.

ok?

bluhm

Index: usr.sbin/syslogd/syslogd.c
===
RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/syslogd/syslogd.c,v
retrieving revision 1.178
diff -u -p -r1.178 syslogd.c
--- usr.sbin/syslogd/syslogd.c  25 Aug 2015 17:14:16 -  1.178
+++ usr.sbin/syslogd/syslogd.c  27 Aug 2015 16:28:22 -
@@ -393,12 +393,16 @@ main(int argc, char *argv[])
path_ctlsock = optarg;
break;
case 'T':   /* allow tcp and listen on address */
-   if (loghost_parse(optarg, NULL, listen_host,
-   listen_port) == -1)
+   if ((p = strdup(optarg)) == NULL)
+   err(1, strdup listen address);
+   if (loghost_parse(p, NULL, listen_host, listen_port)
+   == -1)
errx(1, bad listen address: %s, optarg);
break;
case 'U':   /* allow udp only from address */
-   if (loghost_parse(optarg, NULL, bind_host, bind_port)
+   if ((p = strdup(optarg)) == NULL)
+   err(1, strdup bind address);
+   if (loghost_parse(p, NULL, bind_host, bind_port)
== -1)
errx(1, bad bind address: %s, optarg);
break;

Re: syslogd optarg

[Theo de Raadt]

 On Thu, Aug 27, 2015 at 10:13:25AM -0600, Theo de Raadt wrote:
  Why not strdup?
 
 And now with strdup() as suggested by Theo.

ok, because such style is not really a leak.

 Index: usr.sbin/syslogd/syslogd.c
 ===
 RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/syslogd/syslogd.c,v
 retrieving revision 1.178
 diff -u -p -r1.178 syslogd.c
 --- usr.sbin/syslogd/syslogd.c25 Aug 2015 17:14:16 -  1.178
 +++ usr.sbin/syslogd/syslogd.c27 Aug 2015 16:28:22 -
 @@ -393,12 +393,16 @@ main(int argc, char *argv[])
   path_ctlsock = optarg;
   break;
   case 'T':   /* allow tcp and listen on address */
 - if (loghost_parse(optarg, NULL, listen_host,
 - listen_port) == -1)
 + if ((p = strdup(optarg)) == NULL)
 + err(1, strdup listen address);
 + if (loghost_parse(p, NULL, listen_host, listen_port)
 + == -1)
   errx(1, bad listen address: %s, optarg);
   break;
   case 'U':   /* allow udp only from address */
 - if (loghost_parse(optarg, NULL, bind_host, bind_port)
 + if ((p = strdup(optarg)) == NULL)
 + err(1, strdup bind address);
 + if (loghost_parse(p, NULL, bind_host, bind_port)
   == -1)
   errx(1, bad bind address: %s, optarg);
   break;

Re: syslogd optarg

[Todd C. Miller]

On Thu, 27 Aug 2015 15:47:18 +0200, Alexander Bluhm wrote:

 When syslogd is reloading a modified config, it does a reexec on
 itself.  For this it uses the original arguments of main().  The
 function loghost_parse() modifies the optarg memory it is operating
 on.  To prevent that the exec arguments have been tampered, pass a
 copy of optarg to loghost_parse().

If you are going to do it this way you really need to check the
strlcpy() return value and error out on overflow.

 - todd

Re: syslogd optarg

[Alexander Bluhm]

On Thu, Aug 27, 2015 at 09:44:33AM -0600, Todd C. Miller wrote:
 On Thu, 27 Aug 2015 15:47:18 +0200, Alexander Bluhm wrote:
 
  When syslogd is reloading a modified config, it does a reexec on
  itself.  For this it uses the original arguments of main().  The
  function loghost_parse() modifies the optarg memory it is operating
  on.  To prevent that the exec arguments have been tampered, pass a
  copy of optarg to loghost_parse().
 
 If you are going to do it this way you really need to check the
 strlcpy() return value and error out on overflow.

Yes, that is better.

ok?

bluhm

Index: usr.sbin/syslogd/syslogd.c
===
RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/syslogd/syslogd.c,v
retrieving revision 1.178
diff -u -p -r1.178 syslogd.c
--- usr.sbin/syslogd/syslogd.c  25 Aug 2015 17:14:16 -  1.178
+++ usr.sbin/syslogd/syslogd.c  27 Aug 2015 15:50:29 -
@@ -346,6 +346,8 @@ main(int argc, char *argv[])
struct timeval   to;
const char  *errstr;
char*p;
+   char udparg[NI_MAXHOST+2+NI_MAXSERV];
+   char tcparg[NI_MAXHOST+2+NI_MAXSERV];
int  ch, i;
int  lockpipe[2] = { -1, -1}, pair[2], nullfd, fd;
 
@@ -393,12 +395,18 @@ main(int argc, char *argv[])
path_ctlsock = optarg;
break;
case 'T':   /* allow tcp and listen on address */
-   if (loghost_parse(optarg, NULL, listen_host,
+   if (strlcpy(tcparg, optarg, sizeof(tcparg)) =
+   sizeof(tcparg))
+   errx(1, listen address too long: %s, optarg);
+   if (loghost_parse(tcparg, NULL, listen_host,
listen_port) == -1)
errx(1, bad listen address: %s, optarg);
break;
case 'U':   /* allow udp only from address */
-   if (loghost_parse(optarg, NULL, bind_host, bind_port)
+   if (strlcpy(udparg, optarg, sizeof(udparg)) =
+   sizeof(udparg))
+   errx(1, bind address too long: %s, optarg);
+   if (loghost_parse(udparg, NULL, bind_host, bind_port)
== -1)
errx(1, bad bind address: %s, optarg);
break;

Re: syslogd optarg

[Todd C. Miller]

On Thu, 27 Aug 2015 17:57:45 +0200, Alexander Bluhm wrote:

 On Thu, Aug 27, 2015 at 09:44:33AM -0600, Todd C. Miller wrote:
  On Thu, 27 Aug 2015 15:47:18 +0200, Alexander Bluhm wrote:
  
   When syslogd is reloading a modified config, it does a reexec on
   itself.  For this it uses the original arguments of main().  The
   function loghost_parse() modifies the optarg memory it is operating
   on.  To prevent that the exec arguments have been tampered, pass a
   copy of optarg to loghost_parse().
  
  If you are going to do it this way you really need to check the
  strlcpy() return value and error out on overflow.
 
 Yes, that is better.
 
 ok?

OK.

 - todd