classification
Title: smtplib: unlimited readline() from connection
Type: resource usage Stage: needs patch
Components: email, Library (Lib) Versions: Python 2.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: christian.heimes Nosy List: Arfrever, akuchling, barry, benjamin.peterson, christian.heimes, georg.brandl, giampaolo.rodola, larry, python-dev, r.david.murray, serhiy.storchaka
Priority: release blocker Keywords: patch

Created on 2012-09-25 10:40 by christian.heimes, last changed 2014-09-30 14:37 by Arfrever.

Files
File name Uploaded Description Edit
smtp_readline.patch christian.heimes, 2012-09-25 16:36
smtplib-2.6.patch akuchling, 2013-09-15 15:59
smtplib-2.6-with-test.patch akuchling, 2013-09-15 16:31
smtplib_maxline-2.7.patch serhiy.storchaka, 2013-10-20 14:47 review
3.3-fix.txt akuchling, 2013-11-12 21:22
Messages (37)
msg171245 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2012-09-25 10:40
This bug is similar to #16037 and a modified copy of #16038.

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

Suggestion:
The smtplib module should be modified to use limited readline() with _MAXLINE like the httplib module.
msg171294 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2012-09-25 15:47
RFC 821 [1] specifies rather short line lengths between 512 and 1001 chars including the trailing CRLF. A line limit of a couple of kilobyte should definitely work with all standard conform SMTP clients and servers.

[1] http://www.freesoft.org/CIE/RFC/821/24.htm
msg171296 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2012-09-25 16:36
First patch

I haven't written tests yet nor implemented the size limit on the mock_socket class.
msg171297 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-09-25 16:49
I've only taken a quick glance at this so far.

Why size=-1 instead of size=None?
msg171298 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2012-09-25 16:55
size=-1 mimics the code of the io module. The C implementation of readline() maps all negative values to "unlimited" and values >= 0 as limit.

>>> sys.stdin.readline(None)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: 'NoneType' object cannot be interpreted as an integer
msg171301 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-09-25 17:54
Both io.IOBase.readline() and io.TextIOBase.readline() have parameter named "limit".

I doubt that such a change can be done in 2.7 and 3.2.
msg171303 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-09-25 18:09
Please submit patches in standard Mercurial format to them understood Rietveld. I wanted to make a code review, but I don't see the definition of readline() method in the file Lib/smtplib.py.
msg173413 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2012-10-20 20:37
I understand you, it's a patch against 2.7.

>+        def readline(self, size=-1):

In io.IOBase.readline() and in io.TextIOBase.readline() this parameter named "limit".

>+                if size is not None and len(str) == size:
>+                    break

It can be moved to the while condition:

             while chr != b"\n" and (size is None or len(str) < size):
msg180270 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-01-19 22:21
Christian, do you want to try to complete this before the 2.7.4 RC?
msg180295 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2013-01-20 14:36
Yes, I'm going to work on this issue for 2.7 and 3.3.
msg180306 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-01-20 17:37
mock_socket violates readline() contract. It can return more than size bytes, and this can break SMTP.readline().
msg182200 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2013-02-16 00:04
RFC 2821 says:

   command line
      The maximum total length of a command line including the command
      word and the <CRLF> is 512 characters.  SMTP extensions may be
      used to increase this limit.

   reply line
      The maximum total length of a reply line including the reply code
      and the <CRLF> is 512 characters.  More information may be
      conveyed through multiple-line replies.

   text line
      The maximum total length of a text line including the <CRLF> is
      1000 characters (not counting the leading dot duplicated for
      transparency).  This number may be increased by the use of SMTP
      Service Extensions.

I suggest a response limit of 2048 octets (that is four times the max limit) to be on the safe side for a bugfix release.
msg182201 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2013-02-16 00:04
CVE-2013-1752  Unbound readline() DoS vulnerabilities in Python stdlib
msg182202 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2013-02-16 00:05
Oh, next time I should read my own patch and responses first ... ;)
msg182256 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-02-17 02:34
I doubt that 2048 is safer than 1024 for any meaningful value of safer.  Either the sever respects the rfc limits or it does not.  If it does not,  it is likely to send very long text lines if the sending mua generates them, which I suspect happens.  However, there is no real reason not to arbitrarily pick 2048.
msg185058 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2013-03-23 14:45
Not blocking 2.7.4 as discussed on mailing list.
msg196858 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2013-09-03 18:33
blocker for 2.6.9
msg197779 - (view) Author: A.M. Kuchling (akuchling) * (Python committer) Date: 2013-09-15 15:59
The patch requires a little adjusting to apply against 2.6.
msg197782 - (view) Author: A.M. Kuchling (akuchling) * (Python committer) Date: 2013-09-15 16:31
Here's a final proposed version of the patch for 2.6 that adds a test.  Changes made:

* code now raises SMTPResponseException instead of a new SMTPLineTooLong exception; bwarsaw deemed that adding a new exception class was changing the module API.

* we looked at Serhiy's suggestion to move the length check into the 'while' loop's condition and decided not to -- the code is more obvious with the separate if/break.

* the test class is a cut-and-paste and slight modification of the BadHELOServerTests class; I didn't try to unify them in some way.
msg197786 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-09-15 16:45
It is not important in the context of this issue, but readline(0) is blocked and returns 1-character string. Move the length check above self.sslobj.read(1). For readability you can also move the chr != "\n" inside the loop:

             while size is None or len(str) < size:
                 chr = self.sslobj.read(1)
                 if not chr or chr == "\n": break
                 str += chr
msg197789 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2013-09-15 16:48
Patch looks great, thanks Andrew.  All tests pass.  Feel free to commit to the 2.6 branch along with a NEWS file entry.
msg197792 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2013-09-15 16:54
On Sep 15, 2013, at 04:47 PM, Serhiy Storchaka wrote:

>It is not important in the context of this issue, but readline(0) is blocked
>and returns 1-character string. Move the length check above
>self.sslobj.read(1). For readability you can also move the chr != "\n" inside
>the loop:
>
>             while size is None or len(str) < size:
>                 chr = self.sslobj.read(1)
>                 if not chr or chr == "\n": break
>                 str += chr

Hi Serhiy.  Is there a functional difference to re-arranging this loop?
All things being equal, the minimal change is probably best.

Also, what do you mean by "readline(0) is blocked"?  Do you mean this is a
blocking call or something else?
msg197794 - (view) Author: Roundup Robot (python-dev) Date: 2013-09-15 17:15
New changeset 8a6def3add5b by Andrew Kuchling in branch '2.6':
#16042: CVE-2013-1752: Limit amount of data read by limiting the call to readline().
http://hg.python.org/cpython/rev/8a6def3add5b
msg197796 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-09-15 17:20
I'm not sure what Serhiy means by "is blocked", but the second half makes sense: readline(0) on a file will return the empty string, but here it will read one character and return it.  Like he says, it doesn't break anything in the context of this bug fix, but it is an API bug.  Unless I'm missing something his replacement also has a bug, though: it won't add the \n to the returned string.

A minimal fix for the API bug would be to extend the 'if size' with an elif for 0 that returns the empty string.
msg197799 - (view) Author: A.M. Kuchling (akuchling) * (Python committer) Date: 2013-09-15 17:26
I took Serhiy's suggestion and just moved up the 'if size' check in the loop.
msg197800 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-09-15 17:34
> Also, what do you mean by "readline(0) is blocked"?  Do you mean this is a blocking call or something else?

Yes, I mean this is a blocking call.

> Unless I'm missing something his replacement also has a bug, though: it won't add the \n to the returned string.

Oh, right. The correct code should be as I proposed in msg173413 or... as Andrew has committed. Good.
msg197809 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2013-09-15 18:47
On Sep 15, 2013, at 05:34 PM, Serhiy Storchaka wrote:

>Oh, right. The correct code should be as I proposed in msg173413 or... as
>Andrew has committed. Good.

Excellent.  So we're good for this in 2.6.  Thanks!
msg198302 - (view) Author: Barry A. Warsaw (barry) * (Python committer) Date: 2013-09-22 21:18
Please don't add 2.6 back to the Versions, unless there's actually something to do for 2.6.  AFAIK, this issue is resolved for 2.6.
msg200347 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2013-10-19 01:21
Can we get this fixed in more recent versions?  Like, maybe, trunk, before "beta 1"?
msg200591 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-10-20 14:47
Here is a port of changeset 8a6def3add5b for 2.7. However getreply() is not tested yet.
msg202722 - (view) Author: A.M. Kuchling (akuchling) * (Python committer) Date: 2013-11-12 21:22
Serhiy, your version of the patch for 2.7 looks fine.

I've attached a version of the patch for 3.3.  A change is needed to the MockFile object provided by Lib/test/mock_socket.py
msg210866 - (view) Author: Roundup Robot (python-dev) Date: 2014-02-10 21:04
New changeset d62a67318023 by Georg Brandl in branch '3.3':
#16042: CVE-2013-1752: smtplib fix for unlimited readline() from socket
http://hg.python.org/cpython/rev/d62a67318023
msg211694 - (view) Author: Larry Hastings (larry) * (Python committer) Date: 2014-02-20 06:15
Could someone merge this change from 3.3 into default?  I would cherry-pick it for 3.4.0 if they did.
msg211697 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2014-02-20 06:21
318de3affa3d
msg214909 - (view) Author: A.M. Kuchling (akuchling) * (Python committer) Date: 2014-03-26 20:20
Are we going to apply a fix for this to 2.7?
msg227892 - (view) Author: Roundup Robot (python-dev) Date: 2014-09-30 12:47
New changeset 0f362676460d by Georg Brandl in branch '3.2':
Issue #16042: CVE-2013-1752: smtplib: Limit amount of data read by
https://hg.python.org/cpython/rev/0f362676460d
msg227932 - (view) Author: Arfrever Frehtes Taifersar Arahesis (Arfrever) * Date: 2014-09-30 14:37
New changeset 4065c4539fcb by Georg Brandl in branch '3.2':
Fix-up for 0f362676460d: add missing size argument to SSLFakeFile.readline(), as in 2.6 backport
https://hg.python.org/cpython/rev/4065c4539fcb
History
Date User Action Args
2014-09-30 14:37:07Arfreversetmessages: + msg227932
2014-09-30 12:49:52georg.brandlsetversions: - Python 3.2
2014-09-30 12:47:29python-devsetmessages: + msg227892
2014-09-30 12:17:11georg.brandlsetversions: - Python 3.1
2014-03-26 20:20:25akuchlingsetmessages: + msg214909
2014-02-23 07:08:51georg.brandlsetversions: - Python 3.3, Python 3.4
2014-02-20 06:21:38benjamin.petersonsetmessages: + msg211697
2014-02-20 06:15:04larrysetmessages: + msg211694
2014-02-10 21:04:39python-devsetmessages: + msg210866
2013-11-12 21:22:57akuchlingsetfiles: + 3.3-fix.txt

messages: + msg202722
2013-10-20 14:47:41serhiy.storchakasetfiles: + smtplib_maxline-2.7.patch

messages: + msg200591
2013-10-19 01:21:34larrysetmessages: + msg200347
2013-09-22 21:18:36barrysetmessages: + msg198302
versions: - Python 2.6
2013-09-15 19:46:25Arfreversetversions: + Python 2.6, Python 3.1
2013-09-15 18:47:11barrysetmessages: + msg197809
2013-09-15 17:34:17serhiy.storchakasetmessages: + msg197800
2013-09-15 17:26:03akuchlingsetmessages: + msg197799
2013-09-15 17:20:28r.david.murraysetmessages: + msg197796
2013-09-15 17:15:12python-devsetnosy: + python-dev
messages: + msg197794
2013-09-15 16:54:20barrysetmessages: + msg197792
2013-09-15 16:48:55barrysetmessages: + msg197789
2013-09-15 16:48:11serhiy.storchakasetmessages: - msg197788
2013-09-15 16:47:33serhiy.storchakasetmessages: + msg197788
2013-09-15 16:45:28serhiy.storchakasetmessages: + msg197786
2013-09-15 16:31:57akuchlingsetfiles: + smtplib-2.6-with-test.patch

messages: + msg197782
2013-09-15 15:59:42akuchlingsetfiles: + smtplib-2.6.patch
nosy: + akuchling
messages: + msg197779

2013-09-03 18:33:16barrysetpriority: critical -> release blocker

messages: + msg196858
2013-03-23 14:45:47benjamin.petersonsetpriority: release blocker -> critical

messages: + msg185058
2013-02-22 23:48:28Arfreversetnosy: + Arfrever
2013-02-17 02:34:06r.david.murraysetmessages: + msg182256
2013-02-16 00:05:30christian.heimessetmessages: + msg182202
2013-02-16 00:04:41christian.heimessetmessages: + msg182201
2013-02-16 00:04:23christian.heimessetmessages: + msg182200
2013-02-04 17:12:45christian.heimessetpriority: critical -> release blocker
nosy: + larry, benjamin.peterson, georg.brandl
2013-01-21 11:36:49giampaolo.rodolasetnosy: + giampaolo.rodola
2013-01-20 17:37:28serhiy.storchakasetmessages: + msg180306
2013-01-20 14:36:51christian.heimessetpriority: normal -> critical
versions: + Python 3.4
messages: + msg180295

assignee: christian.heimes
stage: needs patch
2013-01-19 22:21:31r.david.murraysetmessages: + msg180270
2012-10-21 04:45:49r.david.murraysetcomponents: + email
2012-10-20 20:37:44serhiy.storchakasetmessages: + msg173413
2012-09-25 18:09:07serhiy.storchakasetmessages: + msg171303
2012-09-25 17:54:49serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg171301
2012-09-25 16:55:40christian.heimessetmessages: + msg171298
2012-09-25 16:49:56r.david.murraysetmessages: + msg171297
2012-09-25 16:36:10christian.heimessetfiles: + smtp_readline.patch
keywords: + patch
messages: + msg171296
2012-09-25 15:48:40christian.heimessetnosy: + barry, r.david.murray
2012-09-25 15:47:10christian.heimessetmessages: + msg171294
2012-09-25 10:40:09christian.heimescreate