Re: [RFC PATCH ghak32 V2 01/13] audit: add container id
On 2018-04-18 19:47, Paul Moore wrote:
> On Fri, Mar 16, 2018 at 5:00 AM, Richard Guy Briggs wrote:
> > Implement the proc fs write to set the audit container ID of a process,
> > emitting an AUDIT_CONTAINER record to document the event.
> >
> > This is a write from the container orchestrator task to a proc entry of
> > the form /proc/PID/containerid where PID is the process ID of the newly
> > created task that is to become the first task in a container, or an
> > additional task added to a container.
> >
> > The write expects up to a u64 value (unset: 18446744073709551615).
> >
> > This will produce a record such as this:
> > type=CONTAINER msg=audit(1519903238.968:261): op=set pid=596 uid=0
> > subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 auid=0 tty=pts0
> > ses=1 opid=596 old-contid=18446744073709551615 contid=123455 res=0
> >
> > The "op" field indicates an initial set. The "pid" to "ses" fields are
> > the orchestrator while the "opid" field is the object's PID, the process
> > being "contained". Old and new container ID values are given in the
> > "contid" fields, while res indicates its success.
> >
> > It is not permitted to self-set, unset or re-set the container ID. A
> > child inherits its parent's container ID, but then can be set only once
> > after.
> >
> > See: https://github.com/linux-audit/audit-kernel/issues/32
> >
> > Signed-off-by: Richard Guy Briggs
> > ---
> > fs/proc/base.c | 37
> > include/linux/audit.h | 16 +
> > include/linux/init_task.h | 4 ++-
> > include/linux/sched.h | 1 +
> > include/uapi/linux/audit.h | 2 ++
> > kernel/auditsc.c | 84
> > ++
> > 6 files changed, 143 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/proc/base.c b/fs/proc/base.c
> > index 60316b5..6ce4fbe 100644
> > --- a/fs/proc/base.c
> > +++ b/fs/proc/base.c
> > @@ -1299,6 +1299,41 @@ static ssize_t proc_sessionid_read(struct file *
> > file, char __user * buf,
> > .read = proc_sessionid_read,
> > .llseek = generic_file_llseek,
> > };
> > +
> > +static ssize_t proc_containerid_write(struct file *file, const char __user
> > *buf,
> > + size_t count, loff_t *ppos)
> > +{
> > + struct inode *inode = file_inode(file);
> > + u64 containerid;
> > + int rv;
> > + struct task_struct *task = get_proc_task(inode);
> > +
> > + if (!task)
> > + return -ESRCH;
> > + if (*ppos != 0) {
> > + /* No partial writes. */
> > + put_task_struct(task);
> > + return -EINVAL;
> > + }
> > +
> > + rv = kstrtou64_from_user(buf, count, 10, &containerid);
> > + if (rv < 0) {
> > + put_task_struct(task);
> > + return rv;
> > + }
> > +
> > + rv = audit_set_containerid(task, containerid);
> > + put_task_struct(task);
> > + if (rv < 0)
> > + return rv;
> > + return count;
> > +}
> > +
> > +static const struct file_operations proc_containerid_operations = {
> > + .write = proc_containerid_write,
> > + .llseek = generic_file_llseek,
> > +};
> > +
> > #endif
> >
> > #ifdef CONFIG_FAULT_INJECTION
> > @@ -2961,6 +2996,7 @@ static int proc_pid_patch_state(struct seq_file *m,
> > struct pid_namespace *ns,
> > #ifdef CONFIG_AUDITSYSCALL
> > REG("loginuid", S_IWUSR|S_IRUGO, proc_loginuid_operations),
> > REG("sessionid", S_IRUGO, proc_sessionid_operations),
> > + REG("containerid", S_IWUSR, proc_containerid_operations),
> > #endif
> > #ifdef CONFIG_FAULT_INJECTION
> > REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),
> > @@ -3355,6 +3391,7 @@ static int proc_tid_comm_permission(struct inode
> > *inode, int mask)
> > #ifdef CONFIG_AUDITSYSCALL
> > REG("loginuid", S_IWUSR|S_IRUGO, proc_loginuid_operations),
> > REG("sessionid", S_IRUGO, proc_sessionid_operations),
> > + REG("containerid", S_IWUSR, proc_containerid_operations),
> > #endif
> > #ifdef CONFIG_FAULT_INJECTION
> > REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),
...
> > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > index d258826..1b82191 100644
> > --- a/include/linux/sched.h
> > +++ b/include/linux/sched.h
> > @@ -796,6 +796,7 @@ struct task_struct {
> > #ifdef CONFIG_AUDITSYSCALL
> > kuid_t loginuid;
> > unsigned intsessionid;
> > + u64 containerid;
>
> This one line addition to the task_struct scares me the most of
> anything in this patchset. Why? It's a field named "containerid" in
> a perhaps one of the most widely used core kernel structures; the
> possibilities for abuse are endless, and it's foolish to think we
> would ever be able to adequat
Re: [PATCH v3 0/4] Better integrate seccomp logging and auditing
On Thu, May 3, 2018 at 9:08 PM, Tyler Hicks wrote: > Seccomp received improved logging controls in v4.14. Applications can opt into > logging of "handled" actions (SECCOMP_RET_TRAP, SECCOMP_RET_TRACE, > SECCOMP_RET_ERRNO) using the SECCOMP_FILTER_FLAG_LOG bit when loading filters. > They can also debug filter matching with the new SECCOMP_RET_LOG action. > Administrators can prevent specific actions from being logged using the > kernel.seccomp.actions_logged sysctl. > > However, one corner case intentionally wasn't addressed in those v4.14 > changes. > When a process is being inspected by the audit subsystem, seccomp's decision > making for logging ignores the new controls and unconditionally logs every > action taken except for SECCOMP_RET_ALLOW. This isn't particularly useful > since > many existing applications don't intend to log handled actions due to them > occurring very frequently. This amount of logging fills the audit logs without > providing many benefits now that application authors have fine grained > controls > at their disposal. > > This patch set aligns the seccomp logging behavior for both audited and > non-audited processes. It also emits an audit record, if auditing is enabled, > when the kernel.seccomp.actions_logged sysctl is written to so that there's a > paper trail when entire actions are quieted. > > Changes in v3: > * Patch 3 > - Never drop a field when emitting the audit record > - Use the value "?" for the actions field when an error occurred while > writing to the sysctl > - Use the value "?" for the actions and/or old-actions fields when a failure > to translate actions to names > - Use the value "(none)" for the actions and/or old-actions fields when no > actions are specified > + This is possible when writing an empty string to the sysctl > - Update the commit message to note the new values and give an example of > when an empty string is written > * Patch 4 > - Adjust the control flow of seccomp_log() to exit early if nothing should > be > logged > > Changes in v2: > * Patch 2 > - New patch, allowing for a configurable separator between action names > * Patch 3 > - The value of the actions field in the audit record now uses a comma > instead > of a space > - The value of the actions field in the audit record is no longer enclosed > in > quotes > - audit_log_start() is called with the current processes' audit_context in > audit_seccomp_actions_logged() > - audit_seccomp_actions_logged() no longer records the pid, uid, auid, tty, > ses, task context, comm, or executable path > - The new and old value of seccomp_actions_logged is recorded in the > AUDIT_CONFIG_CHANGE record > - The value of the "res" field in the CONFIG_CHANGE audit record is > corrected > (1 indicates success, 0 failure) > - Updated patch 3's commit message to reflect the updated audit record > format > in the examples > * Patch 4 > - A function comment for audit_seccomp() was added to explain, among other > things, that event filtering is performed in seccomp_log() Kees, are you still okay with v3? Also, are you okay with these patches going in via the audit tree, or would you prefer to take them via seccomp? I've got a slight preference for the audit tree myself, but as I said before, as long as it hits Linus' tree I'm happy. -- paul moore www.paul-moore.com -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH v3 0/4] Better integrate seccomp logging and auditing
On Sun, May 6, 2018 at 2:31 PM, Paul Moore wrote: > On Thu, May 3, 2018 at 9:08 PM, Tyler Hicks wrote: >> Seccomp received improved logging controls in v4.14. Applications can opt >> into >> logging of "handled" actions (SECCOMP_RET_TRAP, SECCOMP_RET_TRACE, >> SECCOMP_RET_ERRNO) using the SECCOMP_FILTER_FLAG_LOG bit when loading >> filters. >> They can also debug filter matching with the new SECCOMP_RET_LOG action. >> Administrators can prevent specific actions from being logged using the >> kernel.seccomp.actions_logged sysctl. >> >> However, one corner case intentionally wasn't addressed in those v4.14 >> changes. >> When a process is being inspected by the audit subsystem, seccomp's decision >> making for logging ignores the new controls and unconditionally logs every >> action taken except for SECCOMP_RET_ALLOW. This isn't particularly useful >> since >> many existing applications don't intend to log handled actions due to them >> occurring very frequently. This amount of logging fills the audit logs >> without >> providing many benefits now that application authors have fine grained >> controls >> at their disposal. >> >> This patch set aligns the seccomp logging behavior for both audited and >> non-audited processes. It also emits an audit record, if auditing is enabled, >> when the kernel.seccomp.actions_logged sysctl is written to so that there's a >> paper trail when entire actions are quieted. >> >> Changes in v3: >> * Patch 3 >> - Never drop a field when emitting the audit record >> - Use the value "?" for the actions field when an error occurred while >> writing to the sysctl >> - Use the value "?" for the actions and/or old-actions fields when a >> failure >> to translate actions to names >> - Use the value "(none)" for the actions and/or old-actions fields when no >> actions are specified >> + This is possible when writing an empty string to the sysctl >> - Update the commit message to note the new values and give an example of >> when an empty string is written >> * Patch 4 >> - Adjust the control flow of seccomp_log() to exit early if nothing should >> be >> logged >> >> Changes in v2: >> * Patch 2 >> - New patch, allowing for a configurable separator between action names >> * Patch 3 >> - The value of the actions field in the audit record now uses a comma >> instead >> of a space >> - The value of the actions field in the audit record is no longer enclosed >> in >> quotes >> - audit_log_start() is called with the current processes' audit_context in >> audit_seccomp_actions_logged() >> - audit_seccomp_actions_logged() no longer records the pid, uid, auid, tty, >> ses, task context, comm, or executable path >> - The new and old value of seccomp_actions_logged is recorded in the >> AUDIT_CONFIG_CHANGE record >> - The value of the "res" field in the CONFIG_CHANGE audit record is >> corrected >> (1 indicates success, 0 failure) >> - Updated patch 3's commit message to reflect the updated audit record >> format >> in the examples >> * Patch 4 >> - A function comment for audit_seccomp() was added to explain, among other >> things, that event filtering is performed in seccomp_log() > > Kees, are you still okay with v3? Also, are you okay with these > patches going in via the audit tree, or would you prefer to take them > via seccomp? I've got a slight preference for the audit tree myself, > but as I said before, as long as it hits Linus' tree I'm happy. Yup, it looks good. I have no tree preference, so you win! :) Please consider the whole series: Acked-by: Kees Cook -Kees -- Kees Cook Pixel Security -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
