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

Prohibit direct instantiation of SSLSocket and SSLObject #77132

Closed
tiran opened this issue Feb 25, 2018 · 4 comments
Closed

Prohibit direct instantiation of SSLSocket and SSLObject #77132

tiran opened this issue Feb 25, 2018 · 4 comments
Assignees
Labels
3.7 3.8 expert-SSL type-bug

Comments

@tiran
Copy link
Member

@tiran tiran commented Feb 25, 2018

BPO 32951
Nosy @pitrou, @tiran, @alex, @dstufft, @csabella
PRs
  • #5864
  • #5925
  • 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 2021-04-19.20:18:39.302>
    created_at = <Date 2018-02-25.17:18:04.394>
    labels = ['expert-SSL', '3.8', 'type-bug', '3.7']
    title = 'Prohibit direct instantiation of SSLSocket and SSLObject'
    updated_at = <Date 2021-04-19.20:18:39.301>
    user = 'https://github.com/tiran'

    bugs.python.org fields:

    activity = <Date 2021-04-19.20:18:39.301>
    actor = 'christian.heimes'
    assignee = 'christian.heimes'
    closed = True
    closed_date = <Date 2021-04-19.20:18:39.302>
    closer = 'christian.heimes'
    components = ['SSL']
    creation = <Date 2018-02-25.17:18:04.394>
    creator = 'christian.heimes'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 32951
    keywords = ['patch']
    message_count = 4.0
    messages = ['312823', '312824', '312992', '337706']
    nosy_count = 6.0
    nosy_names = ['janssen', 'pitrou', 'christian.heimes', 'alex', 'dstufft', 'cheryl.sabella']
    pr_nums = ['5864', '5925']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue32951'
    versions = ['Python 3.7', 'Python 3.8']

    @tiran
    Copy link
    Member Author

    @tiran tiran commented Feb 25, 2018

    The constructors of SSLObject and SSLSocket were never documented, tested, or meant to be used directly. Instead users were suppose to use ssl.wrap_socket or an SSLContext object. The ssl.wrap_socket() function and direct instantiation of SSLSocket has multiple issues. From my mail "No hostname matching with ssl.wrap_socket() and SSLSocket() constructor" to PSRT:

    The ssl module has three ways to create a
    SSLSocket object:

    1. ssl.wrap_socket() [1]
    2. ssl.SSLSocket() can be instantiated directly without a context [2]
    3. SSLContext.wrap_socket() [3]

    Variant (1) and (2) are old APIs with insecure default settings.

    Variant (3) is the new and preferred way. With
    ssl.create_default_context() or ssl.SSLContext(ssl.PROTOCOL_TLS_CLIENT)
    the socket is configured securely with hostname matching and cert
    validation enabled.

    While Martin Panter was reviewing my documentation improvements for the
    ssl module, he pointed out an issue,
    #3530 (comment) .
    ssl.wrap_socket() and ssl.SSLSocket() default to CERT_NONE but
    PROTOCOL_TLS_CLIENT is documented to set CERT_REQUIRED. After a closer
    look, it turned out that the code is robust and refuses to accept
    PROTOCOL_TLS_CLIENT + default values with "Cannot set verify_mode to
    CERT_NONE when check_hostname is enabled.". I consider the behavior a
    feature.

    However ssl.SSLSocket() constructor and ssl.wrap_socket() have more
    fundamental security issues. I haven't looked at the old legacy APIs in
    a while and only concentrated on SSLContext. To my surprise both APIs do
    NOT perform or allow hostname matching. The wrap_socket() function does
    not even take a server_hostname argument, so it doesn't send a SNI TLS
    extension either. These bad default settings can lead to suprising
    security bugs in 3rd party code. This example doesn't fail although the
    hostname doesn't match the certificate:

    ---

    import socket
    import ssl
    
    cafile = ssl.get_default_verify_paths().cafile
    
    with socket.socket() as sock:
        ssock = ssl.SSLSocket(
            sock,
            cert_reqs=ssl.CERT_REQUIRED,
            ca_certs=cafile,
            server_hostname='www.python.org'
        )
        ssock.connect(('www.evil.com', 443))

    I don't see a way to fix the issue in a secure way while keeping
    backwards compatibility. We could either modify the default behavior of
    ssl.wrap_socket() and SSLSocket() constructor, or drop both features
    completely. Either way it's going to break software that uses them.
    Since I like to get rid of variants (1) and (2), I would favor to remove
    them in favor of SSLContext.wrap_socket(). At least we should implement
    my documentation bug 28124 [4] and make ssl.wrap_socket() less
    prominent. I'd appreciate any assistance.

    By the way, SSLObject is sane because it always goes through
    SSLContext.wrap_bio(). Thanks Benjamin!

    Regards,
    Christian

    [1] https://docs.python.org/3/library/ssl.html#ssl.wrap_socket
    [2] https://docs.python.org/3/library/ssl.html#ssl.SSLSocket
    [3] https://docs.python.org/3/library/ssl.html#ssl.SSLContext.wrap_socket
    [4] https://bugs.python.org/issue28124

    @tiran tiran self-assigned this Feb 25, 2018
    @tiran tiran added expert-SSL type-bug labels Feb 25, 2018
    @tiran
    Copy link
    Member Author

    @tiran tiran commented Feb 25, 2018

    Antoine Pitrou replied:

    The ssl.SSLSocket constructor was never meant to be called by user
    code directly (and I don't think we document it as such). Anyone doing
    this is asking for trouble (including compatibility breakage as we
    change the constructor signature).

    ssl.wrap_socket() is essentially a legacy API.

    I would suggest the following measures :

    • Deprecate ssl.wrap_socket() and slate it to be removed around 3.8 or
      3.9. SSLContext is now is any recent 2.7 or 3.x version, so the
      compatibility argument doesn't hold anymore.

    • Severely de-emphasize ssl.wrap_socket() in the documentation (relegate
      it in a "legacy API" section at the end), and put a warning that it's
      insecure.

    ---
    Alex Gaynor replied:

    If SSLSocket.__init__ is meant to be private and not called by users,
    perhaps we could clean up the API and remove all the legacy arguments it
    takes, bring it down to just taking a context?

    It'd break anyone who was relying on it, but they weren't supposed to be
    relying on it in the first place... (Is it documented even?)

    ---

    I have implemented Antoine's second proposal in bpo-28124.

    @tiran
    Copy link
    Member Author

    @tiran tiran commented Feb 27, 2018

    New changeset 89c2051 by Christian Heimes in branch '3.7':
    [3.7] bpo-32951: Disable SSLSocket/SSLObject constructor (GH-5864) (bpo-5925)
    89c2051

    @csabella
    Copy link
    Contributor

    @csabella csabella commented Mar 11, 2019

    Can this issue be closed as resolved?

    @tiran tiran closed this as completed Apr 19, 2021
    @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
    3.7 3.8 expert-SSL type-bug
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants