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.

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

First patch

I haven't written tests yet nor implemented the size limit on the mock_socket class.

I've only taken a quick glance at this so far.

Why size=-1 instead of size=None?

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

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.

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.

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

Christian, do you want to try to complete this before the 2.7.4 RC?

Yes, I'm going to work on this issue for 2.7 and 3.3.

mock_socket violates readline() contract. It can return more than size bytes, and this can break SMTP.readline().

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.

CVE-2013-1752  Unbound readline() DoS vulnerabilities in Python stdlib

Oh, next time I should read my own patch and responses first ... ;)

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.

Not blocking 2.7.4 as discussed on mailing list.

blocker for 2.6.9

The patch requires a little adjusting to apply against 2.6.

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.

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

Patch looks great, thanks Andrew.  All tests pass.  Feel free to commit to the 2.6 branch along with a NEWS file entry.

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?

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

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.

I took Serhiy's suggestion and just moved up the 'if size' check in the loop.

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

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!

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.

Can we get this fixed in more recent versions?  Like, maybe, trunk, before "beta 1"?

Here is a port of changeset 8a6def3add5b for 2.7. However getreply() is not tested yet.

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

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

Could someone merge this change from 3.3 into default?  I would cherry-pick it for 3.4.0 if they did.

318de3affa3d

Are we going to apply a fix for this to 2.7?

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

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

New changeset 923aac88a3cc by Benjamin Peterson in branch '2.7':
smtplib: limit amount read from the network (closes #16042)
https://hg.python.org/cpython/rev/923aac88a3cc