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

server-specific SSL context configuration #65212

Closed
pitrou opened this issue Mar 21, 2014 · 17 comments
Closed

server-specific SSL context configuration #65212

pitrou opened this issue Mar 21, 2014 · 17 comments
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@pitrou
Copy link
Member

pitrou commented Mar 21, 2014

BPO 21013
Nosy @pitrou, @tiran, @alex, @dstufft
Files
  • ssl-server-defaults.diff
  • ssl-context-defaults-ssl3-diff
  • ssl-context-defaults-ssl3-guards.diff
  • 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 = None
    closed_at = <Date 2015-04-13.19:02:56.500>
    created_at = <Date 2014-03-21.19:07:46.089>
    labels = ['type-feature', 'library']
    title = 'server-specific SSL context configuration'
    updated_at = <Date 2015-04-13.19:02:56.500>
    user = 'https://github.com/pitrou'

    bugs.python.org fields:

    activity = <Date 2015-04-13.19:02:56.500>
    actor = 'pitrou'
    assignee = 'none'
    closed = True
    closed_date = <Date 2015-04-13.19:02:56.500>
    closer = 'pitrou'
    components = ['Library (Lib)']
    creation = <Date 2014-03-21.19:07:46.089>
    creator = 'pitrou'
    dependencies = []
    files = ['34569', '34577', '34590']
    hgrepos = []
    issue_num = 21013
    keywords = ['patch']
    message_count = 17.0
    messages = ['214405', '214407', '214408', '214487', '214493', '214498', '214499', '214500', '214501', '214502', '214504', '214505', '214538', '214614', '214620', '214649', '214650']
    nosy_count = 5.0
    nosy_names = ['pitrou', 'christian.heimes', 'alex', 'python-dev', 'dstufft']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue21013'
    versions = ['Python 3.4', 'Python 3.5']

    @pitrou
    Copy link
    Member Author

    pitrou commented Mar 21, 2014

    Currently, create_default_context() doesn't do anything special for server use. It seems the configuration could be improved, though:

    • PROTOCOL_TLSv1 is suboptimal for servers: a "TLSv1" server can't accept a TLSv1.2 client, but a "SSLv23" server will; so we should use PROTOCOL_SSLv23 (!)

    • we could enable ECDH by calling SSLContext.set_ecdh_curve(<something>)

    @pitrou pitrou added stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Mar 21, 2014
    @pitrou
    Copy link
    Member Author

    pitrou commented Mar 21, 2014

    (also perhaps enable OP_CIPHER_SERVER_PREFERENCE, although it seems it could cause interoperability problems with some clients)

    @dstufft
    Copy link
    Member

    dstufft commented Mar 21, 2014

    Nah it should be fine to enable that, and it's preferable to do so. The server selects the cipher anyways in the TLS handshake. That just tells the server to prefer it's list for precedence and not the client list.

    @dstufft
    Copy link
    Member

    dstufft commented Mar 22, 2014

    Attached is a patch that:

    • Switches the protocol to SSLv23 so that we can negotiate a TLS1.1 or TLS1.2 connection.
    • Sets OP_CIPHER_SERVER_PREFERENCE for Purpose.CLIENT_AUTH so that our carefully selected cipher priority gives us better encryption and PFS
    • Sets OP_SINGLE_DH_USE and OP_SINGLE_ECDH_USE for Purpose.CLIENT_AUTH to prevent re-use of the DH and ECDH keys in distinct sessions.

    @pitrou
    Copy link
    Member Author

    pitrou commented Mar 22, 2014

    (as an aside, Donald, perhaps you want to consider adding yourself to relevant topics in http://docs.python.org/devguide/experts.html )

    @dstufft
    Copy link
    Member

    dstufft commented Mar 22, 2014

    I'll do that :)

    To be clear about this patch, it raises the upper bounds of security by enabling TLS 1.1, and 1.2 as well as the single use for (EC)DH and preferring the server ciphers.

    However it also lowers the lower bounds of security and includes SSLv3 which has some issues (see https://en.wikipedia.org/wiki/Transport_Layer_Security#SSL_3.0). However there exists clients who only support SSL3 (The primary one I'm aware of is IE6 on Windows XP).

    We can add OP_NO_SSLv3 to the default context to prevent SSL3 but it's sort of a situational thing. If you're doing something where you need SSL3 clients you don't want OP_NO_SSLv3.

    So I guess the question is, do we want to be more secure by default and *not* lower the lower bounds of security and require people to add context.options & ~ssl.OP_NO_SSLv3 if they want to support SSLv3 connections?

    @pitrou
    Copy link
    Member Author

    pitrou commented Mar 22, 2014

    We can add OP_NO_SSLv3 to the default context to prevent SSL3 but it's
    sort of a situational thing. If you're doing something where you need
    SSL3 clients you don't want OP_NO_SSLv3.

    So I guess the question is, do we want to be more secure by default
    and *not* lower the lower bounds of security and require people to add
    context.options & ~ssl.OP_NO_SSLv3 if they want to support SSLv3
    connections?

    Most people won't understand the symptoms if some clients can't connect,
    so I'd say no.
    Also, clients should always use the higher possible protocol version, so
    I don't think security is at stake here.

    @alex
    Copy link
    Member

    alex commented Mar 22, 2014

    Unfortunately most TLS implementations (particularly those in browser stacks) are vulnerable to downgrade attacks, whereby an attacker can send some malicious packets to simulate a connection failure and cause a lower version of the protocol to be negotiated, https://crypto.stackexchange.com/questions/10493/why-is-tls-susceptible-to-protocol-downgrade-attacks has some info on it. As a result, whenever possible it's really desirable to completely disallow as many poor choices as possible.

    @dstufft
    Copy link
    Member

    dstufft commented Mar 22, 2014

    That's not entirely true unfortunately :(

    There are downgrade attacks that work all the way up through TLS 1.2. These are not strictly a problem of the protocol specs but instead of the implementations.

    See: https://crypto.stackexchange.com/questions/10493/why-is-tls-susceptible-to-protocol-downgrade-attacks

    The general gist of it is some servers/firewalls/etc have buggy implementations that cause a TLS1.0+ handshake to fail and some clients (browsers being a big one) decided to handle this by restarting the connection with SSL3.0 instead of TLS1.0+. So thus it is possible to effectively downgrade a client, even one that supports TLS1.2. It is not however possible to do it within a single connection.

    The version selection process should not be considered a security feature but should instead be looked at as a way to opportunistically add newer features.

    @dstufft
    Copy link
    Member

    dstufft commented Mar 22, 2014

    To be clear though, a lot of TLS servers out there still have SSL3.0 enabled by default, primarily because of IE6 / XP. I'm on the fence about what the right answer is for create_default_context. From a strictly "best practices for security" sense of view you want to disable SSLv3 (and this matches what create_default_context did prior to my patch).

    Can we perhaps split the difference and disable SSL3.0 and document what the error looks like when you try to connect with SSL3.0 and how to re-enable it?

    @pitrou
    Copy link
    Member Author

    pitrou commented Mar 22, 2014

    Well, I suppose IE6/XP is starting to look very old (though probably deployed quite widely), and TLS 1.0 was standardized in 1999.

    @pitrou
    Copy link
    Member Author

    pitrou commented Mar 22, 2014

    (by which I mean: ok, let's disable SSLv3)

    @dstufft
    Copy link
    Member

    dstufft commented Mar 23, 2014

    Attached is a new patch. It has:

    • Switches the protocol to SSLv23 so that we can negotiate a TLS1.1 or TLS1.2 connection.
    • Sets OP_CIPHER_SERVER_PREFERENCE for Purpose.CLIENT_AUTH so that our carefully selected cipher priority gives us better encryption and PFS
    • Sets OP_SINGLE_DH_USE and OP_SINGLE_ECDH_USE for Purpose.CLIENT_AUTH to prevent re-use of the DH and ECDH keys in distinct sessions.
    • Disables SSLv3 connections explicitly to match lower bounds of the original security of the created context
    • Moves the "restricted" ciphers to only apply to servers. Servers can be much more picky about which ciphers they accept than clients can, and further more with how our ciphers are laid out now if RC4 is selected it is entirely the fault of the server we are connecting to.
    • Document what the type of error message would be if a SSL 3.0 connection is required and how to re-enable it.

    @dstufft
    Copy link
    Member

    dstufft commented Mar 23, 2014

    I think I'm happy with this patch, if anyone has a chance to review it and see if it looks OK I'd love that and then I can commit it :)

    @dstufft
    Copy link
    Member

    dstufft commented Mar 23, 2014

    Added guards to protect against constants not existing.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Mar 23, 2014

    New changeset 92efd86d1a38 by Donald Stufft in branch '3.4':
    Issue bpo-21013: Enhance ssl.create_default_context() for server side contexts
    http://hg.python.org/cpython/rev/92efd86d1a38

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Mar 23, 2014

    New changeset aa2eb034c4f7 by Donald Stufft in branch 'default':
    Merge the patch for issue bpo-21013 into default
    http://hg.python.org/cpython/rev/aa2eb034c4f7

    @pitrou pitrou closed this as completed Apr 13, 2015
    @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
    stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants