Re: [PATCH] libselinux: Add missing '\n' to avc_log() messages

[Petr Lautrbach <plautrba@redhat.com>]

Christian Göttsche <cgzones@googlemail.com> writes:

> On Fri, 14 Oct 2022 at 16:37, Petr Lautrbach <plautrba@redhat.com> wrote:
>>
>> Daniel Burgener <dburgener@linux.microsoft.com> writes:
>>
>> > On 10/14/2022 6:41 AM, Petr Lautrbach wrote:
>> >> Petr Lautrbach <plautrba@redhat.com> writes:
>> >>
>> >>> Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
>> >>> ---
>> >>>   libselinux/src/avc.c          | 2 +-
>> >>>   libselinux/src/avc_internal.c | 4 ++--
>> >>>   libselinux/src/checkAccess.c  | 4 ++--
>> >>>   3 files changed, 5 insertions(+), 5 deletions(-)
>> >>>
>> >>> diff --git a/libselinux/src/avc.c b/libselinux/src/avc.c
>> >>> index 8d5983a2fe0c..98a3fcae41c8 100644
>> >>> --- a/libselinux/src/avc.c
>> >>> +++ b/libselinux/src/avc.c
>> >>> @@ -725,7 +725,7 @@ void avc_audit(security_id_t ssid, security_id_t tsid,
>> >>>     if (denied)
>> >>>             log_append(avc_audit_buf, " permissive=%u", result ? 0 : 1);
>> >>>
>> >>> -   avc_log(SELINUX_AVC, "%s", avc_audit_buf);
>> >>> +   avc_log(SELINUX_AVC, "%s\n", avc_audit_buf);
>> >>
>> >>
>> >> There is a conflict between this change and commit 142372522c7e ("libselinux: avoid
>> >> newline in avc message").
>> >>
>> >> I'll send another version without it.
>> >
>> > Now that you've pointed out Christian's patch, this feels like
>> > potentially the wrong level to solve this.
>> >
>> > As I understand it, the issue Christian was trying to solve is that
>> > audit doesn't parse as we intend if there is a newline in the middle of
>> > the message, and userspace object managers append additional material to
>> > USER_AVC messages.  Hence his removal of newline above.
>> >
>> > The problem this patch is trying to solve is that when SELinux aware
>> > applications call logging functions in libselinux, they get printed
>> > directly to standard error, and in that case really should end in a newline.
>> >
>> > Secondarily, this patch solves the fact that previously the messages
>> > logged by SELinux were just inconsistent with regards to final newlines.
>> >
>> > It happens that in the current state of things, userspace object
>> > managers append to AVCs above, and rpm had issues with setenforce and
>> > policyload, so segregating newlines based on message type as this patch
>> > with the above hunk dropped would do addresses all the issues.
>> >
>> > I feel like that's sort of a happenstance that this is the current state
>> > of the code though, and if a future change results in SELinux aware
>> > applications dumping AVCs directly to standard error for example, then
>> > there won't be a good solution in the current approach.
>> >
>> > Would it be perhaps a cleaner solution to standardize all libselinux
>> > messages on no newline and then changing default_selinux_log() to append
>> > a newline since that's writing directly to stderr and relying on callers
>> > into libselinux to add a newline if needed?
>>
>> This is exactly my thoughts and reason why I haven't sent the new patch
>> yet.
>>
>> If we do this we would need to check all main consumers whether they
>> depend on the new line or not.
>
> First of all I agree that log messages should fbe consistent in
> whether they end with a newline or not, and that bahavior should be
> documented (probably in selinux_set_callback(3)).
>
> Looking at some callers of selinux_set_callback(3):
>
>
> I. systemd
>
> log_callback() [1] forwards the message either to
> audit_log_user_avc_message(3), which does not expect a trailing
> newline, or to the internal function log_internalv(), which to my
> analysis also do not.
>
> [1]: https://github.com/systemd/systemd/blob/3e15bed410ff616f5015b4e87eb25d1fee8828e5/src/core/selinux-access.c#L97
>
>
> II. dbus-broker
>
> bus_selinux_log() [2] forwards the message to the internal function
> util_audit_log() [3], which either forwards it to
> audit_log_user_avc_message(3) or explitcitly add a newline when
> printing to stderr.
>
> [2]: https://github.com/bus1/dbus-broker/blob/ef1c9f03a6be40474486044b6a28780d12107d9b/src/util/selinux.c#L288
> [3]: https://github.com/bus1/dbus-broker/blob/ef1c9f03a6be40474486044b6a28780d12107d9b/src/util/audit.c#L104
>
>
> III. dbus
>
> log_callback() [4] forwards the message either to
> audit_log_user_avc_message(3) or vsyslog(3), which both do not expect
> a trailing newline.
>
> [4]: https://gitlab.freedesktop.org/dbus/dbus/-/blob/master/bus/selinux.c#L95
>
>
> IV. pam
>
> log_callback() [5] forwards the message either to
> audit_log_user_avc_message(3) or vsyslog(3), which both do not expect
> a trailing newline.
>
> [5]: https://github.com/linux-pam/linux-pam/blob/f69a6042da801096c94b30465c118e17c803f5c2/modules/pam_rootok/pam_rootok.c#L54
>
>
> V. shadow
>
> selinux_log_cb() [6] forwards the message either to
> audit_log_user_avc_message(3) or via a SYSLOG wrapper to syslog(3),
> which both do not expect a trailing newline.
>
> [6]: https://github.com/shadow-maint/shadow/blob/16afe18142bf8e0ba8b315aac10526b8998fa98e/lib/selinux.c#L113
>
>
> Summary:
>
> All of the checked applications expect a message without a trailing
> newline, so I would favor dropping all newlines from existing
> libselinux log messages and explicitly add a newline in the default
> handler, default_selinux_log(), printing to stderr.
>

Thanks!

I will prepare a patch that removes all newline characters from
avc_log() messages, adds a newline character to the default handler, and
adds a note to the selinux_set_callback() man page stating that messages
do not contain newline characters.


Petr





>>
>> Or given the number of avc_log() with "\n" vs those without, we could
>> revert Christian's patch, document that messages are always ended with
>> "\n" and let consumers strip it.
>>
>>
>> Petr
>>
>>
>>
>> >
>> > -Daniel
>> >
>> >>
>> >>
>> >>>
>> >>>     avc_release_lock(avc_log_lock);
>> >>>   }
>> >>> diff --git a/libselinux/src/avc_internal.c b/libselinux/src/avc_internal.c
>> >>> index 71a1357bc564..c550e5788527 100644
>> >>> --- a/libselinux/src/avc_internal.c
>> >>> +++ b/libselinux/src/avc_internal.c
>> >>> @@ -59,7 +59,7 @@ int avc_process_setenforce(int enforcing)
>> >>>     int rc = 0;
>> >>>
>> >>>     avc_log(SELINUX_SETENFORCE,
>> >>> -           "%s:  op=setenforce lsm=selinux enforcing=%d res=1",
>> >>> +           "%s:  op=setenforce lsm=selinux enforcing=%d res=1\n",
>> >>>             avc_prefix, enforcing);
>> >>>     if (avc_setenforce)
>> >>>             goto out;
>> >>> @@ -81,7 +81,7 @@ int avc_process_policyload(uint32_t seqno)
>> >>>     int rc = 0;
>> >>>
>> >>>     avc_log(SELINUX_POLICYLOAD,
>> >>> -           "%s:  op=load_policy lsm=selinux seqno=%u res=1",
>> >>> +           "%s:  op=load_policy lsm=selinux seqno=%u res=1\n",
>> >>>             avc_prefix, seqno);
>> >>>     rc = avc_ss_reset(seqno);
>> >>>     if (rc < 0) {
>> >>> diff --git a/libselinux/src/checkAccess.c b/libselinux/src/checkAccess.c
>> >>> index 022cd6b5ecab..319af267c6a7 100644
>> >>> --- a/libselinux/src/checkAccess.c
>> >>> +++ b/libselinux/src/checkAccess.c
>> >>> @@ -44,7 +44,7 @@ int selinux_check_access(const char *scon, const char *tcon, const char *class,
>> >>>          sclass = string_to_security_class(class);
>> >>>          if (sclass == 0) {
>> >>>            rc = errno;
>> >>> -          avc_log(SELINUX_ERROR, "Unknown class %s", class);
>> >>> +          avc_log(SELINUX_ERROR, "Unknown class %s\n", class);
>> >>>            if (security_deny_unknown() == 0)
>> >>>                    return 0;
>> >>>            errno = rc;
>> >>> @@ -54,7 +54,7 @@ int selinux_check_access(const char *scon, const char *tcon, const char *class,
>> >>>          av = string_to_av_perm(sclass, perm);
>> >>>          if (av == 0) {
>> >>>            rc = errno;
>> >>> -          avc_log(SELINUX_ERROR, "Unknown permission %s for class %s", perm, class);
>> >>> +          avc_log(SELINUX_ERROR, "Unknown permission %s for class %s\n", perm, class);
>> >>>            if (security_deny_unknown() == 0)
>> >>>                    return 0;
>> >>>            errno = rc;
>> >>> --
>> >>> 2.37.3
>>