Daniel Burgener <dburgener@xxxxxxxxxxxxxxxxxxx> writes:
> On 10/14/2022 6:41 AM, Petr Lautrbach wrote:
>> Petr Lautrbach <plautrba@xxxxxxxxxx> writes:
>>
>>> Signed-off-by: Petr Lautrbach <plautrba@xxxxxxxxxx>
>>> ---
>>> 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
