Skip to content

bpo-37463: match_hostname requires quad-dotted IPv4 #14499

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jul 2, 2019
Merged

bpo-37463: match_hostname requires quad-dotted IPv4 #14499

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 20 additions & 9 deletions Lib/ssl.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,12 +327,22 @@ def _inet_paton(ipname):
Supports IPv4 addresses on all platforms and IPv6 on platforms with IPv6
support.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_ipaddress_match() is documented as "Exact matching of IP addresses." but is explicitly strips trailing spaces. Maybe just add a sentence to explain that in its docstring?

_inet_paton() disallow leading and trailing whitespaces: maybe explain that in its docstring?

_inet_paton() allows short form of IPv6 addresses, but not for IPv4 address. Is it a deliberate choice? Again, maybe document it in the docstring.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it is useful to add more doc strings to internal helper methods for a deprecated feature. ssl.match_hostname is deprecated and I plan to drop it in 3.9 or 3.10.

The strip in _ipaddress_match() is an implementation detail that works around a quirk in OpenSSL. The IP address may sometimes contain a newline. IIRC IPv6 addresses only.

Yes, that is deliberate. Short notation of IPv4 addresses is an uncommon and rarely used feature. However short notation of IPv6 is pretty much the default. The behavior also reflects the behavior of the old implementation with ipaddress module.

"""
# inet_aton() also accepts strings like '1'
if ipname.count('.') == 3:
try:
return _socket.inet_aton(ipname)
except OSError:
pass
# inet_aton() also accepts strings like '1', '127.1', some also trailing
# data like '127.0.0.1 whatever'.
try:
addr = _socket.inet_aton(ipname)
except OSError:
# not an IPv4 address
pass
else:
if _socket.inet_ntoa(addr) == ipname:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason not to ipname.rstrip() here for the comparison? Does whitespace matter?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just notice that you're explicitly testing trailing whitespace below.

Copy link
Member Author

@tiran tiran Jul 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rstrip() would not deal with invalid input like 127.0.0.1 whatever, 127.0.0.1\tinvalid, or other embedded white spaces.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just notice that you're explicitly testing trailing whitespace below.

The rstrip call in _ipaddress_match is necessary because OpenSSL includes a trailing new line for IPv6 addresses. The code removes trailing white space in SAN IP Address items of decoded certs, not of the user supplied hostname argument.

# only accept injective ipnames
return addr
else:
# refuse for short IPv4 notation and additional trailing data
raise ValueError(
"{!r} is not a quad-dotted IPv4 address.".format(ipname)
Copy link
Member

@zooba zooba Jul 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, on Windows if you pass an invalid address you get OSError: illegal IP address string passed to inet_aton. Not sure if that's a Python error

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some implementations of inet_aton are strict and don't allow trailing data. glibc's inet_aton is more lean and ignores trailing data if the first character is some sort of white space.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: usually, Python error messages don't end with a dot. (Same comment for the 3 error messages.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm following the style of the surrounding code. The exception messages are full sentences and end with a full stop.

)

try:
return _socket.inet_pton(_socket.AF_INET6, ipname)
Expand All @@ -346,14 +356,15 @@ def _inet_paton(ipname):
raise ValueError("{!r} is not an IPv4 address.".format(ipname))


def _ipaddress_match(ipname, host_ip):
def _ipaddress_match(cert_ipaddress, host_ip):
"""Exact matching of IP addresses.

RFC 6125 explicitly doesn't define an algorithm for this
(section 1.7.2 - "Out of Scope").
"""
# OpenSSL may add a trailing newline to a subjectAltName's IP address
ip = _inet_paton(ipname.rstrip())
# OpenSSL may add a trailing newline to a subjectAltName's IP address,
# commonly woth IPv6 addresses. Strip off trailing \n.
ip = _inet_paton(cert_ipaddress.rstrip())
return ip == host_ip


Expand Down
9 changes: 8 additions & 1 deletion Lib/test/test_ssl.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -669,9 +669,14 @@ def fail(cert, hostname):
cert = {'subject': ((('commonName', 'example.com'),),),
'subjectAltName': (('DNS', 'example.com'),
('IP Address', '10.11.12.13'),
('IP Address', '14.15.16.17'))}
('IP Address', '14.15.16.17'),
('IP Address', '127.0.0.1'))}
ok(cert, '10.11.12.13')
ok(cert, '14.15.16.17')
# socket.inet_ntoa(socket.inet_aton('127.1')) == '127.0.0.1'
fail(cert, '127.1')
fail(cert, '14.15.16.17 ')
fail(cert, '14.15.16.17 extra data')
fail(cert, '14.15.16.18')
fail(cert, 'example.net')

Expand All @@ -684,6 +689,8 @@ def fail(cert, hostname):
('IP Address', '2003:0:0:0:0:0:0:BABA\n'))}
ok(cert, '2001::cafe')
ok(cert, '2003::baba')
fail(cert, '2003::baba ')
fail(cert, '2003::baba extra data')
fail(cert, '2003::bebe')
fail(cert, 'example.net')

Expand Down
4 changes: 4 additions & 0 deletions Misc/NEWS.d/next/Security/2019-07-01-08-46-14.bpo-37463.1CHwjE.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
ssl.match_hostname() no longer accepts IPv4 addresses with additional text
after the address and only quad-dotted notation without trailing
whitespaces. Some inet_aton() implementations ignore whitespace and all data
after whitespace, e.g. '127.0.0.1 whatever'.