Repository: emersion/go-smtp
Pull request #80 (2019-12-07 11:14:21)

client: Restrict the line length for responses
Intended to protect MTA implementations from malicious recipient servers.

I am not sure about the length limit value though, for Server it is
configurable via the MaxLineLength field, for Client I do not see a context
where it can be set. Or perhaps it is not really needed?

codecov[bot]: # [Codecov](https://codecov.io/gh/emersion/go-smtp/pull/80?src=pr&el=h1) Report
> Merging [#80](https://codecov.io/gh/emersion/go-smtp/pull/80?src=pr&el=desc) into [master](https://codecov.io/gh/emersion/go-smtp/commit/1f576e0ec85cbca64789f0abe989ad4f1718d145?src=pr&el=desc) will **increase** coverage by `0.88%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/emersion/go-smtp/pull/80/graphs/tree.svg?width=650&token=vj7HO8ABwC&height=150&src=pr)](https://codecov.io/gh/emersion/go-smtp/pull/80?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master      #80      +/-   ##
==========================================
+ Coverage   62.61%   63.49%   +0.88%     
==========================================
  Files           8        8              
  Lines         904      915      +11     
==========================================
+ Hits          566      581      +15     
+ Misses        250      248       -2     
+ Partials       88       86       -2
```


| [Impacted Files](https://codecov.io/gh/emersion/go-smtp/pull/80?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [client.go](https://codecov.io/gh/emersion/go-smtp/pull/80/diff?src=pr&el=tree#diff-Y2xpZW50Lmdv) | `66.44% <100%> (+2.7%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/emersion/go-smtp/pull/80?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/emersion/go-smtp/pull/80?src=pr&el=footer). Last update [1f576e0...837a778](https://codecov.io/gh/emersion/go-smtp/pull/80?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

emersion: The RFC says the maximum length for replies is 512 bytes (https://tools.ietf.org/html/rfc5321#section-4.5.3.1.5). I'm fine with hardcoding this, at least for now.
emersion: Note, I just realized `textproto.Conn` has a `Reader` field, which has a `bufio.Reader` field, which allows us to set the maximum buffer size.
foxcpp: >bufio.Reader field, which allows us to set the maximum buffer size.

Does it? The only 'size' I see is in NewReaderSize which sets *minimal* buffer size.

Additionally, textproto.Conn.ReadLine* joins results of multiple R.Read calls so it is not limited by the bufio.Reader buffer (see readLineSlice implementation in net/textproto/reader.go)
emersion: >Does it? The only 'size' I see is in NewReaderSize which sets minimal buffer size.

Oh, I'm mistaken, I was thinking about `bufio.Scanner` instead.

Patch:
diff --git a/client.go b/client.go
index ab421f0..9a7accf 100644
--- a/client.go
+++ b/client.go
@@ -65,7 +65,21 @@ func DialTLS(addr string, tlsConfig *tls.Config) (*Client, error) {
 // NewClient returns a new Client using an existing connection and host as a
 // server name to be used when authenticating.
 func NewClient(conn net.Conn, host string) (*Client, error) {
-	text := textproto.NewConn(conn)
+	rwc := struct {
+		io.Reader
+		io.Writer
+		io.Closer
+	}{
+		Reader: lineLimitReader{
+			R: conn,
+			// Doubled maximum line length per RFC 5321 (Section 4.5.3.1.6)
+			LineLimit: 2000,
+		},
+		Writer: conn,
+		Closer: conn,
+	}
+
+	text := textproto.NewConn(rwc)
 	_, _, err := text.ReadResponse(220)
 	if err != nil {
 		text.Close()


Test patch:
diff --git a/client_test.go b/client_test.go
index 315c41c..8734e76 100644
--- a/client_test.go
+++ b/client_test.go
@@ -221,6 +221,52 @@ func TestBasic_SMTPError(t *testing.T) {
 	}
 }
 
+func TestClient_TooLongLine(t *testing.T) {
+	faultyServer := []string{
+		"220 mx.google.com at your service\r\n",
+		"220 mx.google.com at your service\r\n",
+		"500 5.0.0 nU6XC5JJUfiuIkC7NhrxZz36Rl/rXpkfx9QdeZJ+rno6W5J9k9HvniyWXBBi1gOZ/CUXEI6K7Uony70eiVGGGkdFhP1rEvMGny1dqIRo3NM2NifrvvLIKGeX6HrYmkc7NMn9BwHyAnt5oLe5eNVDI+grwIikVPNVFZi0Dg4Xatdg5Cs8rH1x9BWhqyDoxosJst4wRoX4AymYygUcftM3y16nVg/qcb1GJwxSNbah7VjOiSrk6MlTdGR/2AwIIcSw7pZVJjGbCorniOTvKBcyut1YdbrX/4a/dBhvLfZtdSccqyMZAdZno+tGrnu+N2ghFvz6cx6bBab9Z4JJQMlkK/g1y7xjEPr6nKwruAf71NzOclPK5wzs2hY3Ku9xEjU0Cd+g/OjAzVsmeJk2U0q+vmACZsFAiOlRynXKFPLqMAg8skM5lioRTm05K/u3aBaUq0RKloeBHZ/zNp/kfHNp6TmJKAzvsXD3Xdo+PRAgCZRTRAl3ydGdrOOjxTULCVlgOL6xSAJdj9zGkzQoEW4tRmp1OiIab4GSxCtkIo7XnAowJ7EPUfDGTV3hhl5Qn7jvZjPCPlruRTtzVTho7D3HBEouWv1qDsqdED23myw0Ma9ZlobSf9eHqsSv1MxjKG2D5DdFBACu6pXGz3ceGreOHYWnI74TkoHtQ5oNuF6VUkGjGN+f4fOaiypQ54GJ8skTNoSCHLK4XF8ZutSxWzMR+LKoJBWMb6bdAiFNt+vXZOUiTgmTqs6Sw79JXqDX9YFxryJMKjHMiFkm+RZbaK5sIOXqyq+RNmOJ+G0unrQHQMCES476c7uvOlYrNoJtq+uox1qFdisIE/8vfSoKBlTtw+r2m87djIQh4ip/hVmalvtiF5fnVTxigbtwLWv8rAOCXKoktU0c2ie0a5hGtvZT0SXxwX8K2CeYXb81AFD2IaLt/p8Q4WuZ82eOCeXP72qP9yWYj6mIZdgyimm8wjrDowt2yPJU28ZD6k3Ei6C31OKgMpCf8+MW504/VCwld7czAIwjJiZe3DxtUdfM7Q565OzLiWQgI8fxjsvlCKMiOY7q42IGGsVxXJAFMtDKdchgqQA1PJR1vrw+SbI3Mh4AGnn8vKn+WTsieB3qkloo7MZlpMz/bwPXg7XadOVkUaVeHrZ5OsqDWhsWOLtPZLi5XdNazPzn9uxWbpelXEBKAjZzfoawSUgGT5vCYACNfz/yIw1DB067N+HN1KvVddI6TNBA32lpqkQ6VwdWztq6pREE51sNl9p7MUzr+ef0331N5DqQsy+epmRDwebosCx15l/rpvBc91OnxmMMXDNtmxSzVxaZjyGDmJ7RDdTy/Su76AlaMP1zxivxg2MU/9zyTzM16coIAMOd/6Uo9ezKgbZEPeMROKTzAld9BhK9BBPWofoQ0mBkVc7btnahQe3u8HoD6SKCkr9xcTcC9ZKpLkc4svrmxT9e0858pjhis9BbWD/owa6552n2+KwUMRyB8ys7rPL86hh9lBTS+05cVL+BmJfNHOA6ZizdGc3lpwIVbFmzMR5BM0HRf3OCntkWojgsdsP8BGZWHiCGGqA7YGa5AOleR887r8Zhyp47DT3Cn3Rg/icYurIx7Yh0p696gxfANo4jEkE2BOroIscDnhauwck5CCJMcabpTrGwzK8NJ+xZnCUplXnZiIaj85Uh9+yI670B4bybWlZoVmALUxxuQ8bSMAp7CAzMcMWbYJHwBqLF8V2qMj3/g81S3KOptn8b7Idh7IMzAkV8VxE3qAguzwS0zEu8l894sOFUPiJq2/llFeiHNOcEQUGJ+8ATJSAFOMDXAeQS2FoIDOYdesO6yacL0zUkvDydWbA84VXHW8DvdHPli/8hmc++dn5CXSDeBJfC/yypvrpLgkSilZMuHEYHEYHEYEHYEHEYEHEYEHEYEYEYEYEYEYEYEYEYEYEYEYEYEYEYEYEYEYEYYEYEYEYEYEYEYEYYEYEYEYEYEYEYEYEY\r\n",
+		"220 2.0.0 Kk\r\n",
+	}
+
+	// The pipe is used to avoid bufio.Reader reading the too long line ahead
+	// of time (in NewClient) and failing eariler than we expect.
+	pr, pw := io.Pipe()
+
+	go func() {
+		for _, l := range faultyServer {
+			pw.Write([]byte(l))
+		}
+		pw.Close()
+	}()
+
+	var wrote bytes.Buffer
+	var fake faker
+	fake.ReadWriter = struct {
+		io.Reader
+		io.Writer
+	}{
+		pr,
+		&wrote,
+	}
+	c, err := NewClient(fake, "fake.host")
+	if err != nil {
+		t.Fatalf("NewClient failed: %v", err)
+	}
+
+	err = c.Mail("whatever", nil)
+	if err != ErrTooLongLine {
+		t.Fatal("MAIL succeded or returned a different error:", err)
+	}
+
+	// ErrTooLongLine is "sticky" since the connection is in broken state and
+	// the only reasonable way to recover is to close it.
+	err = c.Mail("whatever", nil)
+	if err != ErrTooLongLine {
+		t.Fatal("Second MAIL succeded or returned a different error:", err)
+	}
+}
+
 var basicServer = `250 mx.google.com at your service
 502 Unrecognized command.
 250-mx.google.com at your service