From 149840e21cd0b768f7f7117dfb7f8dbc36589eb9 Mon Sep 17 00:00:00 2001 From: Christian Heimes Date: Tue, 23 Oct 2018 15:18:36 +0200 Subject: [PATCH 1/2] bpo-35050: AF_ALG length check off-by-one error The length check for AF_ALG salg_name and salg_type had a off-by-one error. The code assumed that both values are not necessarily NULL terminated. However the Kernel code for alg_bind() ensures that the last byte of both strings are NULL terminated. Signed-off-by: Christian Heimes --- Lib/test/test_socket.py | 15 +++++++++++++++ .../2018-10-23-15-03-53.bpo-35050.49wraS.rst | 1 + Modules/socketmodule.c | 8 +++++--- 3 files changed, 21 insertions(+), 3 deletions(-) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2018-10-23-15-03-53.bpo-35050.49wraS.rst diff --git a/Lib/test/test_socket.py b/Lib/test/test_socket.py index 663a018dcfda0a1..c9162c5608d84bd 100644 --- a/Lib/test/test_socket.py +++ b/Lib/test/test_socket.py @@ -5970,6 +5970,21 @@ def test_sendmsg_afalg_args(self): with self.assertRaises(TypeError): sock.sendmsg_afalg(op=socket.ALG_OP_ENCRYPT, assoclen=-1) + def test_length_restriction(self): + # bpo-35050, off-by-one error in length check + sock = socket.socket(socket.AF_ALG, socket.SOCK_SEQPACKET, 0) + # salg_type[14] + with self.assertRaises(FileNotFoundError): + sock.bind(("t" * 13, "name")) + with self.assertRaisesRegex(ValueError, "type too long"): + sock.bind(("t" * 14, "name")) + # salg_name[64] + with self.assertRaises(FileNotFoundError): + sock.bind(("type", "n" * 63)) + with self.assertRaisesRegex(ValueError, "name too long"): + sock.bind(("type", "n" * 64)) + + @unittest.skipUnless(sys.platform.startswith("win"), "requires Windows") class TestMSWindowsTCPFlags(unittest.TestCase): knownTCPFlags = { diff --git a/Misc/NEWS.d/next/Core and Builtins/2018-10-23-15-03-53.bpo-35050.49wraS.rst b/Misc/NEWS.d/next/Core and Builtins/2018-10-23-15-03-53.bpo-35050.49wraS.rst new file mode 100644 index 000000000000000..c27181cc1feb73c --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2018-10-23-15-03-53.bpo-35050.49wraS.rst @@ -0,0 +1 @@ +Fix off-by-one bug in length check for AF_ALG name and type. diff --git a/Modules/socketmodule.c b/Modules/socketmodule.c index 9149641fce5a22e..6a2a05b0d0584cc 100644 --- a/Modules/socketmodule.c +++ b/Modules/socketmodule.c @@ -2245,13 +2245,15 @@ getsockaddrarg(PySocketSockObject *s, PyObject *args, { return 0; } - /* sockaddr_alg has fixed-sized char arrays for type and name */ - if (strlen(type) > sizeof(sa->salg_type)) { + /* sockaddr_alg has fixed-sized char arrays for type and name + * both must be NULL terminated + */ + if (strlen(type) >= sizeof(sa->salg_type)) { PyErr_SetString(PyExc_ValueError, "AF_ALG type too long."); return 0; } strncpy((char *)sa->salg_type, type, sizeof(sa->salg_type)); - if (strlen(name) > sizeof(sa->salg_name)) { + if (strlen(name) >= sizeof(sa->salg_name)) { PyErr_SetString(PyExc_ValueError, "AF_ALG name too long."); return 0; } From a739db3e8031510a6b1e3d8f92c50889aa97d6ff Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Mon, 10 Dec 2018 10:00:47 +0100 Subject: [PATCH 2/2] Minor changes * reformat a comment * close the socket in the unit test --- Lib/test/test_socket.py | 3 +++ .../2018-10-23-15-03-53.bpo-35050.49wraS.rst | 2 +- Modules/socketmodule.c | 4 ++-- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/Lib/test/test_socket.py b/Lib/test/test_socket.py index d6669b962d85d92..626a07797358260 100644 --- a/Lib/test/test_socket.py +++ b/Lib/test/test_socket.py @@ -5972,11 +5972,14 @@ def test_sendmsg_afalg_args(self): def test_length_restriction(self): # bpo-35050, off-by-one error in length check sock = socket.socket(socket.AF_ALG, socket.SOCK_SEQPACKET, 0) + self.addCleanup(sock.close) + # salg_type[14] with self.assertRaises(FileNotFoundError): sock.bind(("t" * 13, "name")) with self.assertRaisesRegex(ValueError, "type too long"): sock.bind(("t" * 14, "name")) + # salg_name[64] with self.assertRaises(FileNotFoundError): sock.bind(("type", "n" * 63)) diff --git a/Misc/NEWS.d/next/Core and Builtins/2018-10-23-15-03-53.bpo-35050.49wraS.rst b/Misc/NEWS.d/next/Core and Builtins/2018-10-23-15-03-53.bpo-35050.49wraS.rst index c27181cc1feb73c..9a33416089a2a69 100644 --- a/Misc/NEWS.d/next/Core and Builtins/2018-10-23-15-03-53.bpo-35050.49wraS.rst +++ b/Misc/NEWS.d/next/Core and Builtins/2018-10-23-15-03-53.bpo-35050.49wraS.rst @@ -1 +1 @@ -Fix off-by-one bug in length check for AF_ALG name and type. +:mod:`socket`: Fix off-by-one bug in length check for ``AF_ALG`` name and type. diff --git a/Modules/socketmodule.c b/Modules/socketmodule.c index ff8704b4dfa8bd6..40f1ca64a4ac312 100644 --- a/Modules/socketmodule.c +++ b/Modules/socketmodule.c @@ -2245,8 +2245,8 @@ getsockaddrarg(PySocketSockObject *s, PyObject *args, { return 0; } - /* sockaddr_alg has fixed-sized char arrays for type and name - * both must be NULL terminated + /* sockaddr_alg has fixed-sized char arrays for type, and name + * both must be NULL terminated. */ if (strlen(type) >= sizeof(sa->salg_type)) { PyErr_SetString(PyExc_ValueError, "AF_ALG type too long.");