Re: [GIT PULL] SELinux fixes for v4.19 (#1)
On Mon, Oct 15, 2018 at 06:28:04PM -0400, Paul Moore wrote: > Hi Greg, > > We've got one SELinux "fix" that I'd like to get into v4.19 if > possible. I'm using double quotes on "fix" as this is just an update > to the MAINTAINERS file and not a code change. From my perspective, > MAINTAINERS updates generally don't warrant inclusion during the -rcX > phase, but this is a change to the mailing list location so it seemed > prudent to get this in before v4.19 is released. > > If you don't want this for v4.19 let me know and I'll queue it up for > the upcoming merge window. Not a problem, now merged, thanks. greg k-h ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH v2] general protection fault in sock_has_perm
On Thu, Feb 01, 2018 at 07:37:04AM -0800, Mark Salyzyn wrote:
> In the absence of commit a4298e4522d6 ("net: add SOCK_RCU_FREE socket
> flag") and all the associated infrastructure changes to take advantage
> of a RCU grace period before freeing, there is a heightened
> possibility that a security check is performed while an ill-timed
> setsockopt call races in from user space. It then is prudent to null
> check sk_security, and if the case, reject the permissions.
>
> Because of the nature of this problem, hard to duplicate, no clear
> path, this patch is a simplified band-aid for stable trees lacking the
> infrastructure for the series of commits leading up to providing a
> suitable RCU grace period. This adjustment is orthogonal to
> infrastructure improvements that may nullify the needed check, but
> could be added as good code hygiene in all trees.
>
> general protection fault: [#1] PREEMPT SMP KASAN
> CPU: 1 PID: 14233 Comm: syz-executor2 Not tainted 4.4.112-g5f6325b #28
> task: 8801d1095f00 task.stack: 8800b595
> RIP: 0010:[] []
> sock_has_perm+0x1fe/0x3e0 security/selinux/hooks.c:4069
> RSP: 0018:8800b5957ce0 EFLAGS: 00010202
> RAX: dc00 RBX: 110016b2af9f RCX: 81b69b51
> RDX: 0002 RSI: RDI: 0010
> RBP: 8800b5957de0 R08: 0001 R09: 0001
> R10: R11: 110016b2af68 R12: 8800b5957db8
> R13: R14: 8800b7259f40 R15: 00d7
> FS: 7f72f5ae2700() GS:8801db30() knlGS:
> CS: 0010 DS: ES: CR0: 80050033
> CR2: 00a2fa38 CR3: 0001d798 CR4: 00160670
> DR0: DR1: DR2:
> DR3: DR6: fffe0ff0 DR7: 0400
> Stack:
> 81b69a1f 8800b5957d58 8000b5957d30 41b58ab3
> 83fc82f2 81b69980 0246 8801d1096770
> 8801d3165668 8157844b 8801d1095f00
> 8801
> Call Trace:
> [] selinux_socket_setsockopt+0x4d/0x80
> security/selinux/hooks.c:4338
> [] security_socket_setsockopt+0x7d/0xb0
> security/security.c:1257
> [] SYSC_setsockopt net/socket.c:1757 [inline]
> [] SyS_setsockopt+0xe8/0x250 net/socket.c:1746
> [] entry_SYSCALL_64_fastpath+0x16/0x92
> Code: c2 42 9b b6 81 be 01 00 00 00 48 c7 c7 a0 cb 2b 84 e8
> f7 2f 6d ff 49 8d 7d 10 48 b8 00 00 00 00 00 fc ff df 48 89
> fa 48 c1 ea 03 <0f> b6 04 02 84 c0 74 08 3c 03 0f 8e 83 01 00
> 00 41 8b 75 10 31
> RIP [] sock_has_perm+0x1fe/0x3e0
> security/selinux/hooks.c:4069
> RSP
> ---[ end trace 7b5aaf788fef6174 ]---
>
> Signed-off-by: Mark Salyzyn <saly...@android.com>
> Signed-off-by: Paul Moore <p...@linuxfoundation.org>
> Signed-off-by: Greg KH <gre...@linuxfoundation.org>
> Cc: Eric Dumazet <eduma...@google.com>
> Cc: Stephen Smalley <s...@tycho.nsa.gov>
> Cc: selinux@tycho.nsa.gov
> Cc: linux-security-mod...@vger.kernel.org
> Cc: Eric Paris <epa...@parisplace.org>
> Cc: Serge E. Hallyn <se...@hallyn.com>
> Cc: stable <sta...@vger.kernel.org> # 3.18, 4.4
> Cc: linux-ker...@vger.kernel.org
> ---
> v2: return -EFAULT for null sk_security instead of 0
Now queued up, thanks.
greg k-h
Re: [PATCH v2] general protection fault in sock_has_perm
On Thu, Feb 01, 2018 at 08:20:13AM -0800, Mark Salyzyn wrote:
> On 02/01/2018 08:00 AM, Paul Moore wrote:
> > On Thu, Feb 1, 2018 at 10:37 AM, Mark Salyzyn <saly...@android.com> wrote:
> > > In the absence of commit a4298e4522d6 ("net: add SOCK_RCU_FREE socket
> > > flag") and all the associated infrastructure changes to take advantage
> > > of a RCU grace period before freeing, there is a heightened
> > > possibility that a security check is performed while an ill-timed
> > > setsockopt call races in from user space. It then is prudent to null
> > > check sk_security, and if the case, reject the permissions.
> > >
> > > . . .
> > > ---[ end trace 7b5aaf788fef6174 ]---
> > >
> > > Signed-off-by: Mark Salyzyn <saly...@android.com>
> > > Signed-off-by: Paul Moore <p...@linuxfoundation.org>
> > No, in the previous thread I gave my ack, not my sign-off; please be
> > more careful in the future. It may seem silly, especially in this
> > particular case, but it is an important distinction when things like
> > the DCO are concerned.
> >
> > Anyway, here is my ack again.
> >
> > Acked-by: Paul Moore <p...@paul-moore.com>
> >
> Ok, both Greg KH and yours should be considered Acked-By. Been overstepping
> this boundary for _years_. AFAIK Signed-off-by is still pending from Stephen
> Smalley <s...@tycho.nsa.gov> before this can roll in.
An ack is all I need here, or I can just rely on Paul's :)
I'll edit up Paul's when I apply this.
thanks,
greg k-h
Re: [PATCH] general protection fault in sock_has_perm
On Wed, Jan 31, 2018 at 04:06:37AM -0500, Paul Moore wrote:
> On Tue, Jan 30, 2018 at 5:46 PM, Greg KH <gre...@linuxfoundation.org> wrote:
> > On Tue, Jan 30, 2018 at 11:00:04AM -0800, Mark Salyzyn wrote:
> >> On 01/19/2018 09:41 AM, Stephen Smalley wrote:
> >> > If we can't safely dereference the sock in these hooks, then that seems
> >> > to point back to the approach used in my original code, where in
> >> > ancient history I had sock_has_perm() take the socket and use its inode
> >> > i_security field instead of the sock. commit
> >> > 253bfae6e0ad97554799affa0266052968a45808 switched them to use the sock
> >> > instead.
> >>
> >> Because of the nature of this problem (hard to duplicate, no clear path), I
> >> am understandably not comfortable reverting and submitting for testing in
> >> order to prove this point. It is disruptive because it changes several
> >> subroutine call signatures.
> >>
> >> AFAIK this looks like a user request racing in without reference counting
> >> or
> >> RCU grace period in the callers (could be viewed as not an issue with
> >> security code). Effectively fixed in 4.9-stable, but broken in 4.4-stable.
> >>
> >> hygiene, KISS and small, is all I do feel comfortable to submit to
> >> 4.4-stable without pulling in all the infrastructure improvements.
> >>
> >> -- Mark
> >>
> >> ---
> >> security/selinux/hooks.c | 2 ++
> >> 1 file changed, 2 insertions(+)
> >>
> >> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> >> index 34427384605d..be68992a28cb 100644
> >> --- a/security/selinux/hooks.c
> >> +++ b/security/selinux/hooks.c
> >> @@ -4066,6 +4066,8 @@ static int sock_has_perm(struct task_struct *task,
> >> struct sock *sk, u32 perms)
> >> struct lsm_network_audit net = {0,};
> >> u32 tsid = task_sid(task);
> >>
> >> +if (!sksec)
> >> +return -EFAULT;
> >> if (sksec->sid == SECINITSID_KERNEL)
> >> return 0;
> >>
> >
> > This looks sane to me as a simple 4.4-only fix. If the SELinux
> > maintainer acks it, I can easily queue this up.
>
> This revision addresses my concerns with Mark's previous patch.
>
> Acked-by: Paul Moore <p...@paul-moore.com>
Wonderful!
Mark, can you resend this in a format I can apply it in?
thanks,
greg k-h
Re: [PATCH] general protection fault in sock_has_perm
On Thu, Jan 18, 2018 at 01:58:45PM -0800, Mark Salyzyn wrote:
> general protection fault: [#1] PREEMPT SMP KASAN
> CPU: 1 PID: 14233 Comm: syz-executor2 Not tainted 4.4.112-g5f6325b #28
> task: 8801d1095f00 task.stack: 8800b595
> RIP: 0010:[] []
> sock_has_perm+0x1fe/0x3e0 security/selinux/hooks.c:4069
> RSP: 0018:8800b5957ce0 EFLAGS: 00010202
> RAX: dc00 RBX: 110016b2af9f RCX: 81b69b51
> RDX: 0002 RSI: RDI: 0010
> RBP: 8800b5957de0 R08: 0001 R09: 0001
> R10: R11: 110016b2af68 R12: 8800b5957db8
> R13: R14: 8800b7259f40 R15: 00d7
> FS: 7f72f5ae2700() GS:8801db30() knlGS:
> CS: 0010 DS: ES: CR0: 80050033
> CR2: 00a2fa38 CR3: 0001d798 CR4: 00160670
> DR0: DR1: DR2:
> DR3: DR6: fffe0ff0 DR7: 0400
> Stack:
> 81b69a1f 8800b5957d58 8000b5957d30 41b58ab3
> 83fc82f2 81b69980 0246 8801d1096770
> 8801d3165668 8157844b 8801d1095f00
> 8801
> Call Trace:
> [] selinux_socket_setsockopt+0x4d/0x80
> security/selinux/hooks.c:4338
> [] security_socket_setsockopt+0x7d/0xb0
> security/security.c:1257
> [] SYSC_setsockopt net/socket.c:1757 [inline]
> [] SyS_setsockopt+0xe8/0x250 net/socket.c:1746
> [] entry_SYSCALL_64_fastpath+0x16/0x92
> Code: c2 42 9b b6 81 be 01 00 00 00 48 c7 c7 a0 cb 2b 84 e8
> f7 2f 6d ff 49 8d 7d 10 48 b8 00 00 00 00 00 fc ff df 48 89
> fa 48 c1 ea 03 <0f> b6 04 02 84 c0 74 08 3c 03 0f 8e 83 01 00
> 00 41 8b 75 10 31
> RIP [] sock_has_perm+0x1fe/0x3e0
> security/selinux/hooks.c:4069
> RSP
> ---[ end trace 7b5aaf788fef6174 ]---
>
> In the absence of commit a4298e4522d6 ("net: add SOCK_RCU_FREE socket
> flag") and all the associated infrastructure changes to take advantage
> of a RCU grace period before freeing, there is a heightened
> possibility that a security check is performed while an ill-timed
> setsockopt call races in from user space. It then is prudent to null
> check sk_security, and if the case, reject the permissions.
>
> This adjustment is orthogonal to infrastructure improvements that may
> nullify the needed check, but should be added as good code hygiene.
>
> Signed-off-by: Mark Salyzyn
> Cc: Paul Moore
> Cc: Stephen Smalley
> Cc: Eric Paris
> Cc: James Morris
> Cc: "Serge E. Hallyn"
> Cc: selinux@tycho.nsa.gov
> Cc: linux-security-mod...@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org
> Cc: sta...@vger.kernel.org
> ---
> This patch should be applied to all stable trees (author wants
> minimum of 3.18, 4.4, 4.9 and 4.14)
Note, if you want this type of thing to show up in the patch itself, so
I will see it when it hits Linus's tree, you can just change the stable
line to be:
cc: stable # 3.18+
thanks,
greg k-h
Re: [PATCH] usb, signal, security: only pass the cred, not the secid, to kill_pid_info_as_cred and security_task_kill
On Fri, Sep 08, 2017 at 12:40:01PM -0400, Stephen Smalley wrote:
> commit d178bc3a708f39cbfefc3fab37032d3f2511b4ec ("user namespace: usb:
> make usb urbs user namespace aware (v2)") changed kill_pid_info_as_uid
> to kill_pid_info_as_cred, saving and passing a cred structure instead of
> uids. Since the secid can be obtained from the cred, drop the secid fields
> from the usb_dev_state and async structures, and drop the secid argument to
> kill_pid_info_as_cred. Replace the secid argument to security_task_kill
> with the cred. Update SELinux, Smack, and AppArmor to use the cred, which
> avoids the need for Smack and AppArmor to use a secid at all in this hook.
> Further changes to Smack might still be required to take full advantage of
> this change, since it should now be possible to perform capability
> checking based on the supplied cred. The changes to Smack and AppArmor
> have only been compile-tested.
>
> Signed-off-by: Stephen Smalley
> ---
> drivers/usb/core/devio.c | 10 ++
> include/linux/lsm_hooks.h| 5 +++--
> include/linux/sched/signal.h | 2 +-
> include/linux/security.h | 4 ++--
> kernel/signal.c | 6 +++---
> security/apparmor/lsm.c | 17 -
> security/security.c | 4 ++--
> security/selinux/hooks.c | 7 +--
> security/smack/smack_lsm.c | 12 +---
> 9 files changed, 35 insertions(+), 32 deletions(-)
Acked-by: Greg Kroah-Hartman
