Message-ID: <cf7de129-b801-3f0c-64d6-c58d61fd4c84@schaufler-ca.com>
Date:   Tue, 11 May 2021 07:00:07 -0700
From:   Casey Schaufler <casey@...aufler-ca.com>
To:     Paul Moore <paul@...l-moore.com>
Cc:     Richard Guy Briggs <rgb@...hat.com>, linux-api@...r.kernel.org,
        LKML <linux-kernel@...r.kernel.org>,
        Linux-Audit Mailing List <linux-audit@...hat.com>,
        linux-fsdevel@...r.kernel.org, Eric Paris <eparis@...isplace.org>,
        Casey Schaufler <casey@...aufler-ca.com>
Subject: Re: [PATCH V1] audit: log xattr args not covered by syscall record

On 5/10/2021 6:28 PM, Paul Moore wrote:
> On Mon, May 10, 2021 at 8:37 PM Casey Schaufler <casey@...aufler-ca.com> wrote:
>> On 5/10/2021 4:52 PM, Paul Moore wrote:
>>> On Mon, May 10, 2021 at 12:30 PM Casey Schaufler <casey@...aufler-ca.com> wrote:
>>>> On 5/7/2021 6:54 PM, Richard Guy Briggs wrote:
>>>>> On 2021-05-07 14:03, Casey Schaufler wrote:
>>>>>> On 5/7/2021 12:55 PM, Richard Guy Briggs wrote:
>>>>>>> The *setxattr syscalls take 5 arguments.  The SYSCALL record only lists
>>>>>>> four arguments and only lists pointers of string values.  The xattr name
>>>>>>> string, value string and flags (5th arg) are needed by audit given the
>>>>>>> syscall's main purpose.
>>>>>>>
>>>>>>> Add the auxiliary record AUDIT_XATTR (1336) to record the details not
>>>>>>> available in the SYSCALL record including the name string, value string
>>>>>>> and flags.
>>>>>>>
>>>>>>> Notes about field names:
>>>>>>> - name is too generic, use xattr precedent from ima
>>>>>>> - val is already generic value field name
>>>>>>> - flags used by mmap, xflags new name
>>>>>>>
>>>>>>> Sample event with new record:
>>>>>>> type=PROCTITLE msg=audit(05/07/2021 12:58:42.176:189) : proctitle=filecap /tmp/ls dac_override
>>>>>>> type=PATH msg=audit(05/07/2021 12:58:42.176:189) : item=0 name=(null) inode=25 dev=00:1e mode=file,755 ouid=root ogid=root rdev=00:00 obj=unconfined_u:object_r:user_tmp_t:s0 nametype=NORMAL cap_fp=none cap_fi=none cap_fe=0 cap_fver=0 cap_frootid=0
>>>>>>> type=CWD msg=audit(05/07/2021 12:58:42.176:189) : cwd=/root
>>>>>>> type=XATTR msg=audit(05/07/2021 12:58:42.176:189) : xattr="security.capability" val=01 xflags=0x0
>>>>>> Would it be sensible to break out the namespace from the attribute?
>>>>>>
>>>>>>      attrspace="security" attrname="capability"
>>>>> Do xattrs always follow this nomenclature?  Or only the ones we care
>>>>> about?
>>>> Xattrs always have a namespace (man 7 xattr) of "user", "trusted",
>>>> "system" or "security". It's possible that additional namespaces will
>>>> be created in the future, although it seems unlikely given that only
>>>> "security" is widely used today.
>>> Why should audit care about separating the name into two distinct
>>> fields, e.g. "attrspace" and "attrname", instead of just a single
>>> "xattr" field with a value that follows the "namespace.attribute"
>>> format that is commonly seen by userspace?
>> I asked if it would be sensible. I don't much care myself.
> I was *asking* a question - why would we want separate fields?  I
> guess I thought there might be some reason for asking if it was
> sensible; if not, I think I'd rather see it as a single field.

I thought that it might make searching records easier, but I'm
not the expert on that. One might filter on attrspace=security then
look at the attrname values. But that bikeshed can be either color.

>>>>>> Why isn't val= quoted?
>>>>> Good question.  I guessed it should have been since it used
>>>>> audit_log_untrustedstring(), but even the raw output is unquoted unless
>>>>> it was converted by auditd to unquoted before being stored to disk due
>>>>> to nothing offensive found in it since audit_log_n_string() does add
>>>>> quotes.  (hmmm, bit of a run-on sentence there...)
>>>>>
>>>>>> The attribute value can be a .jpg or worse. I could even see it being an eBPF
>>>>>> program (although That Would Be Wrong) so including it in an audit record could
>>>>>> be a bit of a problem.
>>>>> In these cases it would almost certainly get caught by the control
>>>>> character test audit_string_contains_control() in
>>>>> audit_log_n_untrustedstring() called from audit_log_untrustedstring()
>>>>> and deliver it as hex.
>>>> In that case I'm more concerned with the potential size than with
>>>> quoting. One of original use cases proposed for xattrs (back in the
>>>> SGI Irix days) was to attach a bitmap to be used as the icon in file
>>>> browsers as an xattr. Another was to attach the build instructions
>>>> and source used to create a binary. None of that is information you'd
>>>> want to see in a audit record. On the other hand, if the xattr was an
>>>> eBPF program used to make access control decisions, you would want at
>>>> least a reference to it in the audit record.
>>> It would be interesting to see how this code would handle arbitrarily
>>> large xattr values, or at the very least large enough values to blow
>>> up the audit record size.
>>>
>>> As pointed out elsewhere in this thread, and brought up again above
>>> (albeit indirectly), I'm guessing we don't really care about *all*
>>> xattrs, just the "special" xattrs that are security relevant, in which
>>> case I think we need to reconsider how we collect this data.
>> Right. And you can't know in advance which xattrs are relevant in the
>> face of "security=". We might want something like
>>
>>         bool security_auditable_attribute(struct xattr *xattr)
>>
>> which returns true if the passed xattr is one that an LSM in the stack
>> considers relevant. Much like security_ismaclabel(). I don't think that
>> we can just reuse security_ismaclabel() because there are xattrs that
>> are security relevant but are not MAC labels. SMACK64TRANSMUTE is one.
>> File capability sets are another. I also suggest passing the struct xattr
>> rather than the name because it seems reasonable that an LSM might
>> consider "user.execbyroot=never" relevant while "user.execbyroot=possible"
>> isn't.
> Perhaps instead of having the audit code call into the LSM to
> determine if an xattr is worth recording in the audit event, we leave
> it to the LSMs themselves to add a record to the event?

That would be even better. The LSM has all the information it needs.
No reason to add infrastructure.
 

Powered by blists - more mailing lists