Skip to content
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.

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

Jump to bottom

ftplib should not use the host from the PASV response #87451

Closed
ricexdream mannequin opened this issue Feb 21, 2021 · 16 comments
Closed

ftplib should not use the host from the PASV response #87451

ricexdream mannequin opened this issue Feb 21, 2021 · 16 comments
Labels
3.7 (EOL) end of life release-blocker stdlib Python modules in the Lib dir type-security A security issue

Comments

@ricexdream
Copy link
Mannequin

ricexdream mannequin commented Feb 21, 2021

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
    Copy link
    Mannequin Author

    ricexdream mannequin commented Feb 21, 2021

    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 ricexdream mannequin added 3.9 only security fixes stdlib Python modules in the Lib dir type-security A security issue labels Feb 21, 2021
    @ricexdream
    Copy link
    Mannequin Author

    ricexdream mannequin commented Mar 8, 2021

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

    @gpshead
    Copy link
    Member

    gpshead commented Mar 13, 2021

    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 gpshead added 3.7 (EOL) end of life 3.8 only security fixes 3.10 only security fixes labels Mar 13, 2021
    @gpshead gpshead self-assigned this Mar 13, 2021
    @gpshead gpshead added 3.7 (EOL) end of life 3.8 only security fixes 3.10 only security fixes labels Mar 13, 2021
    @gpshead gpshead self-assigned this Mar 13, 2021
    @gpshead
    Copy link
    Member

    gpshead commented Mar 13, 2021

    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 gpshead changed the title ftplib use host from PASV response ftplib should not use the host from the PASV response Mar 13, 2021
    @gpshead gpshead changed the title ftplib use host from PASV response ftplib should not use the host from the PASV response Mar 13, 2021
    @gpshead
    Copy link
    Member

    gpshead commented Mar 15, 2021

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

    @miss-islington
    Copy link
    Contributor

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

    @gpshead
    Copy link
    Member

    gpshead commented Mar 15, 2021

    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
    Copy link
    Member

    gpshead commented Mar 15, 2021

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

    @gpshead gpshead closed this as completed Mar 15, 2021
    @gpshead gpshead closed this as completed Mar 15, 2021
    @ned-deily
    Copy link
    Member

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

    @gpshead
    Copy link
    Member

    gpshead commented Mar 15, 2021

    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 gpshead reopened this Mar 15, 2021
    @gpshead gpshead reopened this Mar 15, 2021
    @gpshead
    Copy link
    Member

    gpshead commented Mar 16, 2021

    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
    Copy link
    Member

    gpshead commented Mar 16, 2021

    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
    Copy link
    Member

    gpshead commented Mar 16, 2021

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

    @gpshead gpshead removed 3.8 only security fixes 3.9 only security fixes 3.10 only security fixes labels Mar 16, 2021
    @gpshead gpshead assigned ned-deily and unassigned gpshead Mar 16, 2021
    @gpshead gpshead added release-blocker and removed 3.8 only security fixes 3.9 only security fixes 3.10 only security fixes labels Mar 16, 2021
    @gpshead gpshead assigned ned-deily and unassigned gpshead Mar 16, 2021
    @ned-deily
    Copy link
    Member

    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
    Copy link
    Member

    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
    Copy link
    Member

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

    @ned-deily ned-deily removed their assignment Mar 16, 2021
    @ned-deily ned-deily removed their assignment Mar 16, 2021
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees
    No one assigned
    Labels
    3.7 (EOL) end of life release-blocker stdlib Python modules in the Lib dir type-security A security issue
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    3 participants
    @gpshead @ned-deily @miss-islington