Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH V1] audit: log xattr args not covered by syscall record
To:     Paul Moore <paul@paul-moore.com>
Cc:     Richard Guy Briggs <rgb@redhat.com>, linux-api@vger.kernel.org,
        LKML <linux-kernel@vger.kernel.org>,
        Linux-Audit Mailing List <linux-audit@redhat.com>,
        linux-fsdevel@vger.kernel.org, Eric Paris <eparis@parisplace.org>,
        Casey Schaufler <casey@schaufler-ca.com>
References: <604ceafd516b0785fea120f552d6336054d196af.1620414949.git.rgb@redhat.com>
 <7ee601c2-4009-b354-1899-3c8f582bf6ae@schaufler-ca.com>
 <20210508015443.GA447005@madcap2.tricolour.ca>
 <242f107a-3b74-c1c2-abd6-b3f369170023@schaufler-ca.com>
 <CAHC9VhQdV93G5N_BKsxuDCtFbm9-xvAkve02t5sGOi9Mam2Wtg@mail.gmail.com>
From:   Casey Schaufler <casey@schaufler-ca.com>
Message-ID: <195ac224-00fa-b1be-40c8-97e823796262@schaufler-ca.com>
Date:   Mon, 10 May 2021 17:37:19 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.10.1
MIME-Version: 1.0
In-Reply-To: <CAHC9VhQdV93G5N_BKsxuDCtFbm9-xvAkve02t5sGOi9Mam2Wtg@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Content-Language: en-US
Content-Length: 5038
Precedence: bulk

On 5/10/2021 4:52 PM, Paul Moore wrote:
> On Mon, May 10, 2021 at 12:30 PM Casey Schaufler <casey@schaufler-ca.co=
m> 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 l=
ists
>>>>> 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 n=
ot
>>>>> available in the SYSCALL record including the name string, value st=
ring
>>>>> 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=3DPROCTITLE msg=3Daudit(05/07/2021 12:58:42.176:189) : proctit=
le=3Dfilecap /tmp/ls dac_override
>>>>> type=3DPATH msg=3Daudit(05/07/2021 12:58:42.176:189) : item=3D0 nam=
e=3D(null) inode=3D25 dev=3D00:1e mode=3Dfile,755 ouid=3Droot ogid=3Droot=
 rdev=3D00:00 obj=3Dunconfined_u:object_r:user_tmp_t:s0 nametype=3DNORMAL=
 cap_fp=3Dnone cap_fi=3Dnone cap_fe=3D0 cap_fver=3D0 cap_frootid=3D0
>>>>> type=3DCWD msg=3Daudit(05/07/2021 12:58:42.176:189) : cwd=3D/root
>>>>> type=3DXATTR msg=3Daudit(05/07/2021 12:58:42.176:189) : xattr=3D"se=
curity.capability" val=3D01 xflags=3D0x0
>>>> Would it be sensible to break out the namespace from the attribute?
>>>>
>>>>      attrspace=3D"security" attrname=3D"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.

>>>> Why isn't val=3D quoted?
>>> Good question.  I guessed it should have been since it used
>>> audit_log_untrustedstring(), but even the raw output is unquoted unle=
ss
>>> it was converted by auditd to unquoted before being stored to disk du=
e
>>> 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 bein=
g an eBPF
>>>> program (although That Would Be Wrong) so including it in an audit r=
ecord 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=3D". 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=3Dnever" relevant while "user.execbyroot=3Dposs=
ible"
isn't.

> I think it would also be very good to see what requirements may be
> driving this work.  Is it purely a "gee, this seems important" or is
> it a hard requirement in a security certification for example.
>