LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Tyler Hicks <tyhicks@canonical.com>
To: Paul Moore <paul@paul-moore.com>, Steve Grubb <sgrubb@redhat.com>
Cc: linux-kernel@vger.kernel.org, Kees Cook <keescook@chromium.org>,
	Andy Lutomirski <luto@amacapital.net>,
	Will Drewry <wad@chromium.org>, Eric Paris <eparis@redhat.com>,
	Jonathan Corbet <corbet@lwn.net>,
	linux-audit@redhat.com, linux-security-module@vger.kernel.org,
	linux-doc@vger.kernel.org
Subject: Re: [PATCH 2/3] seccomp: Audit attempts to modify the actions_logged sysctl
Date: Wed, 2 May 2018 10:58:43 -0500	[thread overview]
Message-ID: <3ead1d7d-9146-8785-9cc9-51eeee3fd250@canonical.com> (raw)
In-Reply-To: <CAHC9VhRW2fcdMK98=sf7otWDQv4N8ZiSFwv7GZmRfN=sCmGHaw@mail.gmail.com>


[-- Attachment #1.1: Type: text/plain, Size: 8826 bytes --]

On 05/01/2018 12:25 PM, Paul Moore wrote:
> On Tue, May 1, 2018 at 12:41 PM, Steve Grubb <sgrubb@redhat.com> wrote:
>> On Tuesday, May 1, 2018 11:18:55 AM EDT Paul Moore wrote:
>>> On Fri, Apr 27, 2018 at 3:16 PM, Tyler Hicks <tyhicks@canonical.com> wrote:
>>>> The decision to log a seccomp action will always be subject to the
>>>> value of the kernel.seccomp.actions_logged sysctl, even for processes
>>>> that are being inspected via the audit subsystem, in an upcoming patch.
>>>> Therefore, we need to emit an audit record on attempts at writing to the
>>>> actions_logged sysctl when auditing is enabled.
>>>>
>>>> This patch updates the write handler for the actions_logged sysctl to
>>>> emit an audit record on attempts to write to the sysctl. Successful
>>>> writes to the sysctl will result in a record that includes a normalized
>>>> list of logged actions in the "actions" field and a "res" field equal to
>>>> 0. Unsuccessful writes to the sysctl will result in a record that
>>>> doesn't include the "actions" field and has a "res" field equal to 1.
>>>>
>>>> Not all unsuccessful writes to the sysctl are audited. For example, an
>>>> audit record will not be emitted if an unprivileged process attempts to
>>>> open the sysctl file for reading since that access control check is not
>>>> part of the sysctl's write handler.
>>>>
>>>> Below are some example audit records when writing various strings to the
>>>> actions_logged sysctl.
>>>>
>>>> Writing "not-a-real-action" emits:
>>>>  type=CONFIG_CHANGE msg=audit(1524600971.363:119): pid=1651 uid=0
>>>>  auid=1000 tty=pts8 ses=1 comm="tee" exe="/usr/bin/tee"
>>>>  op=seccomp-logging res=1
>>>>
>>>> Writing "kill_process kill_thread errno trace log" emits:
>>>>  type=CONFIG_CHANGE msg=audit(1524601023.982:131): pid=1658 uid=0
>>>>  auid=1000 tty=pts8 ses=1 comm="tee" exe="/usr/bin/tee"
>>>>  op=seccomp-logging actions="kill_process kill_thread errno trace log"
>>>>  res=0
>>>
>>> I've got some additional comments regarding the fields in the code
>>> below, but it would be good to hear Steve comment on the "actions"
>>> field since his userspace tools are extremely picky about what they
>>> will accept.
>>
>> Its not that the audit user space applications are picky, its that we have a
>> coding standard that everyone needs to abide by so that any parser coded to
>> the specification works.
> 
> We're getting a bit off track, but the issue with the "specification"
> is that there has not been enough checking and enforcement of the
> "specification" to give it much weight.  We've made some fixes to
> records of lower impact, but I'm not going to merge disruptive record
> changes.  After a while, the status-quo becomes the "specification".
> 
> At some point in the future I plan to submit patches which disconnect
> the audit data from the record format for in-kernel callers; this is
> really the only way we can enforce any type of record format.  The
> current in-kernel audit API is for too open for misuse and abuse.
> 
>> In short, we should not have spaces inside the ""
>> because that can trick a naive parser. What we typically do in a situation
>> like this is add a comma as a separator. But having "" means that the value
>> is untrusted and subject to escaping. I don't think that is the case here.
>> Output is not controlled by the user. Its a list of well known names.
>>
>>> It looks like you are treating the actions as an untrusted string, which is
>>> good, so I suspect you are okay, but still
>>
>> The function below that logs names is calling audit_log_format which does not
>> handle untrusted strings. I would suggest not treating it as an untrusted
>> string, but as a string with no spaces in it.
>>
>> actions=kill_process,kill_thread,errno,trace,log
> 
> Yes, my mistake, I suspect I was distracted by the comm logging.
> 
>>>> Writing the string "log log errno trace kill_process kill_thread", which
>>>> is unordered and contains the log action twice, results in the same
>>>>
>>>> value as the previous example for the actions field:
>>>>  type=CONFIG_CHANGE msg=audit(1524601204.365:152): pid=1704 uid=0
>>>>  auid=1000 tty=pts8 ses=1 comm="tee" exe="/usr/bin/tee"
>>>>  op=seccomp-logging actions="kill_process kill_thread errno trace log"
>>>>  res=0
>>>>
>>>> No audit records are generated when reading the actions_logged sysctl.
>>>>
>>>> Suggested-by: Steve Grubb <sgrubb@redhat.com>
>>>> Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
>>>> ---
>>>>
>>>>  include/linux/audit.h |  3 +++
>>>>  kernel/auditsc.c      | 37 +++++++++++++++++++++++++++++++++++++
>>>>  kernel/seccomp.c      | 43 ++++++++++++++++++++++++++++++++++---------
>>>>  3 files changed, 74 insertions(+), 9 deletions(-)
>>>
>>> ...
>>>
>>>> diff --git a/include/linux/audit.h b/include/linux/audit.h
>>>> index 75d5b03..b311d7d 100644
>>>> --- a/include/linux/audit.h
>>>> +++ b/include/linux/audit.h
>>>> @@ -233,6 +233,7 @@ extern void __audit_inode_child(struct inode *parent,
>>>>
>>>>                                 const struct dentry *dentry,
>>>>                                 const unsigned char type);
>>>>
>>>>  extern void __audit_seccomp(unsigned long syscall, long signr, int
>>>>  code);
>>>>
>>>> +extern void audit_seccomp_actions_logged(const char *names, int res);
>>>>
>>>>  extern void __audit_ptrace(struct task_struct *t);
>>>>
>>>>  static inline bool audit_dummy_context(void)
>>>>
>>>> @@ -502,6 +503,8 @@ static inline void __audit_seccomp(unsigned long
>>>> syscall, long signr, int code)>
>>>>  { }
>>>>  static inline void audit_seccomp(unsigned long syscall, long signr, int
>>>>  code) { }
>>>>
>>>> +static inline void audit_seccomp_actions_logged(const char *names, int
>>>> res) +{ }
>>>>
>>>>  static inline int auditsc_get_stamp(struct audit_context *ctx,
>>>>
>>>>                               struct timespec64 *t, unsigned int *serial)
>>>>
>>>>  {
>>>>
>>>> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>>>> index 4e0a4ac..3496238 100644
>>>> --- a/kernel/auditsc.c
>>>> +++ b/kernel/auditsc.c
>>>> @@ -2478,6 +2478,43 @@ void __audit_seccomp(unsigned long syscall, long
>>>> signr, int code)>
>>>>         audit_log_end(ab);
>>>>
>>>>  }
>>>>
>>>> +void audit_seccomp_actions_logged(const char *names, int res)
>>>> +{
>>>> +       struct tty_struct *tty;
>>>> +       const struct cred *cred;
>>>> +       struct audit_buffer *ab;
>>>> +       char comm[sizeof(current->comm)];
>>>> +
>>>> +       if (!audit_enabled)
>>>> +               return;
>>>> +
>>>> +       ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
>>>> +       if (unlikely(!ab))
>>>> +               return;
>>>
>>> Instead of NULL, let's pass current->audit_context to
>>> audit_log_start().  Yes, most of the AUDIT_CONFIG_CHANGE users pass
>>> NULL but all of that is going to need to change because of 1) the
>>> audit container ID work and 2) it makes sense to connect records that
>>> are related.  Let's do it right the first time here :)
>>>
>>>> +       cred = current_cred();
>>>> +       tty = audit_get_tty(current);
>>>> +       audit_log_format(ab, "pid=%d uid=%u auid=%u tty=%s ses=%u",
>>>> +                        task_tgid_nr(current),
>>>> +                        from_kuid(&init_user_ns, cred->uid),
>>>> +                        from_kuid(&init_user_ns,
>>>> +                        audit_get_loginuid(current)),
>>>> +                        tty ? tty_name(tty) : "(none)",
>>>> +                        audit_get_sessionid(current));
>>>> +       audit_put_tty(tty);
>>>> +       audit_log_task_context(ab);
>>>> +       audit_log_format(ab, " comm=");
>>>> +       audit_log_untrustedstring(ab, get_task_comm(comm, current));
>>>> +       audit_log_d_path_exe(ab, current->mm);
>>>> +       audit_log_format(ab, " op=seccomp-logging");
>>>> +       if (names)
>>>> +               audit_log_format(ab, " actions=\"%s\"", names);
>>>> +
>>>> +       audit_log_format(ab, " res=%d", res);
>>>> +       audit_log_end(ab);
>>>
>>> One of the benefits of using current->audit_context is that we get a
>>> lot of this info from the associated SYSCALL record (assuming the
>>> admin isn't filtering that, e.g. Fedora defaults).  We can safely drop
>>> most everything except for the "op" and "actions" fields.
>>>
>>> Steve might also want an "old-actions" field, but I'll let him speak to
>>> that.
>>
>> Configuration changes are supposed to show current and new values.
>>
>> -Steve

All of the feedback received for this patch set is addressed in v2:

 https://lkml.kernel.org/r/<1525276400-7161-1-git-send-email-tyhicks@canonical.com>
Thanks for the reviews!

Tyler


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

  reply	other threads:[~2018-05-02 15:59 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-27 19:15 [PATCH 0/3] Better integrate seccomp logging and auditing Tyler Hicks
2018-04-27 19:16 ` [PATCH 1/3] seccomp: Separate read and write code for actions_logged sysctl Tyler Hicks
2018-04-27 19:16 ` [PATCH 2/3] seccomp: Audit attempts to modify the " Tyler Hicks
2018-05-01 15:18   ` Paul Moore
2018-05-01 16:41     ` Steve Grubb
2018-05-01 17:25       ` Paul Moore
2018-05-02 15:58         ` Tyler Hicks [this message]
2018-04-27 19:16 ` [PATCH 3/3] seccomp: Don't special case audited processes when logging Tyler Hicks
2018-05-01 15:27   ` 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 \
    --in-reply-to=3ead1d7d-9146-8785-9cc9-51eeee3fd250@canonical.com \
    --to=tyhicks@canonical.com \
    --cc=corbet@lwn.net \
    --cc=eparis@redhat.com \
    --cc=keescook@chromium.org \
    --cc=linux-audit@redhat.com \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=luto@amacapital.net \
    --cc=paul@paul-moore.com \
    --cc=sgrubb@redhat.com \
    --cc=wad@chromium.org \
    --subject='Re: [PATCH 2/3] seccomp: Audit attempts to modify the actions_logged sysctl' \
    /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).