urllib FTP protocol stream injection

[ecbftw]

BPO	29606
Nosy	@vstinner, @giampaolo, @tiran, @vadmium, @serhiy-storchaka, @supl, @corona10
PRs	bpo-29606: urllib throwing an exception on any URLs that contain one of '\r\n' for the FTP protocol. #1216 bpo-29606: urllib rejects newline in FTP #2800
Superseder	bpo-30119: (ftplib) A remote attacker could possibly attack by containing the newline characters

^{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 2017-07-26.03:28:33.765>
created_at = <Date 2017-02-20.16:49:02.397>
labels = ['type-security', '3.7']
title = 'urllib FTP protocol stream injection'
updated_at = <Date 2017-07-26.03:28:33.762>
user = 'https://bugs.python.org/ecbftw'

bugs.python.org fields:

activity = <Date 2017-07-26.03:28:33.762>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2017-07-26.03:28:33.765>
closer = 'vstinner'
components = []
creation = <Date 2017-02-20.16:49:02.397>
creator = 'ecbftw'
dependencies = []
files = []
hgrepos = []
issue_num = 29606
keywords = []
message_count = 22.0
messages = ['288219', '291998', '292413', '292417', '292419', '292553', '292555', '292582', '292583', '292584', '292677', '296125', '296192', '298795', '298796', '298802', '298803', '298823', '298824', '298831', '298833', '299198']
nosy_count = 8.0
nosy_names = ['vstinner', 'giampaolo.rodola', 'christian.heimes', 'martin.panter', 'serhiy.storchaka', 'ecbftw', 'supl', 'corona10']
pr_nums = ['1216', '2800']
priority = 'normal'
resolution = 'duplicate'
stage = 'resolved'
status = 'closed'
superseder = '30119'
type = 'security'
url = 'https://bugs.python.org/issue29606'
versions = ['Python 2.7', 'Python 3.3', 'Python 3.4', 'Python 3.5', 'Python 3.6', 'Python 3.7']

[ecbftw]

Please see: http://blog.blindspotsecurity.com/2017/02/advisory-javapython-ftp-injections.html

This was reported to security at python dot org, but as far as I can tell, they sat on it for a year.

I don't think there is a proper way to encode newlines in CWD commands, according the FTP RFC. If that is the case, then I suggest throwing an exception on any URLs that contain one of '\r\n\0' or any other characters that the FTP protocol simply can't support.

[corona10]

I uploaded the PR which check invalid URL such as ftp://foo:bar%0d%0aINJECTED@example.net/file.png

[vadmium]

Isn’t bpo-30119 a duplicate of this? In that bug Dong-hee you posted a pull request that changes the “ftplib” module, which makes more sense to me than adding a special case to “urlsplit” that understands FTP. See how this was addressed for HTTP in bpo-22928.

[corona10]

Smillar issue but this issue is about FTP protocal using by httplib. Looks simillar but different.

[corona10]

So if you want not to add this special case for httplib and just solving this issue for ftplib. We could close this issue.

[vadmium]

I understand this bug (as reported by ECBFTW) is about Python injecting unexpected FTP commands when the “urllib” and “urllib2” modules are used. The “httplib” module (“http.client” in Python 3) is unaffected. I only mentioned HTTP as an example of a similar fix made recently; sorry if that was confusing.

To be clear, in Python 2 I think both the “urllib” _and_ “urllib2” modules are affected, as well as “ftplib” directly. In Python 3, “urllib.request” and “ftplib” are affected. But I don’t think “urlparse” and “urllib.parse” should be changed.

[corona10]

Thanks, Martin
I agree with you.
So, in this case, we should update FTPHandler right?
If this approach right, Can I proceed this issue?

[vadmium]

I think changing FTPHandler may be more appropriate than urlsplit. But I thought your other pull request <https://github.com/python/cpython/pull/1214\> in “ftplib” might be enough. Or are you trying to make it more user-friendly?

Also, FTPHandler is not used by URLopener and the Python 2 “urllib” module as far as I know, so on its own just fixing FTPHandler wouldn’t be enough.

[corona10]

Okay, this part is going to require discussion between core-devs. The ftplib committer says we need to modify urllib, and you seem to think that ftplib's fix is ​correct. The conclusion is needed.

Discuss about ftplib is here #1214

[corona10]

And if we update FTPHandler will be only affected on Python3.
The other patch would be needed for python2.

[ecbftw]

It was just pointed out by @giampaolo in (#1214) that an escaping mechanism does actually exist for FTP, as defined in RFC-2640. The relevant passage is as follows:

When a <CR> character is encountered as part of a pathname it MUST be
padded with a <NUL> character prior to sending the command. On
receipt of a pathname containing a <CR><NUL> sequence the <NUL>
character MUST be stripped away. This approach is described in the
Telnet protocol [RFC854] on pages 11 and 12. For example, to store a
pathname foo<CR><LF>boo.bar the pathname would become
foo<CR><NUL><LF>boo.bar prior to sending the command STOR
<SP>foo<CR><NUL><LF>boo.bar<CRLF>. Upon receipt of the altered
pathname the <NUL> character following the <CR> would be stripped
away to form the original pathname.

It isn't clear how good FTP server support for this is, or if firewalls recognize this escaping as well. In the case of firewalls, one could argue that if they don't account for it, the vulnerability lies in them.

[serhiy-storchaka]

Wouldn't be better to solve this issue on the level of the ftplib module or FTP handler in urllib.request instead of urllib.parse?

[corona10]

Yeah, I agree about your approach. I will update it for this weekend.