| Message ID | 20180817221624.10232-6-casey.schaufler@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | LSM: Add and use a hook for side-channel safety checks | expand |
On 08/17/2018 06:16 PM, Casey Schaufler wrote: > SELinux considers tasks to be side-channel safe if they > have PROCESS_SHARE access. Now the description and the code no longer match. > > Signed-off-by: Casey Schaufler <casey.schaufler@intel.com> > --- > security/selinux/hooks.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index a8bf324130f5..7fbd7d7ac1cb 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -4219,6 +4219,14 @@ static void selinux_task_to_inode(struct task_struct *p, > spin_unlock(&isec->lock); > } > > +static int selinux_task_safe_sidechannel(struct task_struct *p) > +{ > + struct av_decision avd; > + > + return avc_has_perm_noaudit(&selinux_state, current_sid(), task_sid(p), > + SECCLASS_FILE, FILE__READ, 0, &avd); > +} And my question from before still stands: why do we need a new hook and new security module instead of just using ptrace_may_access()? > + > /* Returns error only if unable to parse addresses */ > static int selinux_parse_skb_ipv4(struct sk_buff *skb, > struct common_audit_data *ad, u8 *proto) > @@ -7002,6 +7010,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = { > LSM_HOOK_INIT(task_movememory, selinux_task_movememory), > LSM_HOOK_INIT(task_kill, selinux_task_kill), > LSM_HOOK_INIT(task_to_inode, selinux_task_to_inode), > + LSM_HOOK_INIT(task_safe_sidechannel, selinux_task_safe_sidechannel), > > LSM_HOOK_INIT(ipc_permission, selinux_ipc_permission), > LSM_HOOK_INIT(ipc_getsecid, selinux_ipc_getsecid), >
> -----Original Message----- > From: Stephen Smalley [mailto:sds@tycho.nsa.gov] > Sent: Monday, August 20, 2018 9:03 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 > Subject: Re: [PATCH RFC v2 5/5] SELinux: Support SELinux determination of > side-channel vulnerability > > On 08/17/2018 06:16 PM, Casey Schaufler wrote: > > SELinux considers tasks to be side-channel safe if they > > have PROCESS_SHARE access. > > Now the description and the code no longer match. You're right. > > > > Signed-off-by: Casey Schaufler <casey.schaufler@intel.com> > > --- > > security/selinux/hooks.c | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > > index a8bf324130f5..7fbd7d7ac1cb 100644 > > --- a/security/selinux/hooks.c > > +++ b/security/selinux/hooks.c > > @@ -4219,6 +4219,14 @@ static void selinux_task_to_inode(struct > task_struct *p, > > spin_unlock(&isec->lock); > > } > > > > +static int selinux_task_safe_sidechannel(struct task_struct *p) > > +{ > > + struct av_decision avd; > > + > > + return avc_has_perm_noaudit(&selinux_state, current_sid(), > task_sid(p), > > + SECCLASS_FILE, FILE__READ, 0, &avd); > > +} > > And my question from before still stands: why do we need a new hook and > new security module instead of just using ptrace_may_access()? Locking. The SELinux check, for example, will lock up solid while trying to generate an audit record. There is no good reason aside from coding convenience to assume that the same restrictions will apply for side-channel as apply to ptrace. I'm actually a touch surprised you're not suggesting a separate SECCLASS or access mode for the SELinux hook. > > > + > > /* Returns error only if unable to parse addresses */ > > static int selinux_parse_skb_ipv4(struct sk_buff *skb, > > struct common_audit_data *ad, u8 *proto) > > @@ -7002,6 +7010,7 @@ static struct security_hook_list selinux_hooks[] > __lsm_ro_after_init = { > > LSM_HOOK_INIT(task_movememory, selinux_task_movememory), > > LSM_HOOK_INIT(task_kill, selinux_task_kill), > > LSM_HOOK_INIT(task_to_inode, selinux_task_to_inode), > > + LSM_HOOK_INIT(task_safe_sidechannel, > selinux_task_safe_sidechannel), > > > > LSM_HOOK_INIT(ipc_permission, selinux_ipc_permission), > > LSM_HOOK_INIT(ipc_getsecid, selinux_ipc_getsecid), > >
On 08/20/2018 12:59 PM, Schaufler, Casey wrote: >> -----Original Message----- >> From: Stephen Smalley [mailto:sds@tycho.nsa.gov] >> Sent: Monday, August 20, 2018 9:03 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 >> Subject: Re: [PATCH RFC v2 5/5] SELinux: Support SELinux determination of >> side-channel vulnerability >> >> On 08/17/2018 06:16 PM, Casey Schaufler wrote: >>> SELinux considers tasks to be side-channel safe if they >>> have PROCESS_SHARE access. >> >> Now the description and the code no longer match. > > You're right. > >>> >>> Signed-off-by: Casey Schaufler <casey.schaufler@intel.com> >>> --- >>> security/selinux/hooks.c | 9 +++++++++ >>> 1 file changed, 9 insertions(+) >>> >>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c >>> index a8bf324130f5..7fbd7d7ac1cb 100644 >>> --- a/security/selinux/hooks.c >>> +++ b/security/selinux/hooks.c >>> @@ -4219,6 +4219,14 @@ static void selinux_task_to_inode(struct >> task_struct *p, >>> spin_unlock(&isec->lock); >>> } >>> >>> +static int selinux_task_safe_sidechannel(struct task_struct *p) >>> +{ >>> + struct av_decision avd; >>> + >>> + return avc_has_perm_noaudit(&selinux_state, current_sid(), >> task_sid(p), >>> + SECCLASS_FILE, FILE__READ, 0, &avd); >>> +} >> >> And my question from before still stands: why do we need a new hook and >> new security module instead of just using ptrace_may_access()? > > Locking. The SELinux check, for example, will lock up solid while trying > to generate an audit record. There is no good reason aside from coding > convenience to assume that the same restrictions will apply for side-channel > as apply to ptrace. I'm actually a touch surprised you're not suggesting a > separate SECCLASS or access mode for the SELinux hook. The PTRACE_MODE_NOAUDIT flag to ptrace_may_access() would address the locking concern. Duplicating the ptrace access checking logic seems prone to errors and inconsistencies. I can't imagine policy writers understanding what "safe sidechannel" means, much less deciding when to allow it. > >> >>> + >>> /* Returns error only if unable to parse addresses */ >>> static int selinux_parse_skb_ipv4(struct sk_buff *skb, >>> struct common_audit_data *ad, u8 *proto) >>> @@ -7002,6 +7010,7 @@ static struct security_hook_list selinux_hooks[] >> __lsm_ro_after_init = { >>> LSM_HOOK_INIT(task_movememory, selinux_task_movememory), >>> LSM_HOOK_INIT(task_kill, selinux_task_kill), >>> LSM_HOOK_INIT(task_to_inode, selinux_task_to_inode), >>> + LSM_HOOK_INIT(task_safe_sidechannel, >> selinux_task_safe_sidechannel), >>> >>> LSM_HOOK_INIT(ipc_permission, selinux_ipc_permission), >>> LSM_HOOK_INIT(ipc_getsecid, selinux_ipc_getsecid), >>> >
> -----Original Message----- > From: Stephen Smalley [mailto:sds@tycho.nsa.gov] > Sent: Monday, August 20, 2018 10:44 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 > Subject: Re: [PATCH RFC v2 5/5] SELinux: Support SELinux determination of > side-channel vulnerability > > On 08/20/2018 12:59 PM, Schaufler, Casey wrote: > >> -----Original Message----- > >> From: Stephen Smalley [mailto:sds@tycho.nsa.gov] > >> Sent: Monday, August 20, 2018 9:03 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 > >> Subject: Re: [PATCH RFC v2 5/5] SELinux: Support SELinux determination of > >> side-channel vulnerability > >> > >> On 08/17/2018 06:16 PM, Casey Schaufler wrote: > >>> SELinux considers tasks to be side-channel safe if they > >>> have PROCESS_SHARE access. > >> > >> Now the description and the code no longer match. > > > > You're right. > > > >>> > >>> Signed-off-by: Casey Schaufler <casey.schaufler@intel.com> > >>> --- > >>> security/selinux/hooks.c | 9 +++++++++ > >>> 1 file changed, 9 insertions(+) > >>> > >>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > >>> index a8bf324130f5..7fbd7d7ac1cb 100644 > >>> --- a/security/selinux/hooks.c > >>> +++ b/security/selinux/hooks.c > >>> @@ -4219,6 +4219,14 @@ static void selinux_task_to_inode(struct > >> task_struct *p, > >>> spin_unlock(&isec->lock); > >>> } > >>> > >>> +static int selinux_task_safe_sidechannel(struct task_struct *p) > >>> +{ > >>> + struct av_decision avd; > >>> + > >>> + return avc_has_perm_noaudit(&selinux_state, current_sid(), > >> task_sid(p), > >>> + SECCLASS_FILE, FILE__READ, 0, &avd); > >>> +} > >> > >> And my question from before still stands: why do we need a new hook and > >> new security module instead of just using ptrace_may_access()? > > > > Locking. The SELinux check, for example, will lock up solid while trying > > to generate an audit record. There is no good reason aside from coding > > convenience to assume that the same restrictions will apply for side-channel > > as apply to ptrace. I'm actually a touch surprised you're not suggesting a > > separate SECCLASS or access mode for the SELinux hook. > > The PTRACE_MODE_NOAUDIT flag to ptrace_may_access() would address the > locking concern. OK ... > Duplicating the ptrace access checking logic seems > prone to errors and inconsistencies. That's true only if the ptrace logic and the safe-sidechannel logic are identical. I don't believe that is a safe assumption. It would sure be convenient. But I would hate to see a change made for either ptrace or safe_sidechannel that interfered with the correct behavior of the other. > I can't imagine policy writers > understanding what "safe sidechannel" means, much less deciding when to > allow it. I can't argue with that. But then, I have always had trouble with the SELinux policy scheme.
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index a8bf324130f5..7fbd7d7ac1cb 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -4219,6 +4219,14 @@ static void selinux_task_to_inode(struct task_struct *p, spin_unlock(&isec->lock); } +static int selinux_task_safe_sidechannel(struct task_struct *p) +{ + struct av_decision avd; + + return avc_has_perm_noaudit(&selinux_state, current_sid(), task_sid(p), + SECCLASS_FILE, FILE__READ, 0, &avd); +} + /* Returns error only if unable to parse addresses */ static int selinux_parse_skb_ipv4(struct sk_buff *skb, struct common_audit_data *ad, u8 *proto) @@ -7002,6 +7010,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = { LSM_HOOK_INIT(task_movememory, selinux_task_movememory), LSM_HOOK_INIT(task_kill, selinux_task_kill), LSM_HOOK_INIT(task_to_inode, selinux_task_to_inode), + LSM_HOOK_INIT(task_safe_sidechannel, selinux_task_safe_sidechannel), LSM_HOOK_INIT(ipc_permission, selinux_ipc_permission), LSM_HOOK_INIT(ipc_getsecid, selinux_ipc_getsecid),
SELinux considers tasks to be side-channel safe if they have PROCESS_SHARE access. Signed-off-by: Casey Schaufler <casey.schaufler@intel.com> --- security/selinux/hooks.c | 9 +++++++++ 1 file changed, 9 insertions(+)