Received: by 2002:ac0:a594:0:0:0:0:0 with SMTP id m20-v6csp3787320imm; Thu, 17 May 2018 14:57:39 -0700 (PDT) X-Google-Smtp-Source: AB8JxZooz91rR7TJoXNtca0Yai43BedjM0xdO4Bnt98CLcZ/KQ0KlKO5lJ/2/6ezXt5cun32UtSq X-Received: by 2002:a17:902:781:: with SMTP id 1-v6mr6854733plj.150.1526594259640; Thu, 17 May 2018 14:57:39 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1526594259; cv=none; d=google.com; s=arc-20160816; b=sEiDhWLL975yU9znWmAuIrZJpRxHAkUhc4INQ4wS3a6Lz2VLgOxCQ5Z0OUoVBCKnBP Kw0JionAddOrLNd60GybHwg/HtDtCPIO4pKFPGhukjhKRM0VD3WgqXFTjUUSKRAIz9de cEalNkobtl9SvJbsLEhr+D83KRSgnB/bntf7v04leBgdrp0qlPLUYSlIcPhoYdVo/CYX a2aLggt9t6ROtgnvfkq2wBpGmHYJ6Vsmvyx5OcX+9XLjwLXhS0GsRkDN69FkGu49AA7u HCRrDpjiigvvie8/Qb8vRDAstEmffquVoQK71NDnuXOAq5gaXoTOFWxtAB5lblGjlNpN 58JA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:user-agent:in-reply-to :content-disposition:mime-version:references:message-id:subject:cc :to:from:date:arc-authentication-results; bh=tEkwGzrio4SMb+rAdlI737uVahFw3HkFKzVtfJxrVGU=; b=HFqL95t6ZwIl+QCtOUiQ9jWKCOn5Qip9tmMFTWy3IdIcazWk4m791InvwWAnBTIyrd kw8/y1Jp/dr9TZrryiL6TzYx8dYyws2/WhdTi4ImaMFdtKXb8iJ7aCUKAnakae2Z7FR0 QtMLA5UiBUkIREZmON20b3H0pcl7DdNQ31OGR4F988Z2R2bLlpwB+y8UXhD1zqGtXAxU LmM9Wm4SONX4YNmkjMRO0NWW7S3MkvYT0nTPsG8YNHhJhjhjJo+gJQ1LesWYJUY98xEi 4WTiK5nGkcqN+KYHt/IBA3ng0CGHoOEvaFfTTIS/l10+jowxFv8mtW33Fi4UI6m+Xqdm fujw== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=redhat.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id o6-v6si5603567pls.234.2018.05.17.14.56.55; Thu, 17 May 2018 14:57:39 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751985AbeEQV4k (ORCPT + 99 others); Thu, 17 May 2018 17:56:40 -0400 Received: from mx3-rdu2.redhat.com ([66.187.233.73]:38982 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750938AbeEQV4i (ORCPT ); Thu, 17 May 2018 17:56:38 -0400 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id E306F40267A3; Thu, 17 May 2018 21:56:37 +0000 (UTC) Received: from madcap2.tricolour.ca (ovpn-112-24.rdu2.redhat.com [10.10.112.24]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 2C2102024CBB; Thu, 17 May 2018 21:56:34 +0000 (UTC) Date: Thu, 17 May 2018 17:56:00 -0400 From: Richard Guy Briggs To: Steve Grubb Cc: cgroups@vger.kernel.org, containers@lists.linux-foundation.org, linux-api@vger.kernel.org, Linux-Audit Mailing List , linux-fsdevel@vger.kernel.org, LKML , netdev@vger.kernel.org, ebiederm@xmission.com, luto@kernel.org, jlayton@redhat.com, carlos@redhat.com, dhowells@redhat.com, viro@zeniv.linux.org.uk, simo@redhat.com, eparis@parisplace.org, serge@hallyn.com Subject: Re: [RFC PATCH ghak32 V2 01/13] audit: add container id Message-ID: <20180517215600.dyswlkvqdtgjwr5y@madcap2.tricolour.ca> References: <20180517170053.7d4afa87@ivy-bridge> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180517170053.7d4afa87@ivy-bridge> User-Agent: NeoMutt/20171027 X-Scanned-By: MIMEDefang 2.78 on 10.11.54.4 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.6]); Thu, 17 May 2018 21:56:37 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.6]); Thu, 17 May 2018 21:56:37 +0000 (UTC) for IP:'10.11.54.4' DOMAIN:'int-mx04.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'rgb@redhat.com' RCPT:'' Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2018-05-17 17:00, Steve Grubb wrote: > On Fri, 16 Mar 2018 05:00:28 -0400 > 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 was one thing I was wondering about. Currently when we set the > loginuid, the record is AUDIT_LOGINUID. The corollary is that when we > set the container id, the event should be AUDIT_CONTAINERID or > AUDIT_CONTAINER_ID. The record type is actually AUDIT_LOGIN. The field type is AUDIT_LOGINUID. Given that correction, I think we're fine and could potentially violently agree. The existing naming is consistent. > During syscall events, the path info is returned in a a record simply > called AUDIT_PATH, cwd info is returned in AUDIT_CWD. So, rather than > calling the record that gets attached to everything > AUDIT_CONTAINER_INFO, how about simply AUDIT_CONTAINER. Considering the container initiation record is different than the record to document the container involved in an otherwise normal syscall, we need two names. I don't have a strong opinion what they are. I'd prefer AUDIT_CONTAINER and AUDIT_CONTAINER_INFO so that the two are different enough to be visually distinct while leaving AUDIT_CONTAINERID for the field type in patch 4 ("audit: add containerid filtering") > > 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/audit.h > > b/include/linux/audit.h index af410d9..fe4ba3f 100644 > > --- a/include/linux/audit.h > > +++ b/include/linux/audit.h > > @@ -29,6 +29,7 @@ > > > > #define AUDIT_INO_UNSET ((unsigned long)-1) > > #define AUDIT_DEV_UNSET ((dev_t)-1) > > +#define INVALID_CID AUDIT_CID_UNSET > > > > struct audit_sig_info { > > uid_t uid; > > @@ -321,6 +322,7 @@ static inline void audit_ptrace(struct > > task_struct *t) extern int auditsc_get_stamp(struct audit_context > > *ctx, struct timespec64 *t, unsigned int *serial); > > extern int audit_set_loginuid(kuid_t loginuid); > > +extern int audit_set_containerid(struct task_struct *tsk, u64 > > containerid); > > static inline kuid_t audit_get_loginuid(struct task_struct *tsk) > > { > > @@ -332,6 +334,11 @@ static inline unsigned int > > audit_get_sessionid(struct task_struct *tsk) return tsk->sessionid; > > } > > > > +static inline u64 audit_get_containerid(struct task_struct *tsk) > > +{ > > + return tsk->containerid; > > +} > > + > > extern void __audit_ipc_obj(struct kern_ipc_perm *ipcp); > > extern void __audit_ipc_set_perm(unsigned long qbytes, uid_t uid, > > gid_t gid, umode_t mode); extern void __audit_bprm(struct > > linux_binprm *bprm); @@ -517,6 +524,10 @@ static inline unsigned int > > audit_get_sessionid(struct task_struct *tsk) { > > return -1; > > } > > +static inline kuid_t audit_get_containerid(struct task_struct *tsk) > > +{ > > + return INVALID_CID; > > +} > > static inline void audit_ipc_obj(struct kern_ipc_perm *ipcp) > > { } > > static inline void audit_ipc_set_perm(unsigned long qbytes, uid_t > > uid, @@ -581,6 +592,11 @@ static inline bool > > audit_loginuid_set(struct task_struct *tsk) return > > uid_valid(audit_get_loginuid(tsk)); } > > > > +static inline bool audit_containerid_set(struct task_struct *tsk) > > +{ > > + return audit_get_containerid(tsk) != INVALID_CID; > > +} > > + > > static inline void audit_log_string(struct audit_buffer *ab, const > > char *buf) { > > audit_log_n_string(ab, buf, strlen(buf)); > > diff --git a/include/linux/init_task.h b/include/linux/init_task.h > > index 6a53262..046bd0a 100644 > > --- a/include/linux/init_task.h > > +++ b/include/linux/init_task.h > > @@ -18,6 +18,7 @@ > > #include > > #include > > #include > > +#include > > > > #include > > > > @@ -120,7 +121,8 @@ > > #ifdef CONFIG_AUDITSYSCALL > > #define INIT_IDS \ > > .loginuid = INVALID_UID, \ > > - .sessionid = (unsigned int)-1, > > + .sessionid = (unsigned int)-1, \ > > + .containerid = INVALID_CID, > > #else > > #define INIT_IDS > > #endif > > 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 int sessionid; > > + u64 containerid; > > #endif > > struct seccomp seccomp; > > > > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h > > index 4e61a9e..921a71f 100644 > > --- a/include/uapi/linux/audit.h > > +++ b/include/uapi/linux/audit.h > > @@ -71,6 +71,7 @@ > > #define AUDIT_TTY_SET 1017 /* Set TTY auditing > > status */ #define AUDIT_SET_FEATURE 1018 /* Turn an > > audit feature on or off */ #define AUDIT_GET_FEATURE > > 1019 /* Get which features are enabled */ +#define > > AUDIT_CONTAINER 1020 /* Define the container id > > and information */ #define AUDIT_FIRST_USER_MSG 1100 /* > > Userspace messages mostly uninteresting to kernel */ #define > > AUDIT_USER_AVC 1107 /* We filter this > > differently */ @@ -465,6 +466,7 @@ struct audit_tty_status { }; > > > > #define AUDIT_UID_UNSET (unsigned int)-1 > > +#define AUDIT_CID_UNSET ((u64)-1) > > > > /* audit_rule_data supports filter rules with both integer and string > > * fields. It corresponds with AUDIT_ADD_RULE, AUDIT_DEL_RULE and > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > > index 4e0a4ac..29c8482 100644 > > --- a/kernel/auditsc.c > > +++ b/kernel/auditsc.c > > @@ -2073,6 +2073,90 @@ int audit_set_loginuid(kuid_t loginuid) > > return rc; > > } > > > > +static int audit_set_containerid_perm(struct task_struct *task, u64 > > containerid) +{ > > + struct task_struct *parent; > > + u64 pcontainerid, ccontainerid; > > + > > + /* Don't allow to set our own containerid */ > > + if (current == task) > > + return -EPERM; > > + /* Don't allow the containerid to be unset */ > > + if (!cid_valid(containerid)) > > + return -EINVAL; > > + /* if we don't have caps, reject */ > > + if (!capable(CAP_AUDIT_CONTROL)) > > + return -EPERM; > > + /* if containerid is unset, allow */ > > + if (!audit_containerid_set(task)) > > + return 0; > > + /* it is already set, and not inherited from the parent, > > reject */ > > + ccontainerid = audit_get_containerid(task); > > + rcu_read_lock(); > > + parent = rcu_dereference(task->real_parent); > > + rcu_read_unlock(); > > + task_lock(parent); > > + pcontainerid = audit_get_containerid(parent); > > + task_unlock(parent); > > + if (ccontainerid != pcontainerid) > > + return -EPERM; > > + return 0; > > +} > > + > > +static void audit_log_set_containerid(struct task_struct *task, u64 > > oldcontainerid, > > + u64 containerid, int rc) > > +{ > > + struct audit_buffer *ab; > > + uid_t uid; > > + struct tty_struct *tty; > > + > > + if (!audit_enabled) > > + return; > > + > > + ab = audit_log_start(NULL, GFP_KERNEL, AUDIT_CONTAINER); > > + if (!ab) > > + return; > > + > > + uid = from_kuid(&init_user_ns, task_uid(current)); > > + tty = audit_get_tty(current); > > + > > + audit_log_format(ab, "op=set pid=%d uid=%u", > > task_tgid_nr(current), uid); > > + audit_log_task_context(ab); > > + audit_log_format(ab, " auid=%u tty=%s ses=%u opid=%d > > old-contid=%llu contid=%llu res=%d", > > The preferred ordering would be: op, opid, old-contid, contid, pid, uid, > tty, ses, subj, comm, exe, res. This groups the searchable fields > together using the most common ordering so that parsing is simple. There has been a suggestion to make this a syscall-connected record, and if that is the case, we'd simply drop all the fields that would be duplicated by the syscall record. Otherwise, I'll use your suggested order. As you may recall this suggestion was also made for the AUDIT_LOGIN record. > Thanks, > -Steve > > > + from_kuid(&init_user_ns, > > audit_get_loginuid(current)), > > + tty ? tty_name(tty) : "(none)", > > audit_get_sessionid(current), > > + task_tgid_nr(task), oldcontainerid, > > containerid, !rc); + > > + audit_put_tty(tty); > > + audit_log_end(ab); > > +} > > + > > +/** > > + * audit_set_containerid - set current task's audit_context > > containerid > > + * @containerid: containerid value > > + * > > + * Returns 0 on success, -EPERM on permission failure. > > + * > > + * Called (set) from fs/proc/base.c::proc_containerid_write(). > > + */ > > +int audit_set_containerid(struct task_struct *task, u64 containerid) > > +{ > > + u64 oldcontainerid; > > + int rc; > > + > > + oldcontainerid = audit_get_containerid(task); > > + > > + rc = audit_set_containerid_perm(task, containerid); > > + if (!rc) { > > + task_lock(task); > > + task->containerid = containerid; > > + task_unlock(task); > > + } > > + > > + audit_log_set_containerid(task, oldcontainerid, containerid, > > rc); > > + return rc; > > +} > > + > > /** > > * __audit_mq_open - record audit data for a POSIX MQ open > > * @oflag: open flag > - RGB -- Richard Guy Briggs Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada IRC: rgb, SunRaycer Voice: +1.647.777.2635, Internal: (81) 32635