[PATCH]: Smack: type confusion in smak sendmsg() handler
Please note that this problem was not found by me but by Mateusz Fruba
and he takes full credit for all the below details, the patch has been
submitted by me due to corporate rules, all questions/issues etc. can be
submitted here and I will forward them to Mateusz if needed.
--- cut here for patch
Smack security handler for sendmsg() syscall
is vulnerable to type confusion issue what
can allow to privilege escalation into root
or cause denial of service.
A malicious attacker can create socket of one
type for example AF_UNIX and pass is into
sendmsg() function ensuring that this is
AF_INET socket.
Remedy
Do not trust user supplied data.
Proposed fix below.
Signed-off-by: Roman Kubiak <r.kub...@samsung.com>
Signed-off-by: Mateusz Fruba <m.fr...@samsung.com>
---
security/smack/smack_lsm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index ff81026..9258a52 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -3758,7 +3758,7 @@ static int smack_socket_sendmsg(struct socket *sock,
struct msghdr *msg,
if (sip == NULL)
return 0;
- switch (sip->sin_family) {
+ switch (sock->sk->sk_family) {
case AF_INET:
rc = smack_netlabel_send(sock->sk, sip);
break;
--
1.9.1
--- cut here for patch
--- cut here for detailed bug information
Summary:
Smack security handler for sendmsg() syscall is vulnerable to type confusion
issue what can allow to privilege escalation into root or cause denial of
service.
Detail:
Mainline kernel implementation of sendmsg() syscall in smack is vulnerable to
type confusion bug.
Vulnerable code can be found in:
Linux/security/smack/smack_lsm.c
static int smack_socket_sendmsg(struct socket *sock, struct msghdr *msg,
int size)
{
struct sockaddr_in *sip = (struct sockaddr_in *) msg->msg_name;
struct sockaddr_in6 *sap = (struct sockaddr_in6 *) msg->msg_name;
int rc = 0;
/*
* Perfectly reasonable for this to be NULL
*/
if (sip == NULL)
return 0;
switch (sip->sin_family) {
case AF_INET:
rc = smack_netlabel_send(sock->sk, sip);
break;
case AF_INET6:
rc = smk_ipv6_port_check(sock->sk, sap, SMK_SENDING);
break;
}
return rc;
}
As you can see in code above type of socket is checked using user supplied data.
An malicious attacker can create socket of one type for example AF_UNIX and
pass is into sendmsg() function ensuring that this is AF_INET socket.
If we go dipper
smack_socket_sendmsg(...)>smack_netlabel_send(...)>smack_netlabel(...)->cipso_v4_sock_delattr(struct
sock *sk) then we can find following code:
Linux/net/ipv4/cipso_ipv4.c
void cipso_v4_sock_delattr(struct sock *sk)
{
int hdr_delta;
struct ip_options_rcu *opt;
struct inet_sock *sk_inet;
sk_inet = inet_sk(sk);
opt = rcu_dereference_protected(sk_inet->inet_opt, 1);
if (opt == NULL || opt->opt.cipso == 0)
return;
hdr_delta = cipso_v4_delopt(_inet->inet_opt);
if (sk_inet->is_icsk && hdr_delta > 0) {
struct inet_connection_sock *sk_conn = inet_csk(sk);
sk_conn->icsk_ext_hdr_len -= hdr_delta;
sk_conn->icsk_sync_mss(sk, sk_conn->icsk_pmtu_cookie);
}
}
In code above our malicious AF_NETLINK socket is casted into AF_INET and next
"inet_opt" field is dereferenced from it. In case of AF_NETLINK there is no
inet_opt field so an another value will be returned.
Test for x64 kernel 3.18.24:
As verified inet_opt field is under 0x2d0 in struct inet_sock, if we check
other type of sockets:
AF_INET
(gdb) p/x &((struct inet_sock *)0)->inet_opt
$4 = 0x2d0
AF_NETLINK
(gdb) p/x &((struct netlink_sock *)0)->groups
$6 = 0x2d0
AF_UNIX
(gdb) p/x ((struct unix_sock *)sk)->readlock
$7 = 0x2d0
So the offset is dependent from kernel version, socket type (multiple socket
available depending on kernel config), processor architecture.
Summing up in some cases value which will be resolved by inet_opt can be
controlled by attacker what can be used to gain code execution in kernel.
POC:
Issue require applied smack into kernel.
I have tested and confirmed this issue on:
Mainline kernel 3.18.24 (x64)
Z300H BOK8 build - kernel 3.10.65 (arm)
SM-R730S AOKA build - kernel 3.4.0 (arm)
Below you can find poc code which use smack type confusion to crash kernel.
#include
#include
#include
#include
#include
#include
#include
#include
#include
int
main(int argc, char *argv[])
{
int ret = 0;
int sockfd = socket(AF_NETLINK, SOCK_RAW, 0);
printf( "socket = %d\n", sockfd);
struct sockaddr_nl nl_addr;
Re: LSM and sending socketpair created descriptors via unix sockets
Helo again. I need to get back to this issue, we still don't have a fix for socketpair() and UDS sockets. I was wondering if the fix i posted is ok (apart from the variables beeing named inode), as you can see this fields in the structure are initialized in smack_sk_alloc_security (to cover those sockets that won't be connected like those created by socketpair() ) and in smack_unix_stream_connect() for all other situations. Do you have some other ideas about this issue, maybe there could be a more generic way to fix this in smack ? I was also wondering about a situation were a descriptor to a socket would be passed around more then once from a process to process, in the end it could actually end up in the original sending process, should we somehow take into account the path such a descriptor would take in the process tree ? bes regards On 09/25/2015 09:09 PM, Casey Schaufler wrote: > On 9/23/2015 7:07 AM, Stephen Smalley wrote: >> On 09/23/2015 05:00 AM, Roman Kubiak wrote: >>> We found a lot of scenarios where the smack_file_receive() function might >>> fail >>> >>> 1) for pipes a different patch is needed for the function to work >>> (otherwise all pipe descriptors >>> get the "_" label), patch below: >>> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c >>> index a143328..0ebfb07 100644 >>> --- a/security/smack/smack_lsm.c >>> +++ b/security/smack/smack_lsm.c >>> @@ -3099,6 +3099,9 @@ static void smack_d_instantiate(struct dentry >>> *opt_dentry, struct inode *inode) >>> */ >>> isp->smk_inode = smk_of_current(); >>> break; >>> + case PIPEFS_MAGIC: >>> + isp->smk_inode = smk_of_current(); >>> + break; >> We have the equivalent of this in SELinux already, by way of specifying >> fs_use_task for pipefs in policy and the SECURITY_FS_USE_TASK case in >> selinux inode_doinit_with_dentry(). > > This looks like a fix regardless of the file_receive issue. > >> >>> default: >>> isp->smk_inode = sbsp->smk_root; >>> break; >>> >>> 2) for sockets it's a complicated issue with connected/unconnected sockets, >>> also connected means they might be server/client sockets (those with >>> accept/listen and those with only connect), socketpair is another issue. I >>> tried to cover at least one case, where the descriptor is a connected UDS >>> socket (it has a peer), patch below: >> I don't understand the issue here; for SELinux, we just always call >> file_has_perm() in selinux_file_receive(), and it does not matter >> whether the open file refers to a file, a pipe, a socket, or whatever. > > There's a difference in how SELinux and Smack treat socket based > communications. I won't try to describe exactly how SELinux does > it, because I always get it wrong. Smack treats the communication > as a write operation from the sending process to the receiving > process. The information about Smack label of the receiving process > is stored in the socket structure (which is not itself an object in > the Smack model) and the check is made against that. The Smack label > attached to a UDS inode is not relevant. If the file structure for > a UDS lacks an inode it shouldn't matter at all to Smack. > > For file_receive of a UDS what matters is whether the receiving > process can write to the process at the other end. That has nothing > to do with the inode. > > >> If the open file refers to a socket, whether connected, unconnected, >> local, network, or whatever, that socket has an associated inode that >> was created when the socket was created (and that typically has the same >> label as the process that created the socket), and I can check whether >> the current process (the recipient) is allowed to read and write to >> sockets with that label. > > But Smack doesn't really care about the inode. The UDS file system > object is not interesting to Smack. > >> If you want to permit that, then you can allow it via a rule. If not, >> then don't. What's the problem? You don't need to replicate the actual >> checking that gets done on socket send at this point. You are just >> checking whether the recipient can use the socket at all, not whether it >> can talk to the peer. That should get mediated at send time. > > The check is to see if using the descriptor as is would violate > the system policy. If the receiving process does not hav
[PATCH]: Smack: pipefs fix in smack_d_instantiate
This fix writes the task label when smack_d_instantiate is called, before the label of the superblock was written on the pipe's inode. Signed-off-by: Roman Kubiak <r.kub...@samsung.com> --- security/smack/smack_lsm.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c index 996c889..f1d6ef1 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -3354,6 +3354,9 @@ static void smack_d_instantiate(struct dentry *opt_dentry, struct inode *inode) */ isp->smk_inode = smk_of_current(); break; + case PIPEFS_MAGIC: + isp->smk_inode = smk_of_current(); + break; default: isp->smk_inode = sbsp->smk_root; break; -- 1.9.1 -- ------ Roman Kubiak -- -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
