urllib FTP protocol stream injection

[c70d56b8-594c-4cf9-b38d-5c69697ee698]

BPO	29606
Nosy	@vstinner, @giampaolo, @tiran, @vadmium, @serhiy-storchaka, @supl, @corona10
PRs	python/cpython#1216 python/cpython#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: ```python assignee = None closed_at = created_at = labels = ['type-security', '3.7'] title = 'urllib FTP protocol stream injection' updated_at = user = 'https://bugs.python.org/ecbftw' ``` bugs.python.org fields: ```python activity = actor = 'vstinner' assignee = 'none' closed = True closed_date = closer = 'vstinner' components = [] creation = 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'] ```

[c70d56b8-594c-4cf9-b38d-5c69697ee698]

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 https://github.com/python/cpython/pull/1214

[corona10]

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

[c70d56b8-594c-4cf9-b38d-5c69697ee698]

It was just pointed out by @giampaolo in (https://github.com/python/cpython/pull/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.

[vstinner]

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?

I'm not sure that it's possible, ftplib gets the wrong hostname parameter. The best place to reject invalid characters is where the URL is parsed, no? See also my bpo-30713.

[vstinner]

Since corona10 abandonned his https://github.com/python/cpython/pull/1216 I created a new PR: https://github.com/python/cpython/pull/2800

I chose to only reject newline (\n): "\r" and "\0" are not rejected.

My PR rejects any URL containing "\n", even if the newline is part of the "path" part of the URL. While I expect that filenames containing newlines are very rare, my PR is an incompatible change which breaks such use case :-(

I don't know where is the balanace between security and backward compatibility... I started a thread on python-dev: https://mail.python.org/pipermail/python-dev/2017-July/148699.html

[serhiy-storchaka]

What is wrong with an URL containing '\n'? I suppose that when format a request with a text protocol, embedded '\n' can split the request line on two lines and inject a new command. The most robust way would be to check whether the formatted line contains '\n', '\r', '\0' or other illegal characters.

[vstinner]

What is wrong with an URL containing '\n'?

For the attack, see http://blog.blindspotsecurity.com/2017/02/advisory-javapython-ftp-injections.html

Honestly, I don't understand well the bug :) But it doesn't seem correct to me to have a newline in a hostname.

[c70d56b8-594c-4cf9-b38d-5c69697ee698]

The best place to reject invalid characters is where the URL is parsed, no? See also my bpo-30713.

No I don't really agree with that. What other APIs can be used to submit a directory name, user name, password, or other field in an FTP command? If you block unacceptable characters only at URL parsing, then you fail to address those other vectors.

The normal way to fix any injection vulneability is to encode the dangerous characters just before then are included in the surrounding syntax. Unfortunately in FTP's case, there's no widely-accepted way to encode these characters. So I think you should instead throw an exception right before the commands are put on the control channel. Fixing the bug at the "sink" like this is a far more resilient way to address injections.

Any "legitimate" use case that users might have for these characters wouldn't have worked anyway. The code is already broken for new lines in file names. If you change the code such that it throws an exception when they are written to the control channel, that's a better mode of failure than what you have right now.

[c70d56b8-594c-4cf9-b38d-5c69697ee698]

What is wrong with an URL containing '\n'? I suppose that when format a request with a text protocol, embedded '\n' can split the request line on two lines and inject a new command. The most robust way would be to check whether the formatted line contains '\n', '\r', '\0' or other illegal characters.

I agree, there's nothing wrong with an encoded line feed (%0a) in a URL. HTTP can easily handle '\n' in a basic auth password field, for instance. The problem is when these characters are included in a context where they are interpreted as a delimiter of some kind. In FTP's case, they are being interpreted as the delimiter between commands.

[serhiy-storchaka]

The right way of fixing the vulnerability is checking a line for '\n' and '\r' in ftplib.FTP.putline().

[vadmium]

Serhiy, that is what Dong-hee already proposed in \https://github.com/python/cpython/pull/1214\, but someone needs to decide if we want to support RFC 2640, in which I understand LF on its own is legal, and CR is escaped by adding a NUL.

[vstinner]

Hum, bpo-30119 is really the same bug. So I closed this one as a duplicate of bpo-30119.