ftplib should not use the host from the PASV response

[ricexdream]

BPO	43285
Nosy	@gpshead, @giampaolo, @ned-deily, @miss-islington
PRs	bpo-43285 Make ftplib not trust the PASV response. #24838 [3.9] bpo-43285 Make ftplib not trust the PASV response. (GH-24838) #24880 [3.8] bpo-43285 Make ftplib not trust the PASV response. (GH-24838) #24881 [3.6] bpo-43285 Make ftplib not trust the PASV response. (GH-24838) (GH-24881) #24882 [3.7] bpo-43285 Make ftplib not trust the PASV response. (GH-24838) (GH-24881) #24883 bpo-43285: Whats New entry for 3.8.9. #24886 bpo-43285: Add a What's New entry for 3.9.3. #24887 [3.9] bpo-43285: Add a What's New entry for 3.9.3. #24888 [3.8] bpo-43285: Whats New entry for 3.8.9. #24889

^{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 2021-03-16.21:54:25.203>
created_at = <Date 2021-02-21.11:49:34.689>
labels = ['type-security', '3.7', 'library', 'release-blocker']
title = 'ftplib should not use the host from the PASV response'
updated_at = <Date 2021-03-16.21:54:25.202>
user = 'https://bugs.python.org/ricexdream'

bugs.python.org fields:

activity = <Date 2021-03-16.21:54:25.202>
actor = 'ned.deily'
assignee = 'none'
closed = True
closed_date = <Date 2021-03-16.21:54:25.203>
closer = 'ned.deily'
components = ['Library (Lib)']
creation = <Date 2021-02-21.11:49:34.689>
creator = 'ricexdream'
dependencies = []
files = []
hgrepos = []
issue_num = 43285
keywords = ['patch']
message_count = 16.0
messages = ['387455', '388267', '388602', '388610', '388757', '388761', '388762', '388763', '388768', '388777', '388812', '388813', '388815', '388882', '388885', '388891']
nosy_count = 5.0
nosy_names = ['gregory.p.smith', 'giampaolo.rodola', 'ned.deily', 'miss-islington', 'ricexdream']
pr_nums = ['24838', '24880', '24881', '24882', '24883', '24886', '24887', '24888', '24889']
priority = 'release blocker'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'security'
url = 'https://bugs.python.org/issue43285'
versions = ['Python 3.6', 'Python 3.7']

[ricexdream]

Last year, curl had a security update for CVE-2020-8284. more info, see https://hackerone.com/reports/1040166

The problem is ftp client trust the host from PASV response by default, A malicious server can trick ftp client into connecting
back to a given IP address and port. This may make ftp client scan ports and extract service banner from private newwork.

After test and read ftplib module(

cpython/Lib/ftplib.py

Line 346 in 6329893

host, port = self.makepasv()

), I found ftplib has the same problem.

[ricexdream]

Any response here? If you need more information let me know.

[gpshead]

Indeed, the host on that line there should just be ignored with the IP address of the original data connection used in its place.

Your https://hackerone.com/reports/1040166 link provides plenty of information and likes to prior art mitigations other ftp clients including Firefox and Chrome well over a decade ago.

[gpshead]

I'm not interested in chasing down a CVE for this myself. If anyone wants to jump through the hoops to obtain one, the text used for curl in the hackerone link is likely a good guide.

My PR includes a way for people to opt-out of the secure behavior (why would anyone ever want that?) by setting the use_untrusted_server_pasv_ipv4_addr attribute to True on their ftplib.FTP instance. Setting that attribute on a server lacking this fix is a no-op, making it safe to add to code running on any version.

This is an embarrassingly old widespread common issue in a large number of ftp clients. Even the 1998 IPv6 RFC https://tools.ietf.org/html/rfc2428 indirectly acknowledges its existence by disallowing the new EPSV command that replaces PASV from returning anything other than the port number while leaving fields for the other values present but empty...

[gpshead]

New changeset 0ab152c by Gregory P. Smith in branch 'master':
bpo-43285 Make ftplib not trust the PASV response. (GH-24838)
0ab152c

[miss-islington]

New changeset 7dcb4ba by Miss Islington (bot) in branch '3.9':
bpo-43285 Make ftplib not trust the PASV response. (GH-24838)
7dcb4ba

[gpshead]

New changeset 664d1d1 by Gregory P. Smith in branch '3.8':
[3.8] bpo-43285 Make ftplib not trust the PASV response. (GH-24838) (GH-24881)
664d1d1

[gpshead]

3.7 and 3.6 backport PRs created and assigned to release manager Ned for merging.

[ned-deily]

@gps, What about ftplib doc changes and What's new entries for this change in behavior?

[gpshead]

A What's New entry is a good idea. I'll make one and add it to those backport PRs. (reopened to remind me of that)

ftplib docs... I don't actually want to document the attribute that people can set for the old behavior beyond the notes in NEWS or What's New. It is something I anticipate nobody in the world ever actually setting so I'd rather not imply that anyone even should by giving it more prominent doc space.

Other things that have fixed this repeated bug in their program that supports ftp over the years have not added an opt-out as far as I could tell in my quick searching.

[gpshead]

New changeset d0312ce by Gregory P. Smith in branch '3.9':
[3.9] bpo-43285: Add a What's New entry for 3.9.3. (GH-24888)
d0312ce

[gpshead]

New changeset 9eda0df by Gregory P. Smith in branch '3.8':
[3.8] bpo-43285: Whats New entry for 3.8.9. (GH-24889)
9eda0df

[gpshead]

3.7 and 3.6 PRs updated to include a What's New entry.

[ned-deily]

New changeset 4134f15 by Miss Islington (bot) in branch '3.6':
[3.6] bpo-43285 Make ftplib not trust the PASV response. (GH-24838) (GH-24881) (GH-24882)
4134f15

[ned-deily]

New changeset 7937395 by Miss Islington (bot) in branch '3.7':
[3.7] bpo-43285 Make ftplib not trust the PASV response. (GH-24838) (GH-24881) (GH-24883)
7937395

[ned-deily]

Thanks for the PRs and the What's New entries.