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

SSLContext.hostname_checks_common_name appears to have no effect #87688

Closed
pquentin mannequin opened this issue Mar 16, 2021 · 9 comments
Closed

SSLContext.hostname_checks_common_name appears to have no effect #87688

pquentin mannequin opened this issue Mar 16, 2021 · 9 comments
Assignees
Labels
3.8 only security fixes 3.9 only security fixes 3.10 only security fixes topic-SSL type-bug An unexpected behavior, bug, or error

Comments

@pquentin
Copy link
Mannequin

pquentin mannequin commented Mar 16, 2021

BPO 43522
Nosy @tiran, @pquentin
PRs
  • bpo-43522: Fix SSLContext.hostname_checks_common_name (GH-24899) #24899
  • [3.9] bpo-43522: Fix SSLContext.hostname_checks_common_name (GH-24899) (GH-25451) #25451
  • [3.8] bpo-43522: Fix SSLContext.hostname_checks_common_name (GH-24899) (GH-25452) #25452
  • Files
  • no_san_ignored.py: Reproducer
  • app.py: Sample Flask app
  • client.pem: client.pem
  • server.pem: server.pem
  • server.key: server.key
  • 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-17.10:47:03.885>
    created_at = <Date 2021-03-16.20:07:50.463>
    labels = ['expert-SSL', 'type-bug', '3.8', '3.9', '3.10']
    title = 'SSLContext.hostname_checks_common_name appears to have no effect'
    updated_at = <Date 2021-05-04.05:50:13.339>
    user = 'https://github.com/pquentin'

    bugs.python.org fields:

    activity = <Date 2021-05-04.05:50:13.339>
    actor = 'christian.heimes'
    assignee = 'christian.heimes'
    closed = True
    closed_date = <Date 2021-04-17.10:47:03.885>
    closer = 'christian.heimes'
    components = ['SSL']
    creation = <Date 2021-03-16.20:07:50.463>
    creator = 'Quentin.Pradet'
    dependencies = []
    files = ['49879', '49880', '49881', '49882', '49883']
    hgrepos = []
    issue_num = 43522
    keywords = ['patch']
    message_count = 9.0
    messages = ['388875', '388887', '388888', '388907', '391275', '391277', '391278', '391283', '392853']
    nosy_count = 2.0
    nosy_names = ['christian.heimes', 'Quentin.Pradet']
    pr_nums = ['24899', '25451', '25452']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue43522'
    versions = ['Python 3.8', 'Python 3.9', 'Python 3.10']

    Linked PRs

    @pquentin
    Copy link
    Mannequin Author

    pquentin mannequin commented Mar 16, 2021

    urllib3 is preparing a v2 with various SSL improvements, such as leaning on the ssl module to match hostnames when possible and reject certificates without a SAN. See https://urllib3.readthedocs.io/en/latest/v2-roadmap.html#modern-security-by-default for more details.

    For this reason, we want to set hostname_checks_common_name to False on Python 3.7+ and OpenSSL 1.1.0+. (In other cases, we use a modified version of ssl.match_hostname that does not consider common names.)

    I would expect that setting hostname_checks_common_name to False would rejects certificates without SANs, but that does not appear to be the case. I used the following Python code:

        import socket
        import ssl
        
        print(ssl.OPENSSL_VERSION)
        hostname = 'localhost'
        context = ssl.create_default_context()
        context.load_verify_locations("client.pem")
        context.hostname_checks_common_name = False
        
        with socket.create_connection((hostname, 8000)) as sock:
            with context.wrap_socket(sock, server_hostname=hostname) as ssock:
                    assert "subjectAltName" not in ssock.getpeercert()

    which prints OpenSSL 1.1.1i 8 Dec 2020 and does not fail as expected. I'm testing this on macOS 11.2.2 but this currently breaks our test suite on Ubuntu, Windows and macOS, including on Python 3.10, see https://github.com/urllib3/urllib3/runs/2122811894?check_suite_focus=true.

    To reproduce this, I used trustme (https://trustme.readthedocs.io/en/latest/). I modified the code to not include a SAN at all and ran gunicorn --keyfile server.key --certfile server.pem app:app, with app being the Flask quickstart application. I'll try to attach all those files if I manage to do it.

    What am I missing?

    @pquentin pquentin mannequin added the 3.10 only security fixes label Mar 16, 2021
    @pquentin pquentin mannequin assigned tiran Mar 16, 2021
    @pquentin pquentin mannequin added topic-SSL 3.10 only security fixes labels Mar 16, 2021
    @pquentin pquentin mannequin assigned tiran Mar 16, 2021
    @pquentin pquentin mannequin added the topic-SSL label Mar 16, 2021
    @tiran
    Copy link
    Member

    tiran commented Mar 16, 2021

    Oh heck, this is a genuine bug. I'm not yet sure if it's an undocumented API quirk in OpenSSL, a design bug in OpenSSL, or a bug in my code.

    Python sets the host flags on the X509_VERIFY_PARAM of the *SSL_CTX. All flags get copied to *SSL struct and later to *X509_STORE_CTX struct. At least I thought that all flags get copied. Apparently hostflags aren't copied from *SSL_CTX to *SSL because the *SSL_CTX doesn't have any verify hosts configured. They are only ever configured on *SSL struct.

    https://github.com/openssl/openssl/blob/081a7061f3da07318c4b0f5de67b82285630bf6b/crypto/x509/x509_vpm.c#L202-L213

    @tiran
    Copy link
    Member

    tiran commented Mar 16, 2021

    PS: I don't see any remark or warning about the behavior on the man pages https://www.openssl.org/docs/man1.1.1/man3/X509_VERIFY_PARAM_set_flags.html and https://www.openssl.org/docs/man1.1.1/man3/X509_check_host.html

    @pquentin
    Copy link
    Mannequin Author

    pquentin mannequin commented Mar 17, 2021

    Thank you for the quick fix! 🙏 Both the reproducer and the urllib3 test suite run fine with this change.

    However, we can't trust HAS_NEVER_CHECK_COMMON_NAME anymore, because it will be True in Python versions where hostname_checks_common_name does not work. Is it possible to have, uh, REALLY_HAS_NEVER_CHECK_COMMON_NAME_I_PROMISE or something like that? :D It could even be private. Otherwise, we will only be able to use hostname_checks_common_name in Python 3.10.0a7+.

    @tiran
    Copy link
    Member

    tiran commented Apr 17, 2021

    New changeset b467d9a by Christian Heimes in branch 'master':
    bpo-43522: Fix SSLContext.hostname_checks_common_name (GH-24899)
    b467d9a

    @tiran
    Copy link
    Member

    tiran commented Apr 17, 2021

    New changeset cdf0287 by Christian Heimes in branch '3.9':
    [3.9] bpo-43522: Fix SSLContext.hostname_checks_common_name (GH-24899) (GH-25451)
    cdf0287

    @tiran
    Copy link
    Member

    tiran commented Apr 17, 2021

    New changeset f77ca86 by Christian Heimes in branch '3.8':
    [3.8] bpo-43522: Fix SSLContext.hostname_checks_common_name (GH-24899) (GH-25452)
    f77ca86

    @tiran
    Copy link
    Member

    tiran commented Apr 17, 2021

    Workaround has been added to upcoming 3.8 to 3.10 releases. Older versions will get fixed by next OpenSSL update.

    @tiran tiran closed this as completed Apr 17, 2021
    @tiran tiran added the type-bug An unexpected behavior, bug, or error label Apr 17, 2021
    @tiran tiran closed this as completed Apr 17, 2021
    @tiran tiran added the type-bug An unexpected behavior, bug, or error label Apr 17, 2021
    @tiran
    Copy link
    Member

    tiran commented May 4, 2021

    Seth's urllib3 newsletter reminded me that I forgot to link to OpenSSL issues here.

    The problem was caused by a bug in OpenSSL. The issue is fixed in OpenSSL default branch and is scheduled to land in next 1.1.1 release. My changes to Python's ssl module are backports of my OpenSSL fixes for older 1.1.1 releases.

    openssl/openssl#14579
    openssl/openssl#14856
    openssl/openssl@dfccfde

    @tiran tiran added 3.8 only security fixes 3.9 only security fixes labels May 4, 2021
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    miss-islington pushed a commit to miss-islington/cpython that referenced this issue Feb 17, 2024
    …thonGH-100517)
    
    (cherry picked from commit debb138)
    
    Co-authored-by: Rami <72725910+ramikg@users.noreply.github.com>
    miss-islington pushed a commit to miss-islington/cpython that referenced this issue Feb 17, 2024
    …thonGH-100517)
    
    (cherry picked from commit debb138)
    
    Co-authored-by: Rami <72725910+ramikg@users.noreply.github.com>
    serhiy-storchaka pushed a commit that referenced this issue Feb 17, 2024
    …H-100517) (GH-115595)
    
    (cherry picked from commit debb138)
    
    Co-authored-by: Rami <72725910+ramikg@users.noreply.github.com>
    serhiy-storchaka pushed a commit that referenced this issue Feb 17, 2024
    …H-100517) (GH-115594)
    
    (cherry picked from commit debb138)
    
    Co-authored-by: Rami <72725910+ramikg@users.noreply.github.com>
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.8 only security fixes 3.9 only security fixes 3.10 only security fixes topic-SSL type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    1 participant