[issue29606] urllib FTP protocol stream injection

STINNER Victor added the comment: Hum, bpo-30119 is really the same bug. So I closed this one as a duplicate of bpo-30119. -- resolution: -> duplicate stage: patch review -> resolved status: open -> closed superseder: -> (ftplib) A remote attacker could possibly attack by containing

[issue29606] urllib FTP protocol stream injection

Martin Panter added the comment: Serhiy, that is what Dong-hee already proposed in , 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. --

[issue29606] urllib FTP protocol stream injection

Serhiy Storchaka added the comment: The right way of fixing the vulnerability is checking a line for '\n' and '\r' in ftplib.FTP.putline(). -- ___ Python tracker

[issue29606] urllib FTP protocol stream injection

ecbftw added the comment: > 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

[issue29606] urllib FTP protocol stream injection

ecbftw added the comment: > 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

[issue29606] urllib FTP protocol stream injection

STINNER Victor added the comment: > 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.

[issue29606] urllib FTP protocol stream injection

Serhiy Storchaka added the comment: 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

[issue29606] urllib FTP protocol stream injection

STINNER Victor added the comment: 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

[issue29606] urllib FTP protocol stream injection

STINNER Victor added the comment: > 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

[issue29606] urllib FTP protocol stream injection

Changes by STINNER Victor : -- pull_requests: +2850 ___ Python tracker ___ ___

[issue29606] urllib FTP protocol stream injection

Dong-hee Na added the comment: Yeah, I agree about your approach. I will update it for this weekend. -- ___ Python tracker ___

[issue29606] urllib FTP protocol stream injection

Serhiy Storchaka added the comment: 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? -- nosy: +serhiy.storchaka ___ Python tracker

[issue29606] urllib FTP protocol stream injection

ecbftw added the comment: 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 character is encountered as part of a pathname it

[issue29606] urllib FTP protocol stream injection

Dong-hee Na added the comment: And if we update FTPHandler will be only affected on Python3. The other patch would be needed for python2. -- ___ Python tracker

[issue29606] urllib FTP protocol stream injection

Dong-hee Na added the comment: 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

[issue29606] urllib FTP protocol stream injection

Martin Panter added the comment: I think changing FTPHandler may be more appropriate than urlsplit. But I thought your other pull request in “ftplib” might be enough. Or are you trying to make it more user-friendly? Also, FTPHandler is not used by

[issue29606] urllib FTP protocol stream injection

Dong-hee Na added the comment: Thanks, Martin I agree with you. So, in this case, we should update FTPHandler right? If this approach right, Can I proceed this issue? -- ___ Python tracker

[issue29606] urllib FTP protocol stream injection

Changes by Giampaolo Rodola' : -- nosy: +giampaolo.rodola ___ Python tracker ___ ___

[issue29606] urllib FTP protocol stream injection

Martin Panter added the comment: 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

[issue29606] urllib FTP protocol stream injection

Dong-hee Na added the comment: So if you want not to add this special case for httplib and just solving this issue for ftplib. We could close this issue. -- ___ Python tracker

[issue29606] urllib FTP protocol stream injection

Dong-hee Na added the comment: Smillar issue but this issue is about FTP protocal using by httplib. Looks simillar but different. -- ___ Python tracker

[issue29606] urllib FTP protocol stream injection

Martin Panter added the comment: 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

[issue29606] urllib FTP protocol stream injection

Dong-hee Na added the comment: I uploaded the PR which check invalid URL such as ftp://foo:bar%0d%0ainjec...@example.net/file.png -- nosy: +corona10 ___ Python tracker

[issue29606] urllib FTP protocol stream injection

Changes by Dong-hee Na : -- pull_requests: +1339 ___ Python tracker ___ ___

[issue29606] urllib FTP protocol stream injection

Changes by Plenty Su : -- nosy: +supl ___ Python tracker ___ ___ Python-bugs-list

[issue29606] urllib FTP protocol stream injection

Changes by STINNER Victor : -- nosy: +christian.heimes, haypo ___ Python tracker ___

[issue29606] urllib FTP protocol stream injection

New submission from 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