if the first smtp or imap command is invalid, shut down the connection instead of trying to read more

this is quite common on the internet. the other side may be trying some other
protocol, e.g. http, or some common vulnerability. we don't want to spam our
own logs with multiple invalid lines. if the first command is valid, but later
are not, we'll keep trying to process them. so this only affects protocol
sessions that are very likely not smtp/imap.

also remove a few more sleeps during tests, making imapserver and smtpserver tests a bit faster.
This commit is contained in:
Mechiel Lukkien
2023-03-10 10:23:43 +01:00
parent 2c07645ab4
commit e413c906b1
5 changed files with 99 additions and 19 deletions

View File

@ -5,6 +5,7 @@ import (
"encoding/base64"
"errors"
"fmt"
"io"
"net"
"os"
"testing"
@ -106,7 +107,7 @@ func FuzzServer(f *testing.F) {
return
}
err, ok := x.(error)
if !ok || !errors.Is(err, os.ErrDeadlineExceeded) {
if !ok || (!errors.Is(err, os.ErrDeadlineExceeded) && !errors.Is(err, io.EOF)) {
panic(x)
}
}()

View File

@ -130,9 +130,9 @@ func limitersInit() {
}
}
// Delay before reads and after 1-byte writes for probably spammers. Tests set this
// to zero.
var badClientDelay = time.Second
// Delay after bad/suspicious behaviour. Tests set these to zero.
var badClientDelay = time.Second // Before reads and after 1-byte writes for probably spammers.
var authFailDelay = time.Second // After authentication failure.
// Capabilities (extensions) the server supports. Connections will add a few more, e.g. STARTTLS, LOGINDISABLED, AUTH=PLAIN.
// ENABLE: ../rfc/5161
@ -175,6 +175,7 @@ type conn struct {
cmd string // Currently executing, for deciding to applyChanges and logging.
cmdMetric string // Currently executing, for metrics.
cmdStart time.Time
ncmds int // Number of commands processed. Used to abort connection when first incoming command is unknown/invalid.
log *mlog.Log
enabled map[capability]bool // All upper-case.
@ -666,7 +667,7 @@ func serve(listenerName string, cid int64, tlsConfig *tls.Config, nc net.Conn, x
case <-mox.Shutdown.Done():
// ../rfc/9051:5381
c.writelinef("* BYE mox shutting down")
panic(errIO)
return
default:
}
@ -752,6 +753,13 @@ func (c *conn) command() {
var serr serverError
if errors.As(err, &sxerr) {
result = "badsyntax"
if c.ncmds == 0 {
// Other side is likely speaking something else than IMAP, send error message and
// stop processing because there is a good chance whatever they sent has multiple
// lines.
c.writelinef("* BYE please try again speaking imap")
panic(errIO)
}
c.log.Debugx("imap command syntax error", err, logFields...)
c.log.Info("imap syntax error", mlog.Field("lastline", c.lastLine))
fatal := strings.HasSuffix(c.lastLine, "+}")
@ -806,6 +814,7 @@ func (c *conn) command() {
xsyntaxErrorf("unknown command %q", cmd)
}
c.cmdMetric = c.cmd
c.ncmds++
// Check if command is allowed in this state.
if _, ok1 := commandsStateAny[cmdlow]; ok1 {
@ -1425,8 +1434,8 @@ func (c *conn) cmdAuthenticate(tag, cmd string, p *parser) {
// Examples: ../rfc/9051:1520 ../rfc/3501:1631
// For many failed auth attempts, slow down verification attempts.
if c.authFailed > 3 {
mox.Sleep(mox.Context, time.Duration(c.authFailed-3)*time.Second)
if c.authFailed > 3 && authFailDelay > 0 {
mox.Sleep(mox.Context, time.Duration(c.authFailed-3)*authFailDelay)
}
c.authFailed++ // Compensated on success.
defer func() {
@ -1709,8 +1718,8 @@ func (c *conn) cmdLogin(tag, cmd string, p *parser) {
}
// For many failed auth attempts, slow down verification attempts.
if c.authFailed > 3 {
mox.Sleep(mox.Context, time.Duration(c.authFailed-3)*time.Second)
if c.authFailed > 3 && authFailDelay > 0 {
mox.Sleep(mox.Context, time.Duration(c.authFailed-3)*authFailDelay)
}
c.authFailed++ // Compensated on success.
defer func() {

View File

@ -26,6 +26,7 @@ func init() {
// Don't slow down tests.
badClientDelay = 0
authFailDelay = 0
}
func tocrlf(s string) string {
@ -397,8 +398,6 @@ func TestLogin(t *testing.T) {
func TestState(t *testing.T) {
tc := start(t)
tc.transactf("bad", "boguscommand")
notAuthenticated := []string{"starttls", "authenticate", "login"}
authenticatedOrSelected := []string{"enable", "select", "examine", "create", "delete", "rename", "subscribe", "unsubscribe", "list", "namespace", "status", "append", "idle", "lsub"}
selected := []string{"close", "unselect", "expunge", "search", "fetch", "store", "copy", "move", "uid expunge"}
@ -421,6 +420,21 @@ func TestState(t *testing.T) {
for _, cmd := range append(append([]string{}, notAuthenticated...), selected...) {
tc.transactf("no", "%s", cmd)
}
tc.transactf("bad", "boguscommand")
}
func TestNonIMAP(t *testing.T) {
tc := start(t)
defer tc.close()
// imap greeting has already been read, we sidestep the imapclient.
_, err := fmt.Fprintf(tc.conn, "bogus\r\n")
tc.check(err, "write bogus command")
tc.readprefixline("* BYE ")
if _, err := tc.conn.Read(make([]byte, 1)); err == nil {
t.Fatalf("connection not closed after initial bad command")
}
}
func TestLiterals(t *testing.T) {