Bug 3604: fix calls to openlog() with LOG_KERN facility

  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;
  

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;