LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Paul Moore <paul@paul-moore.com>
To: Tyler Hicks <tyhicks@canonical.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>,
	Steve Grubb <sgrubb@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 3/3] seccomp: Don't special case audited processes when logging
Date: Tue, 1 May 2018 11:27:45 -0400	[thread overview]
Message-ID: <CAHC9VhSfOCLASK9k07ENwBBP39rPk2uggcVLbWgDeo4bzbgdqg@mail.gmail.com> (raw)
In-Reply-To: <1524856562-22566-4-git-send-email-tyhicks@canonical.com>

On Fri, Apr 27, 2018 at 3:16 PM, Tyler Hicks <tyhicks@canonical.com> wrote:
> Seccomp logging for "handled" actions such as RET_TRAP, RET_TRACE, or
> RET_ERRNO can be very noisy for processes that are being audited. This
> patch modifies the seccomp logging behavior to treat processes that are
> being inspected via the audit subsystem the same as processes that
> aren't under inspection. Handled actions will no longer be logged just
> because the process is being inspected. Since v4.14, applications have
> the ability to request logging of handled actions by using the
> SECCOMP_FILTER_FLAG_LOG flag when loading seccomp filters.
>
> With this patch, the logic for deciding if an action will be logged is:
>
>   if action == RET_ALLOW:
>     do not log
>   else if action not in actions_logged:
>     do not log
>   else if action == RET_KILL:
>     log
>   else if action == RET_LOG:
>     log
>   else if filter-requests-logging:
>     log
>   else:
>     do not log
>
> Reported-by: Steve Grubb <sgrubb@redhat.com>
> Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
> ---
>  Documentation/userspace-api/seccomp_filter.rst |  7 -------
>  include/linux/audit.h                          | 10 +---------
>  kernel/auditsc.c                               |  2 +-
>  kernel/seccomp.c                               | 15 +++++----------
>  4 files changed, 7 insertions(+), 27 deletions(-)
>
> diff --git a/Documentation/userspace-api/seccomp_filter.rst b/Documentation/userspace-api/seccomp_filter.rst
> index 099c412..82a468b 100644
> --- a/Documentation/userspace-api/seccomp_filter.rst
> +++ b/Documentation/userspace-api/seccomp_filter.rst
> @@ -207,13 +207,6 @@ directory. Here's a description of each file in that directory:
>         to the file do not need to be in ordered form but reads from the file
>         will be ordered in the same way as the actions_avail sysctl.
>
> -       It is important to note that the value of ``actions_logged`` does not
> -       prevent certain actions from being logged when the audit subsystem is
> -       configured to audit a task. If the action is not found in
> -       ``actions_logged`` list, the final decision on whether to audit the
> -       action for that task is ultimately left up to the audit subsystem to
> -       decide for all seccomp return values other than ``SECCOMP_RET_ALLOW``.
> -
>         The ``allow`` string is not accepted in the ``actions_logged`` sysctl
>         as it is not possible to log ``SECCOMP_RET_ALLOW`` actions. Attempting
>         to write ``allow`` to the sysctl will result in an EINVAL being
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index b311d7d..1964fbd 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -232,7 +232,7 @@ extern void __audit_file(const struct file *);
>  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(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);
>
> @@ -303,12 +303,6 @@ static inline void audit_inode_child(struct inode *parent,
>  }
>  void audit_core_dumps(long signr);
>
> -static inline void audit_seccomp(unsigned long syscall, long signr, int code)
> -{
> -       if (audit_enabled && unlikely(!audit_dummy_context()))
> -               __audit_seccomp(syscall, signr, code);
> -}
> -
>  static inline void audit_ptrace(struct task_struct *t)
>  {
>         if (unlikely(!audit_dummy_context()))
> @@ -499,8 +493,6 @@ static inline void audit_inode_child(struct inode *parent,
>  { }
>  static inline void audit_core_dumps(long signr)
>  { }
> -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)
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 3496238..1e64b91 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -2464,7 +2464,7 @@ void audit_core_dumps(long signr)
>         audit_log_end(ab);
>  }
>
> -void __audit_seccomp(unsigned long syscall, long signr, int code)
> +void audit_seccomp(unsigned long syscall, long signr, int code)
>  {
>         struct audit_buffer *ab;

Since it is a bit unusual, it might be nice to add a comment at the
top of audit_seccomp() that the event filtering is being done in the
seccomp_log() function, and we may need to force auditing independent
of the audit_enabled and dummy context state.

> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index e28ddcc..947cc0f 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -584,18 +584,13 @@ static inline void seccomp_log(unsigned long syscall, long signr, u32 action,
>         }
>
>         /*
> -        * Force an audit message to be emitted when the action is RET_KILL_*,
> -        * RET_LOG, or the FILTER_FLAG_LOG bit was set and the action is
> -        * allowed to be logged by the admin.
> +        * Emit an audit message when the action is RET_KILL_*, RET_LOG, or the
> +        * FILTER_FLAG_LOG bit was set. The admin has the ability to silence
> +        * any action from being logged by removing the action name from the
> +        * seccomp_actions_logged sysctl.
>          */
>         if (log)
> -               return __audit_seccomp(syscall, signr, action);
> -
> -       /*
> -        * Let the audit subsystem decide if the action should be audited based
> -        * on whether the current task itself is being audited.
> -        */
> -       return audit_seccomp(syscall, signr, action);
> +               audit_seccomp(syscall, signr, action);
>  }
>
>  /*
> --
> 2.7.4
>



-- 
paul moore
www.paul-moore.com

      reply	other threads:[~2018-05-01 15:27 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
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 [this message]

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=CAHC9VhSfOCLASK9k07ENwBBP39rPk2uggcVLbWgDeo4bzbgdqg@mail.gmail.com \
    --to=paul@paul-moore.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=sgrubb@redhat.com \
    --cc=tyhicks@canonical.com \
    --cc=wad@chromium.org \
    --subject='Re: [PATCH 3/3] seccomp: Don'\''t special case audited processes when logging' \
    /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).