diff --git a/pkg/server/pop3/handler.go b/pkg/server/pop3/handler.go index d83d506..b61d326 100644 --- a/pkg/server/pop3/handler.go +++ b/pkg/server/pop3/handler.go @@ -235,8 +235,11 @@ func (s *Session) authorizationHandler(ctx context.Context, cmd string, args []s s.send("+OK Begin TLS Negotiation") tlsConn := tls.Server(s.conn, s.tlsConfig) if err := tlsConn.HandshakeContext(tlsCtx); err != nil { - s.logger.Error().Msgf("-ERR TLS handshake failed %v", err) - s.ooSeq(cmd) + // RFC 2595 §4: the negotiation must complete before any further + // commands are sent, so a failure is fatal to the session. + s.logger.Error().Msgf("TLS handshake failed: %v", err) + s.enterState(QUIT) + return } s.conn = tlsConn s.reader = bufio.NewReader(tlsConn) @@ -328,6 +331,11 @@ func (s *Session) transactionHandler(cmd string, args []string) { if !ok { return } + if !s.retain[msgNum-1] { + s.logger.Warn().Msgf("Client tried to RETR a message it had deleted") + s.send(fmt.Sprintf("-ERR You deleted message %v", msgNum)) + return + } s.send(fmt.Sprintf("+OK %v bytes follows", s.messages[msgNum-1].Size())) s.sendMessageLines(s.messages[msgNum-1], -1) case "TOP": @@ -353,6 +361,11 @@ func (s *Session) transactionHandler(cmd string, args []string) { s.send("-ERR TOP second argument must be non-negative") return } + if !s.retain[msgNum-1] { + s.logger.Warn().Msgf("Client tried to TOP a message it had deleted") + s.send(fmt.Sprintf("-ERR You deleted message %v", msgNum)) + return + } s.send("+OK Top of message follows") s.sendMessageLines(s.messages[msgNum-1], int(lines)) case "QUIT": diff --git a/pkg/server/pop3/handler_test.go b/pkg/server/pop3/handler_test.go index 704c925..baa17ee 100644 --- a/pkg/server/pop3/handler_test.go +++ b/pkg/server/pop3/handler_test.go @@ -288,6 +288,51 @@ func TestForceTLS(t *testing.T) { } } +// RFC 2595 §4: after the +OK response to STLS, a TLS negotiation begins +// immediately and the client must not issue further commands until it +// completes. A failed negotiation is therefore fatal to the session: the +// server must not keep the session running on the dead connection. +func TestSTLSHandshakeFailure(t *testing.T) { + ds := test.NewStore() + server := setupPOPServer(t, ds, true, false) + pipe := setupPOPSession(t, server) + c := textproto.NewConn(pipe) + defer func() { + _ = c.Close() + server.Drain() + }() + + reply, err := c.ReadLine() + if err != nil { + t.Fatalf("Reading initial line failed %v", err) + } + if !strings.HasPrefix(reply, "+OK") { + t.Fatalf("Initial line is not +OK") + } + + if err := c.PrintfLine("STLS"); err != nil { + t.Fatalf("Failed to send STLS; %v.", err) + } + reply, err = c.ReadLine() + if err != nil { + t.Fatalf("Reading STLS reply line failed %v", err) + } + if !strings.HasPrefix(reply, "+OK") { + t.Fatalf("STLS failed: %s", reply) + } + + // Send bytes that cannot begin a TLS handshake. + if _, err := pipe.Write([]byte("this is not a TLS handshake\r\n")); err != nil { + t.Fatalf("Failed to write garbage; %v.", err) + } + + // The server must terminate the session without further responses. + _, err = c.ReadLine() + if err == nil { + t.Fatal("Expected connection close after failed handshake") + } +} + // net.Pipe does not implement deadlines type mockConn struct { net.Conn diff --git a/pkg/server/pop3/transaction_test.go b/pkg/server/pop3/transaction_test.go index cea162f..2f9fa38 100644 --- a/pkg/server/pop3/transaction_test.go +++ b/pkg/server/pop3/transaction_test.go @@ -452,19 +452,20 @@ func TestTransactionRetrArgumentErrors(t *testing.T) { } } -// Documents current behavior: RFC 1939 requires a negative status indicator -// when RETR refers to a message marked as deleted, but the handler does not -// check the retain flag. Flagged for a follow-up fix. -func TestTransactionRetrDeletedMessage(t *testing.T) { +// RFC 1939 §5: a RETR message-number may NOT refer to a message marked as +// deleted; the server responds with a negative status indicator. +func TestTransactionRetrDeletedMessageRejected(t *testing.T) { ds := newMemStore(t) - test.DeliverToStore(t, ds, "mailbox", "One", time.Now()) - _, size2 := test.DeliverToStore(t, ds, "mailbox", "Two", time.Now()) + _, size1 := test.DeliverToStore(t, ds, "mailbox", "One", time.Now()) + test.DeliverToStore(t, ds, "mailbox", "Two", time.Now()) server := setupPOPServer(t, ds, false, false) script := slices.Concat(loginSteps(2), []scriptStep{ {"DELE 2", []string{"+OK Deleted message 2"}}, - {"RETR 2", []string{fmt.Sprintf("+OK %v bytes follows", size2), - "To: somebody@host", "From: somebodyelse@host", "Subject: Two", "", "Test Body", "."}}, + {"RETR 2", []string{"-ERR You deleted message 2"}}, + // A retained message still downloads fine. + {"RETR 1", []string{fmt.Sprintf("+OK %v bytes follows", size1), + "To: somebody@host", "From: somebodyelse@host", "Subject: One", "", "Test Body", "."}}, {"QUIT", []string{"+OK We will process your deletes"}}, }) playPOP3Session(t, server, script) @@ -514,18 +515,16 @@ func TestTransactionTopArgumentErrors(t *testing.T) { } } -// Documents current behavior: RFC 1939 (TOP, section 7) requires a negative -// status indicator when TOP refers to a message marked as deleted, but the -// handler does not check the retain flag. Flagged for a follow-up fix. -func TestTransactionTopDeletedMessage(t *testing.T) { +// RFC 1939 §7 (TOP): a message-number may NOT refer to a message marked as +// deleted. +func TestTransactionTopDeletedMessageRejected(t *testing.T) { ds := newMemStore(t) test.DeliverToStore(t, ds, "mailbox", "One", time.Now()) server := setupPOPServer(t, ds, false, false) script := slices.Concat(loginSteps(1), []scriptStep{ {"DELE 1", []string{"+OK Deleted message 1"}}, - {"TOP 1 0", []string{"+OK Top of message follows", - "To: somebody@host", "From: somebodyelse@host", "Subject: One", "", "."}}, + {"TOP 1 0", []string{"-ERR You deleted message 1"}}, {"QUIT", []string{"+OK We will process your deletes"}}, }) playPOP3Session(t, server, script)