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 <rgb(a)redhat.com&gt; 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 <rgb(a)redhat.com&gt; > > --- > > 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/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; > > 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 adequately police this. Fair enough. > Unfortunately, we can't add the field to audit_context as things > currently stand because we don't always allocate an audit_context, > it's dependent on the system's configuration, and we need to track the > audit container ID for a given process, regardless of the audit > configuration. Pretty much the same reason why loginuid and sessionid > are located directly in task_struct now. As I stressed during the > design phase, I really want to keep this as an *audit* container ID > and not a general purpose kernel wide container ID. If the kernel > ever grows a general purpose container ID token, I'll be the first in > line to convert the audit code, but I don't want audit to be that > general purpose mechanism ... audit is hated enough as-is ;) When would we need an audit container ID when audit is not enabled enough to have an audit_context?

If it is only used for audit, and audit is the only consumer, and audit can only use it when it is enabled, then we can just return success to any write to the proc filehandle, or not even present it. Nothing will be able to know that value wasn't used. When are loginuid and sessionid used now when audit is not enabled (or should I say, explicitly disabled)?

> > 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) > > I think we need to decide if we want to distinguish between the "host" > (e.g. init ns) and "unset". Looking at this patch (I've only quickly > skimmed the others so far) it would appear that you don't think we > need to worry about this distinction; that's fine, but let's make it > explicit with a comment in the code that AUDIT_CID_UNSET means "unset" > as well as "host". I don't see any reason to distinguish between "host" and "unset". Since a container doesn't have a concrete definition based in namespaces, the initial namespace set is meaningless here.

Is there value in having a container orchestrator process have a reserved container ID that has a policy distinct from any other container?

If so, then I could see the value in making the distinction. For example, I've heard of interest in systemd acting as a container orchestrator, so if it took on that role as PID 1, then every process in the system would inherit that ID and none would be unset. I can't picture how having seperate "host" and "unset" values helps us.

> > /* 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; > > Why not? Is there some obvious security concern that I missing? We then lose the distinction in the AUDIT_CONTAINER record between the initiating PID and the target PID. This was outlined in the proposal.

Having said that, I'm still not sure we have protected sufficiently from a child turning around and setting it's parent's as yet unset or inherited audit container ID.

> I ask because I suppose it might be possible for some container > runtime to do a fork, setup some of the environment and them exec the > container (before you answer the obvious "namespaces!" please remember > we're not trying to define containers). I don't think namespaces have any bearing on this concern since none are required. > > + /* 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", > > + 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; > > Why are audit_set_containerid_perm() and audit_log_containerid() > separate functions? (I assume you mean audit_log_set_containerid()?)

It seemed clearer that all the permission checking was in one function and its return code could be used to report the outcome when logging the (attempted) action. This is the same structure as audit_set_loginuid() and it made sense.

This would be the time to connect it to a syscall if that seems like a good idea and remove pid, uid, auid, tty, ses fields.