From: Chris Wright <hidden> Date: 2004-11-16 08:41:40
* Stephen Smalley (sds@epoch.ncsc.mil) wrote:
On Sun, 2004-11-14 at 13:13, Ross Kendall Axe wrote:
quoted
With CONFIG_SECURITY_NETWORK=y and CONFIG_SECURITY_SELINUX=y, using
SOCK_SEQPACKET unix domain sockets causes an oops in the superfluous(?)
call to security_unix_may_send in sock_dgram_sendmsg. This patch avoids
making this call for SOCK_SEQPACKET sockets.
I'd prefer to track down the actual issue in the SELinux code and
correct it than just omit the security hook call entirely. Do you have
the Oops output and a trivial test case? Thanks.
Well, there is one simple case that will trigger the Oops. Send a
SEQPACKET to a connected but not yet accepted socket. In this case
other->sk_socket is still NULL, and SELinux will deref the NULL pointer
in selinux_socket_may_send() when geting other_isec. There is already
a check in unix_stream_connect, which is all that's used for normal unix
stream sockets. But the seqpacket socket then uses unix_dgram_sendmsg,
so triggers the may_send check as well.
thanks,
-chris
--
Linux Security Modules http://lsm.immunix.orghttp://lsm.bkbits.net
From: Ross Kendall Axe <hidden> Date: 2004-11-17 22:01:05
Chris Wright wrote:
* Stephen Smalley (sds@epoch.ncsc.mil) wrote:
quoted
On Sun, 2004-11-14 at 13:13, Ross Kendall Axe wrote:
quoted
With CONFIG_SECURITY_NETWORK=y and CONFIG_SECURITY_SELINUX=y, using
SOCK_SEQPACKET unix domain sockets causes an oops in the superfluous(?)
call to security_unix_may_send in sock_dgram_sendmsg. This patch avoids
making this call for SOCK_SEQPACKET sockets.
I'd prefer to track down the actual issue in the SELinux code and
correct it than just omit the security hook call entirely. Do you have
the Oops output and a trivial test case? Thanks.
Well, there is one simple case that will trigger the Oops. Send a
SEQPACKET to a connected but not yet accepted socket. In this case
other->sk_socket is still NULL, and SELinux will deref the NULL pointer
in selinux_socket_may_send() when geting other_isec. There is already
a check in unix_stream_connect, which is all that's used for normal unix
stream sockets. But the seqpacket socket then uses unix_dgram_sendmsg,
so triggers the may_send check as well.
thanks,
-chris
A possibility that hadn't occurred to me was using sendto to send packets
without connecting. Is this supposed to work? If so, then my patch is
indeed inappropriate. If not, then that needs fixing also.
Ross
From: Ross Kendall Axe <hidden> Date: 2004-11-18 00:13:02
Ross Kendall Axe wrote:
A possibility that hadn't occurred to me was using sendto to send packets
without connecting. Is this supposed to work? If so, then my patch is
indeed inappropriate. If not, then that needs fixing also.
Ross
Well, my reading of socket(2) suggests that it's _not_ supposed to work.
This patch causes sendmsg on SOCK_SEQPACKET unix domain sockets to return
EISCONN or ENOTSUPP as appropriate if the 'to' address is specified. It
also causes recvmsg to return EINVAL on unconnected sockets. This
behaviour is consistent with SOCK_STREAM sockets.
signed-off-by: Ross Axe <redacted>
From: James Morris <hidden> Date: 2004-11-18 03:43:00
On Thu, 18 Nov 2004, Ross Kendall Axe wrote:
Ross Kendall Axe wrote:
quoted
A possibility that hadn't occurred to me was using sendto to send packets
without connecting. Is this supposed to work? If so, then my patch is
indeed inappropriate. If not, then that needs fixing also.
Well, my reading of socket(2) suggests that it's _not_ supposed to work.
sendto() on a non connected socket should fail with ENOTCONN.
This patch causes sendmsg on SOCK_SEQPACKET unix domain sockets to return
EISCONN or ENOTSUPP as appropriate if the 'to' address is specified.
For sendto():
The address must be ignored on a connected mode socket (i.e. in this
case).
According to the send(2) man page, we may return EISCONN if the address
and addr length are not NULL and zero. I think that the man page is
incorrect. Posix says that EISCONN means "A destination address was
specified and the socket is already connected", not "A destination address
was specified and the socket is connected mode". i.e. we should only
return EISCONN if the socket is in a connected state.
I'm not sure if we should return any error at all if an address is
supplied to sendto() on SOCK_SEQPACKET. We're only required to ignore it.
I would say that we should return an error as it is likely a progamming
mistake in the application and we should let them know.
However, as mentioned above, I don't think EISCONN is appropriate in this
case. EINVAL might be better.
It also causes recvmsg to return EINVAL on unconnected sockets. This
behaviour is consistent with SOCK_STREAM sockets.
This seems incorrect too, Posix says to use ENOTCONN.
There is a non SELinux-related bug lurking in this code. I got this oops
when trying to kill a modified version of seqpacket-crash which keeps
sending in a loop and uses sendto() and an address with SOCK_SEQPACKET.
------------[ cut here ]------------
kernel BUG at include/asm/spinlock.h:133!
invalid operand: 0000 [#1]
PREEMPT SMP
Modules linked in: ipv6 binfmt_misc video ac e1000 3c59x
CPU: 0
EIP: 0060:[<c03393b2>] Not tainted VLI
EFLAGS: 00010282 (2.6.10-rc2)
EIP is at _spin_lock_bh+0x4b/0x55
eax: 0000000e ebx: f757b04c ecx: c038c60c edx: 00000292
esi: f757b04c edi: f73f096c ebp: c1bf8ed4 esp: c1bf8ec8
ds: 007b es: 007b ss: 0068
Process seqpacket-crash (pid: 4989, threadinfo=c1bf8000 task=f75fd530)
Stack: c034c39c c02c171e f757b02c c1bf8ee4 c02c171e f79448d4 f73f098c c1bf8f0c
c02be9d4 f73f0960 f757b02c 00000000 00000000 ffffffff f73f098c 00000000
dfff3b20 c1bf8f1c c02be96b 00000000 f79448d4 c1bf8f38 c0151b2c f73f098c
Call Trace:
[<c010336d>] show_stack+0x7a/0x90
[<c01034ee>] show_registers+0x152/0x1ca
[<c01036f5>] die+0x100/0x184
[<c0103b53>] do_invalid_op+0xd2/0xea
[<c010301b>] error_code+0x2b/0x30
[<c02c171e>] lock_sock+0x20/0x50
[<c02be9d4>] sock_fasync+0x45/0x147
[<c02be96b>] sock_close+0x19/0x3d
[<c0151b2c>] __fput+0x11d/0x15b
[<c015052a>] filp_close+0x42/0x74
[<c011a699>] put_files_struct+0x87/0xfc
[<c011b440>] do_exit+0x17b/0x48d
[<c011b7f9>] do_group_exit+0x32/0x9e
[<c0102525>] sysenter_past_esp+0x52/0x
--------------------
i.e.:
static inline void _raw_spin_lock(spinlock_t *lock)
{
#ifdef CONFIG_DEBUG_SPINLOCK
if (unlikely(lock->magic != SPINLOCK_MAGIC)) {
printk("eip: %p\n", __builtin_return_address(0));
BUG();
}
#endif
- James
--
James Morris
[off-list ref]
From: James Morris <hidden> Date: 2004-11-18 04:25:53
On Wed, 17 Nov 2004, James Morris wrote:
There is a non SELinux-related bug lurking in this code.
I also got this when trying to kill the server (which seems to run at 100%
during exit after receving a message sent with sendto() + address):
Badness in sk_del_node_init at include/net/sock.h:343
[<c010339a>] dump_stack+0x17/0x19
[<c03193bc>] __unix_remove_socket+0x64/0x66
[<c03196e4>] unix_release_sock+0x2b/0x259
[<c02bdf07>] sock_release+0x7a/0xda
[<c02be973>] sock_close+0x21/0x3d
[<c0151b2c>] __fput+0x11d/0x15b
[<c015052a>] filp_close+0x42/0x74
[<c0102525>] sysenter_past_esp+0x52/0x71
Which is:
static __inline__ int sk_del_node_init(struct sock *sk)
{
int rc = __sk_del_node_init(sk);
if (rc) {
/* paranoid for a while -acme */
WARN_ON(atomic_read(&sk->sk_refcnt) == 1); <-- here
- James
--
James Morris
[off-list ref]
From: Ross Kendall Axe <hidden> Date: 2004-11-18 07:26:06
James Morris wrote:
On Thu, 18 Nov 2004, Ross Kendall Axe wrote:
quoted
Ross Kendall Axe wrote:
quoted
A possibility that hadn't occurred to me was using sendto to send packets
without connecting. Is this supposed to work? If so, then my patch is
indeed inappropriate. If not, then that needs fixing also.
Well, my reading of socket(2) suggests that it's _not_ supposed to work.
sendto() on a non connected socket should fail with ENOTCONN.
quoted
This patch causes sendmsg on SOCK_SEQPACKET unix domain sockets to return
EISCONN or ENOTSUPP as appropriate if the 'to' address is specified.
For sendto():
The address must be ignored on a connected mode socket (i.e. in this
case).
According to the send(2) man page, we may return EISCONN if the address
and addr length are not NULL and zero. I think that the man page is
incorrect. Posix says that EISCONN means "A destination address was
specified and the socket is already connected", not "A destination address
was specified and the socket is connected mode". i.e. we should only
return EISCONN if the socket is in a connected state.
The man page then goes on to say "the error ENOTCONN is returned when the
socket was not actually connected". Admittedly, this is not what my patch
does; it returns ENOTSUPP, as do SOCK_STREAM sockets.
I'm not sure if we should return any error at all if an address is
supplied to sendto() on SOCK_SEQPACKET. We're only required to ignore it.
I would say that we should return an error as it is likely a progamming
mistake in the application and we should let them know.
This was, after all, the point of the patch. Well, that and closing the
security hole opened by my earlier patch :-)
However, as mentioned above, I don't think EISCONN is appropriate in this
case. EINVAL might be better.
I would say that ENOTSUPP all the time would be more sensible. However, my
choice of error codes was determined by the ones used by SOCK_STREAM.
SOCK_SEQPACKET and SOCK_STREAM should use the same error codes, I would
say. Further, they should use the codes specified by POSIX.
quoted
It also causes recvmsg to return EINVAL on unconnected sockets. This
behaviour is consistent with SOCK_STREAM sockets.
This seems incorrect too, Posix says to use ENOTCONN.
That seems eminently sensible. Again, I was just cut-n'-pasting from
SOCK_STREAM. If these error codes are wrong, then SOCK_STREAM also needs
fixing.
There is a non SELinux-related bug lurking in this code.
IMHO, there never was an SELinux bug here. SELinux merely exposed an
existing bug.
I got this oops
when trying to kill a modified version of seqpacket-crash which keeps
sending in a loop and uses sendto() and an address with SOCK_SEQPACKET.
I'm unable to reproduce that, or the bug you mention in your other
message. Care to send us your code?
- James
I think that af_unix.c needs a bit of cleaning up. All of the functions
are named as being stream vs dgram, even when the issue is connectionless
vs connection-oriented. For example, unix_connectionless_connect would
make a lot more sense than unix_dgram_connect. sendmsg and recvmsg are the
worst since they require a mixture of SOCK_STREAM and SOCK_DGRAM
semantics. It would be nice to rewite unix_dgram_recvmsg and
unix_stream_recvmsg as four helper functions dealing with the
connectionless, connection-oriented, datagram and stream operations and
then have 3 wrapper functions (one for each socket type) calling the
appropriate helpers. This is all strictly IMHO, of course.
Ross
I think that af_unix.c needs a bit of cleaning up. All of the functions
are named as being stream vs dgram, even when the issue is connectionless
vs connection-oriented.
From: James Morris <hidden> Date: 2004-11-18 08:27:57
Here's a fix for the SELinux related problem.
What's happening is that mixing stream and dgram ops for SEQPACKET is
having some unfortunate side effects.
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.
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.
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.
Note that this only solves the problem for the LSM hook.
Patch below, please review.
The other issue discussed -- server goes into a hard loop (and/or various
lock/refcount related bugs) when the client sends a message via sendto()
with an address supplied -- needs to be resolved separately.
---
net/unix/af_unix.c | 11 ++++++++---
1 files changed, 8 insertions(+), 3 deletions(-)
diff -purN -X dontdiff linux-2.6.10-rc2.o/net/unix/af_unix.c linux-2.6.10-rc2.w/net/unix/af_unix.c
Well, my reading of socket(2) suggests that it's _not_ supposed to work.
sendto() on a non connected socket should fail with ENOTCONN.
Not entirely true at all. A network protocol can implement lazy binding
and
do implicit binding on the sendto. Other protocols might not actually
have
a receiving component so have no bind() functionality at all.
According to the send(2) man page, we may return EISCONN if the address
and addr length are not NULL and zero. I think that the man page is
incorrect. Posix says that EISCONN means "A destination address was
specified and the socket is already connected", not "A destination address
was specified and the socket is connected mode". i.e. we should only
return EISCONN if the socket is in a connected state.
POSIX 1003.1g draft 6.4 permits a user to pass a "null" address for
various things. Indeed some systems implement send() as sendto() with a
NULL, 0 address component and some user space does likewise. It also has
a lot to say on the other cases although I don't think it ever fully got
past draft state.
You also want to look at TCP/IP illustrated to see some of the
assumptions handed down from on high by BSD and which should not be
broken.
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.
What about half closed and other connected states ? This patch seems
inadequate for things like X.25
One thing that looks broken (unrelated to the patch I posted) is that
unix_dgram_sendmsg() already does not check sk->sk_shutdown &
SEND_SHUTDOWN for SOCK_SEQPACKET.
Looks like a real bug yes.
As to the other stuff I think the only change needed is to check the
queued asynchronous error and report that before going on to the
connected test
From: James Morris <hidden> Date: 2004-11-19 02:15:09
On Thu, 18 Nov 2004, Alan Cox wrote:
On Iau, 2004-11-18 at 08:27, James Morris wrote:
quoted
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.
What about half closed and other connected states ? This patch seems
inadequate for things like X.25
The patch only affects the Unix code and does not change existing
semantics for other connected states.
One thing that looks broken (unrelated to the patch I posted) is that
unix_dgram_sendmsg() already does not check sk->sk_shutdown &
SEND_SHUTDOWN for SOCK_SEQPACKET.
- James
--
James Morris
[off-list ref]
From: James Morris <hidden> Date: 2004-11-19 02:17:48
On Thu, 18 Nov 2004, Alan Cox wrote:
On Iau, 2004-11-18 at 03:42, James Morris wrote:
quoted
quoted
Well, my reading of socket(2) suggests that it's _not_ supposed to work.
sendto() on a non connected socket should fail with ENOTCONN.
Not entirely true at all. A network protocol can implement lazy binding
and do implicit binding on the sendto. Other protocols might not
actually have a receiving component so have no bind() functionality at
all.
I got this from the Linux man page for sendto():
If sendto is used on a connection-mode (SOCK_STREAM, SOCK_SEQPACKET)
socket, the parameters to and tolen are ignored (and the error EISCONN
may be returned when they are not NULL and 0), and the error ENOTCONN
is returned when the socket was not actually connected.
And Posix 1003.1 offers the following error code for sendto():
The sendto() function shall fail if:
...
[ENOTCONN]
The socket is connection-mode but is not connected.
(I'm not saying you're wrong).
POSIX 1003.1g draft 6.4 permits a user to pass a "null" address for
various things. Indeed some systems implement send() as sendto() with a
NULL, 0 address component and some user space does likewise. It also has
a lot to say on the other cases although I don't think it ever fully got
past draft state.
sendto() with a NULL address will still work fine.
- James
--
James Morris
[off-list ref]
From: James Morris <hidden> Date: 2004-11-19 03:13:45
On Thu, 18 Nov 2004, Alan Cox wrote:
As to the other stuff I think the only change needed is to check the
queued asynchronous error and report that before going on to the
connected test
How about this?
(Also now ignores any supplied address per
http://www.opengroup.org/onlinepubs/009695399/functions/sendto.html)
---
Signed-off-by: James Morris <redacted>
diff -purN -X dontdiff linux-2.6.10-rc2.o/net/unix/af_unix.c linux-2.6.10-rc2.w3/net/unix/af_unix.c
From: James Morris <hidden> Date: 2004-11-19 16:25:09
On Fri, 19 Nov 2004, Alan Cox wrote:
Looks right to me, the ECONNRESET is no longer being lost.
Ok, here is a relative patch for Dave.
Please apply.
Signed-off-by: James Morris <redacted>
---
diff -purN -X dontdiff linux-2.6.10-rc2.w2/net/unix/af_unix.c linux-2.6.10-rc2.w3/net/unix/af_unix.c