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:     Richard Guy Briggs <rgb@redhat.com>
Cc:     Linux-Audit Mailing List <linux-audit@redhat.com>,
        linux-fsdevel@vger.kernel.org, linux-api@vger.kernel.org,
        LKML <linux-kernel@vger.kernel.org>,
        Eric Paris <eparis@parisplace.org>, sgrubb@redhat.com,
        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>
From:   Casey Schaufler <casey@schaufler-ca.com>
Message-ID: <242f107a-3b74-c1c2-abd6-b3f369170023@schaufler-ca.com>
Date:   Mon, 10 May 2021 09:30:09 -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: <20210508015443.GA447005@madcap2.tricolour.ca>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Content-Language: en-US
Content-Length: 4907
Precedence: bulk

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 lis=
ts
>>> four arguments and only lists pointers of string values.  The xattr n=
ame
>>> string, value string and flags (5th arg) are needed by audit given th=
e
>>> 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 stri=
ng
>>> 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) : proctitle=
=3Dfilecap /tmp/ls dac_override
>>> type=3DPATH msg=3Daudit(05/07/2021 12:58:42.176:189) : item=3D0 name=3D=
(null) inode=3D25 dev=3D00:1e mode=3Dfile,755 ouid=3Droot ogid=3Droot rde=
v=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"secu=
rity.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 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 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 rec=
ord 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.=20

>> It seems that you might want to leave it up to the LSMs to determine w=
hich xattr
>> values are audited. An SELinux system may want to see "security.selinu=
x" values,
>> but it probably doesn't care about "security.SMACK64TRANSMUTE" values.=

> Are you suggesting that any that don't follow this nomenclature are
> irrelevant or harmless at best and are not worth recording?

Nope. I'm saying that unless an xattr is going to be used for security
related purposes it isn't security relevant and hence shouldn't go in
the audit trail. The "security" namespace indication may not be sufficien=
t
to make that determination, although it's a pretty good indicator today.
As I pointed out, setting an attribute used by Smack on an SELinux system=

is not security relevant.

There is nothing preventing an LSM from using "user" namespace attributes=
=2E
I could imagine a security policy that looks a user supplied attributes
(user.runasroot=3Dnever) and restricts when a program can be used. Thus, =
it
needs to be up to the LSM to decide if an attribute should be audited. We=

have to leave ACLs as an exception, of course, because they don't (and in=

their current form can't) use the LSM infrastructure.

> This sounds like you are thinking about your LSM stacking patchset that=

> issues a new record for each LSM attribute if there is more than one.
> And any system that has multiple LSMs active should be able to indicate=

> all of interest.

There's some of that, but realize that Smack already uses multiple xattrs=
,
none of which are "security.selinux". Logging setting of "security.SMACK6=
4EXEC"
makes sense. Logging "security.MyDogHasNoNose" does not. On the other han=
d,
if Yama decided to start using that attribute you'd have to look for it a=
s well.