Re: [PATCH] Selinux/hooks.c: Fix a NULL pointer dereference caused by semop()

[Manfred Spraul]

On 01/21/2015 04:53 AM, Ethan Zhao wrote:
> On Tue, Jan 20, 2015 at 10:10 PM, Stephen Smalley <sds@xxxxxxxxxxxxx> wrote:
> > On 01/20/2015 04:18 AM, Ethan Zhao wrote:
> > >       sys_semget()
> > >       ->newary()
> > >           ->security_sem_alloc()
> > >             ->sem_alloc_security()
> > >                   selinux_sem_alloc_security()
> > >                   ->ipc_alloc_security() {
> > >                     ->rc = avc_has_perm()
> > >                                if (rc) {
> > >                                        ipc_free_security(&sma->sem_perm);
> > >                                        return rc;
> > We free the security structure here to avoid a memory leak on a
> > failed/denied semaphore set creation.  In this situation, we return an
> > error to the caller (ultimately to newary), it does an
> > ipc_rcu_putref(sma, ipc_rcu_free), and it returns an error to the
> > caller.  Thus, it never calls ipc_addid() and the semaphore set is not
> > created.  So how then can you call semtimedop() on it?
> Seems it wouldn't happen after commit e8577d1f0329d4842e8302e289fb2c22156abef4 ?
That was my first guess when I read the bug report - but it can't be the 
fix, because security_sem_alloc() is before the ipc_addid(), with or 
without the patch.

thread A:
            thread B:

semtimedop()
-> sem_obtain_object_check()
            semctl(IPC_RMID)
            -> freeary()
            -> ipc_rcu_putref()
            -> call_rcu()
-> somehow a grace period
            -> sem_rcu_free()
            -> security_sem_free()

Perhaps: modify ipc_free_security() to hexdump perm and a few more bytes 
if the pointer is NULL?

--
    Manfred

> Thanks,
> Ethan
> > >       So ipc_perms->security was NULL, then semtimedop() was called as
> > >       following:
> > >
> > >          sys_semtimedop() / semop()
> > >          ->selinux_sem_semop()
> > >           ->ipc_has_perm()
> > >             ->avc_has_perm(sid, isec->sid, isec->sclass, perms, &ad);
> > >                                            ^- NULL pointer dereference happens
> > >
> > > The test kernel was running on VMware.
> > > This patch use to fix this serious security issue could be triggered by user space.
> > > This patch was tested with v3.19-rc5.
> > >
> > > Signed-off-by: Ethan Zhao <ethan.zhao@xxxxxxxxxx>
> > > ---
> > >   security/selinux/hooks.c | 2 ++
> > >   1 file changed, 2 insertions(+)
> > >
> > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > > index 6da7532..bbe76f5 100644
> > > --- a/security/selinux/hooks.c
> > > +++ b/security/selinux/hooks.c
> > > @@ -5129,6 +5129,8 @@ static int ipc_has_perm(struct kern_ipc_perm *ipc_perms,
> > >        u32 sid = current_sid();
> > >
> > >        isec = ipc_perms->security;
> > > +     if (!isec)
> > > +             return -EACCES;
> > >
> > >        ad.type = LSM_AUDIT_DATA_IPC;
> > >        ad.u.ipc_id = ipc_perms->key;
> > That is not the correct fix; it just hides a bug.  If we reach
> > ipc_has_perm() with a NULL isec, it is a bug in the ipc code.
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/