Skip to content

SSL certificate verification failed if no dNSName entry in subjectAltName #56209

Closed
Closed
SSL certificate verification failed if no dNSName entry in subjectAltName#56209
Labels
stdlibStandard Library Python modules in the Lib/ directorytype-bugAn unexpected behavior, bug, or error

Description

@nbareil
nbareil
mannequin
opened on May 4, 2011
BPO 12000
Nosy @pitrou

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 2011-05-06.13:37:28.965>
created_at = <Date 2011-05-04.11:35:52.055>
labels = ['type-bug', 'library']
title = 'SSL certificate verification failed if no dNSName entry in subjectAltName'
updated_at = <Date 2011-06-16.00:07:05.626>
user = 'https://bugs.python.org/nbareil'

bugs.python.org fields:

activity = <Date 2011-06-16.00:07:05.626>
actor = 'kiilerix'
assignee = 'none'
closed = True
closed_date = <Date 2011-05-06.13:37:28.965>
closer = 'pitrou'
components = ['Library (Lib)']
creation = <Date 2011-05-04.11:35:52.055>
creator = 'nbareil'
dependencies = []
files = []
hgrepos = []
issue_num = 12000
keywords = []
message_count = 9.0
messages = ['135119', '135200', '135205', '135275', '135293', '135294', '135309', '135411', '138405']
nosy_count = 5.0
nosy_names = ['pitrou', 'kiilerix', 'sdaoden', 'python-dev', 'nbareil']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue12000'
versions = ['Python 3.2', 'Python 3.3', 'Python 3.4']

Activity

  1. nbareil commented on May 4, 2011

    @nbareil
    nbareilmannequin
    on May 4, 2011
    MannequinAuthor

    When connecting to a SSL server, the certificate verification failed if
    it has a subjectAltName extension without any dNSName entry inside: it
    should fallback to the Common Name.

    Example:

        >>> cert = conn.getpeercert()
    >>> cert
    {'notAfter': 'May 15 14:31:42 2011 GMT',
     'subject': ((('countryName', u'FR'),),
                 (('stateOrProvinceName', u'Ile-de-France'),),
                 (('localityName', u'Paris'),),
                 (('organizationName', 'xxx'),),
                 (('organizationalUnitName', 'xxx'),),
                 (('commonName', 'foobar.corp'),),
                 (('emailAddress', u'test@test.net'),)),
     'subjectAltName': (('email', text@test.net'),)}

    This certificate is valid according to RFC 2818:

    If a subjectAltName extension of type dNSName is present, that MUST
be used as the identity. Otherwise, the (most specific) Common Name
field in the Subject field of the certificate MUST be used. Although
the use of the Common Name is existing practice, it is deprecated and
Certification Authorities are encouraged to use the dNSName instead.

    Even if the use of CommonName is deprecated, we should not break
    existing systems.

    Current revision of Lib/ssl.py :

    108 def match_hostname(cert, hostname):
...
119     san = cert.get('subjectAltName', ())
120     for key, value in san:
121         if key == 'DNS':
122             if _dnsname_to_pat(value).match(hostname):
123                 return
124             dnsnames.append(value)
125     if not san:
126         # The subject is only checked when subjectAltName is empty
127         for sub in cert.get('subject', ()):
128             for key, value in sub:
129                 # XXX according to RFC 2818, the most specific Common Name
130                 # must be used.
131                 if key == 'commonName':
132                     if _dnsname_to_pat(value).match(hostname):
133                         return
134                     dnsnames.append(value)
...

    Proposed patch is:

    diff -r 513f6dfd3173 Lib/ssl.py
--- a/Lib/ssl.py        Sun May 01 20:24:59 2011 -0500
+++ b/Lib/ssl.py        Mon May 02 11:16:46 2011 +0200
@@ -122,8 +122,9 @@
             if _dnsname_to_pat(value).match(hostname):
                 return
             dnsnames.append(value)
- if not san:
- # The subject is only checked when subjectAltName is empty
+ if not san and not dnsnames:
+ # The subject is only checked when there is no dNSName entry
+ # in subjectAltName
  for sub in cert.get('subject', ()):
      for key, value in sub:
          # XXX according to RFC 2818, the most specific Common Name
  2. nbareil
    added
    stdlibStandard Library Python modules in the Lib/ directory
    type-bugAn unexpected behavior, bug, or error
    on May 4, 2011

  3. pitrou commented on May 5, 2011

    @pitrou
    pitrou
    on May 5, 2011
    Member

    Are you sure about "if not san and not dnsnames"? It is even more restrictive than the currently condition. "if not dnsnames" looks like it would fit the bill better.

    Also, better if you can provide a complete patch, including additional test(s) in Lib/test/test_ssl.py.
    (see http://docs.python.org/devguide/runtests.html if you want information about running/writing tests)

  4. sdaoden commented on May 5, 2011

    @sdaoden
    sdaodenmannequin
    on May 5, 2011
    Mannequin

    P.S.: if you're really right ('have those RFC's, but didn't read
    them yet), you could also open an issue for Mercurial at
    http://mercurial.selenic.com/bts - i think those guys do the very
    same.

    Thanks, Steffen!

  5. nbareil commented on May 6, 2011

    @nbareil
    nbareilmannequin
    on May 6, 2011
    MannequinAuthor

    Hello Antoine, Steffen,

    You are absolutely right about removing the 'not san' part. Here is the
    new patch, with tests :

    diff -r c22d5b37f6a4 Lib/ssl.py
--- a/Lib/ssl.py        Fri May 06 09:31:02 2011 +0300
+++ b/Lib/ssl.py        Fri May 06 12:47:14 2011 +0200
@@ -122,8 +122,9 @@
             if _dnsname_to_pat(value).match(hostname):
                 return
             dnsnames.append(value)
-    if not san:
-        # The subject is only checked when subjectAltName is empty
+    if not dnsnames:
+        # The subject is only checked when there is no dNSName entry
+        # in subjectAltName
         for sub in cert.get('subject', ()):
             for key, value in sub:
                 # XXX according to RFC 2818, the most specific Common Name
diff -r c22d5b37f6a4 Lib/test/test_ssl.py
--- a/Lib/test/test_ssl.py      Fri May 06 09:31:02 2011 +0300
+++ b/Lib/test/test_ssl.py      Fri May 06 12:47:14 2011 +0200
@@ -277,6 +277,24 @@
                             (('organizationName', 'Google Inc'),))}
         fail(cert, 'mail.google.com')
 
+        # No DNS entry in subjectAltName but a commonName
+        cert = {'notAfter': 'Dec 18 23:59:59 2099 GMT',
+                'subject': ((('countryName', 'US'),),
+                            (('stateOrProvinceName', 'California'),),
+                            (('localityName', 'Mountain View'),),
+                            (('commonName', 'mail.google.com'),)),
+                'subjectAltName': (('othername', 'blabla'), )}
+        ok(cert, 'mail.google.com')
+
+        # No DNS entry subjectAltName and no commonName
+        cert = {'notAfter': 'Dec 18 23:59:59 2099 GMT',
+                'subject': ((('countryName', 'US'),),
+                            (('stateOrProvinceName', 'California'),),
+                            (('localityName', 'Mountain View'),),
+                            (('organizationName', 'Google Inc'),)),
+                'subjectAltName': (('othername', 'blabla'),)}
+        fail(cert, 'google.com')
+
         # Empty cert / no cert
         self.assertRaises(ValueError, ssl.match_hostname, None, 'example.com')
         self.assertRaises(ValueError, ssl.match_hostname, {}, 'example.com')

    Steffen, I will submit a bug report to Mercurial as soon as this patch is expected to be integrate in py3k.

  6. python-dev commented on May 6, 2011

    @python-dev
    python-devmannequin
    on May 6, 2011
    Mannequin

    New changeset d4c2a99d1bad by Antoine Pitrou in branch '3.2':
    Issue bpo-12000: When a SSL certificate has a subjectAltName without any
    http://hg.python.org/cpython/rev/d4c2a99d1bad

    New changeset 1b37827984ba by Antoine Pitrou in branch 'default':
    Issue bpo-12000: When a SSL certificate has a subjectAltName without any
    http://hg.python.org/cpython/rev/1b37827984ba

  7. pitrou commented on May 6, 2011

    @pitrou
    pitrou
    on May 6, 2011
    Member

    Patch committed in 3.2 and 3.x, thank you!

  8. pitrou
    closed this as completedon May 6, 2011

  9. kiilerix commented on May 6, 2011

    @kiilerix
    kiilerixmannequin
    on May 6, 2011
    Mannequin

    In my opinion the RFCs are a bit unclear about how iPAddress subjectAltNames should be handled. (I also don't know if Python currently do the right thing by accepting and matching IP addresses if specified in commonName.)

    Until now Python failed to the safe side by not matching on subjectAltName iPAddress but also not falling back to commonName if they were specified. AFAICS, with this change it is possible to create strange certificates that Python would accept when an IP address matched commonName but other implementations would reject because of iPAddress mismatch.

    That is probably not a real problem, but I wanted to point it out as the biggest issue I could find with this fix. Nice catch.

    We could perhaps add IP addresses to dnsnames even though we don't match on them.

  10. nbareil commented on May 7, 2011

    @nbareil
    nbareilmannequin
    on May 7, 2011
    MannequinAuthor

    Hello Mads

    Until now Python failed to the safe side by not matching on
    subjectAltName iPAddress but also not falling back to commonName
    if they were specified. AFAICS, with this change it is possible to
    create strange certificates that Python would accept when an IP
    address matched commonName but other implementations would reject
    because of iPAddress mismatch.

    Good point! But I think we already have this issue with a certificate
    like this one:

    cert = { 'subject': ((('commonName', '192.168.1.1'),),)}
ok(cert, '192.168.1.1')

    Do you think this test should fail?

  11. kiilerix commented on Jun 16, 2011

    @kiilerix
    kiilerixmannequin
    on Jun 16, 2011
    Mannequin

    Nicolas Bareil wrote, On 05/07/2011 09:48 AM:

    Do you think this test should fail?

    Until now I have considered this behaviour OK but undocumented and
    officially unsupported in Python. One (the best?) reason for considering
    it OK is that if someone (intentionally or not) trusts a certificate
    that happens to have the textual representation of an IP address in
    commonName then there is no doubt what the intention with that is. This
    case is thus within what i considered secure behaviour.

    But the more I look at it the more convinced I get that this test should
    fail. RFC 2818 mentions subjectAltName iPAddress as a "must" for IP
    addresses - even though it only uses a lower-case and thus
    perhaps-not-necessarily-authoritative "must". But the best argument
    against IP in commonName is that it isn't mentioned anywhere, and when
    it isn't explicitly permitted we should consider it forbidden.

    A consequence of that is that my previous concern is invalid. There is
    no reason the presence of a subjectAltName iPAddress should prevent
    fallback from dNSName to commonName.

  12. ezio-melotti
    transferred this issue fromon Apr 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    stdlibStandard Library Python modules in the Lib/ directorytype-bugAn unexpected behavior, bug, or error

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions