classification
Title: urllib FTP protocol stream injection
Type: security Stage: patch review
Components: Versions: Python 3.7, Python 3.6, Python 3.5, Python 3.4, Python 3.3, Python 2.7
process
Status: open Resolution:
Dependencies: Superseder:
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-06-16 13:17 by corona10.

Pull Requests
URL Status Linked Edit
PR 1216 closed corona10, 2017-04-20 18:52
Messages (13)
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.
History
Date User Action Args
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