Message ID | 1395b5b8-0fc7-ae01-c8e1-5e13f3a4394c@foxvalley.net |
---|---|
State | Committed |
Delegated to: | Adhemerval Zanella Netto |
Headers |
Return-Path: <libc-alpha-bounces@sourceware.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 52681385801D; Sat, 27 Mar 2021 20:09:18 +0000 (GMT) X-Original-To: libc-alpha@sourceware.org Delivered-To: libc-alpha@sourceware.org Received: from mail.FoxValley.net (mail.FoxValley.net [64.135.192.34]) by sourceware.org (Postfix) with SMTP id 14A09385801A for <libc-alpha@sourceware.org>; Sat, 27 Mar 2021 20:09:15 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 14A09385801A Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=foxvalley.net Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=draymond@foxvalley.net Received: (qmail 22730 invoked from network) for libc-alpha@sourceware.org; 27 Mar 2021 15:09:14 -0500 Received: from unknown (HELO ?192.168.1.3?) (draymond@161.97.241.227) by mail.foxvalley.net with SMTP; 27 Mar 2021 15:09:14 -0500 To: libc-alpha@sourceware.org From: Dan Raymond <draymond@foxvalley.net> Subject: [PATCH] Bug 3604: fix calls to openlog() with LOG_KERN facility Message-ID: <1395b5b8-0fc7-ae01-c8e1-5e13f3a4394c@foxvalley.net> Date: Sat, 27 Mar 2021 14:07:44 -0600 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.8.1 MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US X-Spam-Status: No, score=-9.4 required=5.0 tests=BAYES_00, BODY_8BITS, GIT_PATCH_0, KAM_DMARC_STATUS, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list <libc-alpha.sourceware.org> List-Unsubscribe: <https://sourceware.org/mailman/options/libc-alpha>, <mailto:libc-alpha-request@sourceware.org?subject=unsubscribe> List-Archive: <https://sourceware.org/pipermail/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-request@sourceware.org?subject=help> List-Subscribe: <https://sourceware.org/mailman/listinfo/libc-alpha>, <mailto:libc-alpha-request@sourceware.org?subject=subscribe> Errors-To: libc-alpha-bounces@sourceware.org Sender: "Libc-alpha" <libc-alpha-bounces@sourceware.org> |
Series |
Bug 3604: fix calls to openlog() with LOG_KERN facility
|
|
Commit Message
Dan Raymond
March 27, 2021, 8:07 p.m. UTC
From 93683928886a563a4740e2b42b53752a4a7d431f Mon Sep 17 00:00:00 2001
From: Dan Raymond <draymond@foxvalley.net>
Date: Sat, 27 Mar 2021 13:51:16 -0600
Subject: [PATCH] Bug 3604: fix calls to openlog() with LOG_KERN facility
---
misc/syslog.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
int retry = 0;
Comments
Can I get a review on this please? On 3/27/2021 2:07 PM, Dan Raymond wrote: > From 93683928886a563a4740e2b42b53752a4a7d431f Mon Sep 17 00:00:00 2001 > From: Dan Raymond <draymond@foxvalley.net> > Date: Sat, 27 Mar 2021 13:51:16 -0600 > Subject: [PATCH] Bug 3604: fix calls to openlog() with LOG_KERN facility > > --- > misc/syslog.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/misc/syslog.c b/misc/syslog.c > index 2cc63ef287..bb30cd963a 100644 > --- a/misc/syslog.c > +++ b/misc/syslog.c > @@ -285,7 +285,7 @@ __vsyslog_internal(int pri, const char *fmt, > va_list ap, > > /* Get connected, output the message to the local logger. */ > if (!connected) > - openlog_internal(LogTag, LogStat | LOG_NDELAY, 0); > + openlog_internal(NULL, LogStat | LOG_NDELAY, LogFacility); > > /* If we have a SOCK_STREAM connection, also send ASCII NUL as > a record terminator. */ > @@ -299,7 +299,7 @@ __vsyslog_internal(int pri, const char *fmt, > va_list ap, > /* Try to reopen the syslog connection. Maybe it went > down. */ > closelog_internal (); > - openlog_internal(LogTag, LogStat | LOG_NDELAY, 0); > + openlog_internal(NULL, LogStat | LOG_NDELAY, LogFacility); > } > > if (!connected || __send(LogFile, buf, bufsize, send_flags) < 0) > @@ -343,7 +343,7 @@ openlog_internal(const char *ident, int logstat, > int logfac) > if (ident != NULL) > LogTag = ident; > LogStat = logstat; > - if (logfac != 0 && (logfac &~ LOG_FACMASK) == 0) > + if ((logfac &~ LOG_FACMASK) == 0) > LogFacility = logfac; > > int retry = 0;
On 27/03/2021 17:07, Dan Raymond wrote: > From 93683928886a563a4740e2b42b53752a4a7d431f Mon Sep 17 00:00:00 2001 > From: Dan Raymond <draymond@foxvalley.net> > Date: Sat, 27 Mar 2021 13:51:16 -0600 > Subject: [PATCH] Bug 3604: fix calls to openlog() with LOG_KERN facility > Not allowing LOG_KERN by any user process seems to be de facto behavior on all systems I am aware of: * FreeBSD and MUSL explicit set to previous log facility (they check if the priority against a mask and since on both LOG_KERN is 0 is set to the previous/default value). * Solaris 11.4 man page explicit says: LOG_KERN Messages generated by the kernel. These cannot be gener- ated by any user processes. * AIX 7.2 is similar, but it seems to provide a special symbol for that (which seems to not have a man page): LOG_KERN Logs messages generated by the kernel. Kernel processes should use the bsdlog routine to generate syslog messages. The syntax of bsdlog is identical to syslog. The bsdlog messages can only be created by kernel processes and must be of LOG_KERN priority. The syslog subroutine cannot log LOG_KERN facility messages. Instead it will log LOG_USER facility messages. So before to make glibc an outlier here to fix a very specific issue, I would like to check with other implementation the possible security implication and whether it make sense to change it. Reference: https://sourceware.org/bugzilla/show_bug.cgi?id=3604 > --- > misc/syslog.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/misc/syslog.c b/misc/syslog.c > index 2cc63ef287..bb30cd963a 100644 > --- a/misc/syslog.c > +++ b/misc/syslog.c > @@ -285,7 +285,7 @@ __vsyslog_internal(int pri, const char *fmt, va_list ap, > > /* Get connected, output the message to the local logger. */ > if (!connected) > - openlog_internal(LogTag, LogStat | LOG_NDELAY, 0); > + openlog_internal(NULL, LogStat | LOG_NDELAY, LogFacility); > > /* If we have a SOCK_STREAM connection, also send ASCII NUL as > a record terminator. */ > @@ -299,7 +299,7 @@ __vsyslog_internal(int pri, const char *fmt, va_list ap, > /* Try to reopen the syslog connection. Maybe it went > down. */ > closelog_internal (); > - openlog_internal(LogTag, LogStat | LOG_NDELAY, 0); > + openlog_internal(NULL, LogStat | LOG_NDELAY, LogFacility); > } > > if (!connected || __send(LogFile, buf, bufsize, send_flags) < 0) > @@ -343,7 +343,7 @@ openlog_internal(const char *ident, int logstat, int logfac) > if (ident != NULL) > LogTag = ident; > LogStat = logstat; > - if (logfac != 0 && (logfac &~ LOG_FACMASK) == 0) > + if ((logfac &~ LOG_FACMASK) == 0) > LogFacility = logfac; > > int retry = 0;
On 3/31/21 12:27 PM, Adhemerval Zanella wrote: > > > On 27/03/2021 17:07, Dan Raymond wrote: >> From 93683928886a563a4740e2b42b53752a4a7d431f Mon Sep 17 00:00:00 2001 >> From: Dan Raymond <draymond@foxvalley.net> >> Date: Sat, 27 Mar 2021 13:51:16 -0600 >> Subject: [PATCH] Bug 3604: fix calls to openlog() with LOG_KERN facility >> > > Not allowing LOG_KERN by any user process seems to be de facto behavior > on all systems I am aware of: > > * FreeBSD and MUSL explicit set to previous log facility (they check > if the priority against a mask and since on both LOG_KERN is 0 is > set to the previous/default value). > > * Solaris 11.4 man page explicit says: > > LOG_KERN Messages generated by the kernel. These cannot be gener- > ated by any user processes. The Solaris implementation is similar to FreeBSD & MUSL - LOG_KERN is 0, so appears the same to syslog() as not specifying a facility and letting the default value be used.
On 3/31/2021 1:44 PM, Alan Coopersmith wrote: > On 3/31/21 12:27 PM, Adhemerval Zanella wrote: >> Not allowing LOG_KERN by any user process seems to be de facto behavior >> on all systems I am aware of: >> >> * FreeBSD and MUSL explicit set to previous log facility (they check >> if the priority against a mask and since on both LOG_KERN is 0 is >> set to the previous/default value). >> >> * Solaris 11.4 man page explicit says: >> >> LOG_KERN Messages generated by the kernel. These cannot >> be gener- >> ated by any user processes. > > The Solaris implementation is similar to FreeBSD & MUSL - LOG_KERN is 0, > so appears the same to syslog() as not specifying a facility and letting > the default value be used. It's a fair point that even with this patch the user can't explicitly specify LOG_KERN during a call to syslog(). To use LOG_KERN they must call openlog() first and set it as the default facility. That's a little clumsy but it is good enough to fix the klogd implementation in busybox. What is the alternative? To rewrite klogd so it bypasses syslog() altogether and writes directly to the syslogd socket? That seems inefficient and doesn't really achieve any security. If klogd can do this any user process can do it too.
On Wed, Mar 31, 2021 at 04:27:54PM -0300, Adhemerval Zanella wrote: > > > On 27/03/2021 17:07, Dan Raymond wrote: > > From 93683928886a563a4740e2b42b53752a4a7d431f Mon Sep 17 00:00:00 2001 > > From: Dan Raymond <draymond@foxvalley.net> > > Date: Sat, 27 Mar 2021 13:51:16 -0600 > > Subject: [PATCH] Bug 3604: fix calls to openlog() with LOG_KERN facility > > > > Not allowing LOG_KERN by any user process seems to be de facto behavior > on all systems I am aware of: > > * FreeBSD and MUSL explicit set to previous log facility (they check > if the priority against a mask and since on both LOG_KERN is 0 is > set to the previous/default value). > > * Solaris 11.4 man page explicit says: > > LOG_KERN Messages generated by the kernel. These cannot be gener- > ated by any user processes. > > * AIX 7.2 is similar, but it seems to provide a special symbol for that > (which seems to not have a man page): > > LOG_KERN Logs messages generated by the kernel. Kernel processes > should use the bsdlog routine to generate syslog messages. > The syntax of bsdlog is identical to syslog. The bsdlog > messages can only be created by kernel processes and must > be of LOG_KERN priority. The syslog subroutine cannot log > LOG_KERN facility messages. Instead it will log LOG_USER > facility messages. > > So before to make glibc an outlier here to fix a very specific issue, I > would like to check with other implementation the possible security > implication and whether it make sense to change it. > > Reference: https://sourceware.org/bugzilla/show_bug.cgi?id=3604 It's not a matter of security (this is not a security boundary) but of interface contract: "Values of the priority argument are formed by OR'ing together a severity-level value and an optional facility value. If no facility value is specified, the current default facility value is used." However: > > > --- > > misc/syslog.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/misc/syslog.c b/misc/syslog.c > > index 2cc63ef287..bb30cd963a 100644 > > --- a/misc/syslog.c > > +++ b/misc/syslog.c > > @@ -285,7 +285,7 @@ __vsyslog_internal(int pri, const char *fmt, va_list ap, > > > > /* Get connected, output the message to the local logger. */ > > if (!connected) > > - openlog_internal(LogTag, LogStat | LOG_NDELAY, 0); > > + openlog_internal(NULL, LogStat | LOG_NDELAY, LogFacility); > > > > /* If we have a SOCK_STREAM connection, also send ASCII NUL as > > a record terminator. */ > > @@ -299,7 +299,7 @@ __vsyslog_internal(int pri, const char *fmt, va_list ap, > > /* Try to reopen the syslog connection. Maybe it went > > down. */ > > closelog_internal (); > > - openlog_internal(LogTag, LogStat | LOG_NDELAY, 0); > > + openlog_internal(NULL, LogStat | LOG_NDELAY, LogFacility); > > } > > > > if (!connected || __send(LogFile, buf, bufsize, send_flags) < 0) > > @@ -343,7 +343,7 @@ openlog_internal(const char *ident, int logstat, int logfac) > > if (ident != NULL) > > LogTag = ident; > > LogStat = logstat; > > - if (logfac != 0 && (logfac &~ LOG_FACMASK) == 0) > > + if ((logfac &~ LOG_FACMASK) == 0) > > LogFacility = logfac; > > > > int retry = 0; It looks like the proposed patch is not making syslog violate its interface contract, only fixing glibc's existing violation of the openlog interface contract where it's ignoring the facility argument when the value is zero. If my understanding is correct, that would just bring glibc in line with what musl already does, not make it an outlier. Admittedly having to use openlog to set LOG_KERN is hideous because it involves global state and might break other code in the same process, but at least it works and it's probably acceptable for the rare application that has legitimate reason to send LOG_KERN messages (klogd/syslogd only?). Another possible solution on the libc side would be redefining LOG_KERN to something nonzero and remapping it to the value 0 on the wire. But I suspect this would break syslogd implementations. Rich
On 4/1/2021 9:21 AM, Rich Felker wrote: > On 3/31/2021 7:21 PM, Dan Raymond wrote: >> On 3/31/2021 1:44 PM, Alan Coopersmith wrote: >>> On 3/31/21 12:27 PM, Adhemerval Zanella wrote: >>>> Not allowing LOG_KERN by any user process seems to be de facto behavior >>>> on all systems I am aware of: >>>> >>>> * FreeBSD and MUSL explicit set to previous log facility (they check >>>> if the priority against a mask and since on both LOG_KERN is 0 is >>>> set to the previous/default value). >>>> >>>> * Solaris 11.4 man page explicit says: >>>> >>>> LOG_KERN Messages generated by the kernel. These cannot be gener- >>>> ated by any user processes. >>>> >>>> * AIX 7.2 is similar, but it seems to provide a special symbol for that >>>> (which seems to not have a man page): >>>> >>>> LOG_KERN Logs messages generated by the kernel. Kernel processes >>>> should use the bsdlog routine to generate syslog messages. >>>> The syntax of bsdlog is identical to syslog. The bsdlog >>>> messages can only be created by kernel processes and must >>>> be of LOG_KERN priority. The syslog subroutine cannot log >>>> LOG_KERN facility messages. Instead it will log LOG_USER >>>> facility messages. >>>> >>>> So before to make glibc an outlier here to fix a very specific issue, I >>>> would like to check with other implementation the possible security >>>> implication and whether it make sense to change it. >>> >>> The Solaris implementation is similar to FreeBSD & MUSL - LOG_KERN >>> is 0, >>> so appears the same to syslog() as not specifying a facility and >>> letting >>> the default value be used. >> >> It's a fair point that even with this patch the user can't explicitly >> specify LOG_KERN during a call to syslog(). To use LOG_KERN they >> must call openlog() first and set it as the default facility. That's >> a little clumsy but it is good enough to fix the klogd implementation >> in busybox. What is the alternative? To rewrite klogd so it >> bypasses syslog() altogether and writes directly to the syslogd >> socket? That seems inefficient and doesn't really achieve any >> security. If klogd can do this any user process can do it too. > > It's not a matter of security (this is not a security boundary) but of > interface contract: > > "Values of the priority argument are formed by OR'ing together a > severity-level value and an optional facility value. If no > facility value is specified, the current default facility value is > used." > > However: > > It looks like the proposed patch is not making syslog violate its > interface contract, only fixing glibc's existing violation of the > openlog interface contract where it's ignoring the facility argument > when the value is zero. If my understanding is correct, that would > just bring glibc in line with what musl already does, not make it an > outlier. > > Admittedly having to use openlog to set LOG_KERN is hideous because it > involves global state and might break other code in the same process, > but at least it works and it's probably acceptable for the rare > application that has legitimate reason to send LOG_KERN messages > (klogd/syslogd only?). > > Another possible solution on the libc side would be redefining > LOG_KERN to something nonzero and remapping it to the value 0 on the > wire. But I suspect this would break syslogd implementations. Adhemerval, have you had a chance to look into this?
On 27/03/2021 17:07, Dan Raymond wrote: > From 93683928886a563a4740e2b42b53752a4a7d431f Mon Sep 17 00:00:00 2001 > From: Dan Raymond <draymond@foxvalley.net> > Date: Sat, 27 Mar 2021 13:51:16 -0600 > Subject: [PATCH] Bug 3604: fix calls to openlog() with LOG_KERN facility I agree with Rich remarks [1] that openlog (LOG_KERN) is not a security boundary and making syslog using it after a previously openlog honors the contract interface. I also checked with my tst-syslog.c, and the patch does make KERN_LOG usage without regressions. And I really think that another proposed solution of redefining LOG_KERN to a non-zero value and remapping it when actually creating the message is cumbersome is much complex. So the patch look good, thanks. Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org> [1] https://sourceware.org/pipermail/libc-alpha/2021-April/124650.html [2] https://patchwork.sourceware.org/project/glibc/patch/20210409173031.1508079-1-adhemerval.zanella@linaro.org/ > > --- > misc/syslog.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/misc/syslog.c b/misc/syslog.c > index 2cc63ef287..bb30cd963a 100644 > --- a/misc/syslog.c > +++ b/misc/syslog.c > @@ -285,7 +285,7 @@ __vsyslog_internal(int pri, const char *fmt, va_list ap, > > /* Get connected, output the message to the local logger. */ > if (!connected) > - openlog_internal(LogTag, LogStat | LOG_NDELAY, 0); > + openlog_internal(NULL, LogStat | LOG_NDELAY, LogFacility); > > /* If we have a SOCK_STREAM connection, also send ASCII NUL as > a record terminator. */ > @@ -299,7 +299,7 @@ __vsyslog_internal(int pri, const char *fmt, va_list ap, > /* Try to reopen the syslog connection. Maybe it went > down. */ > closelog_internal (); > - openlog_internal(LogTag, LogStat | LOG_NDELAY, 0); > + openlog_internal(NULL, LogStat | LOG_NDELAY, LogFacility); > } > > if (!connected || __send(LogFile, buf, bufsize, send_flags) < 0) > @@ -343,7 +343,7 @@ openlog_internal(const char *ident, int logstat, int logfac) > if (ident != NULL) > LogTag = ident; > LogStat = logstat; > - if (logfac != 0 && (logfac &~ LOG_FACMASK) == 0) > + if ((logfac &~ LOG_FACMASK) == 0) > LogFacility = logfac; > > int retry = 0;
diff --git a/misc/syslog.c b/misc/syslog.c index 2cc63ef287..bb30cd963a 100644 --- a/misc/syslog.c +++ b/misc/syslog.c @@ -285,7 +285,7 @@ __vsyslog_internal(int pri, const char *fmt, va_list ap, /* Get connected, output the message to the local logger. */ if (!connected) - openlog_internal(LogTag, LogStat | LOG_NDELAY, 0); + openlog_internal(NULL, LogStat | LOG_NDELAY, LogFacility); /* If we have a SOCK_STREAM connection, also send ASCII NUL as a record terminator. */ @@ -299,7 +299,7 @@ __vsyslog_internal(int pri, const char *fmt, va_list ap, /* Try to reopen the syslog connection. Maybe it went down. */ closelog_internal (); - openlog_internal(LogTag, LogStat | LOG_NDELAY, 0); + openlog_internal(NULL, LogStat | LOG_NDELAY, LogFacility); } if (!connected || __send(LogFile, buf, bufsize, send_flags) < 0) @@ -343,7 +343,7 @@ openlog_internal(const char *ident, int logstat, int logfac) if (ident != NULL) LogTag = ident; LogStat = logstat; - if (logfac != 0 && (logfac &~ LOG_FACMASK) == 0) + if ((logfac &~ LOG_FACMASK) == 0) LogFacility = logfac;