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

From: Stephen Smalley <sds@tycho.nsa.gov>
To: Paul Moore <paul@paul-moore.com>, Ondrej Mosnacek <omosnace@redhat.com>
Cc: selinux@vger.kernel.org, linux-audit@redhat.com,
	Daniel Walsh <dwalsh@redhat.com>
Subject: Re: [PATCH v2] selinux: log invalid contexts in AVCs
Date: Tue, 22 Jan 2019 15:00:25 -0500	[thread overview]
Message-ID: <d2e83637-f9fa-9744-a9d0-407ad108db3a@tycho.nsa.gov> (raw)
In-Reply-To: <CAHC9VhRDSMeojFEFz+8rshiP4TmtoGuEuOK0AwZecyJ0SWN3Ug@mail.gmail.com>

On 1/22/19 2:42 PM, Paul Moore wrote:
> 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.

Technically could occur on other permission checks where the source 
context isn't a process context, e.g. filesystem associate check 
(scontext/srawcon is a file context), socket checks (scontext/srawcon is 
a socket context, which may not correspond to any running process if 
passed to another or using /proc/self/attr/sockcreate), msgq enqueue 
(scontext/srawcon is context of message).  Common property is that the 
context had to have been valid at the point it was converted to a SID 
and then invalidated by a policy reload.  In contrast, trawcon can occur 
for file contexts read from the filesystem that were never valid under 
any policy loaded into the kernel since boot.

> 
>> 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
> 