* [PATCH v5 0/5] LSM: Support ptrace sidechannel access checks
@ 2018-09-26 20:34 ` Casey Schaufler
0 siblings, 0 replies; 55+ messages in thread
From: Casey Schaufler @ 2018-09-26 20:34 UTC (permalink / raw)
To: linux-security-module
v5: Revamped to match Jiri Kosina <jkosina@suse.cz>
Harden spectrev2 userspace-userspace protection v7
Fixed locking issues in the LSM code.
Dropped the new LSM hook and use a ptrace hook instead.
v4: select namespace checks if user namespaces are enabled
and credential checks are request.
v3: get_task_cred wasn't a good choice due to refcounts.
Use lower level protection instead
v2: SELinux access policy corrected.
Use real_cred instead of cred.
This patchset provide a mechanism by which a security module
can advise the system about potential side-channel vulnerabilities.
The existing security modules have been updated to avoid locking
issues in the face of PTRACE_MODE_SCHED. A new security
module is provided to make determinations regarding task attributes
including namespaces.
Signed-off-by: Casey Schaufler <casey.schaufler@intel.com>
---
include/linux/lsm_hooks.h | 5 +++
kernel/ptrace.c | 2 -
security/Kconfig | 1 +
security/Makefile | 2 +
security/apparmor/domain.c | 2 +-
security/apparmor/include/ipc.h | 2 +-
security/apparmor/ipc.c | 8 ++--
security/apparmor/lsm.c | 5 ++-
security/commoncap.c | 2 +
security/security.c | 1 +
security/selinux/hooks.c | 2 +
security/sidechannel/Kconfig | 13 ++++++
security/sidechannel/Makefile | 1 +
security/sidechannel/sidechannel.c | 88 ++++++++++++++++++++++++++++++++++++++
security/smack/smack_lsm.c | 3 +-
15 files changed, 127 insertions(+), 10 deletions(-)
^ permalink raw reply [flat|nested] 55+ messages in thread* [PATCH v5 1/5] AppArmor: Prepare for PTRACE_MODE_SCHED
2018-09-26 20:34 ` Casey Schaufler
@ 2018-09-26 20:34 ` Casey Schaufler
-1 siblings, 0 replies; 55+ messages in thread
From: Casey Schaufler @ 2018-09-26 20:34 UTC (permalink / raw)
To: kernel-hardening, linux-kernel, linux-security-module, selinux,
casey.schaufler, dave.hansen, deneen.t.dock, kristen, arjan
From: Casey Schaufler <casey@schaufler-ca.com>
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 <casey.schaufler@intel.com>
---
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));
}
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 8b8b70620bbe..da9d0b228857 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -118,7 +118,8 @@ static int apparmor_ptrace_access_check(struct task_struct *child,
tracee = aa_get_task_label(child);
error = aa_may_ptrace(tracer, tracee,
(mode & PTRACE_MODE_READ) ? AA_PTRACE_READ
- : AA_PTRACE_TRACE);
+ : AA_PTRACE_TRACE,
+ !(mode & PTRACE_MODE_SCHED));
aa_put_label(tracee);
end_current_label_crit_section(tracer);
@@ -132,7 +133,7 @@ static int apparmor_ptrace_traceme(struct task_struct *parent)
tracee = begin_current_label_crit_section();
tracer = aa_get_task_label(parent);
- error = aa_may_ptrace(tracer, tracee, AA_PTRACE_TRACE);
+ error = aa_may_ptrace(tracer, tracee, AA_PTRACE_TRACE, true);
aa_put_label(tracer);
end_current_label_crit_section(tracee);
--
2.17.1
^ permalink raw reply related [flat|nested] 55+ messages in thread* [PATCH v5 1/5] AppArmor: Prepare for PTRACE_MODE_SCHED
@ 2018-09-26 20:34 ` Casey Schaufler
0 siblings, 0 replies; 55+ messages in thread
From: Casey Schaufler @ 2018-09-26 20:34 UTC (permalink / raw)
To: linux-security-module
From: Casey Schaufler <casey@schaufler-ca.com>
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 <casey.schaufler@intel.com>
---
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));
}
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 8b8b70620bbe..da9d0b228857 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -118,7 +118,8 @@ static int apparmor_ptrace_access_check(struct task_struct *child,
tracee = aa_get_task_label(child);
error = aa_may_ptrace(tracer, tracee,
(mode & PTRACE_MODE_READ) ? AA_PTRACE_READ
- : AA_PTRACE_TRACE);
+ : AA_PTRACE_TRACE,
+ !(mode & PTRACE_MODE_SCHED));
aa_put_label(tracee);
end_current_label_crit_section(tracer);
@@ -132,7 +133,7 @@ static int apparmor_ptrace_traceme(struct task_struct *parent)
tracee = begin_current_label_crit_section();
tracer = aa_get_task_label(parent);
- error = aa_may_ptrace(tracer, tracee, AA_PTRACE_TRACE);
+ error = aa_may_ptrace(tracer, tracee, AA_PTRACE_TRACE, true);
aa_put_label(tracer);
end_current_label_crit_section(tracee);
--
2.17.1
^ permalink raw reply related [flat|nested] 55+ messages in thread* Re: [PATCH v5 1/5] AppArmor: Prepare for PTRACE_MODE_SCHED
2018-09-26 20:34 ` Casey Schaufler
@ 2018-09-26 21:16 ` Jann Horn
-1 siblings, 0 replies; 55+ messages in thread
From: Jann Horn @ 2018-09-26 21:16 UTC (permalink / raw)
To: Casey Schaufler
Cc: Kernel Hardening, kernel list, linux-security-module, selinux,
Dave Hansen, deneen.t.dock, kristen, Arjan van de Ven
On Wed, Sep 26, 2018 at 10:35 PM Casey Schaufler
<casey.schaufler@intel.com> 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?
> Signed-off-by: Casey Schaufler <casey.schaufler@intel.com>
> ---
> 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));
> }
>
>
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index 8b8b70620bbe..da9d0b228857 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -118,7 +118,8 @@ static int apparmor_ptrace_access_check(struct task_struct *child,
> tracee = aa_get_task_label(child);
> error = aa_may_ptrace(tracer, tracee,
> (mode & PTRACE_MODE_READ) ? AA_PTRACE_READ
> - : AA_PTRACE_TRACE);
> + : AA_PTRACE_TRACE,
> + !(mode & PTRACE_MODE_SCHED));
> aa_put_label(tracee);
> end_current_label_crit_section(tracer);
>
> @@ -132,7 +133,7 @@ static int apparmor_ptrace_traceme(struct task_struct *parent)
>
> tracee = begin_current_label_crit_section();
> tracer = aa_get_task_label(parent);
> - error = aa_may_ptrace(tracer, tracee, AA_PTRACE_TRACE);
> + error = aa_may_ptrace(tracer, tracee, AA_PTRACE_TRACE, true);
> aa_put_label(tracer);
> end_current_label_crit_section(tracee);
>
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 55+ messages in thread* [PATCH v5 1/5] AppArmor: Prepare for PTRACE_MODE_SCHED
@ 2018-09-26 21:16 ` Jann Horn
0 siblings, 0 replies; 55+ messages in thread
From: Jann Horn @ 2018-09-26 21:16 UTC (permalink / raw)
To: linux-security-module
On Wed, Sep 26, 2018 at 10:35 PM Casey Schaufler
<casey.schaufler@intel.com> 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?
> Signed-off-by: Casey Schaufler <casey.schaufler@intel.com>
> ---
> 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));
> }
>
>
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index 8b8b70620bbe..da9d0b228857 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -118,7 +118,8 @@ static int apparmor_ptrace_access_check(struct task_struct *child,
> tracee = aa_get_task_label(child);
> error = aa_may_ptrace(tracer, tracee,
> (mode & PTRACE_MODE_READ) ? AA_PTRACE_READ
> - : AA_PTRACE_TRACE);
> + : AA_PTRACE_TRACE,
> + !(mode & PTRACE_MODE_SCHED));
> aa_put_label(tracee);
> end_current_label_crit_section(tracer);
>
> @@ -132,7 +133,7 @@ static int apparmor_ptrace_traceme(struct task_struct *parent)
>
> tracee = begin_current_label_crit_section();
> tracer = aa_get_task_label(parent);
> - error = aa_may_ptrace(tracer, tracee, AA_PTRACE_TRACE);
> + error = aa_may_ptrace(tracer, tracee, AA_PTRACE_TRACE, true);
> aa_put_label(tracer);
> end_current_label_crit_section(tracee);
>
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [PATCH v5 1/5] AppArmor: Prepare for PTRACE_MODE_SCHED
2018-09-26 21:16 ` Jann Horn
@ 2018-09-26 21:18 ` Jann Horn
-1 siblings, 0 replies; 55+ messages in thread
From: Jann Horn @ 2018-09-26 21:18 UTC (permalink / raw)
To: Casey Schaufler
Cc: Kernel Hardening, kernel list, linux-security-module, selinux,
Dave Hansen, deneen.t.dock, kristen, Arjan van de Ven
On Wed, Sep 26, 2018 at 11:16 PM Jann Horn <jannh@google.com> wrote:
>
> On Wed, Sep 26, 2018 at 10:35 PM Casey Schaufler
> <casey.schaufler@intel.com> 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?
> > Signed-off-by: Casey Schaufler <casey.schaufler@intel.com>
> > ---
> > 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));
> > }
> >
> >
> > diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> > index 8b8b70620bbe..da9d0b228857 100644
> > --- a/security/apparmor/lsm.c
> > +++ b/security/apparmor/lsm.c
> > @@ -118,7 +118,8 @@ static int apparmor_ptrace_access_check(struct task_struct *child,
> > tracee = aa_get_task_label(child);
> > error = aa_may_ptrace(tracer, tracee,
> > (mode & PTRACE_MODE_READ) ? AA_PTRACE_READ
> > - : AA_PTRACE_TRACE);
> > + : AA_PTRACE_TRACE,
> > + !(mode & PTRACE_MODE_SCHED));
> > aa_put_label(tracee);
> > end_current_label_crit_section(tracer);
> >
> > @@ -132,7 +133,7 @@ static int apparmor_ptrace_traceme(struct task_struct *parent)
> >
> > tracee = begin_current_label_crit_section();
> > tracer = aa_get_task_label(parent);
> > - error = aa_may_ptrace(tracer, tracee, AA_PTRACE_TRACE);
> > + error = aa_may_ptrace(tracer, tracee, AA_PTRACE_TRACE, true);
> > aa_put_label(tracer);
> > end_current_label_crit_section(tracee);
> >
> > --
> > 2.17.1
> >
^ permalink raw reply [flat|nested] 55+ messages in thread* [PATCH v5 1/5] AppArmor: Prepare for PTRACE_MODE_SCHED
@ 2018-09-26 21:18 ` Jann Horn
0 siblings, 0 replies; 55+ messages in thread
From: Jann Horn @ 2018-09-26 21:18 UTC (permalink / raw)
To: linux-security-module
On Wed, Sep 26, 2018 at 11:16 PM Jann Horn <jannh@google.com> wrote:
>
> On Wed, Sep 26, 2018 at 10:35 PM Casey Schaufler
> <casey.schaufler@intel.com> 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?
> > Signed-off-by: Casey Schaufler <casey.schaufler@intel.com>
> > ---
> > 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));
> > }
> >
> >
> > diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> > index 8b8b70620bbe..da9d0b228857 100644
> > --- a/security/apparmor/lsm.c
> > +++ b/security/apparmor/lsm.c
> > @@ -118,7 +118,8 @@ static int apparmor_ptrace_access_check(struct task_struct *child,
> > tracee = aa_get_task_label(child);
> > error = aa_may_ptrace(tracer, tracee,
> > (mode & PTRACE_MODE_READ) ? AA_PTRACE_READ
> > - : AA_PTRACE_TRACE);
> > + : AA_PTRACE_TRACE,
> > + !(mode & PTRACE_MODE_SCHED));
> > aa_put_label(tracee);
> > end_current_label_crit_section(tracer);
> >
> > @@ -132,7 +133,7 @@ static int apparmor_ptrace_traceme(struct task_struct *parent)
> >
> > tracee = begin_current_label_crit_section();
> > tracer = aa_get_task_label(parent);
> > - error = aa_may_ptrace(tracer, tracee, AA_PTRACE_TRACE);
> > + error = aa_may_ptrace(tracer, tracee, AA_PTRACE_TRACE, true);
> > aa_put_label(tracer);
> > end_current_label_crit_section(tracee);
> >
> > --
> > 2.17.1
> >
^ permalink raw reply [flat|nested] 55+ messages in thread* RE: [PATCH v5 1/5] AppArmor: Prepare for PTRACE_MODE_SCHED
2018-09-26 21:18 ` Jann Horn
@ 2018-09-26 22:47 ` Schaufler, Casey
-1 siblings, 0 replies; 55+ messages in thread
From: Schaufler, Casey @ 2018-09-26 22:47 UTC (permalink / raw)
To: Jann Horn
Cc: Kernel Hardening, kernel list, linux-security-module, selinux,
Hansen, Dave, Dock, Deneen T, kristen, Arjan van de Ven
> -----Original Message-----
> From: Jann Horn [mailto:jannh@google.com]
> Sent: Wednesday, September 26, 2018 2:19 PM
> To: Schaufler, Casey <casey.schaufler@intel.com>
> Cc: Kernel Hardening <kernel-hardening@lists.openwall.com>; kernel list
> <linux-kernel@vger.kernel.org>; linux-security-module <linux-security-
> module@vger.kernel.org>; selinux@tycho.nsa.gov; Hansen, Dave
> <dave.hansen@intel.com>; Dock, Deneen T <deneen.t.dock@intel.com>;
> kristen@linux.intel.com; Arjan van de Ven <arjan@linux.intel.com>
> Subject: Re: [PATCH v5 1/5] AppArmor: Prepare for PTRACE_MODE_SCHED
>
> On Wed, Sep 26, 2018 at 11:16 PM Jann Horn <jannh@google.com> wrote:
> >
> > On Wed, Sep 26, 2018 at 10:35 PM Casey Schaufler
> > <casey.schaufler@intel.com> 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 <casey.schaufler@intel.com>
> > > ---
> > > 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));
> > > }
> > >
> > >
> > > diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> > > index 8b8b70620bbe..da9d0b228857 100644
> > > --- a/security/apparmor/lsm.c
> > > +++ b/security/apparmor/lsm.c
> > > @@ -118,7 +118,8 @@ static int apparmor_ptrace_access_check(struct
> task_struct *child,
> > > tracee = aa_get_task_label(child);
> > > error = aa_may_ptrace(tracer, tracee,
> > > (mode & PTRACE_MODE_READ) ? AA_PTRACE_READ
> > > - : AA_PTRACE_TRACE);
> > > + : AA_PTRACE_TRACE,
> > > + !(mode & PTRACE_MODE_SCHED));
> > > aa_put_label(tracee);
> > > end_current_label_crit_section(tracer);
> > >
> > > @@ -132,7 +133,7 @@ static int apparmor_ptrace_traceme(struct
> task_struct *parent)
> > >
> > > tracee = begin_current_label_crit_section();
> > > tracer = aa_get_task_label(parent);
> > > - error = aa_may_ptrace(tracer, tracee, AA_PTRACE_TRACE);
> > > + error = aa_may_ptrace(tracer, tracee, AA_PTRACE_TRACE, true);
> > > aa_put_label(tracer);
> > > end_current_label_crit_section(tracee);
> > >
> > > --
> > > 2.17.1
> > >
^ permalink raw reply [flat|nested] 55+ messages in thread* [PATCH v5 1/5] AppArmor: Prepare for PTRACE_MODE_SCHED
@ 2018-09-26 22:47 ` Schaufler, Casey
0 siblings, 0 replies; 55+ messages in thread
From: Schaufler, Casey @ 2018-09-26 22:47 UTC (permalink / raw)
To: linux-security-module
> -----Original Message-----
> From: Jann Horn [mailto:jannh at google.com]
> Sent: Wednesday, September 26, 2018 2:19 PM
> To: Schaufler, Casey <casey.schaufler@intel.com>
> Cc: Kernel Hardening <kernel-hardening@lists.openwall.com>; kernel list
> <linux-kernel@vger.kernel.org>; linux-security-module <linux-security-
> module at vger.kernel.org>; selinux at tycho.nsa.gov; Hansen, Dave
> <dave.hansen@intel.com>; Dock, Deneen T <deneen.t.dock@intel.com>;
> kristen at linux.intel.com; Arjan van de Ven <arjan@linux.intel.com>
> Subject: Re: [PATCH v5 1/5] AppArmor: Prepare for PTRACE_MODE_SCHED
>
> On Wed, Sep 26, 2018 at 11:16 PM Jann Horn <jannh@google.com> wrote:
> >
> > On Wed, Sep 26, 2018 at 10:35 PM Casey Schaufler
> > <casey.schaufler@intel.com> 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 <casey.schaufler@intel.com>
> > > ---
> > > 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));
> > > }
> > >
> > >
> > > diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> > > index 8b8b70620bbe..da9d0b228857 100644
> > > --- a/security/apparmor/lsm.c
> > > +++ b/security/apparmor/lsm.c
> > > @@ -118,7 +118,8 @@ static int apparmor_ptrace_access_check(struct
> task_struct *child,
> > > tracee = aa_get_task_label(child);
> > > error = aa_may_ptrace(tracer, tracee,
> > > (mode & PTRACE_MODE_READ) ? AA_PTRACE_READ
> > > - : AA_PTRACE_TRACE);
> > > + : AA_PTRACE_TRACE,
> > > + !(mode & PTRACE_MODE_SCHED));
> > > aa_put_label(tracee);
> > > end_current_label_crit_section(tracer);
> > >
> > > @@ -132,7 +133,7 @@ static int apparmor_ptrace_traceme(struct
> task_struct *parent)
> > >
> > > tracee = begin_current_label_crit_section();
> > > tracer = aa_get_task_label(parent);
> > > - error = aa_may_ptrace(tracer, tracee, AA_PTRACE_TRACE);
> > > + error = aa_may_ptrace(tracer, tracee, AA_PTRACE_TRACE, true);
> > > aa_put_label(tracer);
> > > end_current_label_crit_section(tracee);
> > >
> > > --
> > > 2.17.1
> > >
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH v5 2/5] Smack: Prepare for PTRACE_MODE_SCHED
2018-09-26 20:34 ` Casey Schaufler
@ 2018-09-26 20:34 ` Casey Schaufler
-1 siblings, 0 replies; 55+ messages in thread
From: Casey Schaufler @ 2018-09-26 20:34 UTC (permalink / raw)
To: kernel-hardening, linux-kernel, linux-security-module, selinux,
casey.schaufler, dave.hansen, deneen.t.dock, kristen, arjan
From: Casey Schaufler <casey@schaufler-ca.com>
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 <casey.schaufler@intel.com>
---
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) {
smk_ad_init(&ad, func, LSM_AUDIT_DATA_TASK);
smk_ad_setfield_u_tsk(&ad, tracer);
saip = &ad;
--
2.17.1
^ permalink raw reply related [flat|nested] 55+ messages in thread* [PATCH v5 2/5] Smack: Prepare for PTRACE_MODE_SCHED
@ 2018-09-26 20:34 ` Casey Schaufler
0 siblings, 0 replies; 55+ messages in thread
From: Casey Schaufler @ 2018-09-26 20:34 UTC (permalink / raw)
To: linux-security-module
From: Casey Schaufler <casey@schaufler-ca.com>
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 <casey.schaufler@intel.com>
---
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) {
smk_ad_init(&ad, func, LSM_AUDIT_DATA_TASK);
smk_ad_setfield_u_tsk(&ad, tracer);
saip = &ad;
--
2.17.1
^ permalink raw reply related [flat|nested] 55+ messages in thread* Re: [PATCH v5 2/5] Smack: Prepare for PTRACE_MODE_SCHED
2018-09-26 20:34 ` Casey Schaufler
@ 2018-09-26 21:30 ` Jann Horn
-1 siblings, 0 replies; 55+ messages in thread
From: Jann Horn @ 2018-09-26 21:30 UTC (permalink / raw)
To: Casey Schaufler
Cc: Kernel Hardening, kernel list, linux-security-module, selinux,
Dave Hansen, deneen.t.dock, kristen, Arjan van de Ven
On Wed, Sep 26, 2018 at 10:35 PM Casey Schaufler
<casey.schaufler@intel.com> 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 <casey.schaufler@intel.com>
> ---
> 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?
^ permalink raw reply [flat|nested] 55+ messages in thread* [PATCH v5 2/5] Smack: Prepare for PTRACE_MODE_SCHED
@ 2018-09-26 21:30 ` Jann Horn
0 siblings, 0 replies; 55+ messages in thread
From: Jann Horn @ 2018-09-26 21:30 UTC (permalink / raw)
To: linux-security-module
On Wed, Sep 26, 2018 at 10:35 PM Casey Schaufler
<casey.schaufler@intel.com> 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 <casey.schaufler@intel.com>
> ---
> 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?
^ permalink raw reply [flat|nested] 55+ messages in thread* RE: [PATCH v5 2/5] Smack: Prepare for PTRACE_MODE_SCHED
2018-09-26 21:30 ` Jann Horn
@ 2018-09-26 22:53 ` Schaufler, Casey
-1 siblings, 0 replies; 55+ messages in thread
From: Schaufler, Casey @ 2018-09-26 22:53 UTC (permalink / raw)
To: Jann Horn
Cc: Kernel Hardening, kernel list, linux-security-module, selinux,
Hansen, Dave, Dock, Deneen T, kristen, Arjan van de Ven
> -----Original Message-----
> From: Jann Horn [mailto:jannh@google.com]
> Sent: Wednesday, September 26, 2018 2:31 PM
> To: Schaufler, Casey <casey.schaufler@intel.com>
> Cc: Kernel Hardening <kernel-hardening@lists.openwall.com>; kernel list
> <linux-kernel@vger.kernel.org>; linux-security-module <linux-security-
> module@vger.kernel.org>; selinux@tycho.nsa.gov; Hansen, Dave
> <dave.hansen@intel.com>; Dock, Deneen T <deneen.t.dock@intel.com>;
> kristen@linux.intel.com; Arjan van de Ven <arjan@linux.intel.com>
> Subject: Re: [PATCH v5 2/5] Smack: Prepare for PTRACE_MODE_SCHED
>
> On Wed, Sep 26, 2018 at 10:35 PM Casey Schaufler
> <casey.schaufler@intel.com> 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 <casey.schaufler@intel.com>
> > ---
> > 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.
^ permalink raw reply [flat|nested] 55+ messages in thread* [PATCH v5 2/5] Smack: Prepare for PTRACE_MODE_SCHED
@ 2018-09-26 22:53 ` Schaufler, Casey
0 siblings, 0 replies; 55+ messages in thread
From: Schaufler, Casey @ 2018-09-26 22:53 UTC (permalink / raw)
To: linux-security-module
> -----Original Message-----
> From: Jann Horn [mailto:jannh at google.com]
> Sent: Wednesday, September 26, 2018 2:31 PM
> To: Schaufler, Casey <casey.schaufler@intel.com>
> Cc: Kernel Hardening <kernel-hardening@lists.openwall.com>; kernel list
> <linux-kernel@vger.kernel.org>; linux-security-module <linux-security-
> module at vger.kernel.org>; selinux at tycho.nsa.gov; Hansen, Dave
> <dave.hansen@intel.com>; Dock, Deneen T <deneen.t.dock@intel.com>;
> kristen at linux.intel.com; Arjan van de Ven <arjan@linux.intel.com>
> Subject: Re: [PATCH v5 2/5] Smack: Prepare for PTRACE_MODE_SCHED
>
> On Wed, Sep 26, 2018 at 10:35 PM Casey Schaufler
> <casey.schaufler@intel.com> 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 <casey.schaufler@intel.com>
> > ---
> > 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.
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [PATCH v5 2/5] Smack: Prepare for PTRACE_MODE_SCHED
2018-09-26 22:53 ` Schaufler, Casey
@ 2018-09-26 22:58 ` Jann Horn
-1 siblings, 0 replies; 55+ messages in thread
From: Jann Horn @ 2018-09-26 22:58 UTC (permalink / raw)
To: Casey Schaufler, Jiri Kosina
Cc: Kernel Hardening, kernel list, linux-security-module, selinux,
Dave Hansen, deneen.t.dock, kristen, Arjan van de Ven
+Jiri
On Thu, Sep 27, 2018 at 12:54 AM Schaufler, Casey
<casey.schaufler@intel.com> wrote:
> > -----Original Message-----
> > From: Jann Horn [mailto:jannh@google.com]
> > Sent: Wednesday, September 26, 2018 2:31 PM
> > To: Schaufler, Casey <casey.schaufler@intel.com>
> > Cc: Kernel Hardening <kernel-hardening@lists.openwall.com>; kernel list
> > <linux-kernel@vger.kernel.org>; linux-security-module <linux-security-
> > module@vger.kernel.org>; selinux@tycho.nsa.gov; Hansen, Dave
> > <dave.hansen@intel.com>; Dock, Deneen T <deneen.t.dock@intel.com>;
> > kristen@linux.intel.com; Arjan van de Ven <arjan@linux.intel.com>
> > Subject: Re: [PATCH v5 2/5] Smack: Prepare for PTRACE_MODE_SCHED
> >
> > On Wed, Sep 26, 2018 at 10:35 PM Casey Schaufler
> > <casey.schaufler@intel.com> 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 <casey.schaufler@intel.com>
> > > ---
> > > 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.
Jiri, was there a good reason for it, and if so, what was it?
^ permalink raw reply [flat|nested] 55+ messages in thread* [PATCH v5 2/5] Smack: Prepare for PTRACE_MODE_SCHED
@ 2018-09-26 22:58 ` Jann Horn
0 siblings, 0 replies; 55+ messages in thread
From: Jann Horn @ 2018-09-26 22:58 UTC (permalink / raw)
To: linux-security-module
+Jiri
On Thu, Sep 27, 2018 at 12:54 AM Schaufler, Casey
<casey.schaufler@intel.com> wrote:
> > -----Original Message-----
> > From: Jann Horn [mailto:jannh at google.com]
> > Sent: Wednesday, September 26, 2018 2:31 PM
> > To: Schaufler, Casey <casey.schaufler@intel.com>
> > Cc: Kernel Hardening <kernel-hardening@lists.openwall.com>; kernel list
> > <linux-kernel@vger.kernel.org>; linux-security-module <linux-security-
> > module at vger.kernel.org>; selinux at tycho.nsa.gov; Hansen, Dave
> > <dave.hansen@intel.com>; Dock, Deneen T <deneen.t.dock@intel.com>;
> > kristen at linux.intel.com; Arjan van de Ven <arjan@linux.intel.com>
> > Subject: Re: [PATCH v5 2/5] Smack: Prepare for PTRACE_MODE_SCHED
> >
> > On Wed, Sep 26, 2018 at 10:35 PM Casey Schaufler
> > <casey.schaufler@intel.com> 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 <casey.schaufler@intel.com>
> > > ---
> > > 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.
Jiri, was there a good reason for it, and if so, what was it?
^ permalink raw reply [flat|nested] 55+ messages in thread* Re: [PATCH v5 2/5] Smack: Prepare for PTRACE_MODE_SCHED
2018-09-26 22:58 ` Jann Horn
(?)
@ 2018-10-04 7:47 ` Jiri Kosina
2018-10-04 11:36 ` Jann Horn
-1 siblings, 1 reply; 55+ messages in thread
From: Jiri Kosina @ 2018-10-04 7:47 UTC (permalink / raw)
To: Jann Horn
Cc: Casey Schaufler, Kernel Hardening, kernel list,
linux-security-module, selinux, Dave Hansen, deneen.t.dock,
kristen, Arjan van de Ven
On Thu, 27 Sep 2018, Jann Horn wrote:
> > 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.
>
> Jiri, was there a good reason for it, and if so, what was it?
[ FWIW PTRACE_MODE_NOAUDIT being in PTRACE_MODE_IBPB goes back to original
Tim's pre-CRD patchset ]
Well, we can't really call out into audit from scheduler code, and the
previous versions of the patchsets didn't have PTRACE_MODE_SCHED, so it
had to be included in PTRACE_MODE_IBPB in order to make sure we're not
calling into audit from context switch code.
Or did I misunderstand the question?
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v5 2/5] Smack: Prepare for PTRACE_MODE_SCHED
2018-10-04 7:47 ` Jiri Kosina
@ 2018-10-04 11:36 ` Jann Horn
2018-10-16 11:44 ` Jiri Kosina
0 siblings, 1 reply; 55+ messages in thread
From: Jann Horn @ 2018-10-04 11:36 UTC (permalink / raw)
To: Jiri Kosina, Casey Schaufler
Cc: Kernel Hardening, kernel list, linux-security-module, selinux,
Dave Hansen, deneen.t.dock, kristen, Arjan van de Ven
On Thu, Oct 4, 2018 at 9:47 AM Jiri Kosina <jikos@kernel.org> wrote:
> On Thu, 27 Sep 2018, Jann Horn wrote:
> > > 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.
> >
> > Jiri, was there a good reason for it, and if so, what was it?
>
> [ FWIW PTRACE_MODE_NOAUDIT being in PTRACE_MODE_IBPB goes back to original
> Tim's pre-CRD patchset ]
>
> Well, we can't really call out into audit from scheduler code, and the
> previous versions of the patchsets didn't have PTRACE_MODE_SCHED, so it
> had to be included in PTRACE_MODE_IBPB in order to make sure we're not
> calling into audit from context switch code.
>
> Or did I misunderstand the question?
If I understand Casey correctly, he is saying that your patch
(https://lore.kernel.org/lkml/nycvar.YFH.7.76.1809251437340.15880@cbobk.fhfr.pm/)
doesn't include PTRACE_MODE_NOAUDIT for IBPB, but the previous v6 of
your patch (https://lore.kernel.org/lkml/nycvar.YFH.7.76.1809121105330.15880@cbobk.fhfr.pm/)
did include it, and therefore Casey thinks that there is a specific
reason why you removed PTRACE_MODE_NOAUDIT, and therefore Casey is
adding special-case logic for PTRACE_MODE_SCHED to Smack when simply
using PTRACE_MODE_NOAUDIT would also work.
I think that Casey should change ptrace_may_access_sched() to use
"mode | PTRACE_MODE_SCHED | PTRACE_MODE_NOAUDIT".
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v5 2/5] Smack: Prepare for PTRACE_MODE_SCHED
2018-10-04 11:36 ` Jann Horn
@ 2018-10-16 11:44 ` Jiri Kosina
0 siblings, 0 replies; 55+ messages in thread
From: Jiri Kosina @ 2018-10-16 11:44 UTC (permalink / raw)
To: Jann Horn
Cc: Casey Schaufler, Kernel Hardening, kernel list,
linux-security-module, selinux, Dave Hansen, deneen.t.dock,
kristen, Arjan van de Ven
On Thu, 4 Oct 2018, Jann Horn wrote:
> > Well, we can't really call out into audit from scheduler code, and the
> > previous versions of the patchsets didn't have PTRACE_MODE_SCHED, so it
> > had to be included in PTRACE_MODE_IBPB in order to make sure we're not
> > calling into audit from context switch code.
> >
> > Or did I misunderstand the question?
>
> If I understand Casey correctly, he is saying that your patch
> (https://lore.kernel.org/lkml/nycvar.YFH.7.76.1809251437340.15880@cbobk.fhfr.pm/)
> doesn't include PTRACE_MODE_NOAUDIT for IBPB, but the previous v6 of
> your patch (https://lore.kernel.org/lkml/nycvar.YFH.7.76.1809121105330.15880@cbobk.fhfr.pm/)
> did include it, and therefore Casey thinks that there is a specific
> reason why you removed PTRACE_MODE_NOAUDIT,
Quite honestly, I don't remember. I dont't think there is any deadlock
that'd be triggered by this.
> and therefore Casey is adding special-case logic for PTRACE_MODE_SCHED
> to Smack when simply using PTRACE_MODE_NOAUDIT would also work.
>
> I think that Casey should change ptrace_may_access_sched() to use
> "mode | PTRACE_MODE_SCHED | PTRACE_MODE_NOAUDIT".
Agreed, that should work.
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH v5 3/5] SELinux: Prepare for PTRACE_MODE_SCHED
2018-09-26 20:34 ` Casey Schaufler
@ 2018-09-26 20:34 ` Casey Schaufler
-1 siblings, 0 replies; 55+ messages in thread
From: Casey Schaufler @ 2018-09-26 20:34 UTC (permalink / raw)
To: kernel-hardening, linux-kernel, linux-security-module, selinux,
casey.schaufler, dave.hansen, deneen.t.dock, kristen, arjan
From: Casey Schaufler <casey@schaufler-ca.com>
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.
Signed-off-by: Casey Schaufler <casey.schaufler@intel.com>
---
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;
if (mode & PTRACE_MODE_READ)
return avc_has_perm(&selinux_state,
sid, csid, SECCLASS_FILE, FILE__READ, NULL);
--
2.17.1
^ permalink raw reply related [flat|nested] 55+ messages in thread
* [PATCH v5 3/5] SELinux: Prepare for PTRACE_MODE_SCHED
@ 2018-09-26 20:34 ` Casey Schaufler
0 siblings, 0 replies; 55+ messages in thread
From: Casey Schaufler @ 2018-09-26 20:34 UTC (permalink / raw)
To: linux-security-module
From: Casey Schaufler <casey@schaufler-ca.com>
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.
Signed-off-by: Casey Schaufler <casey.schaufler@intel.com>
---
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;
if (mode & PTRACE_MODE_READ)
return avc_has_perm(&selinux_state,
sid, csid, SECCLASS_FILE, FILE__READ, NULL);
--
2.17.1
^ permalink raw reply related [flat|nested] 55+ messages in thread
* Re: [PATCH v5 3/5] SELinux: Prepare for PTRACE_MODE_SCHED
2018-09-26 20:34 ` Casey Schaufler
(?)
@ 2018-09-27 1:53 ` Stephen Smalley
-1 siblings, 0 replies; 55+ messages in thread
From: Stephen Smalley @ 2018-09-27 1:53 UTC (permalink / raw)
To: Schaufler, Casey
Cc: kernel-hardening, Linux Kernel, linux-security-module, selinux,
dave.hansen, deneen.t.dock, kristen, arjan
[-- Attachment #1: Type: text/plain, Size: 1373 bytes --]
On Wed, Sep 26, 2018, 4:35 PM Casey Schaufler <casey.schaufler@intel.com>
wrote:
> From: Casey Schaufler <casey@schaufler-ca.com>
>
> 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.
>
We already have a flag to disable audit. What locking conflict is presented
by the avc, which uses rcu?
> Signed-off-by: Casey Schaufler <casey.schaufler@intel.com>
> ---
> 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;
> if (mode & PTRACE_MODE_READ)
> return avc_has_perm(&selinux_state,
> sid, csid, SECCLASS_FILE, FILE__READ,
> NULL);
> --
> 2.17.1
>
>
[-- Attachment #2: Type: text/html, Size: 2193 bytes --]
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v5 3/5] SELinux: Prepare for PTRACE_MODE_SCHED
2018-09-26 20:34 ` Casey Schaufler
@ 2018-09-27 15:50 ` Stephen Smalley
-1 siblings, 0 replies; 55+ messages in thread
From: Stephen Smalley @ 2018-09-27 15:50 UTC (permalink / raw)
To: Casey Schaufler, kernel-hardening, linux-kernel,
linux-security-module, selinux, dave.hansen, deneen.t.dock,
kristen, arjan, Paul Moore
On 09/26/2018 04:34 PM, Casey Schaufler wrote:
> From: Casey Schaufler <casey@schaufler-ca.com>
>
> 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?
Does the PTRACE_MODE_SCHED check have to occur while holding the
scheduler lock, or could it be performed before taking the lock?
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.15880@cbobk.fhfr.pm/
>
> Signed-off-by: Casey Schaufler <casey.schaufler@intel.com>
> ---
> 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);
>
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH v5 3/5] SELinux: Prepare for PTRACE_MODE_SCHED
@ 2018-09-27 15:50 ` Stephen Smalley
0 siblings, 0 replies; 55+ messages in thread
From: Stephen Smalley @ 2018-09-27 15:50 UTC (permalink / raw)
To: linux-security-module
On 09/26/2018 04:34 PM, Casey Schaufler wrote:
> From: Casey Schaufler <casey@schaufler-ca.com>
>
> 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?
Does the PTRACE_MODE_SCHED check have to occur while holding the
scheduler lock, or could it be performed before taking the lock?
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.15880 at cbobk.fhfr.pm/
>
> Signed-off-by: Casey Schaufler <casey.schaufler@intel.com>
> ---
> 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);
>
^ permalink raw reply [flat|nested] 55+ messages in thread
* RE: [PATCH v5 3/5] SELinux: Prepare for PTRACE_MODE_SCHED
2018-09-27 15:50 ` Stephen Smalley
(?)
@ 2018-09-27 16:23 ` Schaufler, Casey
-1 siblings, 0 replies; 55+ messages in thread
From: Schaufler, Casey @ 2018-09-27 16:23 UTC (permalink / raw)
To: Stephen Smalley, kernel-hardening, linux-kernel,
linux-security-module, selinux, Hansen, Dave, Dock, Deneen T,
kristen, arjan, Paul Moore, Schaufler, Casey
> -----Original Message-----
> From: Stephen Smalley [mailto:sds@tycho.nsa.gov]
> Sent: Thursday, September 27, 2018 8:50 AM
> To: Schaufler, Casey <casey.schaufler@intel.com>; kernel-
> hardening@lists.openwall.com; linux-kernel@vger.kernel.org; linux-security-
> module@vger.kernel.org; selinux@tycho.nsa.gov; Hansen, Dave
> <dave.hansen@intel.com>; Dock, Deneen T <deneen.t.dock@intel.com>;
> kristen@linux.intel.com; arjan@linux.intel.com; Paul Moore <paul@paul-
> 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 <casey@schaufler-ca.com>
> >
> > 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 <peterz@infradead.org> 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.15880@cbobk.fhf
> r.pm/
Yes, that's the one. Sorry, I should have identified that.
>
> >
> > Signed-off-by: Casey Schaufler <casey.schaufler@intel.com>
> > ---
> > 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);
> >
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH v5 3/5] SELinux: Prepare for PTRACE_MODE_SCHED
@ 2018-09-27 16:23 ` Schaufler, Casey
0 siblings, 0 replies; 55+ messages in thread
From: Schaufler, Casey @ 2018-09-27 16:23 UTC (permalink / raw)
To: linux-security-module
> -----Original Message-----
> From: Stephen Smalley [mailto:sds at tycho.nsa.gov]
> Sent: Thursday, September 27, 2018 8:50 AM
> To: Schaufler, Casey <casey.schaufler@intel.com>; kernel-
> hardening at lists.openwall.com; linux-kernel at vger.kernel.org; linux-security-
> module at vger.kernel.org; selinux at tycho.nsa.gov; Hansen, Dave
> <dave.hansen@intel.com>; Dock, Deneen T <deneen.t.dock@intel.com>;
> kristen at linux.intel.com; arjan at linux.intel.com; Paul Moore <paul@paul-
> 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 <casey@schaufler-ca.com>
> >
> > 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 <peterz@infradead.org> 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.15880 at cbobk.fhf
> r.pm/
Yes, that's the one. Sorry, I should have identified that.
>
> >
> > Signed-off-by: Casey Schaufler <casey.schaufler@intel.com>
> > ---
> > 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);
> >
^ permalink raw reply [flat|nested] 55+ messages in thread
* RE: [PATCH v5 3/5] SELinux: Prepare for PTRACE_MODE_SCHED
@ 2018-09-27 16:23 ` Schaufler, Casey
0 siblings, 0 replies; 55+ messages in thread
From: Schaufler, Casey @ 2018-09-27 16:23 UTC (permalink / raw)
To: Stephen Smalley, kernel-hardening, linux-kernel,
linux-security-module, selinux, Hansen, Dave, Dock, Deneen T,
kristen, arjan, Paul Moore, Schaufler, Casey
> -----Original Message-----
> From: Stephen Smalley [mailto:sds@tycho.nsa.gov]
> Sent: Thursday, September 27, 2018 8:50 AM
> To: Schaufler, Casey <casey.schaufler@intel.com>; kernel-
> hardening@lists.openwall.com; linux-kernel@vger.kernel.org; linux-security-
> module@vger.kernel.org; selinux@tycho.nsa.gov; Hansen, Dave
> <dave.hansen@intel.com>; Dock, Deneen T <deneen.t.dock@intel.com>;
> kristen@linux.intel.com; arjan@linux.intel.com; Paul Moore <paul@paul-
> 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 <casey@schaufler-ca.com>
> >
> > 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 <peterz@infradead.org> 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.15880@cbobk.fhf
> r.pm/
Yes, that's the one. Sorry, I should have identified that.
>
> >
> > Signed-off-by: Casey Schaufler <casey.schaufler@intel.com>
> > ---
> > 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);
> >
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH v5 4/5] Capability: Complete PTRACE_MODE_SCHED
2018-09-26 20:34 ` Casey Schaufler
@ 2018-09-26 20:34 ` Casey Schaufler
-1 siblings, 0 replies; 55+ messages in thread
From: Casey Schaufler @ 2018-09-26 20:34 UTC (permalink / raw)
To: kernel-hardening, linux-kernel, linux-security-module, selinux,
casey.schaufler, dave.hansen, deneen.t.dock, kristen, arjan
From: Casey Schaufler <casey@schaufler-ca.com>
Allow a complete ptrace access check with mode PTRACE_MODE_SCHED.
Disable the inappropriate privilege check in the capability code
that does incompatible locking.
Signed-off-by: Casey Schaufler <casey.schaufler@intel.com>
---
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;
if (ns_capable(child_cred->user_ns, CAP_SYS_PTRACE))
goto out;
ret = -EPERM;
--
2.17.1
^ permalink raw reply related [flat|nested] 55+ messages in thread
* [PATCH v5 4/5] Capability: Complete PTRACE_MODE_SCHED
@ 2018-09-26 20:34 ` Casey Schaufler
0 siblings, 0 replies; 55+ messages in thread
From: Casey Schaufler @ 2018-09-26 20:34 UTC (permalink / raw)
To: linux-security-module
From: Casey Schaufler <casey@schaufler-ca.com>
Allow a complete ptrace access check with mode PTRACE_MODE_SCHED.
Disable the inappropriate privilege check in the capability code
that does incompatible locking.
Signed-off-by: Casey Schaufler <casey.schaufler@intel.com>
---
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;
if (ns_capable(child_cred->user_ns, CAP_SYS_PTRACE))
goto out;
ret = -EPERM;
--
2.17.1
^ permalink raw reply related [flat|nested] 55+ messages in thread
* Re: [PATCH v5 4/5] Capability: Complete PTRACE_MODE_SCHED
2018-09-26 20:34 ` Casey Schaufler
@ 2018-09-26 21:26 ` Jann Horn
-1 siblings, 0 replies; 55+ messages in thread
From: Jann Horn @ 2018-09-26 21:26 UTC (permalink / raw)
To: Casey Schaufler
Cc: Kernel Hardening, kernel list, linux-security-module, selinux,
Dave Hansen, deneen.t.dock, kristen, Arjan van de Ven
On Wed, Sep 26, 2018 at 10:35 PM Casey Schaufler
<casey.schaufler@intel.com> 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?
> Signed-off-by: Casey Schaufler <casey.schaufler@intel.com>
> ---
> 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? 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;
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH v5 4/5] Capability: Complete PTRACE_MODE_SCHED
@ 2018-09-26 21:26 ` Jann Horn
0 siblings, 0 replies; 55+ messages in thread
From: Jann Horn @ 2018-09-26 21:26 UTC (permalink / raw)
To: linux-security-module
On Wed, Sep 26, 2018 at 10:35 PM Casey Schaufler
<casey.schaufler@intel.com> 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?
> Signed-off-by: Casey Schaufler <casey.schaufler@intel.com>
> ---
> 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? 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;
^ permalink raw reply [flat|nested] 55+ messages in thread
* RE: [PATCH v5 4/5] Capability: Complete PTRACE_MODE_SCHED
2018-09-26 21:26 ` Jann Horn
@ 2018-09-26 22:24 ` Schaufler, Casey
-1 siblings, 0 replies; 55+ messages in thread
From: Schaufler, Casey @ 2018-09-26 22:24 UTC (permalink / raw)
To: Jann Horn
Cc: Kernel Hardening, kernel list, linux-security-module, selinux,
Hansen, Dave, Dock, Deneen T, kristen, Arjan van de Ven
> -----Original Message-----
> From: Jann Horn [mailto:jannh@google.com]
> Sent: Wednesday, September 26, 2018 2:26 PM
> To: Schaufler, Casey <casey.schaufler@intel.com>
> Cc: Kernel Hardening <kernel-hardening@lists.openwall.com>; kernel list
> <linux-kernel@vger.kernel.org>; linux-security-module <linux-security-
> module@vger.kernel.org>; selinux@tycho.nsa.gov; Hansen, Dave
> <dave.hansen@intel.com>; Dock, Deneen T <deneen.t.dock@intel.com>;
> kristen@linux.intel.com; Arjan van de Ven <arjan@linux.intel.com>
> Subject: Re: [PATCH v5 4/5] Capability: Complete PTRACE_MODE_SCHED
>
> On Wed, Sep 26, 2018 at 10:35 PM Casey Schaufler
> <casey.schaufler@intel.com> 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 <casey.schaufler@intel.com>
> > ---
> > 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;
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH v5 4/5] Capability: Complete PTRACE_MODE_SCHED
@ 2018-09-26 22:24 ` Schaufler, Casey
0 siblings, 0 replies; 55+ messages in thread
From: Schaufler, Casey @ 2018-09-26 22:24 UTC (permalink / raw)
To: linux-security-module
> -----Original Message-----
> From: Jann Horn [mailto:jannh at google.com]
> Sent: Wednesday, September 26, 2018 2:26 PM
> To: Schaufler, Casey <casey.schaufler@intel.com>
> Cc: Kernel Hardening <kernel-hardening@lists.openwall.com>; kernel list
> <linux-kernel@vger.kernel.org>; linux-security-module <linux-security-
> module at vger.kernel.org>; selinux at tycho.nsa.gov; Hansen, Dave
> <dave.hansen@intel.com>; Dock, Deneen T <deneen.t.dock@intel.com>;
> kristen at linux.intel.com; Arjan van de Ven <arjan@linux.intel.com>
> Subject: Re: [PATCH v5 4/5] Capability: Complete PTRACE_MODE_SCHED
>
> On Wed, Sep 26, 2018 at 10:35 PM Casey Schaufler
> <casey.schaufler@intel.com> 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 <casey.schaufler@intel.com>
> > ---
> > 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;
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH v5 5/5] sidechannel: Linux Security Module for sidechannel
2018-09-26 20:34 ` Casey Schaufler
@ 2018-09-26 20:34 ` Casey Schaufler
-1 siblings, 0 replies; 55+ messages in thread
From: Casey Schaufler @ 2018-09-26 20:34 UTC (permalink / raw)
To: kernel-hardening, linux-kernel, linux-security-module, selinux,
casey.schaufler, dave.hansen, deneen.t.dock, kristen, arjan
From: Casey Schaufler <casey@schaufler-ca.com>
This is a new Linux Security Module (LSM) that checks for
potential sidechannel issues that are not covered in the
ptrace PTRACE_MODE_SCHED option. Namespace differences are
checked in this intitial version. Additional checks should
be added when they are determined to be useful.
Signed-off-by: Casey Schaufler <casey.schaufler@intel.com>
---
include/linux/lsm_hooks.h | 5 ++
security/Kconfig | 1 +
security/Makefile | 2 +
security/security.c | 1 +
security/sidechannel/Kconfig | 13 +++++
security/sidechannel/Makefile | 1 +
security/sidechannel/sidechannel.c | 88 ++++++++++++++++++++++++++++++
7 files changed, 111 insertions(+)
create mode 100644 security/sidechannel/Kconfig
create mode 100644 security/sidechannel/Makefile
create mode 100644 security/sidechannel/sidechannel.c
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 97a020c616ad..3cb6516dba3c 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -2081,5 +2081,10 @@ void __init loadpin_add_hooks(void);
#else
static inline void loadpin_add_hooks(void) { };
#endif
+#ifdef CONFIG_SECURITY_SIDECHANNEL
+void __init sidechannel_add_hooks(void);
+#else
+static inline void sidechannel_add_hooks(void) { };
+#endif
#endif /* ! __LINUX_LSM_HOOKS_H */
diff --git a/security/Kconfig b/security/Kconfig
index d9aa521b5206..6b814a3f93ea 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -236,6 +236,7 @@ source security/tomoyo/Kconfig
source security/apparmor/Kconfig
source security/loadpin/Kconfig
source security/yama/Kconfig
+source security/sidechannel/Kconfig
source security/integrity/Kconfig
diff --git a/security/Makefile b/security/Makefile
index 4d2d3782ddef..d0c9e1b227f9 100644
--- a/security/Makefile
+++ b/security/Makefile
@@ -10,6 +10,7 @@ subdir-$(CONFIG_SECURITY_TOMOYO) += tomoyo
subdir-$(CONFIG_SECURITY_APPARMOR) += apparmor
subdir-$(CONFIG_SECURITY_YAMA) += yama
subdir-$(CONFIG_SECURITY_LOADPIN) += loadpin
+subdir-$(CONFIG_SECURITY_SIDECHANNEL) += sidechannel
# always enable default capabilities
obj-y += commoncap.o
@@ -25,6 +26,7 @@ obj-$(CONFIG_SECURITY_TOMOYO) += tomoyo/
obj-$(CONFIG_SECURITY_APPARMOR) += apparmor/
obj-$(CONFIG_SECURITY_YAMA) += yama/
obj-$(CONFIG_SECURITY_LOADPIN) += loadpin/
+obj-$(CONFIG_SECURITY_SIDECHANNEL) += sidechannel/
obj-$(CONFIG_CGROUP_DEVICE) += device_cgroup.o
# Object integrity file lists
diff --git a/security/security.c b/security/security.c
index 736e78da1ab9..2129b0e31d7b 100644
--- a/security/security.c
+++ b/security/security.c
@@ -83,6 +83,7 @@ int __init security_init(void)
capability_add_hooks();
yama_add_hooks();
loadpin_add_hooks();
+ sidechannel_add_hooks();
/*
* Load all the remaining security modules.
diff --git a/security/sidechannel/Kconfig b/security/sidechannel/Kconfig
new file mode 100644
index 000000000000..653033027415
--- /dev/null
+++ b/security/sidechannel/Kconfig
@@ -0,0 +1,13 @@
+config SECURITY_SIDECHANNEL
+ bool "Sidechannel attack safety extra checks"
+ depends on SECURITY
+ default n
+ help
+ Look for a variety of cases where a side-channel attack
+ could potentially be exploited. Instruct the switching
+ code to use the indirect_branch_prediction_barrier in
+ cases where the passed task and the current task may be
+ at risk.
+
+ If you are unsure how to answer this question, answer N.
+
diff --git a/security/sidechannel/Makefile b/security/sidechannel/Makefile
new file mode 100644
index 000000000000..f61d83f28035
--- /dev/null
+++ b/security/sidechannel/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_SECURITY_SIDECHANNEL) += sidechannel.o
diff --git a/security/sidechannel/sidechannel.c b/security/sidechannel/sidechannel.c
new file mode 100644
index 000000000000..18a67d19c020
--- /dev/null
+++ b/security/sidechannel/sidechannel.c
@@ -0,0 +1,88 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Side Channel Safety Security Module
+ *
+ * Copyright (C) 2018 Intel Corporation.
+ *
+ */
+
+#define pr_fmt(fmt) "SideChannel: " fmt
+
+#include <linux/types.h>
+#include <linux/lsm_hooks.h>
+#include <linux/capability.h>
+#include <linux/cred.h>
+#include <linux/sched.h>
+#include <linux/string_helpers.h>
+#include <linux/nsproxy.h>
+#include <linux/pid_namespace.h>
+#include <linux/ptrace.h>
+
+#ifdef CONFIG_NAMESPACES
+/**
+ * safe_by_namespace - Are task and current sidechannel safe?
+ * @p: task to check on
+ *
+ * Returns 0 if the tasks are sidechannel safe, -EACCES otherwise.
+ */
+static int safe_by_namespace(struct task_struct *p)
+{
+ struct cgroup_namespace *ccgn = NULL;
+ struct cgroup_namespace *pcgn = NULL;
+
+ /*
+ * 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;
+}
+#else
+static int safe_by_namespace(struct task_struct *p)
+{
+ return 0;
+}
+#endif
+
+/**
+ * sidechannel_ptrace_access_check - Are task and current sidechannel safe?
+ * @p: task to check on
+ * @mode: ptrace access mode
+ *
+ * Returns 0 if the tasks are sidechannel safe, -EACCES otherwise.
+ */
+static int sidechannel_ptrace_access_check(struct task_struct *p,
+ unsigned int mode)
+{
+ int rc;
+
+ if ((mode & PTRACE_MODE_SCHED) == 0)
+ return 0;
+
+ rc = safe_by_namespace(p);
+ if (rc)
+ return rc;
+ return 0;
+}
+
+static struct security_hook_list sidechannel_hooks[] __lsm_ro_after_init = {
+ LSM_HOOK_INIT(ptrace_access_check, sidechannel_ptrace_access_check),
+};
+
+void __init sidechannel_add_hooks(void)
+{
+ pr_info("Extra sidechannel checks enabled\n");
+ security_add_hooks(sidechannel_hooks, ARRAY_SIZE(sidechannel_hooks),
+ "sidechannel");
+}
--
2.17.1
^ permalink raw reply related [flat|nested] 55+ messages in thread* [PATCH v5 5/5] sidechannel: Linux Security Module for sidechannel
@ 2018-09-26 20:34 ` Casey Schaufler
0 siblings, 0 replies; 55+ messages in thread
From: Casey Schaufler @ 2018-09-26 20:34 UTC (permalink / raw)
To: linux-security-module
From: Casey Schaufler <casey@schaufler-ca.com>
This is a new Linux Security Module (LSM) that checks for
potential sidechannel issues that are not covered in the
ptrace PTRACE_MODE_SCHED option. Namespace differences are
checked in this intitial version. Additional checks should
be added when they are determined to be useful.
Signed-off-by: Casey Schaufler <casey.schaufler@intel.com>
---
include/linux/lsm_hooks.h | 5 ++
security/Kconfig | 1 +
security/Makefile | 2 +
security/security.c | 1 +
security/sidechannel/Kconfig | 13 +++++
security/sidechannel/Makefile | 1 +
security/sidechannel/sidechannel.c | 88 ++++++++++++++++++++++++++++++
7 files changed, 111 insertions(+)
create mode 100644 security/sidechannel/Kconfig
create mode 100644 security/sidechannel/Makefile
create mode 100644 security/sidechannel/sidechannel.c
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 97a020c616ad..3cb6516dba3c 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -2081,5 +2081,10 @@ void __init loadpin_add_hooks(void);
#else
static inline void loadpin_add_hooks(void) { };
#endif
+#ifdef CONFIG_SECURITY_SIDECHANNEL
+void __init sidechannel_add_hooks(void);
+#else
+static inline void sidechannel_add_hooks(void) { };
+#endif
#endif /* ! __LINUX_LSM_HOOKS_H */
diff --git a/security/Kconfig b/security/Kconfig
index d9aa521b5206..6b814a3f93ea 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -236,6 +236,7 @@ source security/tomoyo/Kconfig
source security/apparmor/Kconfig
source security/loadpin/Kconfig
source security/yama/Kconfig
+source security/sidechannel/Kconfig
source security/integrity/Kconfig
diff --git a/security/Makefile b/security/Makefile
index 4d2d3782ddef..d0c9e1b227f9 100644
--- a/security/Makefile
+++ b/security/Makefile
@@ -10,6 +10,7 @@ subdir-$(CONFIG_SECURITY_TOMOYO) += tomoyo
subdir-$(CONFIG_SECURITY_APPARMOR) += apparmor
subdir-$(CONFIG_SECURITY_YAMA) += yama
subdir-$(CONFIG_SECURITY_LOADPIN) += loadpin
+subdir-$(CONFIG_SECURITY_SIDECHANNEL) += sidechannel
# always enable default capabilities
obj-y += commoncap.o
@@ -25,6 +26,7 @@ obj-$(CONFIG_SECURITY_TOMOYO) += tomoyo/
obj-$(CONFIG_SECURITY_APPARMOR) += apparmor/
obj-$(CONFIG_SECURITY_YAMA) += yama/
obj-$(CONFIG_SECURITY_LOADPIN) += loadpin/
+obj-$(CONFIG_SECURITY_SIDECHANNEL) += sidechannel/
obj-$(CONFIG_CGROUP_DEVICE) += device_cgroup.o
# Object integrity file lists
diff --git a/security/security.c b/security/security.c
index 736e78da1ab9..2129b0e31d7b 100644
--- a/security/security.c
+++ b/security/security.c
@@ -83,6 +83,7 @@ int __init security_init(void)
capability_add_hooks();
yama_add_hooks();
loadpin_add_hooks();
+ sidechannel_add_hooks();
/*
* Load all the remaining security modules.
diff --git a/security/sidechannel/Kconfig b/security/sidechannel/Kconfig
new file mode 100644
index 000000000000..653033027415
--- /dev/null
+++ b/security/sidechannel/Kconfig
@@ -0,0 +1,13 @@
+config SECURITY_SIDECHANNEL
+ bool "Sidechannel attack safety extra checks"
+ depends on SECURITY
+ default n
+ help
+ Look for a variety of cases where a side-channel attack
+ could potentially be exploited. Instruct the switching
+ code to use the indirect_branch_prediction_barrier in
+ cases where the passed task and the current task may be
+ at risk.
+
+ If you are unsure how to answer this question, answer N.
+
diff --git a/security/sidechannel/Makefile b/security/sidechannel/Makefile
new file mode 100644
index 000000000000..f61d83f28035
--- /dev/null
+++ b/security/sidechannel/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_SECURITY_SIDECHANNEL) += sidechannel.o
diff --git a/security/sidechannel/sidechannel.c b/security/sidechannel/sidechannel.c
new file mode 100644
index 000000000000..18a67d19c020
--- /dev/null
+++ b/security/sidechannel/sidechannel.c
@@ -0,0 +1,88 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Side Channel Safety Security Module
+ *
+ * Copyright (C) 2018 Intel Corporation.
+ *
+ */
+
+#define pr_fmt(fmt) "SideChannel: " fmt
+
+#include <linux/types.h>
+#include <linux/lsm_hooks.h>
+#include <linux/capability.h>
+#include <linux/cred.h>
+#include <linux/sched.h>
+#include <linux/string_helpers.h>
+#include <linux/nsproxy.h>
+#include <linux/pid_namespace.h>
+#include <linux/ptrace.h>
+
+#ifdef CONFIG_NAMESPACES
+/**
+ * safe_by_namespace - Are task and current sidechannel safe?
+ * @p: task to check on
+ *
+ * Returns 0 if the tasks are sidechannel safe, -EACCES otherwise.
+ */
+static int safe_by_namespace(struct task_struct *p)
+{
+ struct cgroup_namespace *ccgn = NULL;
+ struct cgroup_namespace *pcgn = NULL;
+
+ /*
+ * 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;
+}
+#else
+static int safe_by_namespace(struct task_struct *p)
+{
+ return 0;
+}
+#endif
+
+/**
+ * sidechannel_ptrace_access_check - Are task and current sidechannel safe?
+ * @p: task to check on
+ * @mode: ptrace access mode
+ *
+ * Returns 0 if the tasks are sidechannel safe, -EACCES otherwise.
+ */
+static int sidechannel_ptrace_access_check(struct task_struct *p,
+ unsigned int mode)
+{
+ int rc;
+
+ if ((mode & PTRACE_MODE_SCHED) == 0)
+ return 0;
+
+ rc = safe_by_namespace(p);
+ if (rc)
+ return rc;
+ return 0;
+}
+
+static struct security_hook_list sidechannel_hooks[] __lsm_ro_after_init = {
+ LSM_HOOK_INIT(ptrace_access_check, sidechannel_ptrace_access_check),
+};
+
+void __init sidechannel_add_hooks(void)
+{
+ pr_info("Extra sidechannel checks enabled\n");
+ security_add_hooks(sidechannel_hooks, ARRAY_SIZE(sidechannel_hooks),
+ "sidechannel");
+}
--
2.17.1
^ permalink raw reply related [flat|nested] 55+ messages in thread* Re: [PATCH v5 5/5] sidechannel: Linux Security Module for sidechannel
2018-09-26 20:34 ` Casey Schaufler
@ 2018-09-27 21:45 ` James Morris
-1 siblings, 0 replies; 55+ messages in thread
From: James Morris @ 2018-09-27 21:45 UTC (permalink / raw)
To: Casey Schaufler
Cc: kernel-hardening, linux-kernel, linux-security-module, selinux,
dave.hansen, deneen.t.dock, kristen, arjan
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.
It least make it user configurable.
--
James Morris
<jmorris@namei.org>
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH v5 5/5] sidechannel: Linux Security Module for sidechannel
@ 2018-09-27 21:45 ` James Morris
0 siblings, 0 replies; 55+ messages in thread
From: James Morris @ 2018-09-27 21:45 UTC (permalink / raw)
To: linux-security-module
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.
It least make it user configurable.
--
James Morris
<jmorris@namei.org>
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v5 5/5] sidechannel: Linux Security Module for sidechannel
2018-09-27 21:45 ` James Morris
@ 2018-09-27 22:39 ` Casey Schaufler
-1 siblings, 0 replies; 55+ messages in thread
From: Casey Schaufler @ 2018-09-27 22:39 UTC (permalink / raw)
To: James Morris, Casey Schaufler
Cc: kristen, kernel-hardening, deneen.t.dock, linux-kernel,
dave.hansen, linux-security-module, selinux, arjan
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. I think
these are the most sensible, but I'm willing to be educated.
I was also requested to check on potential issues between containers,
but as there is no kernel concept of containers this is the
best I see we can do.
> It least make it user configurable.
Would you have a suggested granularity? I could have a
configuration option for each of cgroups, user and pid
namespaces but that's getting to be a lot of knobs to
twist.
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH v5 5/5] sidechannel: Linux Security Module for sidechannel
@ 2018-09-27 22:39 ` Casey Schaufler
0 siblings, 0 replies; 55+ messages in thread
From: Casey Schaufler @ 2018-09-27 22:39 UTC (permalink / raw)
To: linux-security-module
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. I think
these are the most sensible, but I'm willing to be educated.
I was also requested to check on potential issues between containers,
but as there is no kernel concept of containers this is the
best I see we can do.
> It least make it user configurable.
Would you have a suggested granularity? I could have a
configuration option for each of cgroups, user and pid
namespaces but that's getting to be a lot of knobs to
twist.
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v5 5/5] sidechannel: Linux Security Module for sidechannel
2018-09-27 22:39 ` Casey Schaufler
@ 2018-09-27 22:47 ` James Morris
-1 siblings, 0 replies; 55+ messages in thread
From: James Morris @ 2018-09-27 22:47 UTC (permalink / raw)
To: Casey Schaufler
Cc: Casey Schaufler, kristen, kernel-hardening, deneen.t.dock,
linux-kernel, dave.hansen, linux-security-module, selinux, arjan
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?
--
James Morris
<jmorris@namei.org>
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH v5 5/5] sidechannel: Linux Security Module for sidechannel
@ 2018-09-27 22:47 ` James Morris
0 siblings, 0 replies; 55+ messages in thread
From: James Morris @ 2018-09-27 22:47 UTC (permalink / raw)
To: linux-security-module
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?
--
James Morris
<jmorris@namei.org>
^ permalink raw reply [flat|nested] 55+ messages in thread
* RE: [PATCH v5 5/5] sidechannel: Linux Security Module for sidechannel
2018-09-27 22:47 ` James Morris
(?)
@ 2018-09-27 23:19 ` Schaufler, Casey
-1 siblings, 0 replies; 55+ messages in thread
From: Schaufler, Casey @ 2018-09-27 23:19 UTC (permalink / raw)
To: James Morris, Casey Schaufler
Cc: kristen, kernel-hardening, Dock, Deneen T, linux-kernel, Hansen,
Dave, linux-security-module, selinux, arjan
> -----Original Message-----
> From: James Morris [mailto:jmorris@namei.org]
> Sent: Thursday, September 27, 2018 3:47 PM
> To: Casey Schaufler <casey@schaufler-ca.com>
> Cc: Schaufler, Casey <casey.schaufler@intel.com>; kristen@linux.intel.com;
> kernel-hardening@lists.openwall.com; Dock, Deneen T
> <deneen.t.dock@intel.com>; linux-kernel@vger.kernel.org; Hansen, Dave
> <dave.hansen@intel.com>; linux-security-module@vger.kernel.org;
> selinux@tycho.nsa.gov; arjan@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.
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH v5 5/5] sidechannel: Linux Security Module for sidechannel
@ 2018-09-27 23:19 ` Schaufler, Casey
0 siblings, 0 replies; 55+ messages in thread
From: Schaufler, Casey @ 2018-09-27 23:19 UTC (permalink / raw)
To: linux-security-module
> -----Original Message-----
> From: James Morris [mailto:jmorris at namei.org]
> Sent: Thursday, September 27, 2018 3:47 PM
> To: Casey Schaufler <casey@schaufler-ca.com>
> Cc: Schaufler, Casey <casey.schaufler@intel.com>; kristen at linux.intel.com;
> kernel-hardening at lists.openwall.com; Dock, Deneen T
> <deneen.t.dock@intel.com>; linux-kernel at vger.kernel.org; Hansen, Dave
> <dave.hansen@intel.com>; linux-security-module at vger.kernel.org;
> selinux at tycho.nsa.gov; arjan at 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.
^ permalink raw reply [flat|nested] 55+ messages in thread
* RE: [PATCH v5 5/5] sidechannel: Linux Security Module for sidechannel
@ 2018-09-27 23:19 ` Schaufler, Casey
0 siblings, 0 replies; 55+ messages in thread
From: Schaufler, Casey @ 2018-09-27 23:19 UTC (permalink / raw)
To: James Morris, Casey Schaufler
Cc: kristen, kernel-hardening, Dock, Deneen T, linux-kernel, Hansen,
Dave, linux-security-module, selinux, arjan
> -----Original Message-----
> From: James Morris [mailto:jmorris@namei.org]
> Sent: Thursday, September 27, 2018 3:47 PM
> To: Casey Schaufler <casey@schaufler-ca.com>
> Cc: Schaufler, Casey <casey.schaufler@intel.com>; kristen@linux.intel.com;
> kernel-hardening@lists.openwall.com; Dock, Deneen T
> <deneen.t.dock@intel.com>; linux-kernel@vger.kernel.org; Hansen, Dave
> <dave.hansen@intel.com>; linux-security-module@vger.kernel.org;
> selinux@tycho.nsa.gov; arjan@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.
^ permalink raw reply [flat|nested] 55+ messages in thread
* RE: [PATCH v5 5/5] sidechannel: Linux Security Module for sidechannel
2018-09-27 23:19 ` Schaufler, Casey
(?)
@ 2018-09-27 23:43 ` James Morris
-1 siblings, 0 replies; 55+ messages in thread
From: James Morris @ 2018-09-27 23:43 UTC (permalink / raw)
To: Schaufler, Casey
Cc: Casey Schaufler, kristen, kernel-hardening, Dock, Deneen T,
linux-kernel, Hansen, Dave, linux-security-module, selinux,
arjan
On Thu, 27 Sep 2018, Schaufler, Casey 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.
A lot of people will not be using user namespaces due to security
concerns, 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.
--
James Morris
<jmorris@namei.org>
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH v5 5/5] sidechannel: Linux Security Module for sidechannel
@ 2018-09-27 23:43 ` James Morris
0 siblings, 0 replies; 55+ messages in thread
From: James Morris @ 2018-09-27 23:43 UTC (permalink / raw)
To: linux-security-module
On Thu, 27 Sep 2018, Schaufler, Casey 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.
A lot of people will not be using user namespaces due to security
concerns, 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.
--
James Morris
<jmorris@namei.org>
^ permalink raw reply [flat|nested] 55+ messages in thread
* RE: [PATCH v5 5/5] sidechannel: Linux Security Module for sidechannel
@ 2018-09-27 23:43 ` James Morris
0 siblings, 0 replies; 55+ messages in thread
From: James Morris @ 2018-09-27 23:43 UTC (permalink / raw)
To: Schaufler, Casey
Cc: Casey Schaufler, kristen, kernel-hardening, Dock, Deneen T,
linux-kernel, Hansen, Dave, linux-security-module, selinux,
arjan
On Thu, 27 Sep 2018, Schaufler, Casey 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.
A lot of people will not be using user namespaces due to security
concerns, 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.
--
James Morris
<jmorris@namei.org>
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v5 5/5] sidechannel: Linux Security Module for sidechannel
2018-09-27 23:43 ` James Morris
@ 2018-09-27 23:47 ` Jann Horn
-1 siblings, 0 replies; 55+ messages in thread
From: Jann Horn @ 2018-09-27 23:47 UTC (permalink / raw)
To: James Morris
Cc: Casey Schaufler, Casey Schaufler, kristen, Kernel Hardening,
deneen.t.dock, kernel list, Dave Hansen, linux-security-module,
selinux, Arjan van de Ven
On Fri, Sep 28, 2018 at 1:43 AM James Morris <jmorris@namei.org> wrote:
> On Thu, 27 Sep 2018, Schaufler, Casey 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.
>
> A lot of people will not be using user namespaces due to security
> concerns,
Ugh.
> 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().
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH v5 5/5] sidechannel: Linux Security Module for sidechannel
@ 2018-09-27 23:47 ` Jann Horn
0 siblings, 0 replies; 55+ messages in thread
From: Jann Horn @ 2018-09-27 23:47 UTC (permalink / raw)
To: linux-security-module
On Fri, Sep 28, 2018 at 1:43 AM James Morris <jmorris@namei.org> wrote:
> On Thu, 27 Sep 2018, Schaufler, Casey 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.
>
> A lot of people will not be using user namespaces due to security
> concerns,
Ugh.
> 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().
^ permalink raw reply [flat|nested] 55+ messages in thread
* Re: [PATCH v5 5/5] sidechannel: Linux Security Module for sidechannel
2018-09-27 23:47 ` Jann Horn
@ 2018-09-28 16:33 ` James Morris
-1 siblings, 0 replies; 55+ messages in thread
From: James Morris @ 2018-09-28 16:33 UTC (permalink / raw)
To: Jann Horn
Cc: Casey Schaufler, Casey Schaufler, kristen, Kernel Hardening,
deneen.t.dock, kernel list, Dave Hansen, linux-security-module,
selinux, Arjan van de Ven
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, 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.
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.
--
James Morris
<jmorris@namei.org>
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH v5 5/5] sidechannel: Linux Security Module for sidechannel
@ 2018-09-28 16:33 ` James Morris
0 siblings, 0 replies; 55+ messages in thread
From: James Morris @ 2018-09-28 16:33 UTC (permalink / raw)
To: linux-security-module
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, 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.
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.
--
James Morris
<jmorris@namei.org>
^ permalink raw reply [flat|nested] 55+ messages in thread
* RE: [PATCH v5 5/5] sidechannel: Linux Security Module for sidechannel
2018-09-28 16:33 ` James Morris
@ 2018-09-28 17:40 ` Schaufler, Casey
-1 siblings, 0 replies; 55+ messages in thread
From: Schaufler, Casey @ 2018-09-28 17:40 UTC (permalink / raw)
To: James Morris, Jann Horn
Cc: Casey Schaufler, kristen, Kernel Hardening, Dock, Deneen T,
kernel list, Hansen, Dave, linux-security-module, selinux,
Arjan van de Ven
> -----Original Message-----
> From: James Morris [mailto:jmorris@namei.org]
> Sent: Friday, September 28, 2018 9:33 AM
> To: Jann Horn <jannh@google.com>
> Cc: Schaufler, Casey <casey.schaufler@intel.com>; Casey Schaufler
> <casey@schaufler-ca.com>; kristen@linux.intel.com; Kernel Hardening
> <kernel-hardening@lists.openwall.com>; Dock, Deneen T
> <deneen.t.dock@intel.com>; kernel list <linux-kernel@vger.kernel.org>;
> Hansen, Dave <dave.hansen@intel.com>; linux-security-module <linux-security-
> module@vger.kernel.org>; selinux@tycho.nsa.gov; Arjan van de Ven
> <arjan@linux.intel.com>
> 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.
^ permalink raw reply [flat|nested] 55+ messages in thread
* [PATCH v5 5/5] sidechannel: Linux Security Module for sidechannel
@ 2018-09-28 17:40 ` Schaufler, Casey
0 siblings, 0 replies; 55+ messages in thread
From: Schaufler, Casey @ 2018-09-28 17:40 UTC (permalink / raw)
To: linux-security-module
> -----Original Message-----
> From: James Morris [mailto:jmorris at namei.org]
> Sent: Friday, September 28, 2018 9:33 AM
> To: Jann Horn <jannh@google.com>
> Cc: Schaufler, Casey <casey.schaufler@intel.com>; Casey Schaufler
> <casey@schaufler-ca.com>; kristen at linux.intel.com; Kernel Hardening
> <kernel-hardening@lists.openwall.com>; Dock, Deneen T
> <deneen.t.dock@intel.com>; kernel list <linux-kernel@vger.kernel.org>;
> Hansen, Dave <dave.hansen@intel.com>; linux-security-module <linux-security-
> module at vger.kernel.org>; selinux at tycho.nsa.gov; Arjan van de Ven
> <arjan@linux.intel.com>
> 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.
^ permalink raw reply [flat|nested] 55+ messages in thread