[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Wed, 26 Sep 2018 22:47:02 +0000
From: "Schaufler, Casey" <casey.schaufler@...el.com>
To: Jann Horn <jannh@...gle.com>
CC: Kernel Hardening <kernel-hardening@...ts.openwall.com>, kernel list
<linux-kernel@...r.kernel.org>, linux-security-module
<linux-security-module@...r.kernel.org>, "selinux@...ho.nsa.gov"
<selinux@...ho.nsa.gov>, "Hansen, Dave" <dave.hansen@...el.com>, "Dock,
Deneen T" <deneen.t.dock@...el.com>, "kristen@...ux.intel.com"
<kristen@...ux.intel.com>, Arjan van de Ven <arjan@...ux.intel.com>
Subject: RE: [PATCH v5 1/5] AppArmor: Prepare for PTRACE_MODE_SCHED
> -----Original Message-----
> From: Jann Horn [mailto:jannh@...gle.com]
> Sent: Wednesday, September 26, 2018 2:19 PM
> To: Schaufler, Casey <casey.schaufler@...el.com>
> Cc: Kernel Hardening <kernel-hardening@...ts.openwall.com>; kernel list
> <linux-kernel@...r.kernel.org>; linux-security-module <linux-security-
> module@...r.kernel.org>; selinux@...ho.nsa.gov; Hansen, Dave
> <dave.hansen@...el.com>; Dock, Deneen T <deneen.t.dock@...el.com>;
> kristen@...ux.intel.com; Arjan van de Ven <arjan@...ux.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@...gle.com> wrote:
> >
> > On Wed, Sep 26, 2018 at 10:35 PM Casey Schaufler
> > <casey.schaufler@...el.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@...el.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
> > >
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
