From: Casey Schaufler <firstname.lastname@example.org> To: Paul Moore <email@example.com> Cc: Richard Guy Briggs <firstname.lastname@example.org>, email@example.com, LKML <firstname.lastname@example.org>, Linux-Audit Mailing List <email@example.com>, firstname.lastname@example.org, Eric Paris <email@example.com>, Casey Schaufler <firstname.lastname@example.org> Subject: Re: [PATCH V1] audit: log xattr args not covered by syscall record Date: Tue, 11 May 2021 07:00:07 -0700 [thread overview] Message-ID: <email@example.com> (raw) In-Reply-To: <CAHC9VhTPQ-LoqUYJ4HGsFF-sAXR+mYqGga7TxRZOG7BUD-55FQ@mail.gmail.com> On 5/10/2021 6:28 PM, Paul Moore wrote: > On Mon, May 10, 2021 at 8:37 PM Casey Schaufler <firstname.lastname@example.org> wrote: >> On 5/10/2021 4:52 PM, Paul Moore wrote: >>> On Mon, May 10, 2021 at 12:30 PM Casey Schaufler <email@example.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.
next prev parent reply other threads:[~2021-05-11 14:00 UTC|newest] Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-05-07 19:55 Richard Guy Briggs 2021-05-07 21:03 ` Casey Schaufler 2021-05-08 1:54 ` Richard Guy Briggs 2021-05-10 16:30 ` Casey Schaufler 2021-05-10 23:52 ` Paul Moore 2021-05-11 0:37 ` Casey Schaufler 2021-05-11 1:28 ` Paul Moore 2021-05-11 14:00 ` Casey Schaufler [this message] 2021-05-11 15:52 ` Paul Moore
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --subject='Re: [PATCH V1] audit: log xattr args not covered by syscall record' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).