classification
Title: ftplib: unlimited readline() from connection
Type: resource usage Stage: patch review
Components: Library (Lib) Versions: Python 3.2, Python 3.1
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: christian.heimes Nosy List: Arfrever, akuchling, barry, benjamin.peterson, christian.heimes, georg.brandl, giampaolo.rodola, inc0, josiahcarlson, larry, neologix, pitrou, python-dev, raduv, serhiy.storchaka, stutzbach
Priority: release blocker Keywords: patch

Created on 2012-09-25 10:32 by christian.heimes, last changed 2014-09-03 12:20 by raduv.

Files
File name Uploaded Description Edit
ftplib_maxline.patch inc0, 2013-02-05 20:30 review
ftplib_maxline.patch inc0, 2013-02-09 16:52 review
ftplib_maxline.patch inc0, 2013-02-16 19:11 review
ftplib_maxline.patch giampaolo.rodola, 2013-09-04 17:31
ftplib.patch akuchling, 2013-09-15 19:23
ftplib_maxline.patch serhiy.storchaka, 2013-09-24 21:59 review
Messages (50)
msg171241 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2012-09-25 10:32
This bug is similar to #16037.

The ftplib module doesn't limit the amount of read data in its call to readline(). An erroneous or malicious FTP server can trick the ftplib module to consume large amounts of memory.

Suggestion:
The ftplib module should be modified to use limited readline() with _MAXLINE like the httplib module.
msg181479 - (view) Author: Michał Jastrzębski (inc0) * Date: 2013-02-05 20:30
Hello,

I've made patch which address this issue.
msg181546 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2013-02-06 17:55
Thank you very much!

Have you read and checked what the RFCs about the FTP protocol say about  maximum line length?
msg181549 - (view) Author: Michał Jastrzębski (inc0) * Date: 2013-02-06 18:09
Well its my understanding, that there is no maximum length specified in RFC959. There is however option to set it up while telnet connection, and that would be other solution to this issue.
msg181553 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-02-06 18:31
Michał, thanks for the patch. Could you sign and e-mail a contributor's agreement? http://www.python.org/psf/contrib/

As for the patch:
- the test could be a separate test_ method
- the offset variable isn't used in cmd_retrlarge, there is no need computing it
msg181559 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2013-02-06 19:02
> Have you read and checked what the RFCs about the
> FTP protocol say about  maximum line length?

vsftpd seems to use a 4096 limit (and the guy knows his stuff :-):
ftp://vsftpd.beasts.org/users/cevans/untar/vsftpd-3.0.2/defs.h
msg181571 - (view) Author: Michał Jastrzębski (inc0) * Date: 2013-02-06 20:17
Well, that is not from RFC (or I hadn't find it):) however I'd lie if I'd call myself an expert, should I change limit to 4096 then?
msg181580 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2013-02-07 07:31
> Well, that is not from RFC (or I hadn't find it):) however I'd lie if I'd call myself an expert, should I change limit to 4096 then?

It's probably not in the RFC: this just shows that the limit chosen
should be enough.
msg181635 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2013-02-07 18:00
LineTooLong should be added to ftplib.all_errors.
4096 looks enough to me.
The longest lines I can think of occur when processing MLSD command which produces an output of like this:

type=file;size=156;perm=r;modify=20071029155301;unique=801cd2; music.mp3
type=dir;size=0;perm=el;modify=20071127230206;unique=801e33; ebooks
type=file;size=211;perm=r;modify=20071103093626;unique=801e32; module.py

Considering that the file names listed in there are forced to consist of base names (as opposed to *full* path names) I doubt we'll ever hit 4096.
In pyftpdlib I used 2048 bytes.
I can't recall any reference about this in any FTP-related RFC.
msg181638 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2013-02-07 18:11
I suggest that we use twice the size of the largest limit (8192) for the DoS fix and reduce it to 2048 for Python 3.4. 8192 is still a small number for modern computers.

I also like to see comments next to the limit that explain on what grounds we have chosen the value. For example

# vfstpd has a limit of 4096 (ftp://vsftpd.beasts.org/users/cevans/untar/vsftpd-3.0.2/defs.h)
# pyftpdlib has a limit of 2048
msg181644 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-02-07 19:53
> I suggest that we use twice the size of the largest limit (8192) for
> the DoS fix and reduce it to 2048 for Python 3.4. 8192 is still a
> small number for modern computers.

Why do you want to reduce it? It doesn't bring any additional security.
msg181745 - (view) Author: Michał Jastrzębski (inc0) * Date: 2013-02-09 16:52
Hello,

I've set up maxline limit to 8192. Also I've add some changes Antoine suggested earlier.
msg181771 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-02-09 23:09
Not sure how I nosied Larry by updating this issue, sorry for the mistake.
msg181772 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-02-09 23:09
Ah, but that's because I added 3.4 in the versions field and the issue is a release blocker :)
msg181773 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2013-02-09 23:14
My spies are everywhere!  You cannot hide your black heart, Pitrou.
msg182195 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2013-02-15 23:58
CVE-2013-1752  Unbound readline() DoS vulnerabilities in Python stdlib
msg182235 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2013-02-16 18:11
Patch looks ok. Just add the new exception class to all_errors list.
msg182236 - (view) Author: Michał Jastrzębski (inc0) * Date: 2013-02-16 19:11
Thank you Giampaolo,

I'm attaching patch changed according to your suggestion.
msg185060 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2013-03-23 14:46
Not blocking 2.7.4 as discussed on mailing list.
msg194170 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2013-08-02 09:24
So, what now?
msg194551 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2013-08-06 14:21
The patches are languishing in the bug tracker for a while...

Benjamin:
I like to apply them to 3.3 and default before the next release of 3.3. Do you want to have the fixes in 2.7, too?
msg194921 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2013-08-12 04:27
I suppose this is fine for 2.7
msg194935 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-08-12 09:46
Error message "got more than %d bytes" is misleading because in most cases (except storlines()) we read not bytes but a text string.

There are 4 changes in the ftplib module but only one of them covered by test.
msg196861 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2013-09-03 18:34
blocker for 2.6.9
msg196927 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2013-09-04 17:31
I'm attaching a slightly different patch including new tests and which uses a 'maxline' class attribute (as opposed to a global var).
Christian if that's OK with you I will wait a while and then make a commit for all Python versions.
msg197791 - (view) Author: A.M. Kuchling (akuchling) * (Python committer) Date: 2013-09-15 16:50
For 2.6 I'll make a revised version of Giampaolo's patch that doesn't add a new exception class.  

Rationale: Adding a new exception class changes the API of the module, which we'd like to avoid.  If someone is writing 2.6 code that wants to catch this exception, they can't write "except ftplib.LineTooLong" because the name isn't present.  Instead they'll have to catch the parent Error exception class and analyze either its type or the exception message.  My conclusion is that adding the new class isn't actually useful.

(bwarsaw and I are at a mini-sprint looking at the 2.6.9 blockers, so we're looking at all of these 'unlimited readline' issues and will continue to remove new exceptions introduced by patches.)
msg197817 - (view) Author: A.M. Kuchling (akuchling) * (Python committer) Date: 2013-09-15 19:23
2.6 version of the patch.  Changes from Giampaolo's version of the patch:

* 2.6 didn't have FTP over TLS, so the patch changes slightly to adapt.

* Removed the LineTooLong exception class and just raise Error instead.  (This repeats the message text for "Line too long" in several place.)
msg197827 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2013-09-15 20:08
======================================================================
FAIL: test_retrlines_too_long (__main__.TestFTPClass)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "Lib/test/test_ftplib.py", line 374, in test_retrlines_too_long
    self.client.retrlines, 'retr', received.append)
AssertionError: Error not raised
msg197921 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2013-09-16 17:41
Looks legitimate to me. I will come up with a separate patch for later Python versions.
msg197957 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2013-09-17 02:44
Yep, confirmed that ftplib.patch causes test_ftplib to fail, at least on Ubuntu 10.04 chroot.
msg197959 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2013-09-17 03:05
Succeeds on OS X 10.8 (although there are other failures)
msg198298 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2013-09-22 19:50
Okay, this one is quite odd.  It's definitely a timing issue.

If I put a `import time; time.sleep(1)` at the beginning of test_retrlines_too_line() -- i.e. first line of the method -- then the test reliably passes.  If I put a `print(len(line))` just before the maxline test in FTP.retrlines(), then the test will pass just as reliably.

If I put that retrlines() print *after* the maxline test, then it passes sometimes and fails sometimes.  When if fails, it's only ready 12 bytes from the `fp.readline()` call.  When it passes, it's reading 8193 bytes (thus triggering the expected exception).

I really hate to put a sleep in the test to make it pass.  Obviously it would be better not to fudge this race condition, but I don't know the code well enough to know where the race is yet.
msg198323 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-09-23 15:36
What about time.sleep(0.1)?
msg198325 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2013-09-23 16:49
On Sep 23, 2013, at 03:36 PM, Serhiy Storchaka wrote:

>What about time.sleep(0.1)?

I usually don't like introducing sleeps to fix race conditions, but if that's
the only option for landing this patch, maybe we'll have to hold our noses and
do it.
msg198327 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2013-09-23 18:33
Barry can you paste the traceback caused by the race condition? What's not clear to me is when (what line) it occurs.
One solution might be to send a "NOOP" command (self.client.sendcmd('noop')) in order to synchronize client and server.
msg198344 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2013-09-23 22:03
On Sep 23, 2013, at 06:33 PM, Giampaolo Rodola' wrote:

>Barry can you paste the traceback caused by the race condition? What's not
>clear to me is when (what line) it occurs.  One solution might be to send a
>"NOOP" command (self.client.sendcmd('noop')) in order to synchronize client
>and server.

There's no traceback other than the test failure that I posted.  It's a race
condition because with a little sleep, the test passes.  Without it, it fails.

This is on various flavors of Ubuntu (only up to 10.04 which is the last
version I can build a full 2.6 against) and Debian.
msg198356 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2013-09-24 13:12
I believe the problem is the set of next_retr_data attribute here:

    def test_retrlines_too_long(self):
        self.server.handler.next_retr_data = 'x' * self.client.maxline * 2

...because self.server.handler runs in a different thread (different than the main one, which is where the setattr() occurs).
We should introduce a new command in the dummy FTP server which sets next_retr_data from within the server thread itself. Will try to work on a patch later this week (I'm sorry but I can't make it earlier).
msg198357 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2013-09-24 13:30
On Sep 24, 2013, at 01:12 PM, Giampaolo Rodola' wrote:

>Giampaolo Rodola' added the comment:
>
>I believe the problem is the set of next_retr_data attribute here:
>
>    def test_retrlines_too_long(self):
>        self.server.handler.next_retr_data = 'x' * self.client.maxline * 2
>
>...because self.server.handler runs in a different thread (different than the
>main one, which is where the setattr() occurs).  We should introduce a new
>command in the dummy FTP server which sets next_retr_data from within the
>server thread itself. Will try to work on a patch later this week (I'm sorry
>but I can't make it earlier).

+1 - that explanation makes a lot of sense, thanks!

Currently 2.6.9rc1 is planned for Monday 30-September.  It would be nice to
get this one in before then, but if not that's okay.  I think it's fairly low
risk.
msg198368 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-09-24 21:59
Here is a patch.
msg198371 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2013-09-24 22:18
On Sep 24, 2013, at 09:59 PM, Serhiy Storchaka wrote:

>Added file: http://bugs.python.org/file31862/ftplib_maxline.patch

This looks great and fixes the test failure problem.  Thanks!  Serhiy, please
feel free to apply this to the 2.6 branch, or let me know if you'd rather I
apply it.
msg198380 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-09-25 09:54
Please apply it yourself.
msg198388 - (view) Author: Roundup Robot (python-dev) Date: 2013-09-25 14:43
New changeset 8b19e7d0be45 by Barry Warsaw in branch '2.6':
- Issue #16038: CVE-2013-1752: ftplib: Limit amount of data read by
http://hg.python.org/cpython/rev/8b19e7d0be45
msg200348 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2013-10-19 01:22
Ping.  Please fix before "beta 1".
msg200386 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2013-10-19 07:51
I think this is already fixed. Barry can we close this out?
msg200387 - (view) Author: Arfrever Frehtes Taifersar Arahesis (Arfrever) * Date: 2013-10-19 08:13
It is fixed in Python 2.6, but not 2.7, 3.1, 3.2, 3.3, 3.4.
msg200588 - (view) Author: Roundup Robot (python-dev) Date: 2013-10-20 14:02
New changeset 44ac81e6d584 by Serhiy Storchaka in branch '2.7':
Issue #16038: CVE-2013-1752: ftplib: Limit amount of data read by
http://hg.python.org/cpython/rev/44ac81e6d584

New changeset 38db4d0726bd by Serhiy Storchaka in branch '3.3':
Issue #16038: CVE-2013-1752: ftplib: Limit amount of data read by
http://hg.python.org/cpython/rev/38db4d0726bd

New changeset 0c48fe975c54 by Serhiy Storchaka in branch 'default':
Issue #16038: CVE-2013-1752: ftplib: Limit amount of data read by
http://hg.python.org/cpython/rev/0c48fe975c54
msg200592 - (view) Author: Arfrever Frehtes Taifersar Arahesis (Arfrever) * Date: 2013-10-20 15:08
(3.1 branch is open to security fixes.)
msg200602 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2013-10-20 18:17
You are right. I will try to provide patches for other Python versions
later next week.

On Sun, Oct 20, 2013 at 5:08 PM, Arfrever Frehtes Taifersar Arahesis <
report@bugs.python.org> wrote:

>
> Arfrever Frehtes Taifersar Arahesis added the comment:
>
> (3.1 branch is open to security fixes.)
>
> ----------
> versions: +Python 3.1
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue16038>
> _______________________________________
>
msg214908 - (view) Author: A.M. Kuchling (akuchling) * (Python committer) Date: 2014-03-26 20:19
Are we likely to actually apply this change to the 3.1 and 3.2 branches, given that even the later 3.3 branch is now in security-fix mode?  If we're not going to change 3.1 or 3.2, this issue can just be closed.
msg226309 - (view) Author: Radu Voicilas (raduv) Date: 2014-09-03 12:20
I'm a little confused about this patch. Please correct me if I'm wrong, but fp.readline([size + 1]) should return a line of length at most size + 1. This means that the check len(line) > size will always be true when reading a line that has a length greater than self.maxline. Also, wouldn't it make more sense to have the line that logs stuff in debugging mode be before raising a LineTooLong exception ? This way you have the option of actually seeing the line.
History
Date User Action Args
2014-09-03 12:20:28raduvsetnosy: + raduv
messages: + msg226309
2014-03-26 20:19:02akuchlingsetmessages: + msg214908
2013-10-20 18:17:05giampaolo.rodolasetmessages: + msg200602
2013-10-20 15:08:07Arfreversetmessages: + msg200592
versions: + Python 3.1
2013-10-20 14:03:38serhiy.storchakasetversions: - Python 3.1, Python 2.7, Python 3.3, Python 3.4
2013-10-20 14:02:57python-devsetmessages: + msg200588
2013-10-19 08:13:47Arfreversetmessages: + msg200387
2013-10-19 07:51:50giampaolo.rodolasetmessages: + msg200386
2013-10-19 01:22:30larrysetmessages: + msg200348
2013-09-25 14:49:34barrysetversions: - Python 2.6
2013-09-25 14:43:17python-devsetnosy: + python-dev
messages: + msg198388
2013-09-25 09:54:00serhiy.storchakasetmessages: + msg198380
2013-09-24 22:18:06barrysetmessages: + msg198371
2013-09-24 21:59:04serhiy.storchakasetfiles: + ftplib_maxline.patch

messages: + msg198368
2013-09-24 13:30:55barrysetmessages: + msg198357
2013-09-24 13:12:08giampaolo.rodolasetmessages: + msg198356
2013-09-23 22:03:15barrysetmessages: + msg198344
2013-09-23 18:33:27giampaolo.rodolasetmessages: + msg198327
2013-09-23 16:49:30barrysetmessages: + msg198325
2013-09-23 15:37:44serhiy.storchakasetnosy: + josiahcarlson, stutzbach
2013-09-23 15:36:00serhiy.storchakasetmessages: + msg198323
2013-09-22 19:50:55barrysetmessages: + msg198298
2013-09-17 03:05:51barrysetmessages: + msg197959
2013-09-17 02:44:37barrysetmessages: + msg197957
2013-09-16 17:41:01giampaolo.rodolasetmessages: + msg197921
2013-09-15 20:08:36barrysetmessages: + msg197827
2013-09-15 19:43:03Arfreversetversions: + Python 2.6, Python 3.1
2013-09-15 19:23:55akuchlingsetfiles: + ftplib.patch

messages: + msg197817
2013-09-15 16:50:46akuchlingsetnosy: + akuchling
messages: + msg197791
2013-09-04 17:31:21giampaolo.rodolasetfiles: + ftplib_maxline.patch

messages: + msg196927
2013-09-03 18:34:57barrysetpriority: critical -> release blocker
nosy: + barry
messages: + msg196861

2013-08-12 09:46:08serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg194935
2013-08-12 04:27:21benjamin.petersonsetmessages: + msg194921
2013-08-06 14:21:50christian.heimessetmessages: + msg194551
2013-08-02 09:24:05neologixsetmessages: + msg194170
2013-03-23 14:46:03benjamin.petersonsetpriority: release blocker -> critical

messages: + msg185060
2013-02-22 23:46:34Arfreversetnosy: + Arfrever
2013-02-16 19:11:19inc0setfiles: + ftplib_maxline.patch

messages: + msg182236
2013-02-16 18:11:25giampaolo.rodolasetmessages: + msg182235
2013-02-15 23:58:54christian.heimessetmessages: + msg182195
2013-02-09 23:14:21larrysetmessages: + msg181773
2013-02-09 23:09:49pitrousetmessages: + msg181772
2013-02-09 23:09:11pitrousetnosy: georg.brandl, pitrou, larry, giampaolo.rodola, christian.heimes, benjamin.peterson, neologix, inc0
messages: + msg181771
2013-02-09 23:02:34pitrousetnosy: + larry
stage: needs patch -> patch review

versions: + Python 3.4
2013-02-09 16:52:49inc0setfiles: + ftplib_maxline.patch

messages: + msg181745
2013-02-07 19:53:36pitrousetmessages: + msg181644
2013-02-07 18:11:03christian.heimessetmessages: + msg181638
2013-02-07 18:00:31giampaolo.rodolasetmessages: + msg181635
2013-02-07 07:31:15neologixsetmessages: + msg181580
2013-02-06 20:17:57inc0setmessages: + msg181571
2013-02-06 19:02:44neologixsetnosy: + neologix
messages: + msg181559
2013-02-06 18:31:42pitrousetnosy: + pitrou
messages: + msg181553
2013-02-06 18:09:17inc0setmessages: + msg181549
2013-02-06 17:55:11christian.heimessetmessages: + msg181546
2013-02-05 20:30:15inc0setfiles: + ftplib_maxline.patch

nosy: + inc0
messages: + msg181479

keywords: + patch
2013-02-04 17:12:29christian.heimessetpriority: critical -> release blocker
nosy: + georg.brandl, benjamin.peterson
2013-01-21 11:36:42giampaolo.rodolasetnosy: + giampaolo.rodola
2013-01-20 14:38:44christian.heimessetpriority: normal -> critical
assignee: christian.heimes
stage: needs patch
2012-09-25 10:32:54christian.heimescreate