From 88d36735a33afad983034fbffbd6dcfd553070ac Mon Sep 17 00:00:00 2001 From: Benjamin Fogle Date: Wed, 22 Mar 2023 10:08:41 -0400 Subject: [PATCH] gh-96931: Fix incorrect results in ssl.SSLSocket.shared_ciphers (GH-96932) (cherry picked from commit af9c34f6ef8dceb21871206eb3e4d350f6e3d3dc) Co-authored-by: Benjamin Fogle --- Doc/library/ssl.rst | 2 +- Lib/test/test_ssl.py | 6 ++-- ...2-09-19-08-12-58.gh-issue-96931.x0WQhh.rst | 1 + Modules/_ssl.c | 36 ++++++++++++++----- 4 files changed, 33 insertions(+), 12 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2022-09-19-08-12-58.gh-issue-96931.x0WQhh.rst diff --git a/Doc/library/ssl.rst b/Doc/library/ssl.rst index de9edcb2621093..36c852974da910 100644 --- a/Doc/library/ssl.rst +++ b/Doc/library/ssl.rst @@ -1311,7 +1311,7 @@ SSL sockets also have the following additional methods and attributes: .. method:: SSLSocket.shared_ciphers() - Return the list of ciphers shared by the client during the handshake. Each + Return the list of ciphers available in both the client and server. Each entry of the returned list is a three-value tuple containing the name of the cipher, the version of the SSL protocol that defines its use, and the number of secret bits the cipher uses. :meth:`~SSLSocket.shared_ciphers` returns diff --git a/Lib/test/test_ssl.py b/Lib/test/test_ssl.py index f97227b11bcde8..addfb6ce43d809 100644 --- a/Lib/test/test_ssl.py +++ b/Lib/test/test_ssl.py @@ -2326,13 +2326,13 @@ def test_bio_handshake(self): self.assertIs(sslobj._sslobj.owner, sslobj) self.assertIsNone(sslobj.cipher()) self.assertIsNone(sslobj.version()) - self.assertIsNotNone(sslobj.shared_ciphers()) + self.assertIsNone(sslobj.shared_ciphers()) self.assertRaises(ValueError, sslobj.getpeercert) if 'tls-unique' in ssl.CHANNEL_BINDING_TYPES: self.assertIsNone(sslobj.get_channel_binding('tls-unique')) self.ssl_io_loop(sock, incoming, outgoing, sslobj.do_handshake) self.assertTrue(sslobj.cipher()) - self.assertIsNotNone(sslobj.shared_ciphers()) + self.assertIsNone(sslobj.shared_ciphers()) self.assertIsNotNone(sslobj.version()) self.assertTrue(sslobj.getpeercert()) if 'tls-unique' in ssl.CHANNEL_BINDING_TYPES: @@ -4310,7 +4310,7 @@ def cb_wrong_return_type(ssl_sock, server_name, initial_context): def test_shared_ciphers(self): client_context, server_context, hostname = testing_context() client_context.set_ciphers("AES128:AES256") - server_context.set_ciphers("AES256") + server_context.set_ciphers("AES256:eNULL") expected_algs = [ "AES256", "AES-256", # TLS 1.3 ciphers are always enabled diff --git a/Misc/NEWS.d/next/Library/2022-09-19-08-12-58.gh-issue-96931.x0WQhh.rst b/Misc/NEWS.d/next/Library/2022-09-19-08-12-58.gh-issue-96931.x0WQhh.rst new file mode 100644 index 00000000000000..766b1d4d477b72 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2022-09-19-08-12-58.gh-issue-96931.x0WQhh.rst @@ -0,0 +1 @@ +Fix incorrect results from :meth:`ssl.SSLSocket.shared_ciphers` diff --git a/Modules/_ssl.c b/Modules/_ssl.c index f1bb39f57b2298..360eb864ad3620 100644 --- a/Modules/_ssl.c +++ b/Modules/_ssl.c @@ -2005,24 +2005,44 @@ static PyObject * _ssl__SSLSocket_shared_ciphers_impl(PySSLSocket *self) /*[clinic end generated code: output=3d174ead2e42c4fd input=0bfe149da8fe6306]*/ { - STACK_OF(SSL_CIPHER) *ciphers; - int i; + STACK_OF(SSL_CIPHER) *server_ciphers; + STACK_OF(SSL_CIPHER) *client_ciphers; + int i, len; PyObject *res; + const SSL_CIPHER* cipher; + + /* Rather than use SSL_get_shared_ciphers, we use an equivalent algorithm because: + + 1) It returns a colon seperated list of strings, in an undefined + order, that we would have to post process back into tuples. + 2) It will return a truncated string with no indication that it has + done so, if the buffer is too small. + */ - ciphers = SSL_get_ciphers(self->ssl); - if (!ciphers) + server_ciphers = SSL_get_ciphers(self->ssl); + if (!server_ciphers) Py_RETURN_NONE; - res = PyList_New(sk_SSL_CIPHER_num(ciphers)); + client_ciphers = SSL_get_client_ciphers(self->ssl); + if (!client_ciphers) + Py_RETURN_NONE; + + res = PyList_New(sk_SSL_CIPHER_num(server_ciphers)); if (!res) return NULL; - for (i = 0; i < sk_SSL_CIPHER_num(ciphers); i++) { - PyObject *tup = cipher_to_tuple(sk_SSL_CIPHER_value(ciphers, i)); + len = 0; + for (i = 0; i < sk_SSL_CIPHER_num(server_ciphers); i++) { + cipher = sk_SSL_CIPHER_value(server_ciphers, i); + if (sk_SSL_CIPHER_find(client_ciphers, cipher) < 0) + continue; + + PyObject *tup = cipher_to_tuple(cipher); if (!tup) { Py_DECREF(res); return NULL; } - PyList_SET_ITEM(res, i, tup); + PyList_SET_ITEM(res, len++, tup); } + Py_SET_SIZE(res, len); return res; }