classification
Title: urllib FTP protocol stream injection
Type: security Stage: resolved
Components: Versions: Python 3.7, Python 3.6, Python 3.5, Python 3.4, Python 3.3, Python 2.7
process
Status: closed Resolution: duplicate
Dependencies: Superseder: (ftplib) A remote attacker could possibly attack by containing the newline characters
View: 30119
Assigned To: Nosy List: christian.heimes, corona10, ecbftw, giampaolo.rodola, haypo, martin.panter, serhiy.storchaka, supl
Priority: normal Keywords:

Created on 2017-02-20 16:49 by ecbftw, last changed 2017-07-26 03:28 by haypo. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 1216 closed corona10, 2017-04-20 18:52
PR 2800 closed haypo, 2017-07-21 10:29
Messages (22)
msg288219 - (view) Author: (ecbftw) Date: 2017-02-20 16:49
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.
msg291998 - (view) Author: Dong-hee Na (corona10) * Date: 2017-04-20 20:32
I uploaded the PR which check invalid URL such as ftp://foo:bar%0d%0aINJECTED@example.net/file.png
msg292413 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2017-04-27 09:35
Isn’t Issue 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 Issue 22928.
msg292417 - (view) Author: Dong-hee Na (corona10) * Date: 2017-04-27 10:33
Smillar issue but this issue is about FTP protocal using by httplib. Looks simillar but different.
msg292419 - (view) Author: Dong-hee Na (corona10) * Date: 2017-04-27 10:51
So if you want not to add this special case for httplib and just solving this issue for ftplib. We could close this issue.
msg292553 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2017-04-29 02:08
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.
msg292555 - (view) Author: Dong-hee Na (corona10) * Date: 2017-04-29 02:46
Thanks, Martin
I agree with you.
So, in this case, we should update FTPHandler right?
If this approach right, Can I proceed this issue?
msg292582 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2017-04-29 09:04
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.
msg292583 - (view) Author: Dong-hee Na (corona10) * Date: 2017-04-29 09:07
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
msg292584 - (view) Author: Dong-hee Na (corona10) * Date: 2017-04-29 09:08
And if we update FTPHandler will be only affected on Python3.
The other patch would be needed for python2.
msg292677 - (view) Author: (ecbftw) Date: 2017-05-01 17:21
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.
msg296125 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-06-15 19:13
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?
msg296192 - (view) Author: Dong-hee Na (corona10) * Date: 2017-06-16 13:17
Yeah, I agree about your approach. I will update it for this weekend.
msg298795 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-07-21 10:39
> 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.
msg298796 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-07-21 10:42
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
msg298802 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-07-21 11:46
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.
msg298803 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-07-21 11:54
> 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.
msg298823 - (view) Author: (ecbftw) Date: 2017-07-21 20:38
> 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.
msg298824 - (view) Author: (ecbftw) Date: 2017-07-21 21:04
> 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.
msg298831 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-07-22 03:35
The right way of fixing the vulnerability is checking a line for '\n' and '\r' in ftplib.FTP.putline().
msg298833 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2017-07-22 04:14
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.
msg299198 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2017-07-26 03:28
Hum, bpo-30119 is really the same bug. So I closed this one as a duplicate of bpo-30119.
History
Date User Action Args
2017-07-26 03:29:01haypounlinkissue30119 superseder
2017-07-26 03:28:33hayposetstatus: open -> closed
superseder: (ftplib) A remote attacker could possibly attack by containing the newline characters
messages: + msg299198

resolution: duplicate
stage: patch review -> resolved
2017-07-22 04:14:20martin.pantersetmessages: + msg298833
2017-07-22 03:35:38serhiy.storchakasetmessages: + msg298831
2017-07-21 21:04:47ecbftwsetmessages: + msg298824
2017-07-21 20:38:23ecbftwsetmessages: + msg298823
2017-07-21 11:54:09hayposetmessages: + msg298803
2017-07-21 11:46:53serhiy.storchakasetmessages: + msg298802
2017-07-21 10:42:55hayposetmessages: + msg298796
2017-07-21 10:39:49hayposetmessages: + msg298795
2017-07-21 10:29:49hayposetpull_requests: + pull_request2850
2017-06-16 13:17:40corona10setmessages: + msg296192
2017-06-15 19:13:40serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg296125
2017-05-01 17:21:01ecbftwsetmessages: + msg292677
2017-04-29 12:00:05martin.panterlinkissue30119 superseder
2017-04-29 09:08:58corona10setmessages: + msg292584
2017-04-29 09:07:38corona10setmessages: + msg292583
2017-04-29 09:04:27martin.pantersetmessages: + msg292582
2017-04-29 02:46:28corona10setmessages: + msg292555
2017-04-29 02:24:50giampaolo.rodolasetnosy: + giampaolo.rodola
2017-04-29 02:08:43martin.pantersetmessages: + msg292553
2017-04-27 10:51:26corona10setmessages: + msg292419
2017-04-27 10:33:51corona10setmessages: + msg292417
2017-04-27 09:35:25martin.pantersetnosy: + martin.panter

messages: + msg292413
stage: patch review
2017-04-20 20:32:08corona10setnosy: + corona10
messages: + msg291998
2017-04-20 18:52:52corona10setpull_requests: + pull_request1339
2017-04-11 10:51:23suplsetnosy: + supl
2017-02-23 14:56:32hayposetnosy: + haypo, christian.heimes
2017-02-20 16:49:02ecbftwcreate