Re: [PATCH v2] selinux: log invalid contexts in AVCs

[Paul Moore <paul@paul-moore.com>]

On Mon, Jan 21, 2019 at 10:36 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> In case a file has an invalid context set, in an AVC record generated
> upon access to such file, the target context is always reported as
> unlabeled. This patch adds new optional fields to the AVC record
> (srawcon and trawcon) that report the actual context string if it
> differs from the one reported in scontext/tcontext. This is useful for
> diagnosing SELinux denials involving invalid contexts.
>
> To trigger an AVC that illustrates this situation:
>
>     # setenforce 0
>     # touch /tmp/testfile
>     # setfattr -n security.selinux -v system_u:object_r:banana_t:s0 /tmp/testfile
>     # runcon system_u:system_r:sshd_t:s0 cat /tmp/testfile
>
> AVC before:
>
> type=AVC msg=audit(1547801083.248:11): avc:  denied  { open } for  pid=1149 comm="cat" path="/tmp/testfile" dev="tmpfs" ino=6608 scontext=system_u:system_r:sshd_t:s0 tcontext=system_u:object_r:unlabeled_t:s15:c0.c1023 tclass=file permissive=1
>
> AVC after:
>
> type=AVC msg=audit(1547801083.248:11): avc:  denied  { open } for  pid=1149 comm="cat" path="/tmp/testfile" dev="tmpfs" ino=6608 scontext=system_u:system_r:sshd_t:s0 tcontext=system_u:object_r:unlabeled_t:s15:c0.c1023 trawcon=system_u:object_r:banana_t:s0 tclass=file permissive=1

I would like us to add new fields at the end of existing records; the
recent audit config changes are a bit of a special case as discussed
previously.

Also, under what cases would we ever see a srawcon field?  This is
only going to happen if we have a running process whose domain is
removed during a policy reload, correct?  I'm find with including this
for the sake of completeness, but I would mention this in the patch
description for the next revision.

> Cc: Daniel Walsh <dwalsh@redhat.com>
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=1135683
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>
> v2: Rename fields to "(s|t)rawcon".
>
>  security/selinux/avc.c | 49 +++++++++++++++++++++++++-----------------
>  1 file changed, 29 insertions(+), 20 deletions(-)
>
> diff --git a/security/selinux/avc.c b/security/selinux/avc.c
> index 9b63d8ee1687..df5490db575b 100644
> --- a/security/selinux/avc.c
> +++ b/security/selinux/avc.c
> @@ -165,6 +165,32 @@ static void avc_dump_av(struct audit_buffer *ab, u16 tclass, u32 av)
>         audit_log_format(ab, " }");
>  }
>
> +static void avc_dump_sid(struct audit_buffer *ab, struct selinux_state *state,
> +                        u32 sid, char type)
> +{
> +       int rc;
> +       char *context, *rcontext;
> +       u32 context_len, rcontext_len;
> +
> +       rc = security_sid_to_context(state, sid, &context, &context_len);
> +       if (rc) {
> +               audit_log_format(ab, "%csid=%d ", type, sid);
> +               return;
> +       }
> +
> +       audit_log_format(ab, "%ccontext=%s ", type, context);
> +
> +       /* in case of invalid context report also the actual context string */
> +       rc = security_sid_to_context_force(state, sid, &rcontext,
> +                                          &rcontext_len);
> +       if (!rc) {
> +               if (strcmp(context, rcontext))
> +                       audit_log_format(ab, "%crawcon=%s ", type, rcontext);
> +               kfree(rcontext);
> +       }
> +       kfree(context);
> +}
> +
>  /**
>   * avc_dump_query - Display a SID pair and a class in human-readable form.
>   * @ssid: source security identifier
> @@ -174,28 +200,11 @@ static void avc_dump_av(struct audit_buffer *ab, u16 tclass, u32 av)
>  static void avc_dump_query(struct audit_buffer *ab, struct selinux_state *state,
>                            u32 ssid, u32 tsid, u16 tclass)
>  {
> -       int rc;
> -       char *scontext;
> -       u32 scontext_len;
> -
> -       rc = security_sid_to_context(state, ssid, &scontext, &scontext_len);
> -       if (rc)
> -               audit_log_format(ab, "ssid=%d", ssid);
> -       else {
> -               audit_log_format(ab, "scontext=%s", scontext);
> -               kfree(scontext);
> -       }
> -
> -       rc = security_sid_to_context(state, tsid, &scontext, &scontext_len);
> -       if (rc)
> -               audit_log_format(ab, " tsid=%d", tsid);
> -       else {
> -               audit_log_format(ab, " tcontext=%s", scontext);
> -               kfree(scontext);
> -       }
> +       avc_dump_sid(ab, state, ssid, 's');
> +       avc_dump_sid(ab, state, tsid, 't');
>
>         BUG_ON(!tclass || tclass >= ARRAY_SIZE(secclass_map));
> -       audit_log_format(ab, " tclass=%s", secclass_map[tclass-1].name);
> +       audit_log_format(ab, "tclass=%s", secclass_map[tclass-1].name);
>  }
>
>  /**
> --
> 2.20.1

-- 
paul moore
www.paul-moore.com