This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: ssl module unnecessarily pins the client curve when using ECDH
Type: behavior Stage: resolved
Components: SSL Versions: Python 3.5, Python 2.7
process
Status: closed Resolution: not a bug
Dependencies: Superseder:
Assigned To: christian.heimes Nosy List: alex, christian.heimes, dstufft, grrrrrrrrr, janssen
Priority: normal Keywords: patch

Created on 2017-10-18 13:15 by grrrrrrrrr, last changed 2022-04-11 14:58 by admin. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 4036 merged christian.heimes, 2017-10-18 16:09
PR 5872 merged miss-islington, 2018-02-25 08:49
Messages (7)
msg304577 - (view) Author: Andy (grrrrrrrrr) Date: 2017-10-18 13:15
When using elliptic curves in combination with the ssl module to wrap a socket, the only curve the client accepts is prime256v1. Expected behavior is to accept all curves that are on the default list and supported by the server.

We noticed the issue when connecting to a server that is configured with a non standard curve, we get

[SSL: WRONG_CURVE] wrong curve (_ssl.c:661)

unless we explicitly specify to use the correct curve for the context using

context.set_ecdh_curve("secp521r1")

The bug happens both when using a context or ssl.wrap_socket directly.



The bug is that cpython calls OpenSSL (or whatever lib is linked) server methods even when the context is purely for client communications. The flow is:

- A ssl.SSLContext gets instantiated that will later be used in SSLContext.wrap_socket with server_side either True or False (bug happens when this is client side, so server_side=False).

- In the context's constructor, where it's not yet clear if the context is for a client or a server socket, there is this code:
https://github.com/python/cpython/blob/b9a860f3bf80b0d4a6c25d0f2f6ef849d9bf3594/Modules/_ssl.c#L2180

That code calls (in many cases, depending on the Python and OpenSSL versions involved) server side only methods, SSL_CTX_set_ecdh_auto or SSL_CTX_set_tmp_ecdh.

Those methods should in theory not influence the client side of the context, there is even a comment in the docs for SSLContext.set_ecdh_curve which does similar things that says "This setting doesn’t apply to client sockets". However, this being OpenSSL, this is not true. The methods actually set the list of acceptable curves that is used for both, server and client side connections, to exactly a single curve, prime256v1. This happens for both, SSL_CTX_set_tmp_ecdh and SSL_CTX_set_ecdh_auto.


Versions affected:

OpenSSL has changed the API twice here. Before 1.0.2, SSL_CTX_set_tmp_ecdh was mandatory, 1.0.2 - <1.1.0 used SSL_CTX_set_ecdh_auto and 1.1.0+ doesn't need this setting at all.

Python 2.7.14+ added a check for 1.1.0+ in https://github.com/python/cpython/commit/f1a696efd6ca674579e25de29ec4053ff5a5ade1 so starting from that version the issue only happens when using OpenSSL <1.1.0. I suspect this is the case for many machines still, including all Macs (haven't confirmed the actual bug there). For LibreSSL the server side methods will be called too, I can't confirm if that combination is vulnerable too.

Python 3.5.3 gives me the same error, no reason to believe that higher versions are not affected.
msg304579 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2017-10-18 14:06
Which version of OpenSSL are you using? Please note that macOS' system python uses either an ancient version of OpenSSL 0.9.8 or an ancient version of LibreSSL (IIRC 2.3.x).

The code in question is:

if !defined(OPENSSL_NO_ECDH) && !defined(OPENSSL_VERSION_1_1)
    /* Allow automatic ECDH curve selection (on OpenSSL 1.0.2+), or use
       prime256v1 by default.  This is Apache mod_ssl's initialization
       policy, so we should be safe. OpenSSL 1.1 has it enabled by default.
     */
#if defined(SSL_CTX_set_ecdh_auto)
    SSL_CTX_set_ecdh_auto(self->ctx, 1);
#else
    {
        EC_KEY *key = EC_KEY_new_by_curve_name(NID_X9_62_prime256v1);
        SSL_CTX_set_tmp_ecdh(self->ctx, key);
        EC_KEY_free(key);
    }
#endif
#endif

The block is executed for all SSLContexts (server and client) because . The behavior depends on the version of OpenSSL:

OpenSSL >= 1.1: not executed
OpenSSL >= 1.0.2, < 1.1: SL_CTX_set_ecdh_auto(ctx, 1)
LibreSSL: SSL_CTX_set_ecdh_auto(ctx, 1)
OpenSSL < 1.0.2: hard-code prime256v1

Since we have no mean to distinguish between a server context and a client context at the moment, we unconditionally call SSL_CTX_set_ecdh_auto(). It may not be perfect under some condition. But it gives most users a sane and secure default to start with.

https://www.openssl.org/docs/man1.0.2/ssl/SSL_CTX_set_ecdh_auto.html
msg304581 - (view) Author: Andy (grrrrrrrrr) Date: 2017-10-18 15:10
While debugging I reproduced this on
- 'OpenSSL 1.1.0f  25 May 2017'
- 'OpenSSL 1.0.1f 6 Jan 2014'
- and 'BoringSSL', latest.

using Python 2.7.12, 2.7.13, 2.7.6 and 3.5.3. This was all on Debian.


Note that since I used Python <2.7.14 (or equivalent for 3.x) for all tests, the check "... && !defined(OPENSSL_VERSION_1_1)" is missing and therefore the bug *always* triggers regardless of OpenSSL version.



I'm not sure I agree that this one curve is a good default. Note that openssl has 81 curves currently (openssl ecparam -list_curves) and probably can use most of them to connect to a server - accommodating a variety of server setups. Restricting this list to one single curve seems suboptimal.

Think about it like this, if tomorrow we find an issue with that particular curve, all servers can just migrate to a different one and all clients will be able to connect just fine - except those that use Python, they will not be able to talk to those servers ever again until they are upgraded. I mean in the end it's your call but having a *client* just accepting one single security parameter and nothing else doesn't seem right.
msg304586 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2017-10-18 16:10
- BoringSSL is not a supported libssl/libcrypto library for Python. We only support 100% OpenSSL-compatible libraries.
- OpenSSL 1.0.1 is no longer supported by upstream. Python's semi-official support policy for 1.0.1 and 0.9.8 is "use at your own risk". You should upgrade.
- Python < 2.7.13 does not support OpenSSL 1.1
- Python >= 2.7.13 has #if !defined(OPENSSL_VERSION_1_1) before SSL_CTX_set_ecdh_auto()

I added a new test that verifies connections with various curve settings. I cannot reproduce your problem with the test case.
msg304978 - (view) Author: Andy (grrrrrrrrr) Date: 2017-10-25 11:42
Thanks for adding the test!

If the official stance is that only the latest OpenSSL is supported then this is definitely WAI. Sounds like a good policy...

I'll close this issue.
msg312785 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2018-02-25 08:49
New changeset b7b9225831a729bff84eb7c43bad138416b994fe by Christian Heimes in branch 'master':
bpo-31809: test secp ECDH curves (#4036)
https://github.com/python/cpython/commit/b7b9225831a729bff84eb7c43bad138416b994fe
msg312794 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2018-02-25 09:56
New changeset ff7528f089b60f8372c658f3bc3b14b059114da9 by Christian Heimes (Miss Islington (bot)) in branch '3.7':
[3.7] bpo-31809: test secp ECDH curves (GH-4036) (#5872)
https://github.com/python/cpython/commit/ff7528f089b60f8372c658f3bc3b14b059114da9
History
Date User Action Args
2022-04-11 14:58:53adminsetgithub: 75990
2018-02-25 09:56:13christian.heimessetmessages: + msg312794
2018-02-25 08:49:40miss-islingtonsetpull_requests: + pull_request5645
2018-02-25 08:49:34christian.heimessetmessages: + msg312785
2017-10-25 11:42:44grrrrrrrrrsetstatus: open -> closed
resolution: not a bug
messages: + msg304978

stage: resolved
2017-10-18 16:10:00christian.heimessetmessages: + msg304586
stage: patch review -> (no value)
2017-10-18 16:09:04christian.heimessetkeywords: + patch
stage: patch review
pull_requests: + pull_request4010
2017-10-18 15:10:20grrrrrrrrrsetmessages: + msg304581
2017-10-18 14:06:53christian.heimessetnosy: + janssen, alex, dstufft
messages: + msg304579
2017-10-18 13:15:40grrrrrrrrrcreate