Browse Source

Fix crashes found during fuzz testing (#70)

fixes issue #69 - crash on "DATA\r\n"

This one fixes a crash when the first input is "DATA\r\n"
The fix was to change envelope.EmailAddress to be a value rather than a pointer.
The reason is that the struct is small and consists of slices anyway, so not much is saved when passing by pointer. Also, it doesn't not mutate after being passed.
Flashmob 8 years ago
parent
commit
4cf37a3eca
7 changed files with 141 additions and 9 deletions
  1. 1 1
      backends/backend.go
  2. 1 1
      backends/guerrilla_db_redis.go
  3. 2 2
      client.go
  4. 1 1
      envelope/envelope.go
  5. 2 2
      server.go
  6. 132 0
      tests/guerrilla_test.go
  7. 2 2
      util.go

+ 1 - 1
backends/backend.go

@@ -145,7 +145,7 @@ func (gw *BackendGateway) Process(e *envelope.Envelope) BackendResult {
 	// place on the channel so that one of the save mail workers can pick it up
 	// place on the channel so that one of the save mail workers can pick it up
 	// TODO: support multiple recipients
 	// TODO: support multiple recipients
 	savedNotify := make(chan *saveStatus)
 	savedNotify := make(chan *saveStatus)
-	gw.saveMailChan <- &savePayload{e, from, &to[0], savedNotify}
+	gw.saveMailChan <- &savePayload{e, &from, &to[0], savedNotify}
 	// wait for the save to complete
 	// wait for the save to complete
 	// or timeout
 	// or timeout
 	select {
 	select {

+ 1 - 1
backends/guerrilla_db_redis.go

@@ -19,7 +19,7 @@ package backends
 // - Changed the MySQL queries to insert in batch
 // - Changed the MySQL queries to insert in batch
 // - Made a Compressor that recycles buffers using sync.Pool
 // - Made a Compressor that recycles buffers using sync.Pool
 // The result was around 400% speed improvement. If you know of any more improvements, please share!
 // The result was around 400% speed improvement. If you know of any more improvements, please share!
-// - Added the recovery mechanisi,
+// - Added the recovery mechanism,
 
 
 import (
 import (
 	"fmt"
 	"fmt"

+ 2 - 2
client.go

@@ -112,7 +112,7 @@ func (c *client) sendResponse(r ...interface{}) {
 // -End of DATA command
 // -End of DATA command
 // TLS handhsake
 // TLS handhsake
 func (c *client) resetTransaction() {
 func (c *client) resetTransaction() {
-	c.MailFrom = &envelope.EmailAddress{}
+	c.MailFrom = envelope.EmailAddress{}
 	c.RcptTo = []envelope.EmailAddress{}
 	c.RcptTo = []envelope.EmailAddress{}
 	c.Data.Reset()
 	c.Data.Reset()
 	c.Subject = ""
 	c.Subject = ""
@@ -123,7 +123,7 @@ func (c *client) resetTransaction() {
 // A transaction starts after a MAIL command gets issued by the client.
 // A transaction starts after a MAIL command gets issued by the client.
 // Call resetTransaction to end the transaction
 // Call resetTransaction to end the transaction
 func (c *client) isInTransaction() bool {
 func (c *client) isInTransaction() bool {
-	isMailFromEmpty := (c.MailFrom == nil || *c.MailFrom == (envelope.EmailAddress{}))
+	isMailFromEmpty := c.MailFrom == (envelope.EmailAddress{})
 	if isMailFromEmpty {
 	if isMailFromEmpty {
 		return false
 		return false
 	}
 	}

+ 1 - 1
envelope/envelope.go

@@ -35,7 +35,7 @@ type Envelope struct {
 	// Message sent in EHLO command
 	// Message sent in EHLO command
 	Helo string
 	Helo string
 	// Sender
 	// Sender
-	MailFrom *EmailAddress
+	MailFrom EmailAddress
 	// Recipients
 	// Recipients
 	RcptTo []EmailAddress
 	RcptTo []EmailAddress
 	// Data stores the header and message body
 	// Data stores the header and message body

+ 2 - 2
server.go

@@ -382,7 +382,7 @@ func (server *server) handleClient(client *client) {
 					break
 					break
 				}
 				}
 				mail := input[10:]
 				mail := input[10:]
-				from := &envelope.EmailAddress{}
+				from := envelope.EmailAddress{}
 
 
 				if !(strings.Index(mail, "<>") == 0) &&
 				if !(strings.Index(mail, "<>") == 0) &&
 					!(strings.Index(mail, " <>") == 0) {
 					!(strings.Index(mail, " <>") == 0) {
@@ -409,7 +409,7 @@ func (server *server) handleClient(client *client) {
 					if !server.allowsHost(to.Host) {
 					if !server.allowsHost(to.Host) {
 						client.sendResponse(response.Canned.ErrorRelayDenied, to.Host)
 						client.sendResponse(response.Canned.ErrorRelayDenied, to.Host)
 					} else {
 					} else {
-						client.RcptTo = append(client.RcptTo, *to)
+						client.RcptTo = append(client.RcptTo, to)
 						client.sendResponse(response.Canned.SuccessRcptCmd)
 						client.sendResponse(response.Canned.SuccessRcptCmd)
 					}
 					}
 				}
 				}

+ 132 - 0
tests/guerrilla_test.go

@@ -1114,3 +1114,135 @@ func TestDataCommand(t *testing.T) {
 	// don't forget to reset
 	// don't forget to reset
 	os.Truncate("./testlog", 0)
 	os.Truncate("./testlog", 0)
 }
 }
+
+// Fuzzer crashed the server by submitting "DATA\r\n" as the first command
+func TestFuzz86f25b86b09897aed8f6c2aa5b5ee1557358a6de(t *testing.T) {
+	if initErr != nil {
+		t.Error(initErr)
+		t.FailNow()
+	}
+
+	if startErrors := app.Start(); startErrors == nil {
+		conn, bufin, err := Connect(config.Servers[0], 20)
+		if err != nil {
+			// handle error
+			t.Error(err.Error(), config.Servers[0].ListenInterface)
+			t.FailNow()
+		} else {
+
+			response, err := Command(
+				conn,
+				bufin,
+				"DATA\r\n")
+			expected := "503 5.5.1 Error: No sender"
+			if strings.Index(response, expected) != 0 {
+				t.Error("Server did not respond with", expected, ", it said:"+response, err)
+			}
+
+		}
+		conn.Close()
+		app.Shutdown()
+	} else {
+		if startErrors := app.Start(); startErrors != nil {
+			t.Error(startErrors)
+			app.Shutdown()
+			t.FailNow()
+		}
+	}
+	// don't forget to reset
+	os.Truncate("./testlog", 0)
+}
+
+// Appears to hang the fuzz test, but not server.
+func TestFuzz21c56f89989d19c3bbbd81b288b2dae9e6dd2150(t *testing.T) {
+	if initErr != nil {
+		t.Error(initErr)
+		t.FailNow()
+	}
+	str := "X_\r\nMAIL FROM:<u\xfd\xfdrU" +
+		"\x10c22695140\xfd727235530" +
+		" Walter Sobchak\x1a\tDon" +
+		"ny, x_6_, Donnyre   " +
+		"\t\t outof89 !om>\r\nMAI" +
+		"L\t\t \t\tFROM:<C4o\xfd\xfdr@e" +
+		"xample.c22695140\xfd727" +
+		"235530 Walter Sobcha" +
+		"k: Donny, you>re out" +
+		" of your element!om>" +
+		"\r\nMAIL RCPT TO:t@IRS" +
+		"ETRCPTIRSETRCP:<\x00\xfd\xfdr" +
+		"@example 7A924_F__4_" +
+		"c22695140\xfd-061.0x30C" +
+		"8bC87fE4d3 Walter MA" +
+		"IL Donny, youiq__n_l" +
+		"wR8qs_0RBcw_0hIY_pS_" +
+		"___x9_E0___sL598_G82" +
+		"_6 out   your elemen" +
+		"t!>\r\nX _9KB___X_p:<o" +
+		"ut\xfd\xfdr@example9gTnr2N" +
+		"__Vl_T7U_AqfU_dPfJ_0" +
+		"HIqKK0037f6W_KGM_y_Z" +
+		"_9_96_w_815Q572py2_9" +
+		"F\xfd727235530Walter\tSo" +
+		"bchakRSET MAIL from:" +
+		" : cows eat\t\t  grass" +
+		" , _S___46_PbG03_iW'" +
+		"__v5L2_2L_J61u_38J55" +
+		"_PpwQ_Fs_7L_3p7S_t__" +
+		"g9XP48T_9HY_EDl_c_C3" +
+		"3_3b708EreT_OR out 9" +
+		"9_pUY4 \t\t\t     \x05om>\r" +
+		"\n FROM<u\xfd\xfdr@example." +
+		"<\xfd-05110602 Walter S" +
+		"obchak: Donny, \t\t  w" +
+		"50TI__m_5EsC___n_l_d" +
+		"__57GP9G02_32n_FR_xw" +
+		"_2_103___rnED5PGIKN7" +
+		"BBs3VIuNV_514qDBp_Gs" +
+		"_qj4\tre out all cows" +
+		" eatof your element\x03" +
+		"om>\r\n_2 FROM:<u\x10\xfdr@e" +
+		"xample.oQ_VLq909_E_5" +
+		"AQ7_4_\xfd1935012674150" +
+		"6773818422493001838." +
+		"-010\tWalter\tSobchak:" +
+		" Donny, youyouteIz2y" +
+		"__Z2q5_qoA're Q6MP2_" +
+		"CT_z70____0c0nU7_83d" +
+		"4jn_eFD7h_9MbPjr_s_L" +
+		"9_X23G_7 of _kU_L9Yz" +
+		"_K52345QVa902H1__Hj_" +
+		"Nl_PP2tW2ODi0_V80F15" +
+		"_i65i_V5uSQdiG eleme" +
+		"nt!om>\r\n"
+
+	if startErrors := app.Start(); startErrors == nil {
+		conn, bufin, err := Connect(config.Servers[0], 20)
+		if err != nil {
+			// handle error
+			t.Error(err.Error(), config.Servers[0].ListenInterface)
+			t.FailNow()
+		} else {
+
+			response, err := Command(
+				conn,
+				bufin,
+				str)
+			expected := "554 5.5.1 Unrecognized command"
+			if strings.Index(response, expected) != 0 {
+				t.Error("Server did not respond with", expected, ", it said:"+response, err)
+			}
+
+		}
+		conn.Close()
+		app.Shutdown()
+	} else {
+		if startErrors := app.Start(); startErrors != nil {
+			t.Error(startErrors)
+			app.Shutdown()
+			t.FailNow()
+		}
+	}
+	// don't forget to reset
+	os.Truncate("./testlog", 0)
+}

+ 2 - 2
util.go

@@ -11,8 +11,8 @@ import (
 
 
 var extractEmailRegex, _ = regexp.Compile(`<(.+?)@(.+?)>`) // go home regex, you're drunk!
 var extractEmailRegex, _ = regexp.Compile(`<(.+?)@(.+?)>`) // go home regex, you're drunk!
 
 
-func extractEmail(str string) (*envelope.EmailAddress, error) {
-	email := &envelope.EmailAddress{}
+func extractEmail(str string) (envelope.EmailAddress, error) {
+	email := envelope.EmailAddress{}
 	var err error
 	var err error
 	if len(str) > RFC2821LimitPath {
 	if len(str) > RFC2821LimitPath {
 		return email, errors.New(response.Canned.FailPathTooLong)
 		return email, errors.New(response.Canned.FailPathTooLong)