On 5/10/2021 4:52 PM, Paul Moore wrote: > On Mon, May 10, 2021 at 12:30 PM Casey Schaufler <casey@xxxxxxxxxxxxxxxx> 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. >>>> 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. > 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. >