From: Richard Guy Briggs <rgb@redhat.com>
To: Paul Moore <paul@paul-moore.com>
Cc: cgroups@vger.kernel.org, containers@lists.linux-foundation.org,
linux-api@vger.kernel.org, linux-audit@redhat.com,
linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
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,
Eric Paris <eparis@parisplace.org>,
serge@hallyn.com
Subject: Re: [RFC PATCH ghak90 (was ghak32) V3 01/10] audit: add container id
Date: Mon, 30 Jul 2018 14:47:31 -0400 [thread overview]
Message-ID: <20180730184731.aycnmknlew4vhnqe@madcap2.tricolour.ca> (raw)
In-Reply-To: <CAHC9VhQ3KwG-YmpJsOa9k=YqQmaQTaJXeB=eM9Fm80hujbngBg@mail.gmail.com>
On 2018-07-24 17:54, Paul Moore wrote:
> On Tue, Jul 24, 2018 at 3:09 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2018-07-20 18:13, Paul Moore wrote:
> > > On Wed, Jun 6, 2018 at 1:00 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > > Implement the proc fs write to set the audit container identifier of a
> > > > process, emitting an AUDIT_CONTAINER_ID record to document the event.
> > > >
> > > > This is a write from the container orchestrator task to a proc entry of
> > > > the form /proc/PID/audit_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).
> > > >
> > > > The writer must have capability CAP_AUDIT_CONTROL.
> > > >
> > > > This will produce a record such as this:
> > > > type=CONTAINER_ID msg=audit(2018-06-06 12:39:29.636:26949) : op=set opid=2209 old-contid=18446744073709551615 contid=123456 pid=628 auid=root uid=root tty=ttyS0 ses=1 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 comm=bash exe=/usr/bin/bash res=yes
> > > >
> > > > 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 audit container identifier values are
> > > > given in the "contid" fields, while res indicates its success.
> > > >
> > > > It is not permitted to unset or re-set the audit container identifier.
> > > > A child inherits its parent's audit container identifier, but then can
> > > > be set only once after.
> > > >
> > > > See: https://github.com/linux-audit/audit-kernel/issues/90
> > > > See: https://github.com/linux-audit/audit-userspace/issues/51
> > > > See: https://github.com/linux-audit/audit-testsuite/issues/64
> > > > See: https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID
> > > >
> > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > > ---
> > > > fs/proc/base.c | 37 ++++++++++++++++++++++++
> > > > include/linux/audit.h | 25 ++++++++++++++++
> > > > include/uapi/linux/audit.h | 2 ++
> > > > kernel/auditsc.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++
> > > > 4 files changed, 135 insertions(+)
>
> ...
>
> > > > @@ -2112,6 +2116,73 @@ int audit_set_loginuid(kuid_t loginuid)
> > > > }
> > > >
> > > > /**
> > > > + * audit_set_contid - set current task's audit_context contid
> > > > + * @contid: contid value
> > > > + *
> > > > + * Returns 0 on success, -EPERM on permission failure.
> > > > + *
> > > > + * Called (set) from fs/proc/base.c::proc_contid_write().
> > > > + */
> > > > +int audit_set_contid(struct task_struct *task, u64 contid)
> > > > +{
> > > > + u64 oldcontid;
> > > > + int rc = 0;
> > > > + struct audit_buffer *ab;
> > > > + uid_t uid;
> > > > + struct tty_struct *tty;
> > > > + char comm[sizeof(current->comm)];
> > > > +
> > > > + /* Can't set if audit disabled */
> > > > + if (!task->audit)
> > > > + return -ENOPROTOOPT;
> > > > + oldcontid = audit_get_contid(task);
> > > > + /* Don't allow the audit containerid to be unset */
> > > > + if (!cid_valid(contid))
> > > > + rc = -EINVAL;
> > > > + /* if we don't have caps, reject */
> > > > + else if (!capable(CAP_AUDIT_CONTROL))
> > > > + rc = -EPERM;
> > > > + /* if task has children or is not single-threaded, deny */
> > > > + else if (!list_empty(&task->children))
> > > > + rc = -EBUSY;
> > >
> > > Is this safe without holding tasklist_lock? I worry we might be
> > > vulnerable to a race with fork().
> > >
> > > > + else if (!(thread_group_leader(task) && thread_group_empty(task)))
> > > > + rc = -EALREADY;
> > >
> > > Similar concern here as well, although related to threads.
> >
> > I think you are correct here and tasklist_lock should cover both. Do we
> > also want rcu_read_lock() immediately preceeding it?
>
> You'll need to take a closer look and determine the locking scheme. I
> simply took a quick look while reviewing this patch to see what of the
> existing locks, if any, would be most applicable here; tasklist_lock
> seemed like a good starting point.
>
> It looks like tasklist_lock is defined as a rwlock_t so I'm not sure
> it would make sense to use it with a RCU protected structure
> (typically it's RCU+spinlock), but maybe that is the case with a
> task_struct, you'll need to check.
All I need is a read rather than write tasklist_lock since I'm not
changing any inter-task relationships, which makes it possible to nest
it inside or outside the task_lock(). I don't think I need the RCU
lock.
> > > > + /* it is already set, and not inherited from the parent, reject */
> > > > + else if (cid_valid(oldcontid) && !task->audit->inherited)
> > > > + rc = -EEXIST;
> > >
> > > Maybe I'm missing something, but why do we care about preventing
> > > reassigning the audit container ID in this case? The task is single
> > > threaded and has no descendants at this point so it should be safe,
> > > yes? So long as the task changing the audit container ID has
> > > capable(CAP_AUDIT_CONTOL) it shouldn't matter, right?
> >
> > Because we hammered out this idea 6 months ago in the design phase and I
> > thought we all firmly agreed that the audit container identifier could
> > only be set once. Has any significant discussion happenned since then
> > to change that wisdom? I just wonder why this is coming up now.
>
> Implementation, and time, can change how one looks at an earlier
> design. I believe this is why most well reasoned specifications have
> a reference design.
>
> Remind me why the design had the restriction of write once for the
> audit container ID? At this point given the CAP_AUDIT_CONTROL and the
> single-thread, no-children restrictions I'm not sure what harm there
> is in allowing the value to be written multiple times (so long as the
> changes are audited of course).
Looking back through the conversations, I think you may be right that we
no longer need it, but it is easy to re-add if we find it necessary.
> > > Related, I'm questioning if we would ever care if the audit container
> > > ID was inherited or not?
> >
> > We do since that is the only way we can tell if the value has been set
> > once already or inherited unless we check if the parent's audit
> > container identifier is identical (which tells us it was inherited).
>
> Tied to the above question. If we don't care about multiple changes,
> given the other constraints, we probably don't need the inherited
> flag.
Agreed.
> paul moore
- RGB
--
Richard Guy Briggs <rgb@redhat.com>
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
next prev parent reply other threads:[~2018-07-30 18:50 UTC|newest]
Thread overview: 51+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-06-06 16:58 [RFC PATCH ghak90 (was ghak32) V3 00/10] audit: implement container identifier Richard Guy Briggs
2018-06-06 16:58 ` [RFC PATCH ghak90 (was ghak32) V3 01/10] audit: add container id Richard Guy Briggs
2018-06-06 17:56 ` Steve Grubb
2018-06-06 20:26 ` Richard Guy Briggs
2018-07-20 22:13 ` Paul Moore
2018-07-24 19:06 ` Richard Guy Briggs
2018-07-24 21:54 ` Paul Moore
2018-07-30 18:47 ` Richard Guy Briggs [this message]
2018-06-06 16:58 ` [RFC PATCH ghak90 (was ghak32) V3 02/10] audit: log container info of syscalls Richard Guy Briggs
2018-06-06 17:58 ` Steve Grubb
2018-07-20 22:13 ` Paul Moore
2018-07-21 20:29 ` Richard Guy Briggs
2018-07-22 13:32 ` Steve Grubb
2018-07-22 20:55 ` Richard Guy Briggs
2018-07-22 21:03 ` Richard Guy Briggs
2018-07-23 13:19 ` Steve Grubb
2018-07-23 15:11 ` Richard Guy Briggs
2018-07-23 16:48 ` Steve Grubb
2018-07-23 18:31 ` Paul Moore
2018-07-26 0:51 ` Richard Guy Briggs
2018-07-31 20:07 ` Richard Guy Briggs
2018-07-23 13:16 ` Paul Moore
2018-06-06 16:58 ` [RFC PATCH ghak90 (was ghak32) V3 03/10] audit: add containerid support for ptrace and signals Richard Guy Briggs
2018-07-20 22:13 ` Paul Moore
2018-06-06 16:58 ` [RFC PATCH ghak90 (was ghak32) V3 04/10] audit: add support for non-syscall auxiliary records Richard Guy Briggs
2018-07-20 22:14 ` Paul Moore
2018-07-24 19:37 ` Richard Guy Briggs
2018-07-24 21:57 ` Paul Moore
2018-07-26 14:30 ` Richard Guy Briggs
2018-06-06 16:58 ` [RFC PATCH ghak90 (was ghak32) V3 05/10] audit: add containerid support for tty_audit Richard Guy Briggs
2018-07-20 22:14 ` Paul Moore
2018-07-24 14:07 ` Richard Guy Briggs
2018-07-24 20:36 ` Paul Moore
2018-06-06 16:58 ` [RFC PATCH ghak90 (was ghak32) V3 06/10] audit: add containerid filtering Richard Guy Briggs
2018-07-20 22:14 ` Paul Moore
2018-06-06 16:58 ` [RFC PATCH ghak90 (was ghak32) V3 07/10] audit: add support for containerid to network namespaces Richard Guy Briggs
2018-07-20 22:14 ` Paul Moore
2018-07-24 14:03 ` Richard Guy Briggs
2018-07-24 20:33 ` Paul Moore
2018-07-26 13:33 ` Richard Guy Briggs
2018-06-06 16:58 ` [RFC PATCH ghak90 (was ghak32) V3 08/10] audit: NETFILTER_PKT: record each container ID associated with a netNS Richard Guy Briggs
2018-07-20 22:15 ` Paul Moore
2018-07-24 19:48 ` Steve Grubb
2018-07-24 20:22 ` Paul Moore
2018-07-24 20:55 ` Richard Guy Briggs
2018-07-21 15:32 ` Laura Garcia
2018-06-06 16:58 ` [RFC PATCH ghak90 (was ghak32) V3 09/10] debug audit: read container ID of a process Richard Guy Briggs
2018-07-20 22:15 ` Paul Moore
2018-07-21 19:21 ` Richard Guy Briggs
2018-06-06 16:58 ` [RFC PATCH ghak90 (was ghak32) V3 10/10] rfkill: fix spelling mistake contidion to condition Richard Guy Briggs
2018-07-18 20:56 ` 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=20180730184731.aycnmknlew4vhnqe@madcap2.tricolour.ca \
--to=rgb@redhat.com \
--cc=carlos@redhat.com \
--cc=cgroups@vger.kernel.org \
--cc=containers@lists.linux-foundation.org \
--cc=dhowells@redhat.com \
--cc=ebiederm@xmission.com \
--cc=eparis@parisplace.org \
--cc=jlayton@redhat.com \
--cc=linux-api@vger.kernel.org \
--cc=linux-audit@redhat.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=luto@kernel.org \
--cc=netdev@vger.kernel.org \
--cc=paul@paul-moore.com \
--cc=serge@hallyn.com \
--cc=simo@redhat.com \
--cc=viro@zeniv.linux.org.uk \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).