| Message ID | 20221011112733.194079-1-plautrba@redhat.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | libselinux: Add missing '\n' to avc_log() messages | expand |
On 10/11/2022 7:27 AM, Petr Lautrbach wrote: > Signed-off-by: Petr Lautrbach <plautrba@redhat.com> Reviewed-by: Daniel Burgener <dburgener@linux.microsoft.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); > > 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;
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. > > 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
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? -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
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. 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
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. > > 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 >
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 >>
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); 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;
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(-)