Bug Report: bug if selinux_msg_queue_msgsnd & and selinux_msg_queue_msgrcv

Bug Report: bug if selinux_msg_queue_msgsnd & and selinux_msg_queue_msgrcv

[John Johansen]

kernel: 5.0-rc2

d8c6e85432944 ("msg/security: Pass kern_ipc_perm not msg_queue into the msg_queue security hooks")

appears to have introduced a bug into selinux_msg_queue_msgsnd and selinux_msg_queue_msgrcv


specifically the portion of the patch that does

-       isec = msq->q_perm.security;
+       isec = msq->security;

which leaves the code
	isec = msq->security;
	msec = msg->security;

however isec and msec are different size structures. specifically isec is an ipc_security_struct and msec is a msg_security_struct. Which are defined as

struct msg_security_struct {
	u32 sid;	/* SID of message */
};

struct ipc_security_struct {
	u16 sclass;	/* security class of this object */
	u32 sid;	/* SID of IPC resource */
};

where the msg->security field is allocated as an ipc_security_struct. Access the msec->sid would thus appear to overlay the isec->sclass.

Re: Bug Report: bug if selinux_msg_queue_msgsnd & and selinux_msg_queue_msgrcv

[Paul Moore]

On Wed, Jan 16, 2019 at 5:14 AM John Johansen
<john.johansen@canonical.com> wrote:
>
> kernel: 5.0-rc2
>
> d8c6e85432944 ("msg/security: Pass kern_ipc_perm not msg_queue into the msg_queue security hooks")
>
> appears to have introduced a bug into selinux_msg_queue_msgsnd and selinux_msg_queue_msgrcv
>
> specifically the portion of the patch that does
>
> -       isec = msq->q_perm.security;
> +       isec = msq->security;
>
> which leaves the code
>         isec = msq->security;
>         msec = msg->security;
>
> however isec and msec are different size structures. specifically isec is an ipc_security_struct and msec is a msg_security_struct ...

I suspect there may be some mistaken identity regarding "msq" (with a
lower-case "Q") and "msg" (with a lower-case "G").

Looking quickly at selinux_msg_queue_msgsnd() and
selinux_msg_queue_msgrcv() it would appear that in both cases the
kern_ipc_perm->security pointer is assigned to an ipc_security_struct
pointer and the msg_msg->security struct is assigned a
msg_security_struct pointer.  This appears to be correct, or is there
something I'm missing in your report?

-- 
paul moore
www.paul-moore.com

Re: Bug Report: bug if selinux_msg_queue_msgsnd & and selinux_msg_queue_msgrcv

[Stephen Smalley]

On 1/16/19 5:14 AM, John Johansen wrote:
> kernel: 5.0-rc2
> 
> d8c6e85432944 ("msg/security: Pass kern_ipc_perm not msg_queue into the msg_queue security hooks")
> 
> appears to have introduced a bug into selinux_msg_queue_msgsnd and selinux_msg_queue_msgrcv
> 
> 
> specifically the portion of the patch that does
> 
> -       isec = msq->q_perm.security;
> +       isec = msq->security;
> 
> which leaves the code
> 	isec = msq->security;
> 	msec = msg->security;
> 
> however isec and msec are different size structures. specifically isec is an ipc_security_struct and msec is a msg_security_struct. Which are defined as
> 
> struct msg_security_struct {
> 	u32 sid;	/* SID of message */
> };
> 
> struct ipc_security_struct {
> 	u16 sclass;	/* security class of this object */
> 	u32 sid;	/* SID of IPC resource */
> };
> 
> where the msg->security field is allocated as an ipc_security_struct. Access the msec->sid would thus appear to overlay the isec->sclass.

The only thing that changed in that commit was directly passing 
&msgq->q_perm to the hook instead of passing msgq to the hook and then 
dereferencing msq->q_perm to reach the ipc security blob.

msg is a different object from msq with its own security blob (msg is an 
individual message; msq is the message queue).  isec and msec point to 
two different security blobs with different structures.

I don't see a problem offhand.

Re: Bug Report: bug if selinux_msg_queue_msgsnd & and selinux_msg_queue_msgrcv

[John Johansen]

On 1/16/19 6:45 AM, Paul Moore wrote:
> On Wed, Jan 16, 2019 at 5:14 AM John Johansen
> <john.johansen@canonical.com> wrote:
>>
>> kernel: 5.0-rc2
>>
>> d8c6e85432944 ("msg/security: Pass kern_ipc_perm not msg_queue into the msg_queue security hooks")
>>
>> appears to have introduced a bug into selinux_msg_queue_msgsnd and selinux_msg_queue_msgrcv
>>
>> specifically the portion of the patch that does
>>
>> -       isec = msq->q_perm.security;
>> +       isec = msq->security;
>>
>> which leaves the code
>>         isec = msq->security;
>>         msec = msg->security;
>>
>> however isec and msec are different size structures. specifically isec is an ipc_security_struct and msec is a msg_security_struct ...
> 
> I suspect there may be some mistaken identity regarding "msq" (with a
> lower-case "Q") and "msg" (with a lower-case "G").
> 
> Looking quickly at selinux_msg_queue_msgsnd() and
> selinux_msg_queue_msgrcv() it would appear that in both cases the
> kern_ipc_perm->security pointer is assigned to an ipc_security_struct
> pointer and the msg_msg->security struct is assigned a
> msg_security_struct pointer.  This appears to be correct, or is there
> something I'm missing in your report?
> 

ha, that is indeed it. I looked at this multiple times and didn't pickup
the q vs g. Thanks sorry for the bad report, guess I should have gone to
bed sarlier :)

Re: Bug Report: bug if selinux_msg_queue_msgsnd & and selinux_msg_queue_msgrcv

[John Johansen]

On 1/16/19 6:47 AM, Stephen Smalley wrote:
> On 1/16/19 5:14 AM, John Johansen wrote:
>> kernel: 5.0-rc2
>>
>> d8c6e85432944 ("msg/security: Pass kern_ipc_perm not msg_queue into the msg_queue security hooks")
>>
>> appears to have introduced a bug into selinux_msg_queue_msgsnd and selinux_msg_queue_msgrcv
>>
>>
>> specifically the portion of the patch that does
>>
>> -       isec = msq->q_perm.security;
>> +       isec = msq->security;
>>
>> which leaves the code
>>     isec = msq->security;
>>     msec = msg->security;
>>
>> however isec and msec are different size structures. specifically isec is an ipc_security_struct and msec is a msg_security_struct. Which are defined as
>>
>> struct msg_security_struct {
>>     u32 sid;    /* SID of message */
>> };
>>
>> struct ipc_security_struct {
>>     u16 sclass;    /* security class of this object */
>>     u32 sid;    /* SID of IPC resource */
>> };
>>
>> where the msg->security field is allocated as an ipc_security_struct. Access the msec->sid would thus appear to overlay the isec->sclass.
> 
> The only thing that changed in that commit was directly passing &msgq->q_perm to the hook instead of passing msgq to the hook and then dereferencing msq->q_perm to reach the ipc security blob.
> 
> msg is a different object from msq with its own security blob (msg is an individual message; msq is the message queue).  isec and msec point to two different security blobs with different structures.
> 
> I don't see a problem offhand.

yep, sorry combination of bad fonts, and being 3am. sorry for the bad report