Repository navigation
urllib FTP protocol stream injection #73792
Description
| BPO | 29606 |
|---|---|
| Nosy | @vstinner, @giampaolo, @tiran, @vadmium, @serhiy-storchaka, @supl, @corona10 |
| PRs | |
| Superseder |
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']
Activity
ecbftw commentedon Feb 20, 2017
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 commentedon Apr 20, 2017
I uploaded the PR which check invalid URL such as ftp://foo:bar%0d%0aINJECTED@example.net/file.png
vadmium commentedon Apr 27, 2017
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 commentedon Apr 27, 2017
Smillar issue but this issue is about FTP protocal using by httplib. Looks simillar but different.
corona10 commentedon Apr 27, 2017
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 commentedon Apr 29, 2017
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 commentedon Apr 29, 2017
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 commentedon Apr 29, 2017
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 commentedon Apr 29, 2017
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 commentedon Apr 29, 2017
And if we update FTPHandler will be only affected on Python3.
The other patch would be needed for python2.
ecbftw commentedon May 1, 2017
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 commentedon Jun 15, 2017
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 commentedon Jun 16, 2017
Yeah, I agree about your approach. I will update it for this weekend.
vstinner commentedon Jul 21, 2017
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 commentedon Jul 21, 2017
Since corona10 abandonned his #1216 I created a new PR:
#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 commentedon Jul 21, 2017
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 commentedon Jul 21, 2017
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.
ecbftw commentedon Jul 21, 2017
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.
ecbftw commentedon Jul 21, 2017
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 commentedon Jul 22, 2017
The right way of fixing the vulnerability is checking a line for '\n' and '\r' in ftplib.FTP.putline().
vadmium commentedon Jul 22, 2017
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 commentedon Jul 26, 2017
Hum, bpo-30119 is really the same bug. So I closed this one as a duplicate of bpo-30119.