The makesockaddr() function in the socket module assumes that
AF_UNIX addresses have a null-terminated sun_path, but Linux
actually allows unterminated addresses using all 108 bytes of
sun_path (for normal filesystem sockets, that is, not just
abstract addresses).

When receiving such an address (e.g. in accept() from a
connecting peer), makesockaddr() will run past the end and return
extraneous bytes from the stack, or fail because they can't be
decoded, or perhaps segfault in extreme cases.

This can't currently be tested from within Python as Python also
refuses to accept address arguments which would fill the whole of
sun_path, but the attached linux-pass-unterminated.diff (for 2.x
and 3.x) enables them for Linux. With the patch applied:

Python 2.7a4+ (trunk, Apr  8 2010, 18:20:28) 
[GCC 4.2.4 (Ubuntu 4.2.4-1ubuntu4)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import socket
>>> s = socket.socket(socket.AF_UNIX)
>>> s.bind("a" * 108)
>>> s.getsockname()
'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\xfa\xbf\xa8)\xfa\xbf\xec\x15\n\x08l\xaaY\xb7\xb8CZ\xb7'
>>> len(_)
126

Also attached are some unit tests for use with the above patch, a
couple of C programs for checking OS behaviour (you can also see
the bug by doing accept() in Python and using the bindconn
program), and patches aimed at fixing the problem.

Firstly, the return-unterminated-* patches make makesockaddr()
scan sun_path for the first null byte as before (if it's not a
Linux abstract address), but now stop at the end of the structure
as indicated by the addrlen argument.

However, there's one more catch before this will work on Linux,
which is that Linux system calls return the length of the address
they *would* have stored in the structure had there been room for
it, which in this case is one byte longer than the official size
of a sockaddr_un structure, due to the missing null terminator.

The addrlen-* patches handle this by always calling
makesockaddr() with the actual buffer size if it is less than the
returned length. This silently ignores any truncation, but I'm
not sure how to do anything sensible about that, and some
operating systems (e.g. FreeBSD) just silently truncate the
address anyway and don't return the original length (POSIX
doesn't make clear which, if either, behaviour is required).
Once these patches are applied, the tests pass.

There is one other issue: the patches for 3.x retain the
assumption that socket paths are in UTF-8, but they should
actually be handled according to PEP-383. I've got a patch for
that, but I'll open a separate issue for it since the handling of
the Linux abstract namespace isn't documented and there's some
slightly unobvious behaviour that people might be depending on.

Attaching the C test programs I forgot to attach yesterday -
sorry about that. I've also tried these programs, and the
patches, on FreeBSD 5.3 (an old version from late 2004). I found
that it accepted unterminated addresses as well, and unlike Linux
it did not normally null-terminate addresses at all - the
existing socket code only worked for addresses shorter than
sun_path because it zero-filled the structure beforehand. The
return-unterminated patches worked with or without the
zero-filling.

Unlike Linux, FreeBSD also accepted oversized sockaddr_un
structures (sun_path longer than its definition), so just
allowing unterminated addresses wouldn't make the full range of
addresses usable there. That said, I did get a kernel panic
shortly after testing with oversized addresses, so perhaps it's
not a good idea to actually use them :)

With the patches applied except linux-pass-unterminated.diff, I get the following test failure:

======================================================================
ERROR: testMaxPathLen (test.test_socket.TestLinuxPathLen)
----------------------------------------------------------------------

Traceback (most recent call last):
  File "/home/antoine/py3k/__svn__/Lib/test/test_socket.py", line 1435, in testMaxPathLen
    self.sock.bind(path)
socket.error: AF_UNIX path too long

I guess this test should simply removed.
In any case, here is an up-to-date patch against 3.x.

I guess this test should simply removed.

(not sure which test you are referring to: the test case, or the
test for too long path names:) I think both tests need to stay.

Instead, I think that testMaxPathLen is incorrect: it doesn't
take into account the terminating NUL, which also must fit
into the 108 bytes (IIUC). baikie: why did the test pass for
you?

baikie: why did the test pass for you?

The test passes (I assume) if linux-pass-unterminated.diff is applied. The latter patch is only meant to exhibit the issue, though, not to be checked in.

> baikie: why did the test pass for you?

The test passes (I assume) if linux-pass-unterminated.diff is applied. The latter patch is only meant to exhibit the issue, though, not to be checked in.

No, I meant for linux-pass-unterminated.diff to be checked in so
that applications could always send datagrams back to the address
they got them from, even when it was 108 bytes long. As it is
run only on Linux, testMaxPathLen does not leave space for a null
terminator because Linux just ignores it (that is what makes it
possible to bind to a 108-byte address and thus trigger the bug).

baikie, coming back to your original message: what precisely makes you believe that sun_path does not need to be null-terminated on Linux?

baikie, coming back to your original message: what precisely makes you believe that sun_path does not need to be null-terminated on Linux?

That's the way I demonstrated the bug - the only way to bind to a
108-byte path is to pass it without null termination, because
Linux will not accept an oversized sockaddr_un structure (e.g. a
108-byte path plus null terminator). Also, unless it's on OS/2,
Python's existing code never includes the null terminator in the
address size it passes to the system call, so a correctly-
behaving OS should never see it.

However, it does now occur to me that a replacement libc
implementation for Linux could try to do something with sun_path
during the call and assume it's null-terminated even though the
kernel doesn't, so it may be best to keep the null termination
requirement after all. In that case, there would be no way to
test for the bug from within Python, so the test problems would
be somewhat moot (although the test code could still be used by
changing UNIX_PATH_MAX from 108 to 107).

Attaching four-space-indent versions of the original patches (for
2.x and 3.x), and tests incorporating Antoine's use of
assertRaisesRegexp.

I've updated the PEP-383 patches at issue bpo-8373 with separate
versions for if the linux-pass-unterminated patch is applied or
not.

If it's not essential to have unit tests for the overrun issue,
I'd suggest applying just the return-unterminated and addrlen
patches and omitting linux-pass-unterminated, for now at least.
This will leave Linux no worse off than a typical BSD-derived
platform.

I see. Looking at net/unix/af_unix.c:unix_mkname of Linux 2.6, there is a comment that says

Check unix socket name: [...]
- if started by not zero, should be NULL terminated (FS object)

However, the code then just does

/*
 * This may look like an off by one error but it is a bit more
 * subtle. 108 is the longest valid AF_UNIX path for a binding.
 * sun_path[108] doesnt as such exist.  However in kernel space
 * we are guaranteed that it is a valid memory location in our
 * kernel address buffer.
 */
((char *)sunaddr)[len] = 0;
len = strlen(sunaddr->sun_path)+1+sizeof(short);
return len;

So it doesn't actually check that it's null-terminated, but always sets the null termination in kernel based on the address length. Interesting.

With all the effort that went into the patch, I recommend to get it right: if there is space for the \0, include it. If the string size is exactly 108, and it's linux, write it unterminated. Else fail.

As for testing: we should then definitely have a test that, if you can create an 108 byte unix socket that its socket name is what we said it should be.

With all the effort that went into the patch, I recommend to get it right: if there is space for the \0, include it. If the string size is exactly 108, and it's linux, write it unterminated. Else fail.

As for testing: we should then definitely have a test that, if you can create an 108 byte unix socket that its socket name is what we said it should be.

The attached patches do those things, if I understand you
correctly (the test patches add such a test for Linux, and
linux-pass-unterminated uses memset() to zero out the area
between the end of the actual path and the end of the sun_path
array).

If you're talking about including the null in the address passed
to the system call, that does no harm on Linux, but I think the
more common practice is not to include it. The FreeBSD SUN_LEN
macro, for instance, is provided to calculate the address length
and does not include the null.

I meant to say that FreeBSD provides the SUN_LEN macro, but it
turns out that Linux does as well, and its version behaves the
same as FreeBSD's. The FreeBSD man pages state that the
terminating null is not part of the address:

http://www.freebsd.org/cgi/man.cgi?query=unix&apropos=0&sektion=0&manpath=FreeBSD+8.1-RELEASE&format=html

The examples in Stevens/Rago's "Advanced Programming in the Unix
Environment" also pass address lengths to bind(), etc. that do
not include the null.

The examples in Stevens/Rago's "Advanced Programming in the Unix
Environment" also pass address lengths to bind(), etc. that do
not include the null.

I didn't (mean to) suggest that the null must be included in the
length - only that it must be included in the path.

Is this a security issue or just a regular bug?

It's a potential security issue.

The patches look good to me, except that instead of passing
(addrlen > buflen) ? buflen : addrlen

as addrlen argument every time makesockaddr is called, I'd prefer if this min was done inside makesockaddr itself, i.e. perform min(addrlen, sizeof(struct sockaddr_un)) in the AF_UNIX switch case (especially since addrlen is only used for AF_UNIX).
Also, this would be the occasion to put a short explanatory comment (possibility of non NULL-terminated sun_path and unreliable length returned by syscalls).

On Sun 12 Jun 2011, Charles-François Natali wrote:

The patches look good to me, except that instead of passing
(addrlen > buflen) ? buflen : addrlen
as addrlen argument every time makesockaddr is called, I'd
prefer if this min was done inside makesockaddr itself,
i.e. perform min(addrlen, sizeof(struct sockaddr_un)) in the
AF_UNIX switch case (especially since addrlen is only used for
AF_UNIX).

Actually, I think it should be clamped at the top of the
function, since the branch for unknown address families ought to
use the length as well (it doesn't, but that's a separate issue).
I'm attaching new patches to do the check in makesockaddr(),
which also change the length parameters from int to socklen_t, in
case the OS returns a really huge value.

I'm also attaching new return-unterminated patches to handle the
possibility that addrlen is unsigned (socklen_t may be unsigned,
and addrlen *is* now unsigned in 3.x). This entailed specifying
what to do if addrlen < offsetof(struct sockaddr_un, sun_path),
i.e. if the address is truncated at least one byte before the
start of sun_path.

This may well never happen (Python's existing code would raise
SystemError if it did, due to calling
PyString_FromStringAndSize() with a negative length), but I've
made the new patches return None if it does, as None is already
returned if addrlen is 0. As another precedent of sorts, Linux
currently returns None (i.e. addrlen = 0) when receiving a
datagram from an unbound Unix socket, despite returning an empty
string (i.e. addrlen = offsetof(..., sun_path)) for the same
unbound address in other situations.

(I think the decoders for other address families should also
return None if addrlen is less than the size of the appropriate
struct, but again, that's a separate issue.)

Also, I noticed that on Linux, Python 3.x currently returns empty
addresses as bytes objects rather than strings, whereas the
patches I've provided make it return strings. In case this
change isn't acceptable for the 3.x maintenance branches, I'm
attaching return-unterminated-3.x-maint-new.diff which still
returns them as bytes (on Linux only).

To sum up the patch order:

2.x:
linux-pass-unterminated-4spc.diff
test-2.x-new.diff
return-unterminated-2.x-new.diff
addrlen-makesockaddr-2.x.diff (or addrlen-2.x-4spc.diff)

3.2:
linux-pass-unterminated-4spc.diff
test-3.x-new.diff
return-unterminated-3.x-maint-new.diff
addrlen-makesockaddr-3.x.diff (or addrlen-3.x-4spc.diff)

default:
linux-pass-unterminated-4spc.diff
test-3.x-new.diff
return-unterminated-3.x-trunk-new.diff
addrlen-makesockaddr-3.x.diff (or addrlen-3.x-4spc.diff)

As this is flagged as a high priority security issue shouldn't we be implementing needed source code changes? According to msg138224 "The patches look good to me".

I've rebased the patches onto all the currently released
branches, but since there are now so many variations required,
I've bundled the pass-unterminated and test patches into a single
set (enable-unterminated-), and the return-unterminated and
addrlen-makesockaddr patches into another (fix-overrun-), which
applies on top.

The fix-overrun patches can be applied on their own, but don't
include any tests.

The 3.5 branch has some more substantial changes which stop the
patches applying - I haven't looked into those yet.

Attaching patches for 3.5.