From 427c0ca79e8ab429681310308a0a87cd5fdd1837 Mon Sep 17 00:00:00 2001 From: Rich Felker Date: Sat, 23 Mar 2013 18:59:30 -0400 Subject: [PATCH] fix multiple bugs in syslog interfaces 1. as reported by William Haddon, the value returned by snprintf was wrongly used as a length passed to sendto, despite it possibly exceeding the buffer length. this could lead to invalid reads and leaking additional data to syslog. 2. openlog was storing a pointer to the ident string passed by the caller, rather than copying it. this bug is shared with (and even documented in) other implementations like glibc, but such behavior does not seem to meet the requirements of the standard. 3. extremely long ident provided to openlog, or corrupt ident due to the above issue, could possibly have resulted in buffer overflows. despite having the potential for smashing the stack, i believe the impact is low since ident points to a short string literal in typical application usage (and per the above bug, other usages will break horribly on other implementations). 4. when used with LOG_NDELAY, openlog was not connecting the newly-opened socket; sendto was being used instead. this defeated the main purpose of LOG_NDELAY: preparing for chroot. 5. the default facility was not being used at all, so all messages without an explicit facility passed to syslog were getting logged at the kernel facility. 6. setlogmask was not thread-safe; no synchronization was performed updating the mask. the fix uses atomics rather than locking to avoid introducing a lock in the fast path for messages whose priority is not in the mask. 7. in some code paths, the syslog lock was being unlocked twice; this could result in releasing a lock that was actually held by a different thread. some additional enhancements to syslog such as a default identifier based on argv[0] or similar may still be desired; at this time, only the above-listed bugs have been fixed. --- src/misc/syslog.c | 51 +++++++++++++++++++++++++---------------------- 1 file changed, 27 insertions(+), 24 deletions(-) diff --git a/src/misc/syslog.c b/src/misc/syslog.c index 8de34f8d..ba9cc62b 100644 --- a/src/misc/syslog.c +++ b/src/misc/syslog.c @@ -9,9 +9,10 @@ #include #include #include "libc.h" +#include "atomic.h" static int lock[2]; -static const char *log_ident; +static char log_ident[32]; static int log_opt; static int log_facility = LOG_USER; static int log_mask = 0xff; @@ -19,9 +20,8 @@ static int log_fd = -1; int setlogmask(int maskpri) { - int old = log_mask; - if (maskpri) log_mask = maskpri; - return old; + if (maskpri) return a_swap(&log_mask, maskpri); + else return log_mask; } static const struct { @@ -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); + if (log_fd >= 0) connect(log_fd, (void *)&log_addr, sizeof log_addr); } void openlog(const char *ident, int opt, int facility) @@ -59,7 +54,19 @@ void openlog(const char *ident, int opt, int facility) int cs; pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &cs); LOCK(lock); - __openlog(ident, opt, facility); + + if (ident) { + size_t n = strnlen(ident, sizeof log_ident - 1); + memcpy(log_ident, ident, n); + log_ident[n] = 0; + } else { + log_ident[0] = 0; + } + log_opt = opt; + log_facility = facility; + + if ((opt & LOG_NDELAY) && log_fd<0) __openlog(); + UNLOCK(lock); pthread_setcancelstate(cs, 0); } @@ -74,30 +81,26 @@ 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; } + if (!(priority & LOG_FACMASK)) priority |= log_facility; + now = time(NULL); gmtime_r(&now, &tm); strftime(timebuf, sizeof timebuf, "%b %e %T", &tm); pid = (log_opt & LOG_PID) ? getpid() : 0; l = snprintf(buf, sizeof buf, "<%d>%s %s%s%.0d%s: ", - priority, timebuf, - log_ident ? log_ident : "", - "["+!pid, pid, "]"+!pid); + priority, timebuf, log_ident, "["+!pid, pid, "]"+!pid); 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) -- 2.25.1