Skip to content

Commit

Permalink
fix(GODT-2165): Detect TLS handshake header to avoid reporter spam
Browse files Browse the repository at this point in the history
When not using SSL/TLS connections, identify whether the line input is a
TSL header and if so, abort the connection without reporting invalid
UTF8 events.
  • Loading branch information
LBeernaertProton committed Jan 18, 2023
1 parent 549c1f0 commit 542c2bf
Show file tree
Hide file tree
Showing 5 changed files with 171 additions and 29 deletions.
18 changes: 18 additions & 0 deletions internal/session/command.go
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
package session

import (
"bytes"
"context"
"encoding/base64"
"fmt"
"github.com/sirupsen/logrus"
"unicode/utf8"

"github.com/bradenaw/juniper/xslices"
Expand All @@ -27,6 +29,14 @@ func (s *Session) startCommandReader(ctx context.Context, del string) <-chan com
logging.GoAnnotated(ctx, func(ctx context.Context) {
defer close(cmdCh)

tlsHeaders := [][]byte{
{0x16, 0x03, 0x01}, // 1.0
{0x16, 0x03, 0x02}, // 1.1
{0x16, 0x03, 0x03}, // 1.2
{0x16, 0x03, 0x04}, // 1.3
{0x16, 0x00, 0x00}, // 0.0
}

for {
line, literals, err := s.liner.Read(func() error { return response.Continuation().Send(s) })
if err != nil {
Expand All @@ -37,6 +47,14 @@ func (s *Session) startCommandReader(ctx context.Context, del string) <-chan com
return fmt.Sprintf("%v: '%s'", k, literals[k])
})...)

// check if we are receiving raw TLS requests, if so skip.
for _, tlsHeader := range tlsHeaders {
if bytes.HasPrefix(line, tlsHeader) {
logrus.Errorf("TLS Handshake detected while not running with TLS/SSL")
return
}
}

// If the input is not valid UTF-8, we drop the connection.
if !utf8.Valid(line) {
reporter.MessageWithContext(ctx,
Expand Down
91 changes: 91 additions & 0 deletions reporter/mock_reporter/mock_reporter.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

29 changes: 0 additions & 29 deletions tests/bad_test.go

This file was deleted.

44 changes: 44 additions & 0 deletions tests/non_utf8_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
package tests

import (
"github.com/ProtonMail/gluon/reporter/mock_reporter"
"github.com/emersion/go-imap/client"
"github.com/golang/mock/gomock"
"github.com/stretchr/testify/require"
"testing"
"unicode/utf8"
)

func TestSSLConnectionOverStartTLS(t *testing.T) {
ctrl := gomock.NewController(t)
reporter := mock_reporter.NewMockReporter(ctrl)

defer ctrl.Finish()

// Ensure the nothing is reported when connecting via TLS connection if we are not running with TLS
runOneToOneTestClientWithAuth(t, defaultServerOptions(t, withReporter(reporter)), func(_ *client.Client, session *testSession) {
_, err := client.DialTLS(session.listener.Addr().String(), nil)
require.Error(t, err)
})
}

func TestNonUtf8CommandTriggersReporter(t *testing.T) {
ctrl := gomock.NewController(t)
reporter := mock_reporter.NewMockReporter(ctrl)

defer ctrl.Finish()

reporter.EXPECT().ReportMessageWithContext("Received invalid UTF-8 command", gomock.Any()).Return(nil).Times(1)

// Ensure the nothing is reported when connecting via TLS connection if we are not running with TLS
runOneToOneTestWithAuth(t, defaultServerOptions(t, withReporter(reporter)), func(c *testConnection, session *testSession) {
// Encode "ééé" as ISO-8859-1.
b := enc("ééé", "ISO-8859-1")

// Assert that b is no longer valid UTF-8.
require.False(t, utf8.Valid(b))

// This will fail and produce a report
c.Cf(`TAG SEARCH CHARSET ISO-8859-1 BODY ` + string(b))
})
}
18 changes: 18 additions & 0 deletions tests/server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"crypto/tls"
"encoding/hex"
"fmt"
"github.com/ProtonMail/gluon/reporter"
"net"
"path/filepath"
"testing"
Expand Down Expand Up @@ -74,6 +75,7 @@ type serverOptions struct {
connectorBuilder connectorBuilder
disableParallelism bool
imapLimits limits.IMAP
reporter reporter.Reporter
}

func (s *serverOptions) defaultUsername() string {
Expand Down Expand Up @@ -164,6 +166,14 @@ func (m imapLimits) apply(options *serverOptions) {
options.imapLimits = m.limits
}

type reporterOption struct {
reporter reporter.Reporter
}

func (r reporterOption) apply(options *serverOptions) {
options.reporter = r.reporter
}

func withIdleBulkTime(idleBulkTime time.Duration) serverOption {
return &idleBulkTimeOption{idleBulkTime: idleBulkTime}
}
Expand Down Expand Up @@ -196,6 +206,10 @@ func withIMAPLimits(limits limits.IMAP) serverOption {
return &imapLimits{limits: limits}
}

func withReporter(reporter reporter.Reporter) serverOption {
return &reporterOption{reporter: reporter}
}

func defaultServerOptions(tb testing.TB, modifiers ...serverOption) *serverOptions {
options := &serverOptions{
credentials: []credentials{{
Expand Down Expand Up @@ -264,6 +278,10 @@ func runServer(tb testing.TB, options *serverOptions, tests func(session *testSe
gluonOptions = append(gluonOptions, gluon.WithDisableParallelism())
}

if options.reporter != nil {
gluonOptions = append(gluonOptions, gluon.WithReporter(options.reporter))
}

// Create a new gluon server.
server, err := gluon.New(gluonOptions...)
require.NoError(tb, err)
Expand Down

0 comments on commit 542c2bf

Please sign in to comment.