RE: [PATCH v5 5/5] sidechannel: Linux Security Module for sidechannel
> -Original Message- > From: James Morris [mailto:jmor...@namei.org] > Sent: Friday, September 28, 2018 9:33 AM > To: Jann Horn > Cc: Schaufler, Casey ; Casey Schaufler > ; kris...@linux.intel.com; Kernel Hardening > ; Dock, Deneen T > ; kernel list ; > Hansen, Dave ; linux-security-module mod...@vger.kernel.org>; selinux@tycho.nsa.gov; Arjan van de Ven > > Subject: Re: [PATCH v5 5/5] sidechannel: Linux Security Module for sidechannel > > On Fri, 28 Sep 2018, Jann Horn wrote: > > > > so with this hard-coded logic, you are saying this case is > > > 'safe' in a sidechannel context. > > > > > > Which hints at the deeper issue that containers are a userland > > > abstraction. Protection of containers needs to be defined by userland > > > policy. > > > > Or just compare mount namespaces additionally/instead. I think that > > containers will always use those, because AFAIK nobody uses chroot() > > for containers, given that the kernel makes absolutely no security > > guarantees about chroot(). > > We can't define this in the kernel. It has no concept of containers. > > People utilize some combination of namespaces and cgroups and call them > containers, There is an amazing variety of things called containers out there. I cite them as a use case, not a requirement. > but we can't make assumptions from the kernel on what any of > this means from a security point of view, and hard-code kernel policy > based on those assumptions. We can assume that namespaces are being used as a separation mechanism. That makes processes in different namespaces potentially vulnerable to side-channel attacks. That's true regardless of whether or not someone is using namespaces to implement containers. > This is violating the principal of separating mechanism and policy, and > also imposing semantics across the kernel/user boundary. The latter > creates an ABI which we can then never break. The effects of the sidechannel security module are not API visible. The potential impact is on performance. This implementation of PTRACE_MODE_SCHED does not change what happens, but may affect when it happens. It is intended to aid in optimizing the use of expensive anti-side-channel countermeasures. ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
RE: [PATCH v5 5/5] sidechannel: Linux Security Module for sidechannel
> -Original Message- > From: James Morris [mailto:jmor...@namei.org] > Sent: Thursday, September 27, 2018 3:47 PM > To: Casey Schaufler > Cc: Schaufler, Casey ; kris...@linux.intel.com; > kernel-harden...@lists.openwall.com; Dock, Deneen T > ; linux-ker...@vger.kernel.org; Hansen, Dave > ; linux-security-mod...@vger.kernel.org; > selinux@tycho.nsa.gov; ar...@linux.intel.com > Subject: Re: [PATCH v5 5/5] sidechannel: Linux Security Module for sidechannel > > On Thu, 27 Sep 2018, Casey Schaufler wrote: > > > On 9/27/2018 2:45 PM, James Morris wrote: > > > On Wed, 26 Sep 2018, Casey Schaufler wrote: > > > > > >> +/* > > >> + * 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; > > >> +if (current->cred->user_ns != p->cred->user_ns) > > >> +return -EACCES; > > >> +if (task_active_pid_ns(current) != task_active_pid_ns(p)) > > >> +return -EACCES; > > >> +return 0; > > > I really don't like the idea of hard-coding namespace security semantics > > > in an LSM. Also, I'm not sure if these semantics make any sense. > > > > Checks on namespaces where explicitly requested. > > By whom and what is the rationale? The rationale is to protect containers. Since those closest thing there is to a definition of containers is "uses namespaces" that becomes the focus. Separating them out does not make too much sense as I would expect someone concerned with one to be concerned with all. ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
RE: [PATCH v5 3/5] SELinux: Prepare for PTRACE_MODE_SCHED
> -Original Message- > From: Stephen Smalley [mailto:s...@tycho.nsa.gov] > Sent: Thursday, September 27, 2018 8:50 AM > To: Schaufler, Casey ; kernel- > harden...@lists.openwall.com; linux-ker...@vger.kernel.org; linux-security- > mod...@vger.kernel.org; selinux@tycho.nsa.gov; Hansen, Dave > ; Dock, Deneen T ; > kris...@linux.intel.com; ar...@linux.intel.com; Paul Moore moore.com> > Subject: Re: [PATCH v5 3/5] SELinux: Prepare for PTRACE_MODE_SCHED > > On 09/26/2018 04:34 PM, Casey Schaufler wrote: > > From: Casey Schaufler > > > > A ptrace access check with mode PTRACE_MODE_SCHED gets called > > from process switching code. This precludes the use of audit or avc, > > as the locking is incompatible. The only available check that > > can be made without using avc is a comparison of the secids. > > This is not very satisfactory as it will indicate possible > > vulnerabilies much too aggressively. > Can you document (in the patch description and/or in the inline > documentation in lsm_hooks.h) what locks can be safely used when this > hook is called with PTRACE_MODE_SCHED? rcu_read_lock() seemingly must > be safe since it is being called by task_sid() below. Are any other > locking primitives safe? Peter Zijlstra had this comment on locking in the SELinux ptrace path. avc_has_perm_noaudit() security_compute_av() read_lock(&state->ss->policy_rwlock); avc_insert() spin_lock_irqsave(); avc_denied() avc_update_node() spin_lock_irqsave(); under the scheduler's raw_spinlock_t, which are invalid lock nestings. I don't know that it would be impossible to address these issues, but as many people have noted over the years I am not now nor have ever been an expert on locking. > > Does the PTRACE_MODE_SCHED check have to occur while holding the > scheduler lock, or could it be performed before taking the lock? My understanding is that the lock is required. > Can you cite the commit or patch posting (e.g. from lore or patchwork) > that defines PTRACE_MODE_SCHED and its usage as part of the patch > description for context? Is this based on the v7 patchset, e.g. > https://lore.kernel.org/lkml/nycvar.yfh.7.76.1809251437340.15...@cbobk.fhf > r.pm/ Yes, that's the one. Sorry, I should have identified that. > > > > > Signed-off-by: Casey Schaufler > > --- > > security/selinux/hooks.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > index ad9a9b8e9979..160239791007 100644 > > --- a/security/selinux/hooks.c > > +++ b/security/selinux/hooks.c > > @@ -2267,6 +2267,8 @@ static int selinux_ptrace_access_check(struct > task_struct *child, > > u32 sid = current_sid(); > > u32 csid = task_sid(child); > > > > + if (mode & PTRACE_MODE_SCHED) > > + return sid == csid ? 0 : -EACCES; > IIUC, this logic is essentially the same as the uid-based check, > including the fact that even a "privileged" process is not given any > special handling since they always return false from ptrace_has_cap() > for PTRACE_MODE_SCHED. If they are ok with applying IBPB whenever uids > differ, then doing so whenever sids/contexts differ does not seem like > an onerous thing. > > > > if (mode & PTRACE_MODE_READ) > > return avc_has_perm(&selinux_state, > > sid, csid, SECCLASS_FILE, FILE__READ, > NULL); > > ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
RE: [PATCH v5 2/5] Smack: Prepare for PTRACE_MODE_SCHED
> -Original Message-
> From: Jann Horn [mailto:ja...@google.com]
> Sent: Wednesday, September 26, 2018 2:31 PM
> To: Schaufler, Casey
> Cc: Kernel Hardening ; kernel list
> ; linux-security-module mod...@vger.kernel.org>; selinux@tycho.nsa.gov; Hansen, Dave
> ; Dock, Deneen T ;
> kris...@linux.intel.com; Arjan van de Ven
> Subject: Re: [PATCH v5 2/5] Smack: Prepare for PTRACE_MODE_SCHED
>
> On Wed, Sep 26, 2018 at 10:35 PM Casey Schaufler
> wrote:
> > A ptrace access check with mode PTRACE_MODE_SCHED gets called
> > from process switching code. This precludes the use of audit,
> > as the locking is incompatible. Don't do audit in the PTRACE_MODE_SCHED
> > case.
> >
> > Signed-off-by: Casey Schaufler
> > ---
> > security/smack/smack_lsm.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> > index 340fc30ad85d..ffa95bcab599 100644
> > --- a/security/smack/smack_lsm.c
> > +++ b/security/smack/smack_lsm.c
> > @@ -422,7 +422,8 @@ static int smk_ptrace_rule_check(struct task_struct
> *tracer,
> > struct task_smack *tsp;
> > struct smack_known *tracer_known;
> >
> > - if ((mode & PTRACE_MODE_NOAUDIT) == 0) {
> > + if ((mode & PTRACE_MODE_NOAUDIT) == 0 &&
> > + (mode & PTRACE_MODE_SCHED) == 0) {
>
> If you ORed PTRACE_MODE_NOAUDIT into the flags when calling the
> security hook, you could drop this patch, right?
Yes. Since the PTRACE_MODE_NOAUDIT was in PTRACE_MODE_IBPB
in Jiri's previous patch set and not in PTRACE_MODE_SCHED in this one
I assumed that there was a good reason for it.
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
RE: [PATCH v5 4/5] Capability: Complete PTRACE_MODE_SCHED
> -Original Message- > From: Jann Horn [mailto:ja...@google.com] > Sent: Wednesday, September 26, 2018 2:26 PM > To: Schaufler, Casey > Cc: Kernel Hardening ; kernel list > ; linux-security-module mod...@vger.kernel.org>; selinux@tycho.nsa.gov; Hansen, Dave > ; Dock, Deneen T ; > kris...@linux.intel.com; Arjan van de Ven > Subject: Re: [PATCH v5 4/5] Capability: Complete PTRACE_MODE_SCHED > > On Wed, Sep 26, 2018 at 10:35 PM Casey Schaufler > wrote: > > Allow a complete ptrace access check with mode PTRACE_MODE_SCHED. > > Disable the inappropriate privilege check in the capability code > > that does incompatible locking. > > What's that locking you're talking about? ns_capable() eventually gets you to an audit call. The audit code is going to do the locking. Fortunately, the preceding cap_issubset() is the check that we really need here. > > > Signed-off-by: Casey Schaufler > > --- > > kernel/ptrace.c | 2 -- > > security/commoncap.c | 2 ++ > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/kernel/ptrace.c b/kernel/ptrace.c > > index 99cfddde6a55..0b6a9df51c3b 100644 > > --- a/kernel/ptrace.c > > +++ b/kernel/ptrace.c > > @@ -331,8 +331,6 @@ static int __ptrace_may_access(struct task_struct > *task, unsigned int mode) > > !ptrace_has_cap(mm->user_ns, mode))) > > return -EPERM; > > > > - if (mode & PTRACE_MODE_SCHED) > > - return 0; > > return security_ptrace_access_check(task, mode); > > } > > > > diff --git a/security/commoncap.c b/security/commoncap.c > > index 2e489d6a3ac8..e77457110d05 100644 > > --- a/security/commoncap.c > > +++ b/security/commoncap.c > > @@ -152,6 +152,8 @@ int cap_ptrace_access_check(struct task_struct > *child, unsigned int mode) > > if (cred->user_ns == child_cred->user_ns && > > cap_issubset(child_cred->cap_permitted, *caller_caps)) > > goto out; > > + if (mode & PTRACE_MODE_SCHED) > > + goto out; > > So for PTRACE_MODE_SCHED, this function always returns 0, right? That can't be right, can it? Determining that we have PTRACE_MODE_SCHED at this point should result in -EPERM. I mucked up on the logic flow. The next revision will fix this. > If that's intentional, perhaps you should instead just put "if (mode & > PTRACE_MODE_SCHED) return 0;" at the start of the function, to avoid > taking the RCU read lock in this case. > > > if (ns_capable(child_cred->user_ns, CAP_SYS_PTRACE)) > > goto out; > > ret = -EPERM; ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
RE: [PATCH v5 1/5] AppArmor: Prepare for PTRACE_MODE_SCHED
> -Original Message-
> From: Jann Horn [mailto:ja...@google.com]
> Sent: Wednesday, September 26, 2018 2:19 PM
> To: Schaufler, Casey
> Cc: Kernel Hardening ; kernel list
> ; linux-security-module mod...@vger.kernel.org>; selinux@tycho.nsa.gov; Hansen, Dave
> ; Dock, Deneen T ;
> kris...@linux.intel.com; Arjan van de Ven
> Subject: Re: [PATCH v5 1/5] AppArmor: Prepare for PTRACE_MODE_SCHED
>
> On Wed, Sep 26, 2018 at 11:16 PM Jann Horn wrote:
> >
> > On Wed, Sep 26, 2018 at 10:35 PM Casey Schaufler
> > wrote:
> > > A ptrace access check with mode PTRACE_MODE_SCHED gets called
> > > from process switching code. This precludes the use of audit,
> > > as the locking is incompatible. Don't do audit in the PTRACE_MODE_SCHED
> > > case.
> >
> > Why is this separate from PTRACE_MODE_NOAUDIT? It looks like
> > apparmor_ptrace_access_check() currently ignores
> PTRACE_MODE_NOAUDIT.
> > Could you, instead of adding a new flag, fix the handling of
> > PTRACE_MODE_NOAUDIT?
>
> Er, after looking at more of the series, I see that PTRACE_MODE_SCHED
> is necessary; but could you handle the "don't audit" part for AppArmor
> using PTRACE_MODE_NOAUDIT instead?
I could have done it a number of ways, but this seemed to maintain
the apparmor AA_PTRACE abstraction the best. If aa_may_ptrace didn't
eschew PTRACE_MODE in favor of AA_PTRACE no change to the interface
would have been required. I'm reluctant to change something like that.
> > > Signed-off-by: Casey Schaufler
> > > ---
> > > security/apparmor/domain.c | 2 +-
> > > security/apparmor/include/ipc.h | 2 +-
> > > security/apparmor/ipc.c | 8 +---
> > > security/apparmor/lsm.c | 5 +++--
> > > 4 files changed, 10 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
> > > index 08c88de0ffda..28300f4c3ef9 100644
> > > --- a/security/apparmor/domain.c
> > > +++ b/security/apparmor/domain.c
> > > @@ -77,7 +77,7 @@ static int may_change_ptraced_domain(struct
> aa_label *to_label,
> > > if (!tracer || unconfined(tracerl))
> > > goto out;
> > >
> > > - error = aa_may_ptrace(tracerl, to_label, PTRACE_MODE_ATTACH);
> > > + error = aa_may_ptrace(tracerl, to_label, PTRACE_MODE_ATTACH,
> true);
> > >
> > > out:
> > > rcu_read_unlock();
> > > diff --git a/security/apparmor/include/ipc.h
> b/security/apparmor/include/ipc.h
> > > index 5ffc218d1e74..299d1c45fef0 100644
> > > --- a/security/apparmor/include/ipc.h
> > > +++ b/security/apparmor/include/ipc.h
> > > @@ -34,7 +34,7 @@ struct aa_profile;
> > > "xcpu xfsz vtalrm prof winch io pwr sys emt lost"
> > >
> > > int aa_may_ptrace(struct aa_label *tracer, struct aa_label *tracee,
> > > - u32 request);
> > > + u32 request, bool audit);
> > > int aa_may_signal(struct aa_label *sender, struct aa_label *target, int
> > > sig);
> > >
> > > #endif /* __AA_IPC_H */
> > > diff --git a/security/apparmor/ipc.c b/security/apparmor/ipc.c
> > > index 527ea1557120..9ed110afc822 100644
> > > --- a/security/apparmor/ipc.c
> > > +++ b/security/apparmor/ipc.c
> > > @@ -121,15 +121,17 @@ static int profile_tracer_perm(struct aa_profile
> *tracer,
> > > * Returns: %0 else error code if permission denied or error
> > > */
> > > int aa_may_ptrace(struct aa_label *tracer, struct aa_label *tracee,
> > > - u32 request)
> > > + u32 request, bool audit)
> > > {
> > > struct aa_profile *profile;
> > > u32 xrequest = request << PTRACE_PERM_SHIFT;
> > > DEFINE_AUDIT_DATA(sa, LSM_AUDIT_DATA_NONE, OP_PTRACE);
> > >
> > > return xcheck_labels(tracer, tracee, profile,
> > > - profile_tracer_perm(profile, tracee, request,
> > > &sa),
> > > - profile_tracee_perm(profile, tracer, xrequest,
> > > &sa));
> > > + profile_tracer_perm(profile, tracee, request,
> > > + audit ? &sa : NULL),
> > > + profile_tracee_perm(profile, tracer, xrequest,
> > > + audit ? &sa : NULL));
> > > }
> > >
&g
RE: [PATCH v3 3/5] LSM: Security module checking for side-channel dangers
> -Original Message- > From: Jann Horn [mailto:ja...@google.com] > Sent: Wednesday, August 22, 2018 10:04 AM > To: Schaufler, Casey > Cc: Kernel Hardening ; kernel list > ; linux-security-module mod...@vger.kernel.org>; selinux@tycho.nsa.gov; Hansen, Dave > ; Dock, Deneen T ; > kris...@linux.intel.com; Arjan van de Ven > Subject: Re: [PATCH v3 3/5] LSM: Security module checking for side-channel > dangers > > [SNIP] > > 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. > > The capability check doesn't always require a namespace match, and I > don't care about non-user namespaces here, but I would prefer it if > A->B with A having some capabilities required A's user namespace to be > ancestor-or-self of B's user namespace. But alternatively: Looking at ancestor relations starts to get us pretty close to the point where the cost of checking will overwhelm the savings. This is something we have to be very careful of. > > 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? > > It seems to me that it would make sense to express this as something > like a Kconfig dependency. I thought about that. It could be the best choice. I will investigate further. > But I guess if you document that the > combination of CONFIG_USER_NS=y, > CONFIG_SECURITY_SIDECHANNEL_NAMESPACES=n and > SECURITY_SIDECHANNEL_CAPABILITIES=y is nonsensical, that works too. I > just don't see why you'd want to provide such a footgun? Point. > Configurability is nice, but if we know that one of the possible > configurations doesn't make sense, it seems like a good idea to just > not allow the system to be configured that way. Another point. > You say that you were asked to make the configuration flexible. Did > whoever told you that actually want the ability to compare raw > capability sets on a system with CONFIG_USER_NS=y, and understand what > semantics that has (and doesn't have)? Or was their intent more along > the lines of "we want to flush if the new task has higher privileges, > capability-wise, than the old task; but we don't explicitly care about > namespaces"? Without going into too much detail, it's a matter of people who understand chips and performance better than they understand security or the user experience. ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
RE: [PATCH v3 3/5] LSM: Security module checking for side-channel dangers
> -Original Message-
> From: Jann Horn [mailto:ja...@google.com]
> Sent: Tuesday, August 21, 2018 6:01 PM
> To: Schaufler, Casey
> Cc: Kernel Hardening ; kernel list
> ; linux-security-module mod...@vger.kernel.org>; selinux@tycho.nsa.gov; Hansen, Dave
> ; Dock, Deneen T ;
> kris...@linux.intel.com; Arjan van de Ven
> 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
> wrote:
> >
> > > -Original Message-
> > > From: Jann Horn [mailto:ja...@google.com]
> > > Sent: Tuesday, August 21, 2018 10:24 AM
> > > To: Schaufler, Casey
> > > Cc: Kernel Hardening ; kernel list
> > > ; linux-security-module > > mod...@vger.kernel.org>; selinux@tycho.nsa.gov; Hansen, Dave
> > > ; Dock, Deneen T ;
> > > kris...@linux.intel.com; Arjan van de Ven
> > > 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
> > > 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
> > > > ---
> > > [...]
> > > > diff --git a/security/sidechannel/Kconfig b/security/sidechannel/Kconfig
> > > > new file mode 100644
> > > > index ..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 ..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 -
RE: [PATCH v3 3/5] LSM: Security module checking for side-channel dangers
> -Original Message-
> From: Jann Horn [mailto:ja...@google.com]
> Sent: Tuesday, August 21, 2018 10:24 AM
> To: Schaufler, Casey
> Cc: Kernel Hardening ; kernel list
> ; linux-security-module mod...@vger.kernel.org>; selinux@tycho.nsa.gov; Hansen, Dave
> ; Dock, Deneen T ;
> kris...@linux.intel.com; Arjan van de Ven
> 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
> 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
> > ---
> [...]
> > diff --git a/security/sidechannel/Kconfig b/security/sidechannel/Kconfig
> > new file mode 100644
> > index ..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 ..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.
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.
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 saf
RE: [PATCH RFC v2 2/5] X86: Support LSM determination of side-channel vulnerability
> -Original Message-
> From: Jann Horn [mailto:ja...@google.com]
> Sent: Tuesday, August 21, 2018 3:20 AM
> To: Schaufler, Casey
> Cc: Kernel Hardening ; kernel list
> ; linux-security-module mod...@vger.kernel.org>; selinux@tycho.nsa.gov; Hansen, Dave
> ; Dock, Deneen T ;
> kris...@linux.intel.com; Arjan van de Ven
> Subject: Re: [PATCH RFC v2 2/5] X86: Support LSM determination of side-
> channel vulnerability
>
> On Mon, Aug 20, 2018 at 4:45 PM Schaufler, Casey
> wrote:
> >
> > > -Original Message-
> > > From: Jann Horn [mailto:ja...@google.com]
> > > Sent: Friday, August 17, 2018 4:55 PM
> > > To: Schaufler, Casey
> > > Cc: Kernel Hardening ; kernel list
> > > ; linux-security-module > > mod...@vger.kernel.org>; selinux@tycho.nsa.gov; Hansen, Dave
> > > ; Dock, Deneen T ;
> > > kris...@linux.intel.com; Arjan van de Ven
> > > Subject: Re: [PATCH RFC v2 2/5] X86: Support LSM determination of side-
> > > channel vulnerability
> > >
> > > On Sat, Aug 18, 2018 at 12:17 AM Casey Schaufler
> > > wrote:
> > > >
> > > > From: Casey Schaufler
> > > >
> > > > When switching between tasks it may be necessary
> > > > to set an indirect branch prediction barrier if the
> > > > tasks are potentially vulnerable to side-channel
> > > > attacks. This adds a call to security_task_safe_sidechannel
> > > > so that security modules can weigh in on the decision.
> > > >
> > > > Signed-off-by: Casey Schaufler
> > > > ---
> > > > arch/x86/mm/tlb.c | 12
> > > > 1 file changed, 8 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c
> > > > index 6eb1f34c3c85..8714d4af06aa 100644
> > > > --- a/arch/x86/mm/tlb.c
> > > > +++ b/arch/x86/mm/tlb.c
> > > > @@ -7,6 +7,7 @@
> > > > #include
> > > > #include
> > > > #include
> > > > +#include
> > > >
> > > > #include
> > > > #include
> > > > @@ -270,11 +271,14 @@ void switch_mm_irqs_off(struct mm_struct
> *prev,
> > > struct mm_struct *next,
> > > > * threads. It will also not flush if we switch to idle
> > > > * thread and back to the same process. It will flush
> > > > if we
> > > > * switch to a different non-dumpable process.
> > > > +* If a security module thinks that the transition
> > > > +* is unsafe do the flush.
> > > > */
> > > > - if (tsk && tsk->mm &&
> > > > - tsk->mm->context.ctx_id != last_ctx_id &&
> > > > - get_dumpable(tsk->mm) != SUID_DUMP_USER)
> > > > - indirect_branch_prediction_barrier();
> > > > + if (tsk && tsk->mm && tsk->mm->context.ctx_id !=
> > > > last_ctx_id)
> {
> > > > + if (get_dumpable(tsk->mm) != SUID_DUMP_USER ||
> > > > + security_task_safe_sidechannel(tsk) != 0)
> > > > + indirect_branch_prediction_barrier();
> > > > + }
> > >
> > > When you posted v1 of this series, I asked:
> > >
> > > | Does this enforce transitivity? What happens if we first switch from
> > > | an attacker task to a task without ->mm, and immediately afterwards
> > > | from the task without ->mm to a victim task? In that case, whether a
> > > | flush happens between the attacker task and the victim task depends on
> > > | whether the LSM thinks that the mm-less task should have access to the
> > > | victim task, right?
> > >
> > > Have you addressed that? I don't see it...
> >
> > Nope. That's going to require maintaining state about all the
> > tasks in the chain that might still have cache involvement.
> >
> > A -> B -> C -> D
>
> Really?
I am willing to be educated otherwise. My understanding
of Modern Processor Technology will never be so deep that
I won't listen to reason.
>
> From what I can tell, it'd be enough to:
>
> - ensure that the LSM-based access checks behave approximately transitively
>(which I think they already do, most
RE: [PATCH RFC v2 5/5] SELinux: Support SELinux determination of side-channel vulnerability
> -Original Message-
> From: Stephen Smalley [mailto:s...@tycho.nsa.gov]
> Sent: Monday, August 20, 2018 10:44 AM
> To: Schaufler, Casey ; kernel-
> harden...@lists.openwall.com; linux-ker...@vger.kernel.org; linux-security-
> mod...@vger.kernel.org; selinux@tycho.nsa.gov; Hansen, Dave
> ; Dock, Deneen T ;
> kris...@linux.intel.com; ar...@linux.intel.com
> Subject: Re: [PATCH RFC v2 5/5] SELinux: Support SELinux determination of
> side-channel vulnerability
>
> On 08/20/2018 12:59 PM, Schaufler, Casey wrote:
> >> -Original Message-
> >> From: Stephen Smalley [mailto:s...@tycho.nsa.gov]
> >> Sent: Monday, August 20, 2018 9:03 AM
> >> To: Schaufler, Casey ; kernel-
> >> harden...@lists.openwall.com; linux-ker...@vger.kernel.org; linux-security-
> >> mod...@vger.kernel.org; selinux@tycho.nsa.gov; Hansen, Dave
> >> ; Dock, Deneen T ;
> >> kris...@linux.intel.com; ar...@linux.intel.com
> >> Subject: Re: [PATCH RFC v2 5/5] SELinux: Support SELinux determination of
> >> side-channel vulnerability
> >>
> >> On 08/17/2018 06:16 PM, Casey Schaufler wrote:
> >>> SELinux considers tasks to be side-channel safe if they
> >>> have PROCESS_SHARE access.
> >>
> >> Now the description and the code no longer match.
> >
> > You're right.
> >
> >>>
> >>> Signed-off-by: Casey Schaufler
> >>> ---
> >>>security/selinux/hooks.c | 9 +
> >>>1 file changed, 9 insertions(+)
> >>>
> >>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> >>> index a8bf324130f5..7fbd7d7ac1cb 100644
> >>> --- a/security/selinux/hooks.c
> >>> +++ b/security/selinux/hooks.c
> >>> @@ -4219,6 +4219,14 @@ static void selinux_task_to_inode(struct
> >> task_struct *p,
> >>> spin_unlock(&isec->lock);
> >>>}
> >>>
> >>> +static int selinux_task_safe_sidechannel(struct task_struct *p)
> >>> +{
> >>> + struct av_decision avd;
> >>> +
> >>> + return avc_has_perm_noaudit(&selinux_state, current_sid(),
> >> task_sid(p),
> >>> + SECCLASS_FILE, FILE__READ, 0, &avd);
> >>> +}
> >>
> >> And my question from before still stands: why do we need a new hook and
> >> new security module instead of just using ptrace_may_access()?
> >
> > Locking. The SELinux check, for example, will lock up solid while trying
> > to generate an audit record. There is no good reason aside from coding
> > convenience to assume that the same restrictions will apply for side-channel
> > as apply to ptrace. I'm actually a touch surprised you're not suggesting a
> > separate SECCLASS or access mode for the SELinux hook.
>
> The PTRACE_MODE_NOAUDIT flag to ptrace_may_access() would address the
> locking concern.
OK ...
> Duplicating the ptrace access checking logic seems
> prone to errors and inconsistencies.
That's true only if the ptrace logic and the safe-sidechannel logic
are identical. I don't believe that is a safe assumption. It would sure
be convenient. But I would hate to see a change made for either
ptrace or safe_sidechannel that interfered with the correct behavior
of the other.
> I can't imagine policy writers
> understanding what "safe sidechannel" means, much less deciding when to
> allow it.
I can't argue with that. But then, I have always had trouble with the
SELinux policy scheme.
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
RE: [PATCH RFC v2 5/5] SELinux: Support SELinux determination of side-channel vulnerability
> -Original Message-
> From: Stephen Smalley [mailto:s...@tycho.nsa.gov]
> Sent: Monday, August 20, 2018 9:03 AM
> To: Schaufler, Casey ; kernel-
> harden...@lists.openwall.com; linux-ker...@vger.kernel.org; linux-security-
> mod...@vger.kernel.org; selinux@tycho.nsa.gov; Hansen, Dave
> ; Dock, Deneen T ;
> kris...@linux.intel.com; ar...@linux.intel.com
> Subject: Re: [PATCH RFC v2 5/5] SELinux: Support SELinux determination of
> side-channel vulnerability
>
> On 08/17/2018 06:16 PM, Casey Schaufler wrote:
> > SELinux considers tasks to be side-channel safe if they
> > have PROCESS_SHARE access.
>
> Now the description and the code no longer match.
You're right.
> >
> > Signed-off-by: Casey Schaufler
> > ---
> > security/selinux/hooks.c | 9 +
> > 1 file changed, 9 insertions(+)
> >
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index a8bf324130f5..7fbd7d7ac1cb 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -4219,6 +4219,14 @@ static void selinux_task_to_inode(struct
> task_struct *p,
> > spin_unlock(&isec->lock);
> > }
> >
> > +static int selinux_task_safe_sidechannel(struct task_struct *p)
> > +{
> > + struct av_decision avd;
> > +
> > + return avc_has_perm_noaudit(&selinux_state, current_sid(),
> task_sid(p),
> > + SECCLASS_FILE, FILE__READ, 0, &avd);
> > +}
>
> And my question from before still stands: why do we need a new hook and
> new security module instead of just using ptrace_may_access()?
Locking. The SELinux check, for example, will lock up solid while trying
to generate an audit record. There is no good reason aside from coding
convenience to assume that the same restrictions will apply for side-channel
as apply to ptrace. I'm actually a touch surprised you're not suggesting a
separate SECCLASS or access mode for the SELinux hook.
>
> > +
> > /* Returns error only if unable to parse addresses */
> > static int selinux_parse_skb_ipv4(struct sk_buff *skb,
> > struct common_audit_data *ad, u8 *proto)
> > @@ -7002,6 +7010,7 @@ static struct security_hook_list selinux_hooks[]
> __lsm_ro_after_init = {
> > LSM_HOOK_INIT(task_movememory, selinux_task_movememory),
> > LSM_HOOK_INIT(task_kill, selinux_task_kill),
> > LSM_HOOK_INIT(task_to_inode, selinux_task_to_inode),
> > + LSM_HOOK_INIT(task_safe_sidechannel,
> selinux_task_safe_sidechannel),
> >
> > LSM_HOOK_INIT(ipc_permission, selinux_ipc_permission),
> > LSM_HOOK_INIT(ipc_getsecid, selinux_ipc_getsecid),
> >
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
RE: [PATCH RFC v2 3/5] LSM: Security module checking for side-channel dangers
> -Original Message-
> From: Jann Horn [mailto:ja...@google.com]
> Sent: Friday, August 17, 2018 4:53 PM
> To: Schaufler, Casey
> Cc: Kernel Hardening ; kernel list
> ; linux-security-module mod...@vger.kernel.org>; selinux@tycho.nsa.gov; Hansen, Dave
> ; Dock, Deneen T ;
> kris...@linux.intel.com; Arjan van de Ven
> Subject: Re: [PATCH RFC v2 3/5] LSM: Security module checking for side-
> channel dangers
>
> On Sat, Aug 18, 2018 at 12:17 AM Casey Schaufler
> wrote:
> >
> > From: Casey Schaufler
> >
> > 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
> [...]
> > +#ifdef CONFIG_SECURITY_SIDECHANNEL_UIDS
> > +static int safe_by_uid(struct task_struct *p)
> > +{
> > + const struct cred *ccred = current_real_cred();
> > + const struct cred *pcred = get_task_cred(p);
> > +
> > + /*
> > +* Credential checks. Considered safe if:
> > +* UIDs are the same
> > +*/
> > + if (ccred != pcred && ccred->euid.val != pcred->euid.val)
> > + return -EACCES;
> > + return 0;
> > +}
>
> This function looks bogus. get_task_cred() bumps the refcount on the
> returned cred struct pointer, but you don't drop it. You probably want
> to use something that doesn't fiddle with the refcount at all here to
> avoid cacheline bouncing - possibly a raw rcu_dereference_protected()
> if there are no better helpers.
>
> Same thing for the other get_task_cred() calls further down in the patch.
Thanks. Looks like I whacked out v2 a bit hastily.
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
RE: [PATCH RFC v2 2/5] X86: Support LSM determination of side-channel vulnerability
> -Original Message-
> From: Jann Horn [mailto:ja...@google.com]
> Sent: Friday, August 17, 2018 4:55 PM
> To: Schaufler, Casey
> Cc: Kernel Hardening ; kernel list
> ; linux-security-module mod...@vger.kernel.org>; selinux@tycho.nsa.gov; Hansen, Dave
> ; Dock, Deneen T ;
> kris...@linux.intel.com; Arjan van de Ven
> Subject: Re: [PATCH RFC v2 2/5] X86: Support LSM determination of side-
> channel vulnerability
>
> On Sat, Aug 18, 2018 at 12:17 AM Casey Schaufler
> wrote:
> >
> > From: Casey Schaufler
> >
> > When switching between tasks it may be necessary
> > to set an indirect branch prediction barrier if the
> > tasks are potentially vulnerable to side-channel
> > attacks. This adds a call to security_task_safe_sidechannel
> > so that security modules can weigh in on the decision.
> >
> > Signed-off-by: Casey Schaufler
> > ---
> > arch/x86/mm/tlb.c | 12
> > 1 file changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c
> > index 6eb1f34c3c85..8714d4af06aa 100644
> > --- a/arch/x86/mm/tlb.c
> > +++ b/arch/x86/mm/tlb.c
> > @@ -7,6 +7,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #include
> > #include
> > @@ -270,11 +271,14 @@ void switch_mm_irqs_off(struct mm_struct *prev,
> struct mm_struct *next,
> > * threads. It will also not flush if we switch to idle
> > * thread and back to the same process. It will flush if we
> > * switch to a different non-dumpable process.
> > +* If a security module thinks that the transition
> > +* is unsafe do the flush.
> > */
> > - if (tsk && tsk->mm &&
> > - tsk->mm->context.ctx_id != last_ctx_id &&
> > - get_dumpable(tsk->mm) != SUID_DUMP_USER)
> > - indirect_branch_prediction_barrier();
> > + if (tsk && tsk->mm && tsk->mm->context.ctx_id !=
> > last_ctx_id) {
> > + if (get_dumpable(tsk->mm) != SUID_DUMP_USER ||
> > + security_task_safe_sidechannel(tsk) != 0)
> > + indirect_branch_prediction_barrier();
> > + }
>
> When you posted v1 of this series, I asked:
>
> | Does this enforce transitivity? What happens if we first switch from
> | an attacker task to a task without ->mm, and immediately afterwards
> | from the task without ->mm to a victim task? In that case, whether a
> | flush happens between the attacker task and the victim task depends on
> | whether the LSM thinks that the mm-less task should have access to the
> | victim task, right?
>
> Have you addressed that? I don't see it...
Nope. That's going to require maintaining state about all the
tasks in the chain that might still have cache involvement.
A -> B -> C -> D
If B and C don't do anything cacheworthy D could conceivably attack A.
The amount of state required to detect this case would be prohibitive.
I think that if you're sufficiently concerned about this case you should just
go ahead and set the barrier. I'm willing to learn something that says I'm
wrong.
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
