[issue29606] urllib FTP protocol stream injection

[STINNER Victor]

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 
the newline characters

___
Python tracker 

___
___
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

[Martin Panter]

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.

--

___
Python tracker 

___
___
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

[Serhiy Storchaka]

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 

___
___
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]

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 

___
___
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]

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 

___
___
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

[STINNER Victor]

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.

--

___
Python tracker 

___
___
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

[Serhiy Storchaka]

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 '\n', '\r', '\0' or other illegal characters.

--

___
Python tracker 

___
___
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

[STINNER Victor]

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 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

--

___
Python tracker 

___
___
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

[STINNER Victor]

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 where the URL is parsed, no? See 
also my bpo-30713.

--

___
Python tracker 

___
___
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

[STINNER Victor]

Changes by STINNER Victor :


--
pull_requests: +2850

___
Python tracker 

___
___
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

[Dong-hee Na]

Dong-hee Na added the comment:

Yeah, I agree about your approach. I will update it for this weekend.

--

___
Python tracker 

___
___
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

[Serhiy Storchaka]

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 

___
___
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]

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 

___
___
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

[Dong-hee Na]

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 

___
___
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

[Dong-hee Na]

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 https://github.com/python/cpython/pull/1214

--

___
Python tracker 

___
___
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

[Martin Panter]

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 URLopener and the Python 2 “urllib” module as 
far as I know, so on its own just fixing FTPHandler wouldn’t be enough.

--

___
Python tracker 

___
___
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

[Dong-hee Na]

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 

___
___
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

[Giampaolo Rodola']

Changes by Giampaolo Rodola' :


--
nosy: +giampaolo.rodola

___
Python tracker 

___
___
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

[Martin Panter]

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 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.

--

___
Python tracker 

___
___
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

[Dong-hee Na]

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 

___
___
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

[Dong-hee Na]

Dong-hee Na added the comment:

Smillar issue but this issue is about FTP protocal using by httplib. Looks 
simillar but different.

--

___
Python tracker 

___
___
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

[Martin Panter]

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 22928.

--
nosy: +martin.panter
stage:  -> patch review

___
Python tracker 

___
___
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

[Dong-hee Na]

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 

___
___
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

[Dong-hee Na]

Changes by Dong-hee Na :


--
pull_requests: +1339

___
Python tracker 

___
___
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

[Plenty Su]

Changes by Plenty Su :


--
nosy: +supl

___
Python tracker 

___
___
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

[STINNER Victor]

Changes by STINNER Victor :


--
nosy: +christian.heimes, haypo

___
Python tracker 

___
___
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]

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 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com