[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 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. -- ___ Python tracker <http://bugs.python.org/issue29606> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[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 '\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. -- ___ Python tracker <http://bugs.python.org/issue29606> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[issue30713] Reject newline character (U+000A) in URLs in urllib.parse
ecbftw added the comment: I'm the guy that did the original security research on this issue. I've been a pentester for over 12 years, where I am regularly helping developers understand how to best correct their injection flaws. Please carefully consider what I'm trying to tell you. I've been trying to get this message through to Python for nearly 2 years now. =( Serhiy has the right idea. His email here shows a firm understanding of the issue and the correct remediation approach: https://mail.python.org/pipermail/python-dev/2017-July/148715.html It seems every developer initially assumes input validation is the way you deal with injection flaws. But this creates a false sense of a "trade off" between security and usability. Rejecting evil-looking things up front just creates more problems for you. Also, you can't be sure every code path leading from user input to the injection point is addressed. (What other ways can new lines reach the FTP library?) Instead, the problem needs to be dealt with in the lines of code just before the injection occurs. Ideally, any special character should be encoded/escaped, in which case you get the best of both worlds: security and full usability. However, when you're dealing with a syntax that was poorly designed and doesn't have a standard way to escape special characters, then you are forced to reject them. Still, it is better to do this rejection right before the commands are dropped onto the TCP stream. That way, if you later decide that there's an acceptable way to encode new lines for *specific* commands, then you can perform that encoding right there and relax the restrictions in specific cases. I like Serhiy's idea of optionally supporting escaping via the syntax defined in RFC 2640. This should be disabled by default, and a short-term security patch shouldn't try to tackle it. But in a new Python release, a switch could be added that causes new lines to be escaped for specific commands, such as the CWD and GET commands. It is likely that this escaping is only going to be useful in specific commands, based on what FTP servers actually support. Of course none of this is possible if you use a heavy-handed approach of blocking %0a in URLs up front. -- nosy: +ecbftw ___ Python tracker <http://bugs.python.org/issue30713> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[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 MUST be padded with a character prior to sending the command. On receipt of a pathname containing a sequence the 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 fooboo.bar the pathname would become fooboo.bar prior to sending the command STOR fooboo.bar. Upon receipt of the altered pathname the character following the 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. -- ___ Python tracker <http://bugs.python.org/issue29606> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
[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 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. -- messages: 288219 nosy: ecbftw priority: normal severity: normal status: open title: urllib FTP protocol stream injection type: security versions: Python 2.7, Python 3.3, Python 3.4, Python 3.5, Python 3.6, Python 3.7 ___ Python tracker <http://bugs.python.org/issue29606> ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
