[issue8372] socket: Buffer overrun while reading unterminated AF_UNIX addresses

[Mark Lawrence]

Changes by Mark Lawrence :


--
nosy:  -BreamoreBoy

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8372] socket: Buffer overrun while reading unterminated AF_UNIX addresses

[Christian Heimes]

Changes by Christian Heimes :


--
versions: +Python 3.6, Python 3.7 -Python 2.6, Python 3.2, Python 3.3

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8372] socket: Buffer overrun while reading unterminated AF_UNIX addresses

[David Watson]

David Watson added the comment:

Attaching patches for 3.5.

--
Added file: 
http://bugs.python.org/file39309/enable-unterminated-3.5-2015-05-06.diff
Added file: http://bugs.python.org/file39310/fix-overrun-3.5-2015-05-06.diff

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue8372
___# HG changeset patch
# Parent  5f2ae82157af71456c4837ff2838d1f0f01e759e
Allow AF_UNIX pathnames up to the maximum 108 bytes on Linux,
since it does not require sun_path to be null terminated.

diff --git a/Lib/test/test_socket.py b/Lib/test/test_socket.py
--- a/Lib/test/test_socket.py
+++ b/Lib/test/test_socket.py
@@ -4477,6 +4477,54 @@ class TestExceptions(unittest.TestCase):
 self.assertTrue(issubclass(socket.timeout, OSError))
 
 @unittest.skipUnless(sys.platform == 'linux', 'Linux specific test')
+class TestLinuxPathLen(unittest.TestCase):
+
+# Test AF_UNIX path length limits on Linux.
+
+UNIX_PATH_MAX = 108
+
+def setUp(self):
+self.sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
+self.to_unlink = []
+
+def tearDown(self):
+self.sock.close()
+for name in self.to_unlink:
+support.unlink(name)
+
+def pathEncodingArgs(self):
+# Return the encoding and error handler used to encode/decode
+# pathnames.
+encoding = sys.getfilesystemencoding()
+if encoding is None:
+encoding = sys.getdefaultencoding()
+return encoding, surrogateescape
+
+def pathname(self, length):
+# Return a bytes pathname of the given length.
+path = os.path.abspath(support.TESTFN)
+path_bytes = path.encode(*self.pathEncodingArgs())
+return path_bytes + ba * (length - len(path_bytes))
+
+def testPathTooLong(self):
+# Check we can't bind to a path longer than the assumed maximum.
+path = self.pathname(self.UNIX_PATH_MAX + 1)
+with self.assertRaisesRegexp(socket.error, AF_UNIX path too long):
+self.sock.bind(path)
+self.to_unlink.append(path)
+
+def testMaxPathLen(self):
+# Test binding to a path of the maximum length and reading the
+# address back.  In this case, sun_path is not null terminated,
+# and makesockaddr() used to read past the end of it.
+path = self.pathname(self.UNIX_PATH_MAX)
+self.sock.bind(path)
+self.to_unlink.append(path)
+self.assertEqual(self.sock.getsockname(),
+ path.decode(*self.pathEncodingArgs()))
+os.stat(path)
+
+@unittest.skipUnless(sys.platform == 'linux', 'Linux specific test')
 class TestLinuxAbstractNamespace(unittest.TestCase):
 
 UNIX_PATH_MAX = 108
@@ -5303,6 +5351,7 @@ def test_main():
 ])
 tests.append(BasicSocketPairTest)
 tests.append(TestUnixDomain)
+tests.append(TestLinuxPathLen)
 tests.append(TestLinuxAbstractNamespace)
 tests.extend([TIPCTest, TIPCThreadableTest])
 tests.extend([BasicCANTest, CANTest])
diff --git a/Modules/socketmodule.c b/Modules/socketmodule.c
--- a/Modules/socketmodule.c
+++ b/Modules/socketmodule.c
@@ -1460,27 +1460,17 @@ getsockaddrarg(PySocketSockObject *s, Py
 
 addr = (struct sockaddr_un*)addr_ret;
 #ifdef linux
-if (path.len  0  *(const char *)path.buf == 0) {
-/* Linux abstract namespace extension */
-if ((size_t)path.len  sizeof addr-sun_path) {
-PyErr_SetString(PyExc_OSError,
-AF_UNIX path too long);
-goto unix_out;
-}
-}
-else
-#endif /* linux */
-{
-/* regular NULL-terminated string */
-if ((size_t)path.len = sizeof addr-sun_path) {
-PyErr_SetString(PyExc_OSError,
-AF_UNIX path too long);
-goto unix_out;
-}
-addr-sun_path[path.len] = 0;
+if ((size_t)path.len  sizeof(addr-sun_path)) {
+#else
+if ((size_t)path.len = sizeof(addr-sun_path)) {
+#endif
+PyErr_SetString(PyExc_OSError, AF_UNIX path too long);
+goto unix_out;
 }
 addr-sun_family = s-sock_family;
 memcpy(addr-sun_path, path.buf, path.len);
+memset(addr-sun_path + path.len, 0,
+   sizeof(addr-sun_path) - path.len);
 *len_ret = path.len + offsetof(struct sockaddr_un, sun_path);
 retval = 1;
 unix_out:
# HG changeset patch
# Parent  ac51dc99c1bd722e1b24b4bfa14520082e01f1a8
When parsing addresses returned by accept(), etc., do not assume
null termination of sun_path in AF_UNIX addresses: rely instead
on the returned address length.  If this is longer then the
original buffer, ignore it and use the original length.

diff --git a/Modules/socketmodule.c b/Modules/socketmodule.c
--- a/Modules/socketmodule.c
+++ b/Modules/socketmodule.c
@@ -1129,13 

[issue8372] socket: Buffer overrun while reading unterminated AF_UNIX addresses

[David Watson]

David Watson added the comment:

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.

--
Added file: 
http://bugs.python.org/file39297/enable-unterminated-2.7-2015-05-05.diff
Added file: http://bugs.python.org/file39298/fix-overrun-2.7-2015-05-05.diff
Added file: 
http://bugs.python.org/file39299/enable-unterminated-3.2-2015-05-05.diff
Added file: http://bugs.python.org/file39300/fix-overrun-3.2-2015-05-05.diff
Added file: 
http://bugs.python.org/file39301/enable-unterminated-3.3-2015-05-05.diff
Added file: http://bugs.python.org/file39302/fix-overrun-3.3-2015-05-05.diff
Added file: 
http://bugs.python.org/file39303/enable-unterminated-3.4-2015-05-05.diff
Added file: http://bugs.python.org/file39304/fix-overrun-3.4-2015-05-05.diff

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue8372
___# HG changeset patch
# Parent  376c2d81d0e2e8ec424d4aafabfbd75e42ea3804
Allow AF_UNIX pathnames up to the maximum 108 bytes on Linux,
since it does not require sun_path to be null terminated.

diff --git a/Lib/test/test_socket.py b/Lib/test/test_socket.py
--- a/Lib/test/test_socket.py
+++ b/Lib/test/test_socket.py
@@ -1581,6 +1581,44 @@ class TestExceptions(unittest.TestCase):
 self.assertTrue(issubclass(socket.gaierror, socket.error))
 self.assertTrue(issubclass(socket.timeout, socket.error))
 
+@unittest.skipUnless(sys.platform.startswith('linux'), 'Linux specific test')
+class TestLinuxPathLen(unittest.TestCase):
+
+# Test AF_UNIX path length limits on Linux.
+
+UNIX_PATH_MAX = 108
+
+def setUp(self):
+self.sock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
+self.to_unlink = []
+
+def tearDown(self):
+self.sock.close()
+for name in self.to_unlink:
+test_support.unlink(name)
+
+def pathname(self, length):
+# Return a pathname of the given length.
+path = os.path.abspath(test_support.TESTFN)
+return path + a * (length - len(path))
+
+def testPathTooLong(self):
+# Check we can't bind to a path longer than the assumed maximum.
+path = self.pathname(self.UNIX_PATH_MAX + 1)
+with self.assertRaisesRegexp(socket.error, AF_UNIX path too long):
+self.sock.bind(path)
+self.to_unlink.append(path)
+
+def testMaxPathLen(self):
+# Test binding to a path of the maximum length and reading the
+# address back.  In this case, sun_path is not null terminated,
+# and makesockaddr() used to read past the end of it.
+path = self.pathname(self.UNIX_PATH_MAX)
+self.sock.bind(path)
+self.to_unlink.append(path)
+self.assertEqual(self.sock.getsockname(), path)
+os.stat(path)
+
 @unittest.skipUnless(sys.platform == 'linux', 'Linux specific test')
 class TestLinuxAbstractNamespace(unittest.TestCase):
 
@@ -1793,6 +1831,7 @@ def test_main():
 NetworkConnectionBehaviourTest,
 ])
 tests.append(BasicSocketPairTest)
+tests.append(TestLinuxPathLen)
 tests.append(TestLinuxAbstractNamespace)
 tests.extend([TIPCTest, TIPCThreadableTest])
 
diff --git a/Modules/socketmodule.c b/Modules/socketmodule.c
--- a/Modules/socketmodule.c
+++ b/Modules/socketmodule.c
@@ -1251,27 +1251,16 @@ getsockaddrarg(PySocketSockObject *s, Py
 
 addr = (struct sockaddr_un*)addr_ret;
 #ifdef linux
-if (len  0  path[0] == 0) {
-/* Linux abstract namespace extension */
-if (len  sizeof addr-sun_path) {
-PyErr_SetString(socket_error,
-AF_UNIX path too long);
-return 0;
-}
-}
-else
-#endif /* linux */
-{
-/* regular NULL-terminated string */
-if (len = sizeof addr-sun_path) {
-PyErr_SetString(socket_error,
-AF_UNIX path too long);
-return 0;
-}
-addr-sun_path[len] = 0;
+if (len  sizeof(addr-sun_path)) {
+#else
+if (len = sizeof(addr-sun_path)) {
+#endif
+PyErr_SetString(socket_error, AF_UNIX path too long);
+return 0;
 }
 addr-sun_family = s-sock_family;
 memcpy(addr-sun_path, path, len);
+memset(addr-sun_path + len, 0, sizeof(addr-sun_path) - len);
 #if defined(PYOS_OS2)
 *len_ret = sizeof(*addr);
 #else
# HG changeset patch
# Parent  

[issue8372] socket: Buffer overrun while reading unterminated AF_UNIX addresses

[Mark Lawrence]

Mark Lawrence added the comment:

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.

--
nosy: +BreamoreBoy
versions: +Python 3.5

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue8372
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8372] socket: Buffer overrun while reading unterminated AF_UNIX addresses

[Serhiy Storchaka]

Changes by Serhiy Storchaka storch...@gmail.com:


--
versions: +Python 3.4 -Python 3.1

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue8372
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8372] socket: Buffer overrun while reading unterminated AF_UNIX addresses

[David Watson]

David Watson bai...@users.sourceforge.net added the comment:

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)

--
Added file: http://bugs.python.org/file22384/addrlen-makesockaddr-2.x.diff
Added file: http://bugs.python.org/file22385/addrlen-makesockaddr-3.x.diff
Added file: http://bugs.python.org/file22386/return-unterminated-2.x-new.diff
Added file: 
http://bugs.python.org/file22387/return-unterminated-3.x-maint-new.diff
Added file: 
http://bugs.python.org/file22388/return-unterminated-3.x-trunk-new.diff

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue8372
___If accept(), etc. return a larger addrlen than was supplied,
ignore it and use the original buffer length.

diff --git a/Modules/socketmodule.c b/Modules/socketmodule.c
--- a/Modules/socketmodule.c
+++ b/Modules/socketmodule.c
@@ -969,13 +969,22 @@ makebdaddr(bdaddr_t *bdaddr)
 
 /*ARGSUSED*/
 static PyObject *
-makesockaddr(int sockfd, struct sockaddr *addr, int addrlen, int proto)
+makesockaddr(int sockfd, struct sockaddr *addr, socklen_t addrlen,
+ socklen_t buflen, int proto)
 {
 if (addrlen == 0) {
 /* No address -- may be recvfrom() from known socket */
 Py_INCREF(Py_None);
 return Py_None;
 }
+/* buflen is the length of the buffer containing the address, and
+   addrlen is either the same, or is the length returned by the OS
+   after writing an address into the buffer.  Some systems return
+   the length they would have written if there had been space
+   (e.g. when an oversized AF_UNIX address has its sun_path
+   truncated). */
+if (addrlen  buflen)
+addrlen = buflen;
 
 #ifdef __BEOS__
 /* XXX: BeOS version of accept() doesn't set family correctly */
@@ -1632,6 +1641,7 @@ sock_accept(PySocketSockObject *s)
 sock_addr_t addrbuf;
 SOCKET_T newfd;
 socklen_t addrlen;
+socklen_t buflen;
 PyObject *sock = NULL;
 PyObject *addr = NULL;
 PyObject *res = NULL;
@@ -1639,6 +1649,7 @@ sock_accept(PySocketSockObject *s)
 
 if (!getsockaddrlen(s, addrlen))
 return NULL;
+buflen = addrlen;
 memset(addrbuf, 0, addrlen);
 
 #ifdef MS_WINDOWS
@@ -1680,7 +1691,7 @@ sock_accept(PySocketSockObject *s)
 goto finally;
 }
 addr = makesockaddr(s-sock_fd, SAS2SA(addrbuf),
-   

[issue8372] socket: Buffer overrun while reading unterminated AF_UNIX addresses

[Terry J. Reedy]

Terry J. Reedy tjre...@udel.edu added the comment:

Is this a security issue or just a regular bug?

--
nosy: +terry.reedy

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue8372
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8372] socket: Buffer overrun while reading unterminated AF_UNIX addresses

[Antoine Pitrou]

Antoine Pitrou pit...@free.fr added the comment:

It's a potential security issue.

--
nosy: +neologix, rosslagerwall
versions: +Python 3.3

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue8372
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8372] socket: Buffer overrun while reading unterminated AF_UNIX addresses

[Terry J. Reedy]

Changes by Terry J. Reedy tjre...@udel.edu:


--
type: behavior - security

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue8372
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8372] socket: Buffer overrun while reading unterminated AF_UNIX addresses

[Charles-François Natali]

Charles-François Natali neolo...@free.fr added the comment:

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

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue8372
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8372] socket: Buffer overrun while reading unterminated AF_UNIX addresses

[Martin v . Löwis]

Martin v. Löwis mar...@v.loewis.de added the comment:

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.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue8372
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8372] socket: Buffer overrun while reading unterminated AF_UNIX addresses

[David Watson]

David Watson bai...@users.sourceforge.net added the comment:

 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.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue8372
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8372] socket: Buffer overrun while reading unterminated AF_UNIX addresses

[David Watson]

David Watson bai...@users.sourceforge.net added the comment:

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=unixapropos=0sektion=0manpath=FreeBSD+8.1-RELEASEformat=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.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue8372
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8372] socket: Buffer overrun while reading unterminated AF_UNIX addresses

[Martin v . Löwis]

Martin v. Löwis mar...@v.loewis.de added the comment:

 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.

--
title: socket: Buffer overrun while reading unterminated AF_UNIX addresses - 
socket: Buffer overrun while reading unterminated AF_UNIX addresses

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue8372
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8372] socket: Buffer overrun while reading unterminated AF_UNIX addresses

[David Watson]

David Watson bai...@users.sourceforge.net added the comment:

I've updated the PEP 383 patches at issue #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.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue8372
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8372] socket: Buffer overrun while reading unterminated AF_UNIX addresses

[David Watson]

David Watson bai...@users.sourceforge.net added the comment:

 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.

--
Added file: http://bugs.python.org/file18770/linux-pass-unterminated-4spc.diff
Added file: http://bugs.python.org/file18771/return-unterminated-2.x-4spc.diff
Added file: http://bugs.python.org/file18772/return-unterminated-3.x-4spc.diff
Added file: http://bugs.python.org/file18773/addrlen-2.x-4spc.diff
Added file: http://bugs.python.org/file18774/addrlen-3.x-4spc.diff
Added file: http://bugs.python.org/file18775/test-2.x-new.diff
Added file: http://bugs.python.org/file18776/test-3.x-new.diff

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue8372
___Allow AF_UNIX pathnames up to the maximum 108 bytes on Linux,
since it does not require sun_path to be null terminated.

diff --git a/Modules/socketmodule.c b/Modules/socketmodule.c
--- a/Modules/socketmodule.c
+++ b/Modules/socketmodule.c
@@ -1187,27 +1187,16 @@ getsockaddrarg(PySocketSockObject *s, Py
 
 addr = (struct sockaddr_un*)addr_ret;
 #ifdef linux
-if (len  0  path[0] == 0) {
-/* Linux abstract namespace extension */
-if (len  sizeof addr-sun_path) {
-PyErr_SetString(socket_error,
-AF_UNIX path too long);
-return 0;
-}
-}
-else
-#endif /* linux */
-{
-/* regular NULL-terminated string */
-if (len = sizeof addr-sun_path) {
-PyErr_SetString(socket_error,
-AF_UNIX path too long);
-return 0;
-}
-addr-sun_path[len] = 0;
+if (len  sizeof(addr-sun_path)) {
+#else
+if (len = sizeof(addr-sun_path)) {
+#endif
+PyErr_SetString(socket_error, AF_UNIX path too long);
+return 0;
 }
 addr-sun_family = s-sock_family;
 memcpy(addr-sun_path, path, len);
+memset(addr-sun_path + len, 0, sizeof(addr-sun_path) - len);
 #if defined(PYOS_OS2)
 *len_ret = sizeof(*addr);
 #else
When parsing sockaddr_un structures returned by accept(), etc.,
only examine bytes up to supplied addrlen and do not require null
termination.

diff --git a/Modules/socketmodule.c b/Modules/socketmodule.c
--- a/Modules/socketmodule.c
+++ b/Modules/socketmodule.c
@@ -998,19 +998,22 @@ makesockaddr(int sockfd, struct sockaddr
 #if defined(AF_UNIX)
 case AF_UNIX:
 {
+Py_ssize_t len, splen;
 struct sockaddr_un *a = (struct sockaddr_un *) addr;
+splen = addrlen - offsetof(struct sockaddr_un, sun_path);
 #ifdef linux
-if (a-sun_path[0] == 0) {  /* Linux abstract namespace */
-addrlen -= offsetof(struct sockaddr_un, sun_path);
-return PyString_FromStringAndSize(a-sun_path,
-  addrlen);
+if (splen  0  a-sun_path[0] == 0) {
+/* Linux abstract namespace */
+len = splen;
 }
 else
 #endif /* linux */
 {
-/* regular NULL-terminated string */
-return PyString_FromString(a-sun_path);
+/* String, up to null terminator if present */
+for (len = 0; len  splen  a-sun_path[len] != 0; len++)
+;
 }
+return PyString_FromStringAndSize(a-sun_path, len);
 }
 #endif /* AF_UNIX */
 
When parsing sockaddr_un structures returned by accept(), etc.,
only examine bytes up to supplied addrlen and do not require null
termination.

diff --git a/Modules/socketmodule.c b/Modules/socketmodule.c
--- a/Modules/socketmodule.c
+++ b/Modules/socketmodule.c
@@ -962,18 +962,22 @@ makesockaddr(SOCKET_T sockfd, struct soc
 #if defined(AF_UNIX)
 case 

[issue8372] socket: Buffer overrun while reading unterminated AF_UNIX addresses

[David Watson]

David Watson bai...@users.sourceforge.net added the comment:

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

--
title: socket: Buffer overrun while reading unterminated AF_UNIX
addresses - socket: Buffer overrun while reading unterminated AF_UNIX addresses

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue8372
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8372] socket: Buffer overrun while reading unterminated AF_UNIX addresses

[Martin v . Löwis]

Martin v. Löwis mar...@v.loewis.de added the comment:

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

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue8372
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8372] socket: Buffer overrun while reading unterminated AF_UNIX addresses

[Antoine Pitrou]

Antoine Pitrou pit...@free.fr added the comment:

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.

--
nosy: +pitrou
Added file: http://bugs.python.org/file18753/issue8372.patch

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue8372
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8372] socket: Buffer overrun while reading unterminated AF_UNIX addresses

[Martin v . Löwis]

Martin v. Löwis mar...@v.loewis.de added the comment:

 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?

--
title: socket: Buffer overrun while reading unterminated AF_UNIX addresses - 
socket: Buffer overrun while reading unterminated AF_UNIX addresses

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue8372
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8372] socket: Buffer overrun while reading unterminated AF_UNIX addresses

[Antoine Pitrou]

Antoine Pitrou pit...@free.fr added the comment:

 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.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue8372
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8372] socket: Buffer overrun while reading unterminated AF_UNIX addresses

[David Watson]

David Watson bai...@users.sourceforge.net added the comment:

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

--
Added file: http://bugs.python.org/file16898/bindconn.c

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue8372
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8372] socket: Buffer overrun while reading unterminated AF_UNIX addresses

[David Watson]

Changes by David Watson bai...@users.sourceforge.net:


Added file: http://bugs.python.org/file16899/accept.c

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue8372
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8372] socket: Buffer overrun while reading unterminated AF_UNIX addresses

[David Watson]

New submission from David Watson bai...@users.sourceforge.net:

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()
'\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.

--
components: Extension Modules
files: linux-pass-unterminated.diff
keywords: patch
messages: 102861
nosy: baikie
severity: normal
status: open
title: socket: Buffer overrun while reading unterminated AF_UNIX addresses
type: behavior
versions: Python 2.5, Python 2.6, Python 2.7, Python 3.1, Python 3.2, Python 3.3
Added file: http://bugs.python.org/file16874/linux-pass-unterminated.diff

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue8372
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8372] socket: Buffer overrun while reading unterminated AF_UNIX addresses

[David Watson]

Changes by David Watson bai...@users.sourceforge.net:


Added file: http://bugs.python.org/file16875/return-unterminated-2.x.diff

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue8372
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8372] socket: Buffer overrun while reading unterminated AF_UNIX addresses

[David Watson]

Changes by David Watson bai...@users.sourceforge.net:


Added file: http://bugs.python.org/file16876/return-unterminated-3.x.diff

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue8372
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8372] socket: Buffer overrun while reading unterminated AF_UNIX addresses

[David Watson]

Changes by David Watson bai...@users.sourceforge.net:


Added file: http://bugs.python.org/file16877/addrlen-2.x.diff

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue8372
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8372] socket: Buffer overrun while reading unterminated AF_UNIX addresses

[David Watson]

Changes by David Watson bai...@users.sourceforge.net:


Added file: http://bugs.python.org/file16878/addrlen-3.x.diff

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue8372
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8372] socket: Buffer overrun while reading unterminated AF_UNIX addresses

[David Watson]

Changes by David Watson bai...@users.sourceforge.net:


Added file: http://bugs.python.org/file16879/test-2.x.diff

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue8372
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8372] socket: Buffer overrun while reading unterminated AF_UNIX addresses

[David Watson]

Changes by David Watson bai...@users.sourceforge.net:


Added file: http://bugs.python.org/file16880/test-3.x.diff

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue8372
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue8372] socket: Buffer overrun while reading unterminated AF_UNIX addresses

[Antoine Pitrou]

Changes by Antoine Pitrou pit...@free.fr:


--
nosy: +haypo, loewis
priority:  - high
stage:  - patch review
versions:  -Python 2.5, Python 3.3

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue8372
___
___
Python-bugs-list mailing list
Unsubscribe: 
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com