RE: [PATCH v3 3/5] LSM: Security module checking for side-channel dangers

[Schaufler, Casey]

> -----Original Message-----
> From: Jann Horn [mailto:jannh@xxxxxxxxxx]
> Sent: Tuesday, August 21, 2018 6:01 PM
> To: Schaufler, Casey <casey.schaufler@xxxxxxxxx>
> Cc: Kernel Hardening <kernel-hardening@xxxxxxxxxxxxxxxxxx>; kernel list
> <linux-kernel@xxxxxxxxxxxxxxx>; linux-security-module <linux-security-
> module@xxxxxxxxxxxxxxx>; selinux@xxxxxxxxxxxxx; Hansen, Dave
> <dave.hansen@xxxxxxxxx>; Dock, Deneen T <deneen.t.dock@xxxxxxxxx>;
> kristen@xxxxxxxxxxxxxxx; Arjan van de Ven <arjan@xxxxxxxxxxxxxxx>
> Subject: Re: [PATCH v3 3/5] LSM: Security module checking for side-channel
> dangers
> 
> On Wed, Aug 22, 2018 at 1:44 AM Schaufler, Casey
> <casey.schaufler@xxxxxxxxx> wrote:
> >
> > > -----Original Message-----
> > > From: Jann Horn [mailto:jannh@xxxxxxxxxx]
> > > Sent: Tuesday, August 21, 2018 10:24 AM
> > > To: Schaufler, Casey <casey.schaufler@xxxxxxxxx>
> > > Cc: Kernel Hardening <kernel-hardening@xxxxxxxxxxxxxxxxxx>; kernel list
> > > <linux-kernel@xxxxxxxxxxxxxxx>; linux-security-module <linux-security-
> > > module@xxxxxxxxxxxxxxx>; selinux@xxxxxxxxxxxxx; Hansen, Dave
> > > <dave.hansen@xxxxxxxxx>; Dock, Deneen T <deneen.t.dock@xxxxxxxxx>;
> > > kristen@xxxxxxxxxxxxxxx; Arjan van de Ven <arjan@xxxxxxxxxxxxxxx>
> > > Subject: Re: [PATCH v3 3/5] LSM: Security module checking for side-channel
> > > dangers
> > >
> > > On Tue, Aug 21, 2018 at 2:05 AM Casey Schaufler
> > > <casey.schaufler@xxxxxxxxx> wrote:
> > > >
> > > > The sidechannel LSM checks for cases where a side-channel
> > > > attack may be dangerous based on security attributes of tasks.
> > > > This includes:
> > > >         Effective UID of the tasks is different
> > > >         Capablity sets are different
> > > >         Tasks are in different namespaces
> > > > An option is also provided to assert that task are never
> > > > to be considered safe. This is high paranoia, and expensive
> > > > as well.
> > > >
> > > > Signed-off-by: Casey Schaufler <casey.schaufler@xxxxxxxxx>
> > > > ---
> > > [...]
> > > > diff --git a/security/sidechannel/Kconfig b/security/sidechannel/Kconfig
> > > > new file mode 100644
> > > > index 000000000000..af9396534128
> > > > --- /dev/null
> > > > +++ b/security/sidechannel/Kconfig
> > > [...]
> > > > +config SECURITY_SIDECHANNEL_CAPABILITIES
> > > > +       bool "Sidechannel check on capability sets"
> > > > +       depends on SECURITY_SIDECHANNEL
> > > > +       default n
> > > > +       help
> > > > +         Assume that tasks with different sets of privilege may be
> > > > +         subject to side-channel attacks. Potential interactions
> > > > +         where the attacker lacks capabilities the attacked has
> > > > +         are blocked.
> > > > +
> > > > +          If you are unsure how to answer this question, answer N.
> > > > +
> > > > +config SECURITY_SIDECHANNEL_NAMESPACES
> > > > +       bool "Sidechannel check on namespaces"
> > > > +       depends on SECURITY_SIDECHANNEL
> > > > +       depends on NAMESPACES
> > > > +       default n
> > > > +       help
> > > > +         Assume that tasks in different namespaces may be
> > > > +         subject to side-channel attacks. User, PID and cgroup
> > > > +         namespaces are checked.
> > > > +
> > > > +          If you are unsure how to answer this question, answer N.
> > > [...]
> > > > diff --git a/security/sidechannel/sidechannel.c
> > > b/security/sidechannel/sidechannel.c
> > > > new file mode 100644
> > > > index 000000000000..4da7d6dafdc5
> > > > --- /dev/null
> > > > +++ b/security/sidechannel/sidechannel.c
> > > [...]
> > > > +/*
> > > > + * safe_by_capability - Are task and current sidechannel safe?
> > > > + * @p: task to check on
> > > > + *
> > > > + * Returns 0 if the tasks are sidechannel safe, -EACCES otherwise.
> > > > + */
> > > > +#ifdef CONFIG_SECURITY_SIDECHANNEL_CAPABILITIES
> > > > +static int safe_by_capability(struct task_struct *p)
> > > > +{
> > > > +       const struct cred *ccred = current_real_cred();
> > > > +       const struct cred *pcred = rcu_dereference_protected(p->real_cred,
> 1);
> > > > +
> > > > +       /*
> > > > +        * Capabilities checks. Considered safe if:
> > > > +        *      current has all the capabilities p does
> > > > +        */
> > > > +       if (ccred != pcred &&
> > > > +           !cap_issubset(pcred->cap_effective, ccred->cap_effective))
> > > > +               return -EACCES;
> > > > +       return 0;
> > > > +}
> > >
> > > On its own (without safe_by_namespace()), this check makes no sense, I
> > > think. You're performing a test on the namespaced capability sets
> > > without looking at which user namespaces they are relative to. Maybe
> > > either introduce a configuration dependency or add an extra namespace
> > > check here?
> >
> > If you don't have namespaces the check is correct. If you do, and use
> > safe_by_namespace() you're also correct. If you use namespaces and
> > care about side-channel attacks you should enable the namespace checks.
> 
> By "use namespaces", you mean "have CONFIG_USER_NS=y set in the kernel
> config", right?

That's correct.

> It doesn't matter much whether processes on your system are
> intentionally using namespaces;

Also correct.

> what matters is whether some random
> process can just use unshare(CLONE_NEWUSER) to increase its apparent
> capabilities and bypass the checks performed by this LSM.

Which puts it in a new user namespace, which gets caught by the
safe_by_namespace() check.

> My expectation is that unshare(CLONE_NEWUSER) should not increase the
> caller's abilities. Your patch seems to violate that expectation.

If you have CONFIG_USER_NS and not
CONFIG_SECURITY_SIDECHANNEL_NAMESPACES you do not increase the
caller's abilities from what you have without safesidechannel. If you have
CONFIG_SECURITY_SIDECHANNEL_NAMESPACES you have additional
restriction (assuming one considers setting the barrier a restriction) that
the tasks must be in the same namespace(s). As I said, if you care about
namespace implications you should configure the system accordingly.

> > I don't see real value in adding namespace checks in the capability checks
> > for the event where someone has said they don't want namespace checks.
> 
> Capabilities are meaningless if you don't consider the namespaces
> relative to which they are effective.

Agreed. But if CONFIG_NAMESPACES is off you are always in the same
namespace and if it is on you should use the sidechannel namespace check.

> Anyone can get CAP_SYS_ADMIN or
> whatever other capabilities they want, by design - just not relative
> to objects they don't own. Look:
> 
> $ grep ^Cap /proc/self/status
> CapInh: 0000000000000000
> CapPrm: 0000000000000000
> CapEff: 0000000000000000
> CapBnd: 0000003fffffffff
> CapAmb: 0000000000000000
> $ unshare -Ur grep ^Cap /proc/self/status
> CapInh: 0000000000000000
> CapPrm: 0000003fffffffff
> CapEff: 0000003fffffffff
> CapBnd: 0000003fffffffff
> CapAmb: 0000000000000000
> 
> Ta-daa! Full capability set.

Yes, but in a different namespace. Hence the namespace check.

What I hear you saying is that you don't want the capability check
to be independent of the namespace check. This conflicts with the
strong desire expressed to me when I started this that the configuration
should be flexible. I can beef up the description of the various options.
Would that address the issue?

> 
> > I got early feedback that configurability was considered important.
> > This is the correct behavior if you want namespace checks to be
> > separately configurable from capability checks. You could ask for
> > distinct configuration options for each kind of namespace, but, well, yuck.
> >
> > > > +static int safe_by_namespace(struct task_struct *p)
> > > > +{
> > > > +       struct cgroup_namespace *ccgn = NULL;
> > > > +       struct cgroup_namespace *pcgn = NULL;
> > > > +       const struct cred *ccred;
> > > > +       const struct cred *pcred;
> > > > +
> > > > +       /*
> > > > +        * Namespace checks. Considered safe if:
> > > > +        *      cgroup namespace is the same
> > > > +        *      User namespace is the same
> > > > +        *      PID namespace is the same
> > > > +        */
> > > > +       if (current->nsproxy)
> > > > +               ccgn = current->nsproxy->cgroup_ns;
> > > > +       if (p->nsproxy)
> > > > +               pcgn = p->nsproxy->cgroup_ns;
> > > > +       if (ccgn != pcgn)
> > > > +               return -EACCES;
> > > > +
> > > > +       ccred = current_real_cred();
> > > > +       pcred = rcu_dereference_protected(p->real_cred, 1);
> > > > +
> > > > +       if (ccred->user_ns != pcred->user_ns)
> > > > +               return -EACCES;
> > > > +       if (task_active_pid_ns(current) != task_active_pid_ns(p))
> > > > +               return -EACCES;
> > > > +       return 0;
> > > > +}