classification
Title: DoS smtpd vulnerability
Type: security Stage:
Components: Library (Lib) Versions: Python 3.1, Python 3.2, Python 2.7, Python 2.6, Python 2.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: josiahcarlson Nosy List: BreamoreBoy, barry, georg.brandl, giampaolo.rodola, henriquebastos, jafo, jcea, josiahcarlson, saviosena
Priority: high Keywords: patch

Created on 2007-06-28 19:44 by giampaolo.rodola, last changed 2010-12-03 07:38 by georg.brandl. This issue is now closed.

Files
File name Uploaded Description Edit
smtpd.py giampaolo.rodola, 2007-10-21 19:17
smtpd.diff giampaolo.rodola, 2007-10-21 19:18
smtpd.diff giampaolo.rodola, 2008-01-23 19:13 Updated patch (fixes described issue and adds a smaller timeout for asyncore.loop)
issue1745035-saviosena-101121.diff saviosena, 2010-11-22 02:11
issue1745035-101123-saviosena.diff saviosena, 2010-11-23 18:47
issue1745035-101123-saviosena.diff saviosena, 2010-11-23 19:15
issue1745035-101123-saviosena.diff saviosena, 2010-11-23 19:37
Messages (17)
msg32418 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2007-06-28 19:44
Method "collect_incoming_data" of "SMTPChannel" class should stop buffering if received lines are too long (possible Denial-of-Service attacks).
Without truncating such buffer a simple malicious script sending extremely long lines without "\r\n" terminator could easily saturate system resources.
msg32419 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2007-06-28 19:45
--- malicious script

import socket
s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
s.connect(("127.0.0.1", 8025))
while 1:
    s.sendall('x' * 1024)


--- proposed smtpd.py patch

124a125
>         self.__in_buffer_len = 0
135a137,139
>         self.__in_buffer_len += len(data)
>         if self.__in_buffer_len > 4096:
>             self.__line = []

msg32420 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2007-06-29 18:00
Sorry, I realized I've forgotten to reset to zero the bytes counter.
Here's the patch of the patch:

124a125
>         self.__in_buffer_len = 0
135a137,140
>         self.__in_buffer_len += len(data)
>         if self.__in_buffer_len > 4096:
>             self.__line = []
>             self.__in_buffer_len = 0
141a147
>         self.__in_buffer_len = 0
msg56019 - (view) Author: Sean Reifschneider (jafo) * (Python committer) Date: 2007-09-19 00:49
Patch is inline above.

RFC2822 says lines MUST be less than 998 bytes long, so this should be fine.

What does this do when a line longer than 4096 bytes is found?  Does it
report an error to the SMTP client?  That's my only concern.
msg56627 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2007-10-21 19:17
> What does this do when a line longer than 4096 bytes 
> is found?  Does it report an error to the SMTP client?  
> That's my only concern.

Sorry for replying so late. 
No, it does not report the error and this is bad.
I've searched through RFCs and I found that RFC 821 and RFC 2821 at
chapter 4.2.2 say that a 500 "Syntax error, command unrecognized"
response could be used to report errors such as command lines too long.

Modified smtpd.py in attachment. It should be definitively fine for
inclusion now.
msg61599 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2008-01-23 19:13
I update this bug as GvR requested here:
http://groups.google.it/group/python-dev2/browse_thread/thread/33cad7b7c1cdb19f?hl=it
The patch in attachment fixes what discussed before.

In addition it sets a smaller timeout for asyncore.loop() for permitting
to stop the daemon via KeyboardInterrupt immediately.
msg74047 - (view) Author: Josiah Carlson (josiahcarlson) * (Python triager) Date: 2008-09-29 20:53
The patch does not work as Giampaolo intends.  If the patch were applied 
as-is, no emails longer than 998 bytes could be sent.

Instead, incrementing linelen in the collect_incoming_data() method 
should only be performed if self.terminator == '\r\n'.

I can apply a modified version of this patch against trunk after 2.6 is 
released.  Backports to 2.5 and 2.6 should then be discussed.
msg74073 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2008-09-30 13:12
Yes, you're right. I mixed up SMTP with FTP which does not send data on
the same connection used for receiving commands.
msg116697 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2010-09-17 18:02
Given the title, type and severity shouldn't someone take a look at this?
msg121874 - (view) Author: Savio Sena (saviosena) Date: 2010-11-21 07:22
The definite (and only?) solution would be to implement 'Message Size Declaration[1]' Service Extension[2]. We can limit the size of commands and text lines, but not the message size as a whole[3]. RFC1870 was created exactly with the purpose of solving DoS issues[4].

[1] RFC 1870
[2] RFC 1869
[3] RFC 2821, 4.5.3.1
[4] RFC 1870, 9.0

I'm willing to implement a fix (actually, I'm already working on that :-)) --- but I'd make good use of advices since I'm new to this project.

The minimalistic fix to this issue would be to implement Message Size Extension alone and apart from subclasses awareness --- without any changes to the interface, not even to allow changes in the default maximum message size [once SMPTServer is fully-RFC1869-compliant changing these values will be straightforward].

A second (extensive!) approach is to implement a complete ESMTPServer. That's a way bigger task though. It would be wise in this case to implement all the standard extensions, plus support (parsing/error handling) to all RFC1869.

Any thoughts?
My best regards.

NOTE: In my opinion there's no bug, really. All non-extented SMPT servers are vulnerable to resource exhaustion. Maybe we should take this as a feature request not a bug-fix request.
msg122064 - (view) Author: Savio Sena (saviosena) Date: 2010-11-22 02:11
The attached patch adopts the minimalistic approach described in previous post. It pretends to implement Message Size Extension, defining a maximum message data size to 32M bytes and maximum command length to 512 bytes.

In my opinion this is the best way to accomplish a patch to the DoS issue alone. It's still a good idea though to implement full ESMTP support -- I know, I'm repeating myself. 

Please note this is my first patch. I'm new to Python and even though I made my best to be in conformance with the "standards" and good practices I may have missed something. Please review this patch with clinical eyes. 

For the records: this work is due to Python Bug Day. It worked to attract another curious developer. :-) This project is really awesome. Congratz you all. 

My best regards.
msg122231 - (view) Author: Savio Sena (saviosena) Date: 2010-11-23 18:47
Attaching a more concise patch, as requested by georg.brandl.
msg122232 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2010-11-23 18:55
I think data_size_limit and command_size_limit should be class attributes instead of instance attributes.
msg122235 - (view) Author: Savio Sena (saviosena) Date: 2010-11-23 19:15
Previous patch was incorrect. I'm attaching another one, I'm really sorry.

@giampaolo, about making the limits class attributes, it's not a good idea IMHO. According to RFC1869 command sizes can change depending on which Service Extensions are supported.
msg122239 - (view) Author: Savio Sena (saviosena) Date: 2010-11-23 19:37
size_limits are not class attributes instead of instance attributes, as suggested by giampaolo.rodola.
msg122241 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2010-11-23 20:10
AFAICT patch looks ok to me.
msg123195 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2010-12-03 07:38
Committed in r86955.  Thanks!
History
Date User Action Args
2010-12-03 07:38:35georg.brandlsetstatus: open -> closed
resolution: fixed
messages: + msg123195
2010-11-23 20:53:54georg.brandlsetnosy: + georg.brandl
2010-11-23 20:10:29giampaolo.rodolasetmessages: + msg122241
2010-11-23 19:37:10saviosenasetfiles: + issue1745035-101123-saviosena.diff

messages: + msg122239
2010-11-23 19:15:45saviosenasetfiles: + issue1745035-101123-saviosena.diff

messages: + msg122235
2010-11-23 18:55:59giampaolo.rodolasetmessages: + msg122232
2010-11-23 18:47:57saviosenasetfiles: + issue1745035-101123-saviosena.diff

messages: + msg122231
2010-11-22 02:11:47saviosenasetfiles: + issue1745035-saviosena-101121.diff

messages: + msg122064
2010-11-21 15:16:25henriquebastossetnosy: + henriquebastos
2010-11-21 07:22:22saviosenasetmessages: + msg121874
2010-11-21 02:39:08saviosenasetnosy: + saviosena
2010-09-17 18:02:34BreamoreBoysetnosy: + BreamoreBoy
messages: + msg116697
2010-05-11 20:40:19terry.reedysetversions: + Python 3.1, Python 2.7, Python 3.2, - Python 3.0
2008-09-30 13:12:54giampaolo.rodolasetmessages: + msg74073
2008-09-29 20:53:42josiahcarlsonsetassignee: barry -> josiahcarlson
messages: + msg74047
nosy: + josiahcarlson
2008-02-11 15:25:54giampaolo.rodolasetversions: + Python 2.6, Python 3.0
2008-01-30 15:40:24jceasetnosy: + jcea
2008-01-23 19:13:18giampaolo.rodolasetfiles: + smtpd.diff
messages: + msg61599
versions: + Python 2.5
2007-10-21 19:18:13giampaolo.rodolasetfiles: + smtpd.diff
2007-10-21 19:17:47giampaolo.rodolasetfiles: + smtpd.py
messages: + msg56627
type: security
severity: normal -> urgent
2007-09-19 00:49:10jafosetkeywords: + patch
assignee: barry
messages: + msg56019
nosy: + barry, jafo
2007-06-28 19:44:15giampaolo.rodolacreate