server: DATA command timeout should close the connection

[kayrus]

trafficstars

Basically go-smtp issues two timeouts, configured by ReadTimeout: for DATA command, for the idle connection:

354 2.0.0 Go ahead. End your data with <CR><LF>.<CR><LF>

# doing nothing in DATA command

421 4.0.0 read tcp 127.0.0.1:1025->127.0.0.1:56290: i/o timeout # timeout triggered after X time
221 2.4.2 Idle timeout, bye bye # another timeout triggered after another X time
# TCP connection is closed

For instance AWS SES closes the connection on data timeout:

354 End data with <CR><LF>.<CR><LF>

# doing nothing in DATA command

451 4.4.2 Timeout waiting for data from client.
# TCP connection is closed

See RFC5321 451 Requested action aborted: error in processing.

Not closing the connection on DATA timeout causes a slow postfix client talking to go-smtp server to send the data even, when the go-smtp server responds with 421 4.0.0 and the DATA's leftover data is considered as a mangled SMTP command in go-smtp server.

I suggest to catch the DATA timeout exception and close the connection, e.g.

--- a/conn.go
+++ b/conn.go
@@ -673,18 +673,25 @@ func (c *Conn) handleData(arg string) {
 	// We have recipients, go to accept data
 	c.WriteResponse(354, EnhancedCode{2, 0, 0}, "Go ahead. End your data with <CR><LF>.<CR><LF>")
 
-	defer c.reset()
-
 	if c.server.LMTP {
 		c.handleDataLMTP()
+		c.reset()
 		return
 	}
 
 	r := newDataReader(c)
-	code, enhancedCode, msg := toSMTPStatus(c.Session().Data(r))
+	err := c.Session().Data(r)
+	code, enhancedCode, msg := toSMTPStatus(err)
 	r.limited = false
 	io.Copy(ioutil.Discard, r) // Make sure all the data has been consumed
 	c.WriteResponse(code, enhancedCode, msg)
+
+	c.reset()
+
+	// close a connection on data command timeout
+	if err == ErrDataTimeout {
+		c.Close()
+	}
 }
 
 func (c *Conn) handleBdat(arg string) {
--- a/data.go
+++ b/data.go
@@ -3,6 +3,7 @@ package smtp
 import (
 	"bufio"
 	"io"
+	"net"
 )
 
 type EnhancedCode [3]int
@@ -42,6 +43,12 @@ var ErrDataTooLarge = &SMTPError{
 	Message:      "Maximum message size exceeded",
 }
 
+var ErrDataTimeout = &SMTPError{
+	Code:         451,
+	EnhancedCode: EnhancedCode{4, 4, 2},
+	Message:      "Timeout waiting for data from client",
+}
+
 type dataReader struct {
 	r     *bufio.Reader
 	state int
@@ -93,6 +100,9 @@ func (r *dataReader) Read(b []byte) (n int, err error) {
 			if err == io.EOF {
 				err = io.ErrUnexpectedEOF
 			}
+			if e, ok := err.(net.Error); ok && e.Timeout() {
+				err = ErrDataTimeout
+			}
 			break
 		}
 		switch r.state {

Ideally it would be better to preserve the original err inside the SMTPError struct, e.g. implement some kind of err wrapping like it's done with https://go.dev/blog/go1.13-errors#errors-in-go-113

[kayrus]

I also noticed that DATA timeout is implemented incorrectly. While client sends email data, the deadline isn't updated. For instance the read timeout is set to 5 secs. If an SMTP client cannot finish the data within 5 seconds, the timeout is triggered. However timeout should trigger only when client stopped sending the data within 5 secs.

In order to fix this particular issue I suggest the fix below:

--- a/data.go
+++ b/data.go
@@ -1,8 +1,8 @@
 package smtp
 
 import (
-	"bufio"
 	"io"
+	"time"
 )
 
 type EnhancedCode [3]int
@@ -43,7 +43,7 @@ var ErrDataTooLarge = &SMTPError{
 }
 
 type dataReader struct {
-	r     *bufio.Reader
+	c     *Conn
 	state int
 
 	limited bool
@@ -52,7 +52,7 @@ type dataReader struct {
 
 func newDataReader(c *Conn) *dataReader {
 	dr := &dataReader{
-		r: c.text.R,
+		c: c,
 	}
 
 	if c.server.MaxMessageBytes > 0 {
@@ -87,8 +87,14 @@ func (r *dataReader) Read(b []byte) (n int, err error) {
 		stateEOF              // reached .\r\n end marker line
 	)
 	for n < len(b) && r.state != stateEOF {
+		if r.c.server.ReadTimeout != 0 {
+			err = r.c.conn.SetReadDeadline(time.Now().Add(r.c.server.ReadTimeout))
+			if err != nil {
+				break
+			}
+		}
 		var c byte
-		c, err = r.r.ReadByte()
+		c, err = r.c.text.R.ReadByte()
 		if err != nil {
 			if err == io.EOF {
 				err = io.ErrUnexpectedEOF

[emersion]

While client sends email data, the deadline isn't updated. For instance the read timeout is set to 5 secs. If an SMTP client cannot finish the data within 5 seconds, the timeout is triggered.

This is by design. ReadTimeout applies to the handling of a whole SMTP command. This prevents clients from sending data very slowly, e.g. 1 byte at a time.

[emersion]

Ideally the server would be improved to have two options, one for regular command timeouts, the other for the DATA command timeout, like the client has.