[issue8372] socket: Buffer overrun while reading unterminated AF_UNIX addresses
David Watson
report at bugs.python.org
Thu Jun 16 23:07:32 CEST 2011
David Watson <baikie at 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 <report at bugs.python.org>
<http://bugs.python.org/issue8372>
_______________________________________
-------------- next part --------------
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),
- addrlen, s->sock_proto);
+ addrlen, buflen, s->sock_proto);
if (addr == NULL)
goto finally;
@@ -2178,16 +2189,18 @@ sock_getsockname(PySocketSockObject *s)
sock_addr_t addrbuf;
int res;
socklen_t addrlen;
+ socklen_t buflen;
if (!getsockaddrlen(s, &addrlen))
return NULL;
+ buflen = addrlen;
memset(&addrbuf, 0, addrlen);
Py_BEGIN_ALLOW_THREADS
res = getsockname(s->sock_fd, SAS2SA(&addrbuf), &addrlen);
Py_END_ALLOW_THREADS
if (res < 0)
return s->errorhandler();
- return makesockaddr(s->sock_fd, SAS2SA(&addrbuf), addrlen,
+ return makesockaddr(s->sock_fd, SAS2SA(&addrbuf), addrlen, buflen,
s->sock_proto);
}
@@ -2207,16 +2220,18 @@ sock_getpeername(PySocketSockObject *s)
sock_addr_t addrbuf;
int res;
socklen_t addrlen;
+ socklen_t buflen;
if (!getsockaddrlen(s, &addrlen))
return NULL;
+ buflen = addrlen;
memset(&addrbuf, 0, addrlen);
Py_BEGIN_ALLOW_THREADS
res = getpeername(s->sock_fd, SAS2SA(&addrbuf), &addrlen);
Py_END_ALLOW_THREADS
if (res < 0)
return s->errorhandler();
- return makesockaddr(s->sock_fd, SAS2SA(&addrbuf), addrlen,
+ return makesockaddr(s->sock_fd, SAS2SA(&addrbuf), addrlen, buflen,
s->sock_proto);
}
@@ -2542,11 +2557,13 @@ sock_recvfrom_guts(PySocketSockObject *s
int timeout;
ssize_t n = -1;
socklen_t addrlen;
+ socklen_t buflen;
*addr = NULL;
if (!getsockaddrlen(s, &addrlen))
return -1;
+ buflen = addrlen;
if (!IS_SELECTABLE(s)) {
select_error();
@@ -2582,7 +2599,7 @@ sock_recvfrom_guts(PySocketSockObject *s
}
if (!(*addr = makesockaddr(s->sock_fd, SAS2SA(&addrbuf),
- addrlen, s->sock_proto)))
+ addrlen, buflen, s->sock_proto)))
return -1;
return n;
@@ -4114,7 +4131,8 @@ socket_getaddrinfo(PyObject *self, PyObj
goto err;
for (res = res0; res; res = res->ai_next) {
PyObject *addr =
- makesockaddr(-1, res->ai_addr, res->ai_addrlen, protocol);
+ makesockaddr(-1, res->ai_addr, res->ai_addrlen, res->ai_addrlen,
+ protocol);
if (addr == NULL)
goto err;
single = Py_BuildValue("iiisO", res->ai_family,
-------------- next part --------------
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
@@ -1026,13 +1026,22 @@ makebdaddr(bdaddr_t *bdaddr)
/*ARGSUSED*/
static PyObject *
-makesockaddr(SOCKET_T sockfd, struct sockaddr *addr, size_t addrlen, int proto)
+makesockaddr(SOCKET_T 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;
switch (addr->sa_family) {
@@ -1684,12 +1693,14 @@ sock_accept(PySocketSockObject *s)
sock_addr_t addrbuf;
SOCKET_T newfd = INVALID_SOCKET;
socklen_t addrlen;
+ socklen_t buflen;
PyObject *sock = NULL;
PyObject *addr = NULL;
PyObject *res = NULL;
int timeout;
if (!getsockaddrlen(s, &addrlen))
return NULL;
+ buflen = addrlen;
memset(&addrbuf, 0, addrlen);
if (!IS_SELECTABLE(s))
@@ -1719,7 +1730,7 @@ sock_accept(PySocketSockObject *s)
}
addr = makesockaddr(s->sock_fd, SAS2SA(&addrbuf),
- addrlen, s->sock_proto);
+ addrlen, buflen, s->sock_proto);
if (addr == NULL)
goto finally;
@@ -2169,16 +2180,18 @@ sock_getsockname(PySocketSockObject *s)
sock_addr_t addrbuf;
int res;
socklen_t addrlen;
+ socklen_t buflen;
if (!getsockaddrlen(s, &addrlen))
return NULL;
+ buflen = addrlen;
memset(&addrbuf, 0, addrlen);
Py_BEGIN_ALLOW_THREADS
res = getsockname(s->sock_fd, SAS2SA(&addrbuf), &addrlen);
Py_END_ALLOW_THREADS
if (res < 0)
return s->errorhandler();
- return makesockaddr(s->sock_fd, SAS2SA(&addrbuf), addrlen,
+ return makesockaddr(s->sock_fd, SAS2SA(&addrbuf), addrlen, buflen,
s->sock_proto);
}
@@ -2198,16 +2211,18 @@ sock_getpeername(PySocketSockObject *s)
sock_addr_t addrbuf;
int res;
socklen_t addrlen;
+ socklen_t buflen;
if (!getsockaddrlen(s, &addrlen))
return NULL;
+ buflen = addrlen;
memset(&addrbuf, 0, addrlen);
Py_BEGIN_ALLOW_THREADS
res = getpeername(s->sock_fd, SAS2SA(&addrbuf), &addrlen);
Py_END_ALLOW_THREADS
if (res < 0)
return s->errorhandler();
- return makesockaddr(s->sock_fd, SAS2SA(&addrbuf), addrlen,
+ return makesockaddr(s->sock_fd, SAS2SA(&addrbuf), addrlen, buflen,
s->sock_proto);
}
@@ -2476,11 +2491,13 @@ sock_recvfrom_guts(PySocketSockObject *s
int timeout;
Py_ssize_t n = -1;
socklen_t addrlen;
+ socklen_t buflen;
*addr = NULL;
if (!getsockaddrlen(s, &addrlen))
return -1;
+ buflen = addrlen;
if (!IS_SELECTABLE(s)) {
select_error();
@@ -2518,7 +2535,7 @@ sock_recvfrom_guts(PySocketSockObject *s
}
if (!(*addr = makesockaddr(s->sock_fd, SAS2SA(&addrbuf),
- addrlen, s->sock_proto)))
+ addrlen, buflen, s->sock_proto)))
return -1;
return n;
@@ -4110,7 +4127,8 @@ socket_getaddrinfo(PyObject *self, PyObj
for (res = res0; res; res = res->ai_next) {
PyObject *single;
PyObject *addr =
- makesockaddr(-1, res->ai_addr, res->ai_addrlen, protocol);
+ makesockaddr(-1, res->ai_addr, res->ai_addrlen, res->ai_addrlen,
+ protocol);
if (addr == NULL)
goto err;
single = Py_BuildValue("iiisO", res->ai_family,
-------------- next part --------------
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
@@ -1000,19 +1000,27 @@ 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;
+
+ if (addrlen < offsetof(struct sockaddr_un, sun_path))
+ Py_RETURN_NONE;
+ else
+ 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);
+ /* Path text can occupy all of sun_path[], and therefore
+ lack null termination */
+ for (len = 0; len < splen && a->sun_path[len] != 0; len++)
+ ;
}
+ return PyString_FromStringAndSize(a->sun_path, len);
}
#endif /* AF_UNIX */
-------------- next part --------------
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
@@ -1038,18 +1038,28 @@ makesockaddr(SOCKET_T sockfd, struct soc
#if defined(AF_UNIX)
case AF_UNIX:
{
+ Py_ssize_t len, splen;
struct sockaddr_un *a = (struct sockaddr_un *) addr;
+
+ if (addrlen < offsetof(struct sockaddr_un, sun_path))
+ Py_RETURN_NONE;
+ else
+ 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 PyBytes_FromStringAndSize(a->sun_path, addrlen);
+ /* Backwards compatibility: return empty addresses as bytes */
+ if (splen == 0 || (splen > 0 && a->sun_path[0] == 0)) {
+ /* Linux abstract namespace */
+ return PyBytes_FromStringAndSize(a->sun_path, splen);
}
else
#endif /* linux */
{
- /* regular NULL-terminated string */
- return PyUnicode_FromString(a->sun_path);
+ /* Path text can occupy all of sun_path[], and therefore
+ lack null termination */
+ for (len = 0; len < splen && a->sun_path[len] != 0; len++)
+ ;
}
+ return PyUnicode_FromStringAndSize(a->sun_path, len);
}
#endif /* AF_UNIX */
-------------- next part --------------
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
@@ -1052,18 +1052,27 @@ makesockaddr(SOCKET_T sockfd, struct soc
#if defined(AF_UNIX)
case AF_UNIX:
{
+ Py_ssize_t len, splen;
struct sockaddr_un *a = (struct sockaddr_un *) addr;
+
+ if (addrlen < offsetof(struct sockaddr_un, sun_path))
+ Py_RETURN_NONE;
+ else
+ 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 PyBytes_FromStringAndSize(a->sun_path, addrlen);
+ if (splen > 0 && a->sun_path[0] == 0) {
+ /* Linux abstract namespace */
+ return PyBytes_FromStringAndSize(a->sun_path, splen);
}
else
#endif /* linux */
{
- /* regular NULL-terminated string */
- return PyUnicode_FromString(a->sun_path);
+ /* Path text can occupy all of sun_path[], and therefore
+ lack null termination */
+ for (len = 0; len < splen && a->sun_path[len] != 0; len++)
+ ;
}
+ return PyUnicode_FromStringAndSize(a->sun_path, len);
}
#endif /* AF_UNIX */
More information about the Python-bugs-list
mailing list