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

[Ethan Zhao <ethan.kernel@gmail.com>]

On Wed, Jan 21, 2015 at 1:30 PM, Manfred Spraul
<manfred@colorfullife.com> wrote:
> On 01/21/2015 04:53 AM, Ethan Zhao wrote:
>>
>> On Tue, Jan 20, 2015 at 10:10 PM, Stephen Smalley <sds@tycho.nsa.gov>
>> 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?

I tried to ask for vmcore and do more analysis, basically, the race condition
still exists and open a hole to be DoS.

Thanks,
Ethan

>
> --
>     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@oracle.com>
>>>> ---
>>>>   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@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>> Please read the FAQ at  http://www.tux.org/lkml/
>
>