| From 4134f154ae2f621f25c5d698cc0f1748035a1b88 Mon Sep 17 00:00:00 2001 |
| From: "Miss Islington (bot)" |
| <31488909+miss-islington@users.noreply.github.com> |
| Date: Tue, 16 Mar 2021 14:08:30 -0700 |
| Subject: [PATCH] [3.6] bpo-43285 Make ftplib not trust the PASV response. |
| (GH-24838) (GH-24881) (GH-24882) |
| |
| 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.6.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.6.rst b/Doc/whatsnew/3.6.rst |
| index 296935adad..561fb67d66 100644 |
| --- a/Doc/whatsnew/3.6.rst |
| +++ b/Doc/whatsnew/3.6.rst |
| @@ -2472,3 +2472,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.6.14 |
| +================================ |
| + |
| +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 2ff251a0f7..385e4324c6 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 @@ class FTP: |
| 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 4ff2f71afb..3ca7cc1c30 100644 |
| --- a/Lib/test/test_ftplib.py |
| +++ b/Lib/test/test_ftplib.py |
| @@ -94,6 +94,10 @@ class DummyFTPHandler(asynchat.async_chat): |
| 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 @@ class DummyFTPHandler(asynchat.async_chat): |
| 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() |
| @@ -694,6 +699,26 @@ class TestFTPClass(TestCase): |
| # 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 0000000000..8312b7e885 |
| --- /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. |
| -- |
| 2.41.0.255.g8b1d071c50-goog |
| |
