classification
Title: SSL shared_ciphers implementation wrong - returns configured but not shared ciphers
Type: behavior Stage:
Components: SSL Versions: Python 3.8, Python 3.7, Python 3.6
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: christian.heimes Nosy List: alex, benjamin.peterson, christian.heimes, dstufft, janssen, ned.deily, noxxi
Priority: normal Keywords:

Created on 2018-02-10 07:04 by noxxi, last changed 2018-02-12 00:47 by benjamin.peterson.

Messages (10)
msg311940 - (view) Author: Steffen Ullrich (noxxi) Date: 2018-02-10 07:04
The current implementation of shared_ciphers uses the SSL_get_ciphers method. This method returns the list of configured ciphers (i.e. from the context) and not the list of ciphers shared between client and server. 

To get this list one can use the documented SSL_get_client_ciphers for OpenSSL >= 1.1.0, access ssl->sessions->ciphers directly or parse the result from the undocumented SSL_get_shared_ciphers for older versions of OpenSSL.

See also https://stackoverflow.com/questions/48717497/python-ssl-shared-ciphers-not-as-documented/48718081#48718081
msg311944 - (view) Author: Steffen Ullrich (noxxi) Date: 2018-02-10 08:53
Actually, it looks like that neither SSL_get_shared ciphers nor SSL_get_client_ciphers nor accessing ssl->session->ciphers nor SSL_get_ciphers return the **shared** ciphers. The first three seem to return the ciphers offered by the client and the last one returns the ciphers set for the server. 

It looks like even the OpenSSL developers do not really know what they are doing. The same contents of ssl->session->ciphers is made available through the functions SSL_get_shared_ciphers and SSL_get_client_ciphers which based on the names should return different information. And, the ciphers member of the ssl_session_st structure is documented in for the newest and even the oldest versions (i.e. like 0.9.8) as:

   STACK_OF(SSL_CIPHER) *ciphers; /* shared ciphers? */

In other words: the developers are not sure themselves if this contains the shared ciphers or not (and it does not, at least in OpenSSL 1.0.2 and OpenSSL 1.1.0).

In other words:  I doubt that there is a documented way to get the actually shared ciphers. One need probably to reimplement parts of  the internal ssl3_choose_cipher function since this is the place where cipher_list and session->ciphers gets combined with various other restrictions (i.e. like type of certificate) to get the shared and thus the final cipher.
msg311956 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2018-02-10 20:37
Benjamin, you implemented the feature in bpo23186. Do you have any idea what's going on?
msg311961 - (view) Author: Steffen Ullrich (noxxi) Date: 2018-02-10 21:06
It looks like the function shared_ciphers actually returned the list of client ciphers when initially implemented although I think that the name is misleading and suggests that it would return the ciphers shared between client and server (i.e. same meaning of shared as in the infamous "no shared cipher" SSL handshake problem).

Anyway, it looks like the original functionality was broken while adding support for OpenSSL 1.1.0 "Issue #26470: Port ssl and hashlib module to OpenSSL 1.1.0.". The relevant part of this change: 
https://github.com/python/cpython/commit/598894ff48e9c1171cb2ec1c798235826a75c7e0#diff-e1cc5bf74055e388cda128367a814c8fR1534
msg311963 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2018-02-10 21:24
I suppose we should fix .shared_ciphers to be SSL_get_shared_ciphers() again and perhaps add .client_ciphers, which calls SSL_get_client_ciphers?
msg311966 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2018-02-10 22:38
Yeah, looks like I used the wrong API when I ported to Python to OpenSSL 1.1.0, because there is no correct API.

For

client_context.set_ciphers("AES128-SHA256:AES256-SHA256")
server_context.set_ciphers("AES128-SHA:AES256-SHA256")

I'm getting:

client_ciphers = {'AES128-SHA256', 'AES256-SHA256'}
server_ciphers = {'AES256-SHA256', 'AES128-SHA'}
shared_ciphers = {'AES128-SHA256', 'AES256-SHA256'}  # SSL_get_client_ciphers()
shared_ciphers = {'AES256-SHA256', 'AES128-SHA'}  # SSL_get_ciphers()

which are clearly both wrong. The only shared cipher is {'AES256-SHA256'}.
msg311968 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2018-02-10 22:41
SSL_get_shared_ciphers() won't help either. Internally it gets the ciphers from s->session->ciphers just like SSL_get_client_ciphers(). It doesn't perform additional filtering.

https://github.com/openssl/openssl/blob/OpenSSL_1_0_2f/ssl/ssl_lib.c#L1392-L1427

"works well for SSLv2, not so good for SSLv3" :/
msg311983 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2018-02-11 05:17
Christian, what is your take on the criticality of this?
msg311991 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2018-02-11 09:51
I don't see the issue as critical. For me, the method is just a debugging tool. Benjamin, for which purpose did you add the method?
msg312027 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2018-02-12 00:47
I think at the time I was writing a Python version of the "openssl" tool. "openssl s_client" prints out what it says are the "Shared ciphers". So, yes, I agree the usecase is mostly debugging.
History
Date User Action Args
2018-02-12 00:47:27benjamin.petersonsetmessages: + msg312027
2018-02-11 09:51:42christian.heimessetmessages: + msg311991
2018-02-11 05:17:05ned.deilysetnosy: + ned.deily
messages: + msg311983
2018-02-10 22:41:45christian.heimessetmessages: + msg311968
2018-02-10 22:38:35christian.heimessetmessages: + msg311966
2018-02-10 21:24:47benjamin.petersonsetmessages: + msg311963
2018-02-10 21:06:52noxxisetmessages: + msg311961
2018-02-10 20:37:16christian.heimessetversions: - Python 3.5
nosy: + janssen, benjamin.peterson, alex, dstufft

messages: + msg311956

assignee: christian.heimes
components: + SSL
2018-02-10 15:02:33ned.deilysetnosy: + christian.heimes
2018-02-10 08:53:30noxxisetmessages: + msg311944
2018-02-10 07:04:15noxxicreate