142 lines
6.5 KiB
Diff
142 lines
6.5 KiB
Diff
From 79373951b3eab585d42e0f0ab83718cbe1d0ee33 Mon Sep 17 00:00:00 2001
|
|
From: "Miss Islington (bot)"
|
|
<31488909+miss-islington@users.noreply.github.com>
|
|
Date: Tue, 16 Mar 2021 14:19:55 -0700
|
|
Subject: [PATCH] [3.7] bpo-43285 Make ftplib not trust the PASV response.
|
|
(GH-24838) (GH-24881) (GH-24883)
|
|
|
|
The IPv4 address value returned from the server in response to the PASV command
|
|
should not be trusted. This prevents a malicious FTP server from using the
|
|
response to probe IPv4 address and port combinations on the client network.
|
|
|
|
Instead of using the returned address, we use the IP address we're
|
|
already connected to. This is the strategy other ftp clients adopted,
|
|
and matches the only strategy available for the modern IPv6 EPSV command
|
|
where the server response must return a port number and nothing else.
|
|
|
|
For the rare user who _wants_ this ugly behavior, set a `trust_server_pasv_ipv4_address`
|
|
attribute on your `ftplib.FTP` instance to True..
|
|
(cherry picked from commit 0ab152c6b5d95caa2dc1a30fa96e10258b5f188e)
|
|
|
|
Co-authored-by: Gregory P. Smith <greg@krypto.org>
|
|
(cherry picked from commit 664d1d16274b47eea6ec92572e1ebf3939a6fa0c)
|
|
---
|
|
Doc/whatsnew/3.7.rst | 9 +++++++
|
|
Lib/ftplib.py | 9 ++++++-
|
|
Lib/test/test_ftplib.py | 27 ++++++++++++++++++-
|
|
.../2021-03-13-03-48-14.bpo-43285.g-Hah3.rst | 8 ++++++
|
|
4 files changed, 51 insertions(+), 2 deletions(-)
|
|
create mode 100644 Misc/NEWS.d/next/Security/2021-03-13-03-48-14.bpo-43285.g-Hah3.rst
|
|
|
|
diff --git a/Doc/whatsnew/3.7.rst b/Doc/whatsnew/3.7.rst
|
|
index 25e231dcc7dfa..85f924b2e41f3 100644
|
|
--- a/Doc/whatsnew/3.7.rst
|
|
+++ b/Doc/whatsnew/3.7.rst
|
|
@@ -2585,3 +2585,12 @@ separator key, with ``&`` as the default. This change also affects
|
|
functions internally. For more details, please see their respective
|
|
documentation.
|
|
(Contributed by Adam Goldschmidt, Senthil Kumaran and Ken Jin in :issue:`42967`.)
|
|
+
|
|
+Notable changes in Python 3.7.11
|
|
+================================
|
|
+
|
|
+A security fix alters the :class:`ftplib.FTP` behavior to not trust the
|
|
+IPv4 address sent from the remote server when setting up a passive data
|
|
+channel. We reuse the ftp server IP address instead. For unusual code
|
|
+requiring the old behavior, set a ``trust_server_pasv_ipv4_address``
|
|
+attribute on your FTP instance to ``True``. (See :issue:`43285`)
|
|
diff --git a/Lib/ftplib.py b/Lib/ftplib.py
|
|
index 9611282ecacb2..83ed0aa9c140e 100644
|
|
--- a/Lib/ftplib.py
|
|
+++ b/Lib/ftplib.py
|
|
@@ -104,6 +104,8 @@ class FTP:
|
|
welcome = None
|
|
passiveserver = 1
|
|
encoding = "latin-1"
|
|
+ # Disables https://bugs.python.org/issue43285 security if set to True.
|
|
+ trust_server_pasv_ipv4_address = False
|
|
|
|
# Initialization method (called by class instantiation).
|
|
# Initialize host to localhost, port to standard ftp port
|
|
@@ -333,8 +335,13 @@ def makeport(self):
|
|
return sock
|
|
|
|
def makepasv(self):
|
|
+ """Internal: Does the PASV or EPSV handshake -> (address, port)"""
|
|
if self.af == socket.AF_INET:
|
|
- host, port = parse227(self.sendcmd('PASV'))
|
|
+ untrusted_host, port = parse227(self.sendcmd('PASV'))
|
|
+ if self.trust_server_pasv_ipv4_address:
|
|
+ host = untrusted_host
|
|
+ else:
|
|
+ host = self.sock.getpeername()[0]
|
|
else:
|
|
host, port = parse229(self.sendcmd('EPSV'), self.sock.getpeername())
|
|
return host, port
|
|
diff --git a/Lib/test/test_ftplib.py b/Lib/test/test_ftplib.py
|
|
index da8ba32917be7..73a6250a5e4ae 100644
|
|
--- a/Lib/test/test_ftplib.py
|
|
+++ b/Lib/test/test_ftplib.py
|
|
@@ -94,6 +94,10 @@ def __init__(self, conn):
|
|
self.rest = None
|
|
self.next_retr_data = RETR_DATA
|
|
self.push('220 welcome')
|
|
+ # We use this as the string IPv4 address to direct the client
|
|
+ # to in response to a PASV command. To test security behavior.
|
|
+ # https://bugs.python.org/issue43285/.
|
|
+ self.fake_pasv_server_ip = '252.253.254.255'
|
|
|
|
def collect_incoming_data(self, data):
|
|
self.in_buffer.append(data)
|
|
@@ -136,7 +140,8 @@ def cmd_pasv(self, arg):
|
|
sock.bind((self.socket.getsockname()[0], 0))
|
|
sock.listen()
|
|
sock.settimeout(TIMEOUT)
|
|
- ip, port = sock.getsockname()[:2]
|
|
+ port = sock.getsockname()[1]
|
|
+ ip = self.fake_pasv_server_ip
|
|
ip = ip.replace('.', ','); p1 = port / 256; p2 = port % 256
|
|
self.push('227 entering passive mode (%s,%d,%d)' %(ip, p1, p2))
|
|
conn, addr = sock.accept()
|
|
@@ -698,6 +703,26 @@ def test_makepasv(self):
|
|
# IPv4 is in use, just make sure send_epsv has not been used
|
|
self.assertEqual(self.server.handler_instance.last_received_cmd, 'pasv')
|
|
|
|
+ def test_makepasv_issue43285_security_disabled(self):
|
|
+ """Test the opt-in to the old vulnerable behavior."""
|
|
+ self.client.trust_server_pasv_ipv4_address = True
|
|
+ bad_host, port = self.client.makepasv()
|
|
+ self.assertEqual(
|
|
+ bad_host, self.server.handler_instance.fake_pasv_server_ip)
|
|
+ # Opening and closing a connection keeps the dummy server happy
|
|
+ # instead of timing out on accept.
|
|
+ socket.create_connection((self.client.sock.getpeername()[0], port),
|
|
+ timeout=TIMEOUT).close()
|
|
+
|
|
+ def test_makepasv_issue43285_security_enabled_default(self):
|
|
+ self.assertFalse(self.client.trust_server_pasv_ipv4_address)
|
|
+ trusted_host, port = self.client.makepasv()
|
|
+ self.assertNotEqual(
|
|
+ trusted_host, self.server.handler_instance.fake_pasv_server_ip)
|
|
+ # Opening and closing a connection keeps the dummy server happy
|
|
+ # instead of timing out on accept.
|
|
+ socket.create_connection((trusted_host, port), timeout=TIMEOUT).close()
|
|
+
|
|
def test_with_statement(self):
|
|
self.client.quit()
|
|
|
|
diff --git a/Misc/NEWS.d/next/Security/2021-03-13-03-48-14.bpo-43285.g-Hah3.rst b/Misc/NEWS.d/next/Security/2021-03-13-03-48-14.bpo-43285.g-Hah3.rst
|
|
new file mode 100644
|
|
index 0000000000000..8312b7e885441
|
|
--- /dev/null
|
|
+++ b/Misc/NEWS.d/next/Security/2021-03-13-03-48-14.bpo-43285.g-Hah3.rst
|
|
@@ -0,0 +1,8 @@
|
|
+:mod:`ftplib` no longer trusts the IP address value returned from the server
|
|
+in response to the PASV command by default. This prevents a malicious FTP
|
|
+server from using the response to probe IPv4 address and port combinations
|
|
+on the client network.
|
|
+
|
|
+Code that requires the former vulnerable behavior may set a
|
|
+``trust_server_pasv_ipv4_address`` attribute on their
|
|
+:class:`ftplib.FTP` instances to ``True`` to re-enable it.
|