Linux-Fsdevel Archive on lore.kernel.org
 help / color / Atom feed
From: Richard Guy Briggs <rgb@redhat.com>
To: Paul Moore <paul@paul-moore.com>
Cc: nhorman@tuxdriver.com, linux-api@vger.kernel.org,
	containers@lists.linux-foundation.org,
	LKML <linux-kernel@vger.kernel.org>,
	dhowells@redhat.com,
	Linux-Audit Mailing List <linux-audit@redhat.com>,
	netfilter-devel@vger.kernel.org, ebiederm@xmission.com,
	simo@redhat.com, netdev@vger.kernel.org,
	linux-fsdevel@vger.kernel.org, Eric Paris <eparis@parisplace.org>,
	mpatel@redhat.com, Serge Hallyn <serge@hallyn.com>
Subject: Re: [PATCH ghak90 V8 16/16] audit: add capcontid to set contid outside init_user_ns
Date: Thu, 12 Mar 2020 17:58:08 -0400
Message-ID: <20200312215808.bkezqotlhewconiy@madcap2.tricolour.ca> (raw)
In-Reply-To: <CAHC9VhT8RsFtmqD22p_NxJaqoAg+do9mX45Luw9fEkr+nQjvxg@mail.gmail.com>

On 2020-02-13 16:58, Paul Moore wrote:
> On Thu, Feb 6, 2020 at 7:52 AM Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2020-02-05 17:56, Paul Moore wrote:
> > > On Tue, Feb 4, 2020 at 7:39 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > > 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:
> > > > > >
> > > > > > Provide a mechanism similar to CAP_AUDIT_CONTROL to explicitly give a
> > > > > > process in a non-init user namespace the capability to set audit
> > > > > > container identifiers.
> > > > > >
> > > > > > Provide /proc/$PID/audit_capcontid interface to capcontid.
> > > > > > Valid values are: 1==enabled, 0==disabled
> > > > >
> > > > > It would be good to be more explicit about "enabled" and "disabled" in
> > > > > the commit description.  For example, which setting allows the target
> > > > > task to set audit container IDs of it's children processes?
> > > >
> > > > Ok...
> > > >
> > > > > > Report this action in message type AUDIT_SET_CAPCONTID 1022 with fields
> > > > > > opid= capcontid= old-capcontid=
> > > > > >
> > > > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > > > > ---
> > > > > >  fs/proc/base.c             | 55 ++++++++++++++++++++++++++++++++++++++++++++++
> > > > > >  include/linux/audit.h      | 14 ++++++++++++
> > > > > >  include/uapi/linux/audit.h |  1 +
> > > > > >  kernel/audit.c             | 35 +++++++++++++++++++++++++++++
> > > > > >  4 files changed, 105 insertions(+)
> > >
> > > ...
> > >
> > > > > > diff --git a/kernel/audit.c b/kernel/audit.c
> > > > > > index 1287f0b63757..1c22dd084ae8 100644
> > > > > > --- a/kernel/audit.c
> > > > > > +++ b/kernel/audit.c
> > > > > > @@ -2698,6 +2698,41 @@ static bool audit_contid_isowner(struct task_struct *tsk)
> > > > > >         return false;
> > > > > >  }
> > > > > >
> > > > > > +int audit_set_capcontid(struct task_struct *task, u32 enable)
> > > > > > +{
> > > > > > +       u32 oldcapcontid;
> > > > > > +       int rc = 0;
> > > > > > +       struct audit_buffer *ab;
> > > > > > +
> > > > > > +       if (!task->audit)
> > > > > > +               return -ENOPROTOOPT;
> > > > > > +       oldcapcontid = audit_get_capcontid(task);
> > > > > > +       /* if task is not descendant, block */
> > > > > > +       if (task == current)
> > > > > > +               rc = -EBADSLT;
> > > > > > +       else if (!task_is_descendant(current, task))
> > > > > > +               rc = -EXDEV;
> > > > >
> > > > > See my previous comments about error code sanity.
> > > >
> > > > I'll go with EXDEV.
> > > >
> > > > > > +       else if (current_user_ns() == &init_user_ns) {
> > > > > > +               if (!capable(CAP_AUDIT_CONTROL) && !audit_get_capcontid(current))
> > > > > > +                       rc = -EPERM;
> > > > >
> > > > > I think we just want to use ns_capable() in the context of the current
> > > > > userns to check CAP_AUDIT_CONTROL, yes?  Something like this ...
> > > >
> > > > I thought we had firmly established in previous discussion that
> > > > CAP_AUDIT_CONTROL in anything other than init_user_ns was completely irrelevant
> > > > and untrustable.
> > >
> > > In the case of a container with multiple users, and multiple
> > > applications, one being a nested orchestrator, it seems relevant to
> > > allow that container to control which of it's processes are able to
> > > exercise CAP_AUDIT_CONTROL.  Granted, we still want to control it
> > > within the overall host, e.g. the container in question must be
> > > allowed to run a nested orchestrator, but allowing the container
> > > itself to provide it's own granularity seems like the right thing to
> > > do.
> >
> > Looking back to discussion on the v6 patch 2/10 (2019-05-30 15:29 Paul
> > Moore[1], 2019-07-08 14:05 RGB[2]) , it occurs to me that the
> > ns_capable(CAP_AUDIT_CONTROL) application was dangerous since there was
> > no parental accountability in storage or reporting.  Now that is in
> > place, it does seem a bit more reasonable to allow it, but I'm still not
> > clear on why we would want both mechanisms now.  I don't understand what
> > the last line in that email meant: "We would probably still want a
> > ns_capable(CAP_AUDIT_CONTROL) restriction in this case."  Allow
> > ns_capable(CAP_AUDIT_CONTROL) to govern these actions, or restrict
> > ns_capable(CAP_AUDIT_CONTROL) from being used to govern these actions?
> >
> > If an unprivileged user has been given capcontid to be able run their
> > own container orchestrator/engine and spawns a user namespace with
> > CAP_AUDIT_CONTROL, what matters is capcontid, and not CAP_AUDIT_CONTROL.
> > I could see needing CAP_AUDIT_CONTROL *in addition* to capcontid to give
> > it finer grained control, but since capcontid would have to be given to
> > each process explicitly anways, I don't see the point.
> >
> > If that unprivileged user had not been given capcontid,
> > giving itself or one of its descendants CAP_AUDIT_CONTROL should not let
> > it jump into the game all of a sudden unless the now chained audit
> > container identifiers are deemed accountable enough.  And then now we
> > need those hard limits on container depth and network namespace
> > container membership.
> 
> Perhaps I'm not correctly understanding what you are trying to do with
> this patchset, but my current understanding is that you are trying to
> use capcontid to control which child audit container IDs (ACIDs) are
> allowed to manage their own ACIDs.  Further, I believe that the
> capcontid setting operates at a per-ACID level, meaning there is no
> provision for the associated container to further restrict that
> ability, i.e. no access control granularity below the ACID level.  My
> thinking is that ns_capable(CAP_AUDIT_CONTROL) could be used within an
> ACID to increase the granularity of the access controls so that only
> privileged processes running inside the ACID would be able to manage
> the ACIDs.  Does that make sense?

The capcontid is not inherited like the contid (or contobj) in
audit_alloc(), so it stops at that process that was granted capcontid.
That process that was granted capcontid can then explicitly further
grant capcontid to any of its children should it deem necessary.

Since it is a boolean, it defaults to unset in init_struct_audit which
isn't relevant anyways since that is in the initial user namespace.
It isn't set in audit_alloc() and would default to false.
I can set them explicitly both to false to be certain if that makes
things clearer and more certain.

I still believe ns_capable() is irrelevant here.

> 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 index

Thread overview: 99+ 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-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
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 [this message]

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=20200312215808.bkezqotlhewconiy@madcap2.tricolour.ca \
    --to=rgb@redhat.com \
    --cc=containers@lists.linux-foundation.org \
    --cc=dhowells@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=paul@paul-moore.com \
    --cc=serge@hallyn.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

Linux-Fsdevel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-fsdevel/0 linux-fsdevel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-fsdevel linux-fsdevel/ https://lore.kernel.org/linux-fsdevel \
		linux-fsdevel@vger.kernel.org
	public-inbox-index linux-fsdevel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-fsdevel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git