Further bugs in syslog()

Further bugs in syslog()

[Rich Felker]

Hi all,

William Haddon's report about syslog prompted me to review the file,
and there seem to be several additional bugs:

1. log_ident stores the actual pointer passed by the caller rather
   than a copy of the string. This probably works in practice for most
   callers but it's definitely not correct.

2. As a specific case of the previously reported bug, overflows will
   happen if log_ident is too long. This is unlikely to happen
   intentionally, but could happen if log_ident points to storage on
   the stack whose lifetime ended and which was subsequently reused.

3. Opening the log fd with LOG_NDELAY only obtains the socket, but
   does not connect it. The socket is a datagram socket, so connect is
   not needed to use it, but if sendto is used instead of connect,
   the idiom of using openlog with LOG_NDELAY before chroot will not
   work.

I'm going to review the proposed patches and probably put together a
big syslog fix...

Rich

Re: Further bugs in syslog()

[Rich Felker]

On Fri, Mar 22, 2013 at 11:45:39PM -0400, Rich Felker wrote:
> Hi all,
> 
> William Haddon's report about syslog prompted me to review the file,
> and there seem to be several additional bugs:
> 
> 1. log_ident stores the actual pointer passed by the caller rather
>    than a copy of the string. This probably works in practice for most
>    callers but it's definitely not correct.

It should be noted that, per glibc's documentation of openlog, it
shares this bug. POSIX has no text that allows such an implementation,
but I suspect all historic implementations have this bad behavior, so
it's possibly an issue that should be raised on the Austin Group
tracker...

glibc also documents closelog as clearing log_ident to the default
value. This behavior does not seem to be permitted (much less
required) by the standard.

Rich

Proposed syslog patch [Re: [musl] Further bugs in syslog()]

[Rich Felker]

[-- Attachment #1: Type: text/plain, Size: 185 bytes --]

On Fri, Mar 22, 2013 at 11:45:39PM -0400, Rich Felker wrote:
> I'm going to review the proposed patches and probably put together a
> big syslog fix...

Comments on the attached?

Rich

[-- Attachment #2: syslog.diff --]
[-- Type: text/plain, Size: 2052 bytes --]

diff --git a/src/misc/syslog.c b/src/misc/syslog.c
index 8de34f8..81fb114 100644
--- a/src/misc/syslog.c
+++ b/src/misc/syslog.c
@@ -11,7 +11,7 @@
 #include "libc.h"
 
 static int lock[2];
-static const char *log_ident;
+static char *log_ident;
 static int log_opt;
 static int log_facility = LOG_USER;
 static int log_mask = 0xff;
@@ -43,15 +43,10 @@ void closelog(void)
 	pthread_setcancelstate(cs, 0);
 }
 
-static void __openlog(const char *ident, int opt, int facility)
+static void __openlog()
 {
-	log_ident = ident;
-	log_opt = opt;
-	log_facility = facility;
-
-	if (!(opt & LOG_NDELAY) || log_fd>=0) return;
-
 	log_fd = socket(AF_UNIX, SOCK_DGRAM|SOCK_CLOEXEC, 0);
+	connect(log_fd, (void *)&log_addr, sizeof log_addr);
 }
 
 void openlog(const char *ident, int opt, int facility)
@@ -59,7 +54,14 @@ void openlog(const char *ident, int opt, int facility)
 	int cs;
 	pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &cs);
 	LOCK(lock);
-	__openlog(ident, opt, facility);
+
+	free(log_ident);
+	log_ident = strdup(ident);
+	log_opt = opt;
+	log_facility = facility;
+
+	if ((opt & LOG_NDELAY) && log_fd<0) __openlog();
+
 	UNLOCK(lock);
 	pthread_setcancelstate(cs, 0);
 }
@@ -74,11 +76,8 @@ static void _vsyslog(int priority, const char *message, va_list ap)
 	int l, l2;
 
 	if (log_fd < 0) {
-		__openlog(log_ident, log_opt | LOG_NDELAY, log_facility);
-		if (log_fd < 0) {
-			UNLOCK(lock);
-			return;
-		}
+		__openlog();
+		if (log_fd < 0) return;
 	}
 
 	now = time(NULL);
@@ -90,14 +89,14 @@ static void _vsyslog(int priority, const char *message, va_list ap)
 		priority, timebuf,
 		log_ident ? log_ident : "",
 		"["+!pid, pid, "]"+!pid);
+	if (l >= sizeof buf) return;
 	l2 = vsnprintf(buf+l, sizeof buf - l, message, ap);
 	if (l2 >= 0) {
-		l += l2;
+		if (l2 >= sizeof buf - l) l = sizeof buf - 1;
+		else l += l2;
 		if (buf[l-1] != '\n') buf[l++] = '\n';
-		sendto(log_fd, buf, l, 0, (void *)&log_addr, 11);
+		send(log_fd, buf, l, 0);
 	}
-
-	UNLOCK(lock);
 }
 
 void __vsyslog(int priority, const char *message, va_list ap)

Re: Proposed syslog patch [Re: [musl] Further bugs in syslog()]

[Szabolcs Nagy]

looks ok, one nitpick

* Rich Felker <dalias@aerifal.cx> [2013-03-23 00:05:58 -0400]:
>  void openlog(const char *ident, int opt, int facility)
> @@ -59,7 +54,14 @@ void openlog(const char *ident, int opt, int facility)
>  	int cs;
>  	pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &cs);
>  	LOCK(lock);
> -	__openlog(ident, opt, facility);
> +
> +	free(log_ident);
> +	log_ident = strdup(ident);

this can fail and then log_ident = 0

...
> @@ -90,14 +89,14 @@ static void _vsyslog(int priority, const char *message, va_list ap)
>  		priority, timebuf,
>  		log_ident ? log_ident : "",
>  		"["+!pid, pid, "]"+!pid);
> +	if (l >= sizeof buf) return;

if ident is longer than buf, then syslog will silently fail here,
but if ident is so big that strdup fails above it does not fail here

maybe we should limit ident to a sensible value and truncate it
(i dont know if posix allows that but seems more consistent
and then no dynamic allocation is needed in openlog)

>  	l2 = vsnprintf(buf+l, sizeof buf - l, message, ap);
>  	if (l2 >= 0) {
> -		l += l2;
> +		if (l2 >= sizeof buf - l) l = sizeof buf - 1;
> +		else l += l2;
>  		if (buf[l-1] != '\n') buf[l++] = '\n';
> -		sendto(log_fd, buf, l, 0, (void *)&log_addr, 11);
> +		send(log_fd, buf, l, 0);
>  	}
> -
> -	UNLOCK(lock);
>  }

Re: Proposed syslog patch [Re: [musl] Further bugs in syslog()]

[Rich Felker]

On Sat, Mar 23, 2013 at 05:17:55PM +0100, Szabolcs Nagy wrote:
> looks ok, one nitpick
> 
> * Rich Felker <dalias@aerifal.cx> [2013-03-23 00:05:58 -0400]:
> >  void openlog(const char *ident, int opt, int facility)
> > @@ -59,7 +54,14 @@ void openlog(const char *ident, int opt, int facility)
> >  	int cs;
> >  	pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &cs);
> >  	LOCK(lock);
> > -	__openlog(ident, opt, facility);
> > +
> > +	free(log_ident);
> > +	log_ident = strdup(ident);
> 
> this can fail and then log_ident = 0

Yes, this was the intent; there's no way to report errors, and being
in a status of NULL log_ident is the same as the case where it was
never set at all. However I agree with your reasoning below...

> ....
> > @@ -90,14 +89,14 @@ static void _vsyslog(int priority, const char *message, va_list ap)
> >  		priority, timebuf,
> >  		log_ident ? log_ident : "",
> >  		"["+!pid, pid, "]"+!pid);
> > +	if (l >= sizeof buf) return;
> 
> if ident is longer than buf, then syslog will silently fail here,
> but if ident is so big that strdup fails above it does not fail here

That it violates the principle of least surprise that moderately long
values of log_ident will prevent any log message from appearing, but
insanely long ones will be silently replaced with a blank string.

> maybe we should limit ident to a sensible value and truncate it
> (i dont know if posix allows that but seems more consistent
> and then no dynamic allocation is needed in openlog)

POSIX provides no way to inspect the output that's produced, so there
are no testable conformance requirements on the output...

I think I like your approach: just using a small fixed-size buffer.

Rich