socket: Buffer overrun while reading unterminated AF_UNIX addresses

[50cef828-0e94-47a2-843d-cb13c9fb9120]

BPO	8372
Nosy	@loewis, @terryjreedy, @pitrou, @vstinner
Files	linux-pass-unterminated.diff: Allow non-null-terminated AF_UNIX addresses on Linux return-unterminated-2.x.diff: Stop at end of address as given by addrlen (2.x) return-unterminated-3.x.diff: Stop at end of address as given by addrlen (3.x) addrlen-2.x.diff: Don't use addrlen larger than original buffer (2.x) addrlen-3.x.diff: Don't use addrlen larger than original buffer (3.x) test-2.x.diff: Tests for Linux (2.x) test-3.x.diff: Tests for Linux (3.x) bindconn.c: Test program to bind to an address and connect to a server accept.c: Test program to accept connections and print the contents of sun_path issue8372.patch linux-pass-unterminated-4spc.diff return-unterminated-2.x-4spc.diff return-unterminated-3.x-4spc.diff addrlen-2.x-4spc.diff addrlen-3.x-4spc.diff test-2.x-new.diff test-3.x-new.diff addrlen-makesockaddr-2.x.diff addrlen-makesockaddr-3.x.diff return-unterminated-2.x-new.diff return-unterminated-3.x-maint-new.diff return-unterminated-3.x-trunk-new.diff enable-unterminated-2.7-2015-05-05.diff fix-overrun-2.7-2015-05-05.diff enable-unterminated-3.2-2015-05-05.diff fix-overrun-3.2-2015-05-05.diff enable-unterminated-3.3-2015-05-05.diff fix-overrun-3.3-2015-05-05.diff enable-unterminated-3.4-2015-05-05.diff fix-overrun-3.4-2015-05-05.diff enable-unterminated-3.5-2015-05-06.diff fix-overrun-3.5-2015-05-06.diff

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields: ```python assignee = None closed_at = None created_at = labels = ['type-security', 'extension-modules', '3.7'] title = 'socket: Buffer overrun while reading unterminated AF_UNIX\taddresses' updated_at = user = 'https://bugs.python.org/baikie' ``` bugs.python.org fields: ```python activity = actor = 'BreamoreBoy' assignee = 'none' closed = False closed_date = None closer = None components = ['Extension Modules'] creation = creator = 'baikie' dependencies = [] files = ['16874', '16875', '16876', '16877', '16878', '16879', '16880', '16898', '16899', '18753', '18770', '18771', '18772', '18773', '18774', '18775', '18776', '22384', '22385', '22386', '22387', '22388', '39297', '39298', '39299', '39300', '39301', '39302', '39303', '39304', '39309', '39310'] hgrepos = [] issue_num = 8372 keywords = ['patch'] message_count = 20.0 messages = ['102861', '102964', '115595', '115606', '115615', '115669', '115673', '115716', '116112', '116178', '116226', '116234', '116236', '138213', '138214', '138224', '138472', '242577', '242621', '242695'] nosy_count = 7.0 nosy_names = ['loewis', 'terry.reedy', 'pitrou', 'vstinner', 'baikie', 'neologix', 'rosslagerwall'] pr_nums = [] priority = 'high' resolution = None stage = 'patch review' status = 'open' superseder = None type = 'security' url = 'https://bugs.python.org/issue8372' versions = ['Python 2.7', 'Python 3.4', 'Python 3.5', 'Python 3.6', 'Python 3.7'] ```

[50cef828-0e94-47a2-843d-cb13c9fb9120]

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.

[50cef828-0e94-47a2-843d-cb13c9fb9120]

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 :)

[pitrou]

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.

[61337411-43fc-4a9c-b8d5-4060aede66d0]

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?

[pitrou]

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.

[50cef828-0e94-47a2-843d-cb13c9fb9120]

> 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).

[61337411-43fc-4a9c-b8d5-4060aede66d0]

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

[50cef828-0e94-47a2-843d-cb13c9fb9120]

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.

[50cef828-0e94-47a2-843d-cb13c9fb9120]

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.

[61337411-43fc-4a9c-b8d5-4060aede66d0]

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.

[50cef828-0e94-47a2-843d-cb13c9fb9120]

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.

[50cef828-0e94-47a2-843d-cb13c9fb9120]

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.

[61337411-43fc-4a9c-b8d5-4060aede66d0]

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.

[terryjreedy]

Is this a security issue or just a regular bug?

[pitrou]

It's a potential security issue.

[79528080-9d85-4d18-8a2a-8b1f07640dd7]

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).

[50cef828-0e94-47a2-843d-cb13c9fb9120]

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)

[83d2e70e-e599-4a04-b820-3814bbdb9bef]

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".

[50cef828-0e94-47a2-843d-cb13c9fb9120]

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.

[50cef828-0e94-47a2-843d-cb13c9fb9120]

Attaching patches for 3.5.