Bug#645469: bind fails for AF_UNIX sockets with EINVAL
Btw, could we restrict this kludge to platforms that need it? See attached patch. I like the idea, slightly different patch is in. Petr -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
Bug#645469: bind fails for AF_UNIX sockets with EINVAL
2011/10/17 Robert Millan :
> 2011/10/17 Petr Salinger :
>> I just commited r3740 and r3739, at least one have to be active
>> with r3735 kernel.
Btw, could we restrict this kludge to platforms that need it? See
attached patch.
--
Robert Millan
Index: sys/un.h
===
--- sys/un.h (revision 3741)
+++ sys/un.h (working copy)
@@ -31,7 +31,9 @@
{
__SOCKADDR_COMMON (sun_);
char sun_path[104]; /* Path name, the kernel restrict it to 104, */
+#if defined(__i386__) || defined(__amd64__)
char __sun_user_compat[4]; /* but former user header used 108 */
+#endif
};
Index: sa_len.c
===
--- sa_len.c (revision 3741)
+++ sa_len.c (working copy)
@@ -22,6 +22,7 @@
#include
#include
#include
+#include
int
__libc_sa_len (sa_family_t af)
@@ -37,7 +38,7 @@
case AF_IPX:
return sizeof (struct sockaddr_ipx);
case AF_LOCAL:
- return sizeof (struct sockaddr_un) - sizeof(((struct sockaddr_un *) 0)->__sun_user_compat);
+ return offsetof (struct sockaddr_un, sun_path) + sizeof (((struct sockaddr_un *) 0)->sun_path);
}
return 0;
}
Bug#645469: bind fails for AF_UNIX sockets with EINVAL
2011/10/17 Petr Salinger :
> I just commited r3740 and r3739, at least one have to be active
> with r3735 kernel.
> I consider it cleanest way from current mess :-(
Yeah, I guess when the problem is so ugly the solution must be ugly too :-(
Have you tested r3740?
Btw, you forgot kfreebsd-{9,10}
--
Robert Millan
--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
Bug#645469: bind fails for AF_UNIX sockets with EINVAL
Yes, I think this should be handled in glibc, and the sockaddr_un be fixed to match what the kernel expects, the compat code would be there to fix applications built against the bogus sockaddr_un type. In fact, we already clip the passed size in our eglibc in bind() and connect(), it might suffice in eglibc to Silent truncation sounds a bit dangerous. Could this introduce a security problem? E.g. a malliciously placed socket which matches the truncated path. We do truncating already, only at 108, not at 104. It could only match if the socket name will be really 104 bytes long. The current situation with truncating is not worse than previous one. On the other hand we cannot change size of userland sockaddr_un without ABI change. To do it correctly it would mean to raise soname of eglibc and perform transition involving all packages. Is that really so bad? I guess the impact is minimal. How many libraries would make sockaddr_un part of their ABI? Affected might be all libraries which use sockaddr_un internally. I just commited r3740 and r3739, at least one have to be active with r3735 kernel. I consider it cleanest way from current mess :-( Petr -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
Bug#645469: bind fails for AF_UNIX sockets with EINVAL
2011/10/16 Petr Salinger :
>> Yes, I think this should be handled in glibc, and the sockaddr_un be
>> fixed to match what the kernel expects, the compat code would be there
>> to fix applications built against the bogus sockaddr_un type.
>
> In fact, we already clip the passed size in our eglibc in
> bind() and connect(), it might suffice in eglibc to
Silent truncation sounds a bit dangerous. Could this introduce a
security problem? E.g. a malliciously placed socket which matches the
truncated path.
> On the other hand we cannot change size of userland
> sockaddr_un without ABI change. To do it correctly it would mean
> to raise soname of eglibc and perform transition involving all packages.
Is that really so bad? I guess the impact is minimal. How many
libraries would make sockaddr_un part of their ABI?
> May be the limit can be raised in upstream in kernel only,
> to allow 108 instead of 104 only bytes.
Maybe. You mean something like this?
struct sockaddr_un
{
...
#ifdef _KERNEL
sun_path[108];
#else
sun_path[104];
#endif
}
It's ugly though. I wouldn't count on this being accepted, specially
not backported. If it doesn't serve us to support people running
upstream kernels, is there a point in pushing this change?
--
Robert Millan
--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
Bug#645469: bind fails for AF_UNIX sockets with EINVAL
More importantly, there is the question you raised of whether this should be done in userspace by libc instead. That would avoid upstream having to wonder, "why should we care what happens when someone using a BSD4.3-style bind() calls our BSD4.4-style kernel"? So it's tempting. Yes, I think this should be handled in glibc, and the sockaddr_un be fixed to match what the kernel expects, the compat code would be there to fix applications built against the bogus sockaddr_un type. In fact, we already clip the passed size in our eglibc in bind() and connect(), it might suffice in eglibc to Index: glibc-ports/kfreebsd/sa_len.c === --- glibc-ports/kfreebsd/sa_len.c (revision 3738) +++ glibc-ports/kfreebsd/sa_len.c (working copy) @@ -37,7 +37,7 @@ case AF_IPX: return sizeof (struct sockaddr_ipx); case AF_LOCAL: - return sizeof (struct sockaddr_un); + return sizeof (struct sockaddr_un) - 4; } return 0; } On the other hand we cannot change size of userland sockaddr_un without ABI change. To do it correctly it would mean to raise soname of eglibc and perform transition involving all packages. I guess upstream would appreciate if we get rid of the length limit. It sets the maximum path length for sockets to 104 chars, is that so? I'm not entirely sure what you mean with that. If you mean making sockaddr_un variable size, well that cannot be done, as it's expected that sockaddr_storage can hold space for any sockaddr type. May be the limit can be raised in upstream in kernel only, to allow 108 instead of 104 only bytes. The sockaddr_storage size is 128 bytes, the technical limit for sun_path is therefore 126 bytes. It would help in upstream kernel to the linux emulation layer. Petr
Bug#645469: bind fails for AF_UNIX sockets with EINVAL
More importantly, there is the question you raised of whether this
should be done in userspace by libc instead. That would avoid
upstream having to wonder, "why should we care what happens when
someone using a BSD4.3-style bind() calls our BSD4.4-style kernel"?
So it's tempting.
For now I propose to silently shorten too long size in kernel.
I.e instead of
if (soun->sun_len > sizeof(struct sockaddr_un))
return (EINVAL);
add
if (soun->sun_len > sizeof(struct sockaddr_un))
{
if (soun->sun_len > (4 + sizeof(struct sockaddr_un)))
return (EINVAL);
else
soun->sun_len = sizeof(struct sockaddr_un);
}
This would allow all usual situation to be handled without problem.
Even upstream have problem with added check in linux emulation, see
http://www.freebsd.org/cgi/cvsweb.cgi/src/sys/compat/linux/linux_socket.c.diff?r1=1.108;r2=1.109;f=h
In long term, we could clip the size in libc, but for stable security
upload we should just silently clip the passed size.
Petr
--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
Bug#645469: bind() fails for AF_UNIX sockets with EINVAL
On Sun, 2011-10-16 at 13:44:29 +0200, Robert Millan wrote: > 2011/10/16 Jonathan Nieder : > > More importantly, there is the question you raised of whether this > > should be done in userspace by libc instead. That would avoid > > upstream having to wonder, "why should we care what happens when > > someone using a BSD4.3-style bind() calls our BSD4.4-style kernel"? > > So it's tempting. Yes, I think this should be handled in glibc, and the sockaddr_un be fixed to match what the kernel expects, the compat code would be there to fix applications built against the bogus sockaddr_un type. > I guess upstream would appreciate if we get rid of the length limit. > It sets the maximum path length for sockets to 104 chars, is that so? I'm not entirely sure what you mean with that. If you mean making sockaddr_un variable size, well that cannot be done, as it's expected that sockaddr_storage can hold space for any sockaddr type. regards, guillem -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
Bug#645469: bind() fails for AF_UNIX sockets with EINVAL
2011/10/16 Jonathan Nieder : > More importantly, there is the question you raised of whether this > should be done in userspace by libc instead. That would avoid > upstream having to wonder, "why should we care what happens when > someone using a BSD4.3-style bind() calls our BSD4.4-style kernel"? > So it's tempting. I guess upstream would appreciate if we get rid of the length limit. It sets the maximum path length for sockets to 104 chars, is that so? -- Robert Millan -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
Bug#645469: bind() fails for AF_UNIX sockets with EINVAL
Jonathan Nieder wrote: > --- i/sys/kern/uipc_syscalls.c > +++ w/sys/kern/uipc_syscalls.c > @@ -1703,11 +1703,18 @@ getsockaddr(namp, uaddr, len) [...] > sa->sa_len = len; > + datalen = len - offsetof(struct sockaddr, sa_data[0]); > + p = memchr(sa->sa_data, '\0', datalen); > + if (p) > + sa_len = p - (const char *)sa; > *namp = sa; Ah, this is clearly broken since it applies to address types other than AF_UNIX. Guarding it with a test of sa_family would take care of that. More importantly, there is the question you raised of whether this should be done in userspace by libc instead. That would avoid upstream having to wonder, "why should we care what happens when someone using a BSD4.3-style bind() calls our BSD4.4-style kernel"? So it's tempting. -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
Bug#645469: bind() fails for AF_UNIX sockets with EINVAL
Robert Millan wrote:
> 2011/10/16 Jonathan Nieder :
>> unsigned char sun_len;
>> unsigned char sun_family;
>> char sun_path[108]; /* Path name. */
>
> Is this 108 the actual length? ISTR it was just a placeholder.
It's the actual size of the sun_path field of struct sockaddr_un,
if that's what you mean.
> has a macro to determine actual length:
>
> /* Evaluate to actual length of the `sockaddr_un' structure. */
> # define SUN_LEN(ptr) ((size_t) (((struct sockaddr_un *) 0)->sun_path)
> \
> + strlen ((ptr)->sun_path))
Ah, thanks for this clarification. glibc's reference manual says:
You should compute the "length" parameter for a socket address in
the local namespace as the sum of the size of the "sun_family"
component and the string length (/not/ the allocation size!) of
the file name string. This can be done using the macro "SUN_LEN":
In other words, you can call bind() with SUN_LEN(ptr) as the addrlen
argument instead of sizeof(struct sockaddr_un) to tell the kernel not to
copy so much, and to avoid depending on the magic number chosen in the
definition of sockaddr_un.
However, the POSIX documentation for bind() gives an example of an AF_UNIX
socket with
if (bind(sfd, (struct sockaddr *) &my_addr,
sizeof(struct sockaddr_un)) == -1)
Similarly, the example in the bind(2) manpage from the man-pages project
uses sizeof(struct sockaddr_un) as the addrlen argument.
Regarding the magic number, POSIX provides some more insight:
The size of "sun_path" has intentionally been left undefined.
This is because different implementations use different sizes.
For example, 4.3 BSD uses a size of 108, and 4.4 BSD uses a
size of 104. Since most implementations originate from BSD
versions, the size is typically in the range 92 to 108.
[...]
>> I wonder whether there would be any downside to changing that 104 in
>> the kernel to 108.
>
> Breaking kernel ABI for everyone not using a patched kernel. Not a good
> thing.
If it's desirable, the change could be made upstream.
>> Separately from that, it would be helpful to know where the buffer
>> overflowed in #645377 is, since maybe it could be made bigger without
>> changing the layout of struct sockaddr_un.
>
> I don't think this matters, it's an instance of sockaddr_un.
If sockaddr_un is part of the ABI as an argument to some function,
it would matter.
> If you
> make sun_path[] bigger in the kernel, then instead of 160 you can
> overflow it with a larger value
[...]
> The kernel-side of things is now doing the right thing AFAICS.
It's breaking userspace apps that followed documentation to the letter
and worked before.
How about this (untested)?
diff --git i/sys/kern/uipc_syscalls.c w/sys/kern/uipc_syscalls.c
index 3b83e1c..7b4a11e 100644
--- i/sys/kern/uipc_syscalls.c
+++ w/sys/kern/uipc_syscalls.c
@@ -1703,11 +1703,18 @@ getsockaddr(namp, uaddr, len)
if (error) {
free(sa, M_SONAME);
} else {
+ const char *p;
+ size_t datalen;
+
#if defined(COMPAT_OLDSOCK) && BYTE_ORDER != BIG_ENDIAN
if (sa->sa_family == 0 && sa->sa_len < AF_MAX)
sa->sa_family = sa->sa_len;
#endif
sa->sa_len = len;
+ datalen = len - offsetof(struct sockaddr, sa_data[0]);
+ p = memchr(sa->sa_data, '\0', datalen);
+ if (p)
+ sa_len = p - (const char *)sa;
*namp = sa;
}
return (error);
--
To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org
with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
Bug#645469: bind() fails for AF_UNIX sockets with EINVAL
2011/10/16 Jonathan Nieder : > Here's "struct sockaddr_un" in eglibc (socket/sys/un.h, after a little > typedef-chasing): > > unsigned char sun_len; > unsigned char sun_family; > char sun_path[108]; /* Path name. */ Is this 108 the actual length? ISTR it was just a placeholder. has a macro to determine actual length: /* Evaluate to actual length of the `sockaddr_un' structure. */ # define SUN_LEN(ptr) ((size_t) (((struct sockaddr_un *) 0)->sun_path)\ + strlen ((ptr)->sun_path)) Does someone know what this means? > And here it is in the kernel: > > unsigned char sun_len; > unsigned char sun_family; > char sun_path[104]; /* Path name. */ > > I wonder whether there would be any downside to changing that 104 in > the kernel to 108. Breaking kernel ABI for everyone not using a patched kernel. Not a good thing. > Separately from that, it would be helpful to know where the buffer > overflowed in #645377 is, since maybe it could be made bigger without > changing the layout of struct sockaddr_un. I don't think this matters, it's an instance of sockaddr_un. If you make sun_path[] bigger in the kernel, then instead of 160 you can overflow it with a larger value (exploit [1] works by declaring its own sockaddr_un, which has larger sun_path and whose content is not null-terminated). The kernel-side of things is now doing the right thing AFAICS. [1] http://www.exploit-db.com/exploits/17908/ -- Robert Millan -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
Bug#645469: bind() fails for AF_UNIX sockets with EINVAL
Robert Millan wrote: > http://security.freebsd.org/patches/SA-11:05/unix.patch Thanks for the pointer. Here's "struct sockaddr_un" in eglibc (socket/sys/un.h, after a little typedef-chasing): unsigned char sun_len; unsigned char sun_family; char sun_path[108]; /* Path name. */ And here it is in the kernel: unsigned char sun_len; unsigned char sun_family; char sun_path[104]; /* Path name. */ I wonder whether there would be any downside to changing that 104 in the kernel to 108. That is, which interfaces exposing the kernel's "struct sockaddr_un" to userspace should we be paying attention to? Separately from that, it would be helpful to know where the buffer overflowed in #645377 is, since maybe it could be made bigger without changing the layout of struct sockaddr_un. Sleepily, Jonathan -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
Bug#645469: bind() fails for AF_UNIX sockets with EINVAL
2011/10/16 Jonathan Nieder : > E: unable to bind server socket to file '/tmp/cuptyL8GPC': Invalid > argument > E: error performing command 'update' > > By contrast, kfreebsd-image-8.2-1-amd64 8.2-8+gcc45 works fine. > > Trying to track this down, I tried the example from the bind(2) > manpage (from manpages-dev 3.32-0.2), with MY_SOCK_PATH set to > "/tmp/somepath" so it wouldn't require root permissions. Same story > there --- the 8.2 kernel works fine, while kfreebsd 9 gives me > > bind: Invalid argument Upstream just added an EINVAL condition in bind(2) to fix #645377 (see http://security.freebsd.org/patches/SA-11:05/unix.patch). Maybe this is a collateral effect? Could you check with 9.0~svn225586-1 [1] (which unlike 9.0~svn225873 doesn't have the security fix), or with 8.2-9 [2] (which unlike 8.2-8 *does* have it)? Btw 8.2-9 is urgency=high, if it's affected by #645469 please let us know ASAP so we can put its migration on hold. [1] http://snapshot.debian.org/package/kfreebsd-9/9.0~svn225586-1/ [2] currently preparing an upload, will be in incoming in an hour or so -- Robert Millan -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
Bug#645469: bind() fails for AF_UNIX sockets with EINVAL
Package: kfreebsd-9 Version: 9.0~svn225873 Severity: important Hi, Typical kfreebsd-amd64 experimental system. When I try to use "cupt update" after booting with kfreebsd 9, it always fails: E: unable to bind server socket to file '/tmp/cuptyL8GPC': Invalid argument E: error performing command 'update' By contrast, kfreebsd-image-8.2-1-amd64 8.2-8+gcc45 works fine. Trying to track this down, I tried the example from the bind(2) manpage (from manpages-dev 3.32-0.2), with MY_SOCK_PATH set to "/tmp/somepath" so it wouldn't require root permissions. Same story there --- the 8.2 kernel works fine, while kfreebsd 9 gives me bind: Invalid argument For reference, the installed libc0.1 is 2.13-21. Anyway, thought you might like to know. Regards, Jonathan -- To UNSUBSCRIBE, email to debian-bugs-dist-requ...@lists.debian.org with a subject of "unsubscribe". Trouble? Contact listmas...@lists.debian.org
