From 985f2702f256ef29b61a4708f93b23efe5354345 Mon Sep 17 00:00:00 2001
From: James Hillyerd <james@hillyerd.com>
Date: Sun, 11 Jul 2021 12:00:28 -0700
Subject: [PATCH] Fix command line length bug (#221)

* handler: Don't fail on 8 character command lines

Fixes #214

* handler: Test that STARTTLS is parsed correctly.
---
 pkg/server/smtp/handler.go      | 37 ++++++++++++++++-----------------
 pkg/server/smtp/handler_test.go | 15 +++++++++++++
 2 files changed, 33 insertions(+), 19 deletions(-)

diff --git a/pkg/server/smtp/handler.go b/pkg/server/smtp/handler.go
index 5171be0..e39a589 100644
--- a/pkg/server/smtp/handler.go
+++ b/pkg/server/smtp/handler.go
@@ -243,7 +243,7 @@ func (s *Server) startSession(id int, conn net.Conn) {
 				}
 				break
 			}
-			// not an EOF
+			// Not an EOF
 			ssn.logger.Warn().Msgf("Connection error: %v", err)
 			if netErr, ok := err.(net.Error); ok {
 				if netErr.Timeout() {
@@ -281,7 +281,7 @@ func (s *Session) greetHandler(cmd string, arg string) {
 			return
 		}
 		s.remoteDomain = domain
-		// features before SIZE per RFC
+		// Features before SIZE per RFC
 		s.send("250-" + readyBanner)
 		s.send("250-8BITMIME")
 		s.send("250-AUTH PLAIN LOGIN")
@@ -331,20 +331,21 @@ func (s *Session) passwordHandler(line string) {
 func (s *Session) readyHandler(cmd string, arg string) {
 	if cmd == "STARTTLS" {
 		if !s.Server.config.TLSEnabled {
-			// invalid command since unconfigured
+			// Invalid command since TLS unconfigured.
 			s.logger.Debug().Msgf("454 TLS unavailable on the server")
 			s.send("454 TLS unavailable on the server")
 			return
 		}
 		if s.tlsState != nil {
-			// tls state previously valid
+			// TLS state previously valid.
 			s.logger.Debug().Msg("454 A TLS session already agreed upon.")
 			s.send("454 A TLS session already agreed upon.")
 			return
 		}
 		s.logger.Debug().Msg("Initiating TLS context.")
+
+		// Start TLS connection handshake.
 		s.send("220 STARTTLS")
-		// start tls connection handshake
 		tlsConn := tls.Server(s.conn, s.Server.tlsConfig)
 		s.conn = tlsConn
 		s.text = textproto.NewConn(s.conn)
@@ -591,30 +592,28 @@ func (s *Session) readLine() (line string, err error) {
 
 func (s *Session) parseCmd(line string) (cmd string, arg string, ok bool) {
 	line = strings.TrimRight(line, "\r\n")
-	l := len(line)
+
+	// Find length of command or entire line.
+	hasArg := true
+	l := strings.IndexByte(line, ' ')
+	if l == -1 {
+		hasArg = false
+		l = len(line)
+	}
+
 	switch {
 	case l == 0:
 		return "", "", true
 	case l < 4:
 		s.logger.Warn().Msgf("Command too short: %q", line)
 		return "", "", false
-	case l == 4 || l == 8:
-		return strings.ToUpper(line), "", true
-	case l == 5:
-		// Too long to be only command, too short to have args
-		s.logger.Warn().Msgf("Mangled command: %q", line)
-		return "", "", false
 	}
 
-	// If we made it here, command is long enough to have args
-	if line[4] != ' ' {
-		// There wasn't a space after the command?
-		s.logger.Warn().Msgf("Mangled command: %q", line)
-		return "", "", false
+	if hasArg {
+		return strings.ToUpper(line[0:l]), strings.Trim(line[l+1:], " "), true
 	}
 
-	// I'm not sure if we should trim the args or not, but we will for now
-	return strings.ToUpper(line[0:4]), strings.Trim(line[5:], " "), true
+	return strings.ToUpper(line), "", true
 }
 
 // parseArgs takes the arguments proceeding a command and files them
diff --git a/pkg/server/smtp/handler_test.go b/pkg/server/smtp/handler_test.go
index 08393d1..e1eaa79 100644
--- a/pkg/server/smtp/handler_test.go
+++ b/pkg/server/smtp/handler_test.go
@@ -56,6 +56,9 @@ func TestGreetState(t *testing.T) {
 	if err := playSession(t, server, []scriptStep{{"helo 127.0.0.1", 250}}); err != nil {
 		t.Error(err)
 	}
+	if err := playSession(t, server, []scriptStep{{"HELO ABC", 250}}); err != nil {
+		t.Error(err)
+	}
 
 	// Valid EHLOs
 	if err := playSession(t, server, []scriptStep{{"EHLO mydomain", 250}}); err != nil {
@@ -70,6 +73,9 @@ func TestGreetState(t *testing.T) {
 	if err := playSession(t, server, []scriptStep{{"ehlo 127.0.0.1", 250}}); err != nil {
 		t.Error(err)
 	}
+	if err := playSession(t, server, []scriptStep{{"EHLO a", 250}}); err != nil {
+		t.Error(err)
+	}
 
 	if t.Failed() {
 		// Wait for handler to finish logging
@@ -199,6 +205,15 @@ func TestReadyState(t *testing.T) {
 		t.Error(err)
 	}
 
+	// Test Start TLS parsing.
+	script = []scriptStep{
+		{"HELO localhost", 250},
+		{"STARTTLS", 454}, // TLS unconfigured.
+	}
+	if err := playSession(t, server, script); err != nil {
+		t.Error(err)
+	}
+
 	if t.Failed() {
 		// Wait for handler to finish logging
 		time.Sleep(2 * time.Second)