Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ssl module unnecessarily pins the client curve when using ECDH #75990

Closed
grrrrrrrrr mannequin opened this issue Oct 18, 2017 · 7 comments
Closed

ssl module unnecessarily pins the client curve when using ECDH #75990

grrrrrrrrr mannequin opened this issue Oct 18, 2017 · 7 comments
Assignees
Labels
topic-SSL type-bug An unexpected behavior, bug, or error

Comments

@grrrrrrrrr
Copy link
Mannequin

grrrrrrrrr mannequin commented Oct 18, 2017

BPO 31809
Nosy @tiran, @alex, @dstufft, @grrrrrrrrr
PRs
  • bpo-31809: test secp ECDH curves #4036
  • [3.7] bpo-31809: test secp ECDH curves (GH-4036) #5872
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/tiran'
    closed_at = <Date 2017-10-25.11:42:44.283>
    created_at = <Date 2017-10-18.13:15:40.243>
    labels = ['expert-SSL', 'type-bug', 'invalid']
    title = 'ssl module unnecessarily pins the client curve when using ECDH'
    updated_at = <Date 2018-02-25.09:56:13.947>
    user = 'https://github.com/grrrrrrrrr'

    bugs.python.org fields:

    activity = <Date 2018-02-25.09:56:13.947>
    actor = 'christian.heimes'
    assignee = 'christian.heimes'
    closed = True
    closed_date = <Date 2017-10-25.11:42:44.283>
    closer = 'grrrrrrrrr'
    components = ['SSL']
    creation = <Date 2017-10-18.13:15:40.243>
    creator = 'grrrrrrrrr'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 31809
    keywords = ['patch']
    message_count = 7.0
    messages = ['304577', '304579', '304581', '304586', '304978', '312785', '312794']
    nosy_count = 5.0
    nosy_names = ['janssen', 'christian.heimes', 'alex', 'dstufft', 'grrrrrrrrr']
    pr_nums = ['4036', '5872']
    priority = 'normal'
    resolution = 'not a bug'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue31809'
    versions = ['Python 2.7', 'Python 3.5']

    @grrrrrrrrr
    Copy link
    Mannequin Author

    grrrrrrrrr mannequin commented Oct 18, 2017

    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:

      #if !defined(OPENSSL_NO_ECDH) && !defined(OPENSSL_VERSION_1_1)

    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 f1a696e 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.

    @grrrrrrrrr grrrrrrrrr mannequin assigned tiran Oct 18, 2017
    @grrrrrrrrr grrrrrrrrr mannequin added topic-SSL type-bug An unexpected behavior, bug, or error labels Oct 18, 2017
    @tiran
    Copy link
    Member

    tiran commented Oct 18, 2017

    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

    @grrrrrrrrr
    Copy link
    Mannequin Author

    grrrrrrrrr mannequin commented Oct 18, 2017

    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.

    @tiran
    Copy link
    Member

    tiran commented Oct 18, 2017

    • 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.

    @grrrrrrrrr
    Copy link
    Mannequin Author

    grrrrrrrrr mannequin commented Oct 25, 2017

    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.

    @grrrrrrrrr grrrrrrrrr mannequin closed this as completed Oct 25, 2017
    @grrrrrrrrr grrrrrrrrr mannequin added the invalid label Oct 25, 2017
    @tiran
    Copy link
    Member

    tiran commented Feb 25, 2018

    New changeset b7b9225 by Christian Heimes in branch 'master':
    bpo-31809: test secp ECDH curves (bpo-4036)
    b7b9225

    @tiran
    Copy link
    Member

    tiran commented Feb 25, 2018

    New changeset ff7528f by Christian Heimes (Miss Islington (bot)) in branch '3.7':
    [3.7] bpo-31809: test secp ECDH curves (GH-4036) (bpo-5872)
    ff7528f

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    topic-SSL type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    1 participant