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 match_hostname fails for internationalized domain names #72600

Closed
abracadaber mannequin opened this issue Oct 11, 2016 · 21 comments
Closed

SSL match_hostname fails for internationalized domain names #72600

abracadaber mannequin opened this issue Oct 11, 2016 · 21 comments

Comments

@abracadaber
Copy link
Mannequin

@abracadaber abracadaber mannequin commented Oct 11, 2016

BPO 28414
Nosy @tiran, @ned-deily, @alex, @njsmith, @1st1, @dstufft, @wumpus, @abracadaber, @kedare, @tialaramex
PRs
  • #3010
  • #5128
  • #5395
  • #5843
  • 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 2018-02-24.06:08:31.831>
    created_at = <Date 2016-10-11.08:02:36.045>
    labels = ['expert-SSL', 'deferred-blocker', '3.7', '3.8', 'type-feature', 'expert-asyncio']
    title = 'SSL match_hostname fails for internationalized domain names'
    updated_at = <Date 2018-02-24.06:08:31.830>
    user = 'https://github.com/abracadaber'

    bugs.python.org fields:

    activity = <Date 2018-02-24.06:08:31.830>
    actor = 'njs'
    assignee = 'christian.heimes'
    closed = True
    closed_date = <Date 2018-02-24.06:08:31.831>
    closer = 'njs'
    components = ['asyncio', 'SSL']
    creation = <Date 2016-10-11.08:02:36.045>
    creator = 'abracadaber'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 28414
    keywords = ['patch']
    message_count = 21.0
    messages = ['278466', '278483', '278488', '278519', '279165', '279920', '292025', '295384', '295391', '295775', '298583', '299812', '299813', '299846', '306450', '310992', '311128', '311130', '312476', '312685', '312696']
    nosy_count = 12.0
    nosy_names = ['janssen', 'christian.heimes', 'ned.deily', 'alex', 'njs', 'yselivanov', 'dstufft', 'wumpus', 'abracadaber', 'Socob', 'kedare', 'tialaramex']
    pr_nums = ['3010', '5128', '5395', '5843']
    priority = 'deferred blocker'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue28414'
    versions = ['Python 3.7', 'Python 3.8']

    @abracadaber
    Copy link
    Mannequin Author

    @abracadaber abracadaber mannequin commented Oct 11, 2016

    In accordance with http://tools.ietf.org/html/rfc6125#section-6.4.2:
    "If the DNS domain name portion of a reference identifier is an internationalized domain name, then an implementation MUST convert any U-labels [IDNA-DEFS] in the domain name to A-labels before checking the domain name."
    The question is: Where in python stdlib should it to convert domain name from U-label to A-label? Should it be in ssl._dnsname_match, e.g.:
    ...
    hostname = hostname.encode('idna').decode('utf-8')
    ...
    Or should it be at ssl._dnsname_match caller level?

    I found that error appears after using ssl.SSLContext.wrap_bio, which in turn uses internal newPySSLSocket, which in turn always decode server_hostname through:
    PySSLSocket *self;
    ...
    PyObject *hostname = PyUnicode_Decode(server_hostname, strlen(server_hostname), "idna", "strict");
    ...
    self->server_hostname = hostname;
    In this way, SSLSocket always contains U-label in its server_hostname field, and ssl._dnsname_match falis with "ssl.CertificateError: hostname ... doesn't match either of ..."

    And i don't understand where is a bug, or is it a bug.

    @abracadaber abracadaber mannequin added expert-asyncio type-feature expert-SSL labels Oct 11, 2016
    @abracadaber abracadaber mannequin assigned tiran Oct 11, 2016
    @tiran
    Copy link
    Member

    @tiran tiran commented Oct 11, 2016

    Thanks for bringing this to my attention. I can confirm that the code is broken. Further more there are no tests for IDN for server_hostname.

    • server_hostname must be an IDN U-label (locälhost)
    • SSL handshake correctly converts and sends TLS SNI as IDN A-label (xn--loclhost-2za)
    • getpeercert() returns DNS SAN as IDN A-label. It's less than ideal but required.
    • the serverhostname_callback is called with IDN U-label
    • match_hostname() is called with IDN U-label

    The bug is clearly in match_hostname(). The function fails to convert the hostname U-label to A-label before it compares the certificate.

    I have a rough draft of a patch here https://github.com/tiran/cpython/tree/issue28414_idna_verify

    By the way IDNA support in Python is broken in general, bpo-17305. We still don't support the latest IDNA standard from 2008 (!). IDNA 2003 is not compatible with German, Greek, Farsi and Sinhalese domains, http://unicode.org/faq/idn.html.

    @tiran tiran added the 3.7 label Oct 11, 2016
    @abracadaber
    Copy link
    Mannequin Author

    @abracadaber abracadaber mannequin commented Oct 11, 2016

    Yes, I misspelled, match_hostname() fails with ssl.CertificateError.

    @abracadaber
    Copy link
    Mannequin Author

    @abracadaber abracadaber mannequin commented Oct 12, 2016

    Christian, thanks a lot for your comment and for patch you provide. It becomes much clearer.
    I'll be watching for bpo-17305.

    @1st1
    Copy link
    Member

    @1st1 1st1 commented Oct 21, 2016

    Christian, what's the status on this one?

    @tiran
    Copy link
    Member

    @tiran tiran commented Nov 2, 2016

    It's a big, complicated mess. I can't implement IDN support correctly because Python lacks UTS#46 and IDNA2008 support. I just found out that IDNA 2008 is not enough because it does not provide a case mapping. Lack of case mapping broke my fix for curl CVE-2016-8625.

    At the moment IDN support is broken in a sane way: it just doesn't work and fails.

    A partial fix will introduce security issues. http://unicode.org/reports/tr46/#Processing lists "www.sparkasse-gießen.de" as a critical example. It's the domain of a German savings and loan bank.

    @kedare
    Copy link
    Mannequin

    @kedare kedare mannequin commented Apr 21, 2017

    Hello Christian.

    Is there any update about this issue ?
    Do we have any alternative to avoid this problem ?

    Thank you.

    @njsmith
    Copy link
    Contributor

    @njsmith njsmith commented Jun 8, 2017

    If the SSL module followed the pattern of encoding all str to bytes at the edges while leaving bytes alone, and used exclusively bytes internally (and in this case by "bytes" I mean "bytes objects containing A-labels"), then it would at least fix this bug and also make it possible for library authors to implement their own IDNA handling. Right now if you pass in a pre-encoded byte-string, exactly what ssl.py needs to compare to the certificate, then ssl.py will convert it *back* to text :-(.

    @tiran
    Copy link
    Member

    @tiran tiran commented Jun 8, 2017

    I have an idea for a different approach that can be applied to both ssl and socket module. Stay tuned to this station for a PEP broadcast!

    @tialaramex
    Copy link
    Mannequin

    @tialaramex tialaramex mannequin commented Jun 12, 2017

    I endorse njs' recommended fix here. Don't try to get clever, this is a security component, it should be the dumbest it can be possibly be while being correct, because if it's smarter it will probably be wrong.

    @tialaramex
    Copy link
    Mannequin

    @tialaramex tialaramex mannequin commented Jul 18, 2017

    Did I miss Christian's "PEP Broadcast"?

    @alex
    Copy link
    Member

    @alex alex commented Aug 6, 2017

    This came up on m.d.s.p. today: https://groups.google.com/d/msg/mozilla.dev.security.policy/K3sk5ZMv2DE/fx6c3WWFBgAJ

    I haven't dug in deeply, but it sounds like we handle IDNs in CNs and SANs differently?

    I think we should look for a way to solve that specific problem, without biting off the whole thing -- one solution would be to simply drop support for CNs in match_hostname, as both Chrome and Firefox have already done :-)

    @njsmith
    Copy link
    Contributor

    @njsmith njsmith commented Aug 6, 2017

    I haven't dug in deeply, but it sounds like we handle IDNs in CNs and SANs differently?

    No -- Python's ssl module uses exactly the same hostname checking logic in both cases, and it's equally broken regardless. But, since CAs do all kinds of weird stuff with CNs, there's some chance that our brokenness and their brokenness will align and make things work by chance. Specifically, this will happen if the CA puts the U-encoded name in the CN field. Nick Lamb's concern is that CAs may be using this as justification for continuing to issue certs that are broken in this way. I don't know if that's true, but it's possible.

    one solution would be to simply drop support for CNs in match_hostname

    That would indeed fix Nick Lamb's concern, but I'm dubious about this word "simply" :-). Obviously we should do this eventually, but it's going to break a bunch of people, you'll have to have a big fight about Python 2 and Redhat will probably refuse to take the patch and etc etc. OTOH fixing match_hostname to use A-labels would provide immediate benefits to Python's users (right now Python just... can't do SSL connections to IDNs) with minimal breakage, so you can call it a bug fix, and then worry about deprecating the CN field on its own schedule.

    @tiran
    Copy link
    Member

    @tiran tiran commented Aug 7, 2017

    For the record, I'm now considering match_hostname() on U-Labels crazy level 'A sure sign of someone who wears his underpants on his head.'. Once upon a time I had some hope to make it work and keep server_hostname to be an IDN U-Label. I no longer think it feasible and safe at the same time.

    Pros:

    • ACE is native encoding in SNI TLSEXT.
    • ACE is native encoding in X509v3 SAN extension.
    • ACE is native encoding in DNS.
    • ACE is required to avoid partial wildcards on punycode ("x*" must not match "xn--...").
    • OpenSSL's hostname verification operates on ACE.
    • ACE is not ambiguous, ACE -> U-label -> ACE depends on IDNA standard and settings.

    Cons:

    • Making SSLSocket.server_hostname IDN A-label instead of U-label is backwards incompatible.

    Self-quote from pyca/cryptography#3357 (comment)

    ---
    I have been struggling with similar issues in Python's ssl module. The current implementation cannot verify and match IDN host names. It's also a bit of a mess, SNI callback and server_hostname are IDN U-labels, cert attributes are IDN A-labels. I have played with several approaches to fix the issue. So far only one approach is both simple enough to be memorable and not a potential source of security issues. It's also fully backwards compatible with ASCII-only host names.

    User supplied input (hostname for TCP connection, server hostname) can be specified as either IDN U-label (str), IDN A-label (aka ACE, str) or ACE bytes. Internally the socket module and SSL module use ACE bytes only. Text (str) are converted to ACE bytes using IDNA. Since ACE str are just ASCII, IDNA encoding of ACE str is equivalent to encoding with ASCII encoding.

    All output (SAN dNSName, SAN URI, CN, SNI callback, server_hostname attribute) are decoded as ACE strings. Since IDN is not a bijective mapping and also depends on the IDNA standard (2003, 2008, UTS46), this avoids some potential security issues. X.509 hostname verification and matching is defined on ACE, not IDN U-labels. I would rather keep them as bytes, but it wouldn't be backwards compatible. Also the aligns the SSL module with the socket module. socket.getnameinfo() decodes with ASCII, not with IDNA.

    The new approach will make the SSL module compatible with the external idna package and IDNA 2008. Users just have to pass in ACE bytes as server_hostname.
    ---

    @tialaramex
    Copy link
    Mannequin

    @tialaramex tialaramex mannequin commented Nov 17, 2017

    As much for myself when I next run into this on my checklist as for any other readers: Despite the appearance of nothing happening PR 3010 (linked) actually has a little bit of momentum and seems likely to eventually land in Python.

    @ned-deily
    Copy link
    Member

    @ned-deily ned-deily commented Jan 28, 2018

    At Christian's request and considering the importance of the ssl module, I'm going to allow an extension for landing of this feature until 3.7.0b2, currently scheduled for 2018-02-26. If anyone else can help Christian get this in before b2, that would be great. I'm removing older versions for now. We can discuss potential backports after the feature lands.

    @tiran
    Copy link
    Member

    @tiran tiran commented Jan 29, 2018

    New changeset 66e5742 by Christian Heimes in branch 'master':
    bpo-28414: ssl module idna test (bpo-5395)
    66e5742

    @tiran
    Copy link
    Member

    @tiran tiran commented Jan 29, 2018

    In PR #5395 I added a test to verify that most IDNA domains are now working. IDNA 2008 deviations and the fundamental issue of IDNA server callback and IDNA encoded server_hostname attribute are still open. I'll address them in another PR.

    @njsmith
    Copy link
    Contributor

    @njsmith njsmith commented Feb 21, 2018

    Christian: we're less than a week out from b2. Do you need any help here?

    @njsmith
    Copy link
    Contributor

    @njsmith njsmith commented Feb 24, 2018

    New changeset 11a1493 by Nathaniel J. Smith (Christian Heimes) in branch 'master':
    [bpo-28414] Make all hostnames in SSL module IDN A-labels (GH-5128)
    11a1493

    @njsmith
    Copy link
    Contributor

    @njsmith njsmith commented Feb 24, 2018

    New changeset 1c37e27 by Nathaniel J. Smith (Miss Islington (bot)) in branch '3.7':
    [bpo-28414] Make all hostnames in SSL module IDN A-labels (GH-5128) (GH-5843)
    1c37e27

    @njsmith njsmith closed this as completed Feb 24, 2018
    @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
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants