linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Richard Guy Briggs <rgb@redhat.com>
To: Paul Moore <paul@paul-moore.com>
Cc: containers@lists.linux-foundation.org, linux-api@vger.kernel.org,
	Linux-Audit Mailing List <linux-audit@redhat.com>,
	linux-fsdevel@vger.kernel.org,
	LKML <linux-kernel@vger.kernel.org>,
	netdev@vger.kernel.org, netfilter-devel@vger.kernel.org,
	sgrubb@redhat.com, omosnace@redhat.com, dhowells@redhat.com,
	simo@redhat.com, Eric Paris <eparis@parisplace.org>,
	Serge Hallyn <serge@hallyn.com>,
	ebiederm@xmission.com, nhorman@tuxdriver.com,
	Dan Walsh <dwalsh@redhat.com>,
	mpatel@redhat.com
Subject: Re: [PATCH ghak90 V8 12/16] audit: contid check descendancy and nesting
Date: Thu, 23 Jan 2020 16:02:40 -0500	[thread overview]
Message-ID: <20200123210240.sq64tptjm3ds7xss@madcap2.tricolour.ca> (raw)
In-Reply-To: <CAHC9VhT1+mx_tVzyXD=UBqagqYgAFjZ=X1A6oBiMvjVCn8=V-w@mail.gmail.com>

On 2020-01-22 16:29, Paul Moore wrote:
> On Tue, Dec 31, 2019 at 2:51 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> >
> > Require the target task to be a descendant of the container
> > orchestrator/engine.
> >
> > You would only change the audit container ID from one set or inherited
> > value to another if you were nesting containers.
> >
> > If changing the contid, the container orchestrator/engine must be a
> > descendant and not same orchestrator as the one that set it so it is not
> > possible to change the contid of another orchestrator's container.
> >
> > Since the task_is_descendant() function is used in YAMA and in audit,
> > remove the duplication and pull the function into kernel/core/sched.c
> >
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > ---
> >  include/linux/sched.h    |  3 +++
> >  kernel/audit.c           | 44 ++++++++++++++++++++++++++++++++++++--------
> >  kernel/sched/core.c      | 33 +++++++++++++++++++++++++++++++++
> >  security/yama/yama_lsm.c | 33 ---------------------------------
> >  4 files changed, 72 insertions(+), 41 deletions(-)
> 
> ...
> 
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index f7a8d3288ca0..ef8e07524c46 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -2603,22 +2610,43 @@ int audit_set_contid(struct task_struct *task, u64 contid)
> >         oldcontid = audit_get_contid(task);
> >         read_lock(&tasklist_lock);
> >         /* Don't allow the contid to be unset */
> > -       if (!audit_contid_valid(contid))
> > +       if (!audit_contid_valid(contid)) {
> >                 rc = -EINVAL;
> > +               goto unlock;
> > +       }
> >         /* Don't allow the contid to be set to the same value again */
> > -       else if (contid == oldcontid) {
> > +       if (contid == oldcontid) {
> >                 rc = -EADDRINUSE;
> > +               goto unlock;
> > +       }
> >         /* if we don't have caps, reject */
> > -       else if (!capable(CAP_AUDIT_CONTROL))
> > +       if (!capable(CAP_AUDIT_CONTROL)) {
> >                 rc = -EPERM;
> > -       /* if task has children or is not single-threaded, deny */
> > -       else if (!list_empty(&task->children))
> > +               goto unlock;
> > +       }
> > +       /* if task has children, deny */
> > +       if (!list_empty(&task->children)) {
> >                 rc = -EBUSY;
> > -       else if (!(thread_group_leader(task) && thread_group_empty(task)))
> > +               goto unlock;
> > +       }
> > +       /* if task is not single-threaded, deny */
> > +       if (!(thread_group_leader(task) && thread_group_empty(task))) {
> >                 rc = -EALREADY;
> > -       /* if contid is already set, deny */
> > -       else if (audit_contid_set(task))
> > +               goto unlock;
> > +       }
> 
> It seems like the if/else-if conversion above should be part of an
> earlier patchset.

I had considered that, but it wasn't obvious where that conversion
should happen since it wasn't necessary earlier and is now.  I can move
it earlier if you feel strongly about it.

> > +       /* if task is not descendant, block */
> > +       if (task == current) {
> > +               rc = -EBADSLT;
> > +               goto unlock;
> > +       }
> > +       if (!task_is_descendant(current, task)) {
> > +               rc = -EXDEV;
> > +               goto unlock;
> > +       }
> 
> I understand you are trying to provide a unique error code for each
> failure case, but this is getting silly.  Let's group the descendent
> checks under the same error code.

Ok.  I was trying to provide more information for debugging for me and
for users.

> > +       /* only allow contid setting again if nesting */
> > +       if (audit_contid_set(task) && audit_contid_isowner(task))
> >                 rc = -ECHILD;
> 
> Should that be "!audit_contid_isowner()"?

No.  If the contid is already set on this task and if it is the same
orchestrator that already owns this one, then block it since the same
orchestrator is not allowed to set it again.  Another orchestrator that
has been shown by previous tests to be a descendant of the orchestrator
that already owns this one would be permitted.

Now that I say this explicitly, it appears I need another test to check:

	/* only allow contid setting again if nesting */
	if (audit_contid_set(task) && ( audit_contid_isowner(task) || !task_is_descendant(_audit_contobj(task)->owner, current) ))
		rc = -ECHILD;

So we're back to audit_contobj_owner() like in the previous patchset
that would make this cleaner.

> 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


  reply	other threads:[~2020-01-23 21:03 UTC|newest]

Thread overview: 102+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-31 19:48 [PATCH ghak90 V8 00/16] audit: implement container identifier Richard Guy Briggs
2019-12-31 19:48 ` [PATCH ghak90 V8 01/16] audit: collect audit task parameters Richard Guy Briggs
2019-12-31 19:48 ` [PATCH ghak90 V8 02/16] audit: add container id Richard Guy Briggs
2020-01-22 21:28   ` Paul Moore
2020-01-30 17:53     ` Richard Guy Briggs
2019-12-31 19:48 ` [PATCH ghak90 V8 03/16] audit: read container ID of a process Richard Guy Briggs
2019-12-31 19:48 ` [PATCH ghak90 V8 04/16] audit: convert to contid list to check for orch/engine ownership Richard Guy Briggs
2020-01-22 21:28   ` Paul Moore
2020-02-04 22:51     ` Richard Guy Briggs
2020-02-05 22:40       ` Paul Moore
2019-12-31 19:48 ` [PATCH ghak90 V8 05/16] audit: log drop of contid on exit of last task Richard Guy Briggs
2020-01-22 21:28   ` Paul Moore
2020-02-04 23:02     ` Richard Guy Briggs
2019-12-31 19:48 ` [PATCH ghak90 V8 06/16] audit: log container info of syscalls Richard Guy Briggs
2019-12-31 19:48 ` [PATCH ghak90 V8 07/16] audit: add contid support for signalling the audit daemon Richard Guy Briggs
2020-01-22 21:28   ` Paul Moore
2020-01-23 16:29     ` Richard Guy Briggs
2020-01-23 17:09       ` Paul Moore
2020-01-23 20:04         ` Richard Guy Briggs
2020-01-23 21:35           ` Paul Moore
2020-02-04 23:14             ` Richard Guy Briggs
2020-02-05 22:50               ` Paul Moore
2020-02-12 22:38                 ` Steve Grubb
2020-02-13  0:09                   ` Paul Moore
2020-02-13 21:44                     ` Paul Moore
2020-03-12 19:30                       ` Richard Guy Briggs
2020-03-13 16:29                         ` Paul Moore
2020-03-13 18:59                           ` Richard Guy Briggs
2020-03-18 20:56                             ` Paul Moore
2020-03-18 21:26                               ` Richard Guy Briggs
2020-03-18 21:42                                 ` Paul Moore
2020-03-18 21:55                                   ` Richard Guy Briggs
2020-03-18 22:06                                     ` Paul Moore
2020-03-19 22:02                                       ` Richard Guy Briggs
2020-03-24  0:16                                         ` Paul Moore
2020-03-24 21:01                                           ` Richard Guy Briggs
2020-03-29  3:11                                             ` Paul Moore
2020-03-30 13:47                                               ` Richard Guy Briggs
2020-03-30 14:26                                                 ` Paul Moore
2020-03-30 16:21                                                   ` Richard Guy Briggs
2020-03-30 17:34                                                     ` Paul Moore
2020-03-30 17:49                                                       ` Richard Guy Briggs
2020-03-30 19:55                                                         ` Paul Moore
2020-04-16 20:33                                                           ` Eric W. Biederman
2020-04-16 21:53                                                             ` Paul Moore
2020-04-17 22:23                                                               ` Eric W. Biederman
2020-04-22 17:24                                                                 ` Paul Moore
2020-06-08 18:03                                                                   ` Richard Guy Briggs
2020-06-17 21:33                                                                     ` Paul Moore
2020-06-19 15:22                                                                 ` Richard Guy Briggs
2020-03-12 20:27                     ` Richard Guy Briggs
2020-03-13 16:42                       ` Paul Moore
2020-03-13 16:45                         ` Steve Grubb
2020-03-13 16:49                           ` Paul Moore
2020-03-13 19:23                         ` Richard Guy Briggs
2020-03-18 21:01                           ` Paul Moore
2020-03-18 21:41                             ` Richard Guy Briggs
2020-03-18 21:47                               ` Paul Moore
2020-03-19 21:47                                 ` Richard Guy Briggs
2020-03-20 21:56                                   ` Paul Moore
2020-03-25 12:29                                     ` Richard Guy Briggs
2020-03-29  3:17                                       ` Paul Moore
2020-03-30 15:23                                         ` Richard Guy Briggs
2019-12-31 19:48 ` [PATCH ghak90 V8 08/16] audit: add support for non-syscall auxiliary records Richard Guy Briggs
2019-12-31 19:48 ` [PATCH ghak90 V8 09/16] audit: add containerid support for user records Richard Guy Briggs
2019-12-31 19:48 ` [PATCH ghak90 V8 10/16] audit: add containerid filtering Richard Guy Briggs
2019-12-31 19:48 ` [PATCH ghak90 V8 11/16] audit: add support for containerid to network namespaces Richard Guy Briggs
2020-01-22 21:28   ` Paul Moore
2020-02-04 23:42     ` Richard Guy Briggs
2020-02-05 22:51       ` Paul Moore
2019-12-31 19:48 ` [PATCH ghak90 V8 12/16] audit: contid check descendancy and nesting Richard Guy Briggs
2020-01-22 21:29   ` Paul Moore
2020-01-23 21:02     ` Richard Guy Briggs [this message]
2020-01-23 21:47       ` Paul Moore
2019-12-31 19:48 ` [PATCH ghak90 V8 13/16] audit: track container nesting Richard Guy Briggs
2020-01-22 21:29   ` Paul Moore
2020-01-30 19:27     ` Richard Guy Briggs
2020-02-05 23:05       ` Paul Moore
2020-02-05 23:50         ` Richard Guy Briggs
2020-02-13 21:49           ` Paul Moore
2020-03-12 20:51             ` Richard Guy Briggs
2020-03-13 16:47               ` Paul Moore
2020-03-14 22:42                 ` Richard Guy Briggs
2020-03-17 18:28                   ` Richard Guy Briggs
2020-03-18 21:08                   ` Paul Moore
2020-01-31 14:50     ` Steve Grubb
2020-02-04 13:19       ` Richard Guy Briggs
2020-02-04 15:47         ` Steve Grubb
2020-02-04 15:52           ` Paul Moore
2020-02-04 18:12             ` Steve Grubb
2020-02-05 22:57               ` Paul Moore
2019-12-31 19:48 ` [PATCH ghak90 V8 14/16] audit: check contid depth and add limit config param Richard Guy Briggs
2020-01-22 21:29   ` Paul Moore
2019-12-31 19:48 ` [PATCH ghak90 V8 15/16] audit: check contid count per netns and add config param limit Richard Guy Briggs
2020-01-22 21:29   ` Paul Moore
2019-12-31 19:48 ` [PATCH ghak90 V8 16/16] audit: add capcontid to set contid outside init_user_ns Richard Guy Briggs
2020-01-22 21:29   ` Paul Moore
2020-02-05  0:39     ` Richard Guy Briggs
2020-02-05 22:56       ` Paul Moore
2020-02-06 12:51         ` Richard Guy Briggs
2020-02-13 21:58           ` Paul Moore
2020-03-12 21:58             ` Richard Guy Briggs

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=20200123210240.sq64tptjm3ds7xss@madcap2.tricolour.ca \
    --to=rgb@redhat.com \
    --cc=containers@lists.linux-foundation.org \
    --cc=dhowells@redhat.com \
    --cc=dwalsh@redhat.com \
    --cc=ebiederm@xmission.com \
    --cc=eparis@parisplace.org \
    --cc=linux-api@vger.kernel.org \
    --cc=linux-audit@redhat.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mpatel@redhat.com \
    --cc=netdev@vger.kernel.org \
    --cc=netfilter-devel@vger.kernel.org \
    --cc=nhorman@tuxdriver.com \
    --cc=omosnace@redhat.com \
    --cc=paul@paul-moore.com \
    --cc=serge@hallyn.com \
    --cc=sgrubb@redhat.com \
    --cc=simo@redhat.com \
    /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).