On Wed, Jan 21, 2015 at 1:30 PM, Manfred Spraul
<manfred@xxxxxxxxxxxxxxxx> wrote:
> 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?
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@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/
>
>
_______________________________________________
Selinux mailing list
Selinux@xxxxxxxxxxxxx
To unsubscribe, send email to Selinux-leave@xxxxxxxxxxxxx.
To get help, send an email containing "help" to Selinux-request@xxxxxxxxxxxxx.
