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

http.client.HTTPSConnection checks hostname when SSL context has check_hostname==False #67148

Closed
desbma mannequin opened this issue Nov 27, 2014 · 10 comments
Closed

http.client.HTTPSConnection checks hostname when SSL context has check_hostname==False #67148

desbma mannequin opened this issue Nov 27, 2014 · 10 comments
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@desbma
Copy link
Mannequin

desbma mannequin commented Nov 27, 2014

BPO 22959
Nosy @tiran, @benjaminp, @alex, @desbma
Files
  • ch-weirdness.aptch
  • 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 2014-12-07.18:49:12.781>
    created_at = <Date 2014-11-27.19:42:39.785>
    labels = ['type-bug', 'library']
    title = 'http.client.HTTPSConnection checks hostname when SSL context has check_hostname==False'
    updated_at = <Date 2014-12-07.23:36:37.519>
    user = 'https://github.com/desbma'

    bugs.python.org fields:

    activity = <Date 2014-12-07.23:36:37.519>
    actor = 'desbma'
    assignee = 'none'
    closed = True
    closed_date = <Date 2014-12-07.18:49:12.781>
    closer = 'benjamin.peterson'
    components = ['Library (Lib)']
    creation = <Date 2014-11-27.19:42:39.785>
    creator = 'desbma'
    dependencies = []
    files = ['37325']
    hgrepos = []
    issue_num = 22959
    keywords = []
    message_count = 10.0
    messages = ['231775', '231886', '231887', '231890', '231892', '231893', '231896', '232274', '232275', '232289']
    nosy_count = 5.0
    nosy_names = ['christian.heimes', 'benjamin.peterson', 'alex', 'python-dev', 'desbma']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = None
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue22959'
    versions = ['Python 3.4']

    @desbma
    Copy link
    Mannequin Author

    desbma mannequin commented Nov 27, 2014

    http.client.HTTPSConnection has both a check_hostname parameter, and a context parameter to pass an already setup SSL context.
    When check_hostname is not set and thus is None, and when passing a SSL context set to NOT check hostnames, ie:

    import http.client
    import ssl
    
    ssl_context = ssl.create_default_context() 
    ssl_context.check_hostname = False
    https = http.client.HTTPSConnection(..., context=ssl_context)

    The https object WILL check hostname.

    In my opinion the line :
    https://hg.python.org/cpython/file/3.4/Lib/http/client.py#l1207
    will_verify = context.verify_mode != ssl.CERT_NONE

    Should be changed to:
    will_verify = (context.verify_mode != ssl.CERT_NONE) and (context.check_hostname)

    @desbma desbma mannequin added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Nov 27, 2014
    @benjaminp
    Copy link
    Contributor

    Why do you think it still verifies the hostname? It will certainly check if the certificate has a valid trust chain, but it won't do matching on the hostname.

    @desbma
    Copy link
    Mannequin Author

    desbma mannequin commented Nov 30, 2014

    I think it does, when passing a context with ssl_context.verify_mode != ss.CERT_NONE, and when not setting the check_hostname parameter:

    1. will_verify will be True (https://hg.python.org/cpython/file/3.4/Lib/http/client.py#l1207)
    2. check_hostname will be True too (https://hg.python.org/cpython/file/3.4/Lib/http/client.py#l1209)
    3. ssl.match_hostname will be called after the handshake in wrap_socket (https://hg.python.org/cpython/file/3.4/Lib/http/client.py#l1230)

    The following code shows the problem:

    import http.client
    import ssl
    
    ssl_context = ssl.SSLContext(ssl.PROTOCOL_TLSv1_2)
    ssl_context.verify_mode = ssl.CERT_REQUIRED
    ssl_context.check_hostname = False
    https = http.client.HTTPSConnection("localhost", context=ssl_context)
    print(https._check_hostname)

    @benjaminp
    Copy link
    Contributor

    As the documentation says "If context is specified and has a verify_mode of either CERT_OPTIONAL or CERT_REQUIRED, then by default host is matched against the host name(s) allowed by the server’s certificate. If you want to change that behaviour, you can explicitly set check_hostname to False."

    It is rather weird that HTTPSConnection has its own "check_hostname" parameter independent of the one on the SSL context.

    Alex, what do you think of this patch?

    @alex
    Copy link
    Member

    alex commented Nov 30, 2014

    This will cause it to not validate in some cases where it currently is validating? That seems like a regression to me.

    @benjaminp
    Copy link
    Contributor

    On Sun, Nov 30, 2014, at 11:20, Alex Gaynor wrote:

    Alex Gaynor added the comment:

    This will cause it to not validate in some cases where it currently is
    validating? That seems like a regression to me.

    I suppose. Certainly, none of the "default" cases are affected. The
    problem is it's impossible to have cert validation w/o hostname checking
    by passing a context to some higher level API than HTTPSConnection (like
    xmlrpclib) because HTTPSConnection tries to be clever. Ideally, the
    check_hostname argument wouldn't exist, and everything would come from
    the context.

    @desbma
    Copy link
    Mannequin Author

    desbma mannequin commented Nov 30, 2014

    I agree that changing a default to something less secure is not something to do lightly, however I think forcing a check that is explicitly disabled is a bug and can be counter productive security wise.

    People who don't have time to look at the stdlib code, and file bug like this are likely to react with "fuck it, I'll use plain HTTP".

    By the way, my use case is precisely xmlrpc.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Dec 7, 2014

    New changeset a409a7cd908d by Benjamin Peterson in branch '3.4':
    HTTPSConnection: prefer the context's check_hostname attribute over the constructor parameter (bpo-22959)
    https://hg.python.org/cpython/rev/a409a7cd908d

    New changeset 41021c771510 by Benjamin Peterson in branch '2.7':
    remove HTTPSConnection's check_hostname parameter (bpo-22959)
    https://hg.python.org/cpython/rev/41021c771510

    New changeset ee095a2e2b35 by Benjamin Peterson in branch 'default':
    merge 3.4 (bpo-22959)
    https://hg.python.org/cpython/rev/ee095a2e2b35

    @benjaminp
    Copy link
    Contributor

    Okay, I basically applied my patch to 3.4/3.5. I simply removed the check_hostname parameter from 2.7, since it was to be added in 2.7.9.

    @desbma
    Copy link
    Mannequin Author

    desbma mannequin commented Dec 7, 2014

    Thank you

    @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-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants