From: Chris Wright <hidden> Date: 2004-11-18 16:49:12
* James Morris (jmorris@redhat.com) wrote:
What's happening is that mixing stream and dgram ops for SEQPACKET is
having some unfortunate side effects.
Agreed.
One of these is that there is a race between client sendmsg() and server
accept(). The server child socket is attached via sock_graft() after the
client has entered unix_dgram_sendmsg() and called
security_unix_may_send(sk->sk_socket, other->sk_socket);
other->sk_socket will thus be null, causing the oops in SELinux and any
other LSM which tries to dereference the pointer.
Yup. And it's not much of a race, the window is wide open. One
malicious app simply has to do:
bind()
listen()
connect()
send() <-- Oops
The fix is a combination of some of Ross's ideas:
1) SOCK_SEQPACKET is connection oriented, and there no need to call
security_unix_may_send() for each packet. security_unix_stream_connect()
is sufficient.
Why not make a unix_seq_sendmsg, which is a very small wrapper?
e.g.
static int unix_seq_sendmsg(struct kiocb *kiocb, struct socket *sock,
struct msghdr *msg, size_t len)
{
struct sock *sk = sock->sk;
if (sk->sk_type == SOCK_SEQPACKET && sk->sk_state != TCP_ESTABLISHED)
return -ENOTCONN;
if (msg->msg_name || msg->msg_namelen)
return -EINVAL;
return unix_dgram_sendmsg(kiocb, sock, msg, len);
}
Also, I missed how MSG_EOR is honored.
2) Ensure that unix_dgram_sendmsg() fails for SOCK_SEQPACKET sockets which
are not connected, otherwise someone could bypass LSM by sending on an
unconnected socket.
Agreed, not connected, it should fail IMHO.
Note that this only solves the problem for the LSM hook.
From: James Morris <hidden> Date: 2004-11-18 17:02:12
On Thu, 18 Nov 2004, Chris Wright wrote:
Why not make a unix_seq_sendmsg, which is a very small wrapper?
Good idea, patch forthcoming.
Does the above stop the other issue? My laptop died, so I'm not able to
test ATM.
No, it seems to be caused when addrlen in sendto() is non-zero, causing
unix_find_other() to be called instead of unix_peer_get(), which is
screwing up reference counts.
As for MSG_EOR, apart from the generic socket code, nothing is being done.
This would be a separate issue.
- James
--
James Morris
[off-list ref]
From: James Morris <hidden> Date: 2004-11-18 17:31:15
Updated patch below (with Chris Wright's wrapper idea).
This now fixes both issues.
1) Don't call security_unix_may_send() hook during sendmsg() for
SOCK_SEQPACKET, and ensure that sendmsg() can only be called on a
connected socket so as not to bypass the security_unix_stream_connect()
hook.
2) Return -EINVAL if sendto() is called on SOCK_SEQPACKET with an address
supplied.
Please review and apply if ok.
Signed-off-by: James Morris <redacted>
---
net/unix/af_unix.c | 26 ++++++++++++++++++++++----
1 files changed, 22 insertions(+), 4 deletions(-)
diff -purN -X dontdiff linux-2.6.10-rc2.o/net/unix/af_unix.c linux-2.6.10-rc2.w2/net/unix/af_unix.c
1) Don't call security_unix_may_send() hook during sendmsg() for
SOCK_SEQPACKET, and ensure that sendmsg() can only be called on a
connected socket so as not to bypass the security_unix_stream_connect()
hook.
2) Return -EINVAL if sendto() is called on SOCK_SEQPACKET with an address
supplied.
Consider shutdown(). A sendmsg into shutdown must return the pending
ECONNRESET
first.
From: "David S. Miller" <davem@davemloft.net> Date: 2004-11-18 22:53:57
On Thu, 18 Nov 2004 12:25:21 -0500 (EST)
James Morris [off-list ref] wrote:
Updated patch below (with Chris Wright's wrapper idea).
This now fixes both issues.
1) Don't call security_unix_may_send() hook during sendmsg() for
SOCK_SEQPACKET, and ensure that sendmsg() can only be called on a
connected socket so as not to bypass the security_unix_stream_connect()
hook.
2) Return -EINVAL if sendto() is called on SOCK_SEQPACKET with an address
supplied.
Please review and apply if ok.
Signed-off-by: James Morris <redacted>
From: Ross Kendall Axe <hidden> Date: 2004-11-19 03:24:02
Chris Wright wrote:
Why not make a unix_seq_sendmsg, which is a very small wrapper?
e.g.
static int unix_seq_sendmsg(struct kiocb *kiocb, struct socket *sock,
struct msghdr *msg, size_t len)
{
struct sock *sk = sock->sk;
if (sk->sk_type == SOCK_SEQPACKET && sk->sk_state != TCP_ESTABLISHED)
return -ENOTCONN;
if (msg->msg_name || msg->msg_namelen)
return -EINVAL;
return unix_dgram_sendmsg(kiocb, sock, msg, len);
}
-chris
Taking this idea further, couldn't we split unix_dgram_sendmsg into 2
functions, do_unix_dgram_sendmsg and do_unix_connectionless_sendmsg (and
similarly for unix_stream_sendmsg), then all we'd need is:
<pseudocode>
static int do_unix_dgram_sendmsg(...);
static int do_unix_stream_sendmsg(...);
static int do_unix_connectionless_sendmsg(...);
static int do_unix_connectional_sendmsg(...);
static int unix_dgram_sendmsg(struct kiocb *kiocb, struct socket *sock,
struct msghdr *msg, size_t len)
{
return do_unix_connectionless_sendmsg(kiocb, sock, msg, len,
do_unix_dgram_sendmsg);
}
static int unix_stream_sendmsg(struct kiocb *kiocb, struct socket *sock,
struct msghdr *msg, size_t len)
{
return do_unix_connectional_sendmsg(kiocb, sock, msg, len,
do_unix_stream_sendmsg);
}
static int unix_seqpacket_sendmsg(struct kiocb *kiocb, struct socket *sock,
struct msghdr *msg, size_t len)
{
return do_unix_connectional_sendmsg(kiocb, sock, msg, len,
do_unix_dgram_sendmsg);
}
</pseudocode>
What do we think?
Ross