Skip to content
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.

Sign up for GitHub

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

Jump to bottom

server: DATA command timeout should close the connection #196

Open
kayrus opened this issue Aug 16, 2022 · 3 comments
Open

server: DATA command timeout should close the connection #196

kayrus opened this issue Aug 16, 2022 · 3 comments
Labels
server

Comments

@kayrus
Copy link
Contributor

kayrus commented Aug 16, 2022

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
Copy link
Contributor Author

kayrus commented Aug 17, 2022

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

@kayrus kayrus mentioned this issue Aug 17, 2022
Closed
@emersion
Copy link
Owner

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
Copy link
Owner

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
server
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Properly handle DATA read timeouts kayrus/go-smtp
2 participants
@emersion @kayrus