New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ftplib: unlimited readline() from connection #60242
Comments
This bug is similar to bpo-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: |
Hello, I've made patch which address this issue. |
Thank you very much! Have you read and checked what the RFCs about the FTP protocol say about maximum line length? |
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. |
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:
|
vsftpd seems to use a 4096 limit (and the guy knows his stuff :-): |
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 |
LineTooLong should be added to ftplib.all_errors. 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. |
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) |
Why do you want to reduce it? It doesn't bring any additional security. |
Hello, I've set up maxline limit to 8192. Also I've add some changes Antoine suggested earlier. |
Not sure how I nosied Larry by updating this issue, sorry for the mistake. |
Ah, but that's because I added 3.4 in the versions field and the issue is a release blocker :) |
My spies are everywhere! You cannot hide your black heart, Pitrou. |
CVE-2013-1752 Unbound readline() DoS vulnerabilities in Python stdlib |
Patch looks ok. Just add the new exception class to all_errors list. |
Thank you Giampaolo, I'm attaching patch changed according to your suggestion. |
Not blocking 2.7.4 as discussed on mailing list. |
So, what now? |
The patches are languishing in the bug tracker for a while... Benjamin: |
I suppose this is fine for 2.7 |
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. |
blocker for 2.6.9 |
I'm attaching a slightly different patch including new tests and which uses a 'maxline' class attribute (as opposed to a global var). |
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.) |
2.6 version of the patch. Changes from Giampaolo's version of the patch:
|
====================================================================== 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 |
Looks legitimate to me. I will come up with a separate patch for later Python versions. |
Yep, confirmed that ftplib.patch causes test_ftplib to fail, at least on Ubuntu 10.04 chroot. |
Succeeds on OS X 10.8 (although there are other failures) |
Okay, this one is quite odd. It's definitely a timing issue. If I put a 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 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. |
What about time.sleep(0.1)? |
On Sep 23, 2013, at 03:36 PM, Serhiy Storchaka wrote:
I usually don't like introducing sleeps to fix race conditions, but if that's |
Barry can you paste the traceback caused by the race condition? What's not clear to me is when (what line) it occurs. |
On Sep 23, 2013, at 06:33 PM, Giampaolo Rodola' wrote:
There's no traceback other than the test failure that I posted. It's a race This is on various flavors of Ubuntu (only up to 10.04 which is the last |
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). |
On Sep 24, 2013, at 01:12 PM, Giampaolo Rodola' wrote:
+1 - that explanation makes a lot of sense, thanks! Currently 2.6.9rc1 is planned for Monday 30-September. It would be nice to |
Here is a patch. |
On Sep 24, 2013, at 09:59 PM, Serhiy Storchaka wrote:
This looks great and fixes the test failure problem. Thanks! Serhiy, please |
Please apply it yourself. |
New changeset 8b19e7d0be45 by Barry Warsaw in branch '2.6':
|
Ping. Please fix before "beta 1". |
I think this is already fixed. Barry can we close this out? |
It is fixed in Python 2.6, but not 2.7, 3.1, 3.2, 3.3, 3.4. |
New changeset 44ac81e6d584 by Serhiy Storchaka in branch '2.7': New changeset 38db4d0726bd by Serhiy Storchaka in branch '3.3': New changeset 0c48fe975c54 by Serhiy Storchaka in branch 'default': |
(3.1 branch is open to security fixes.) |
You are right. I will try to provide patches for other Python versions On Sun, Oct 20, 2013 at 5:08 PM, Arfrever Frehtes Taifersar Arahesis <
|
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. |
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. |
New changeset 783e7b4375ac by Georg Brandl in branch '3.2': |
I have a question about this old patch, as it just came down in a CentOS 6 update. I think the patch is applied to the data channel in ASCII mode and not just the control channel. On the data channel in ASCII mode, there should be no assumption of maximum line length before EOL. I saw that your current value came from vsftpd's header file. I'm guessing if you look at the implementation, it's either only applied to the control channel or it's just used to set a single read size inside of a loop. Examples of ASCII mode files that can exceed nearly any MAXLINE value without EOL are XML files or EDI files. |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: