From 97ee89cc67d89fa1e3adf480155835f4e26ecebe Mon Sep 17 00:00:00 2001
From: Ivan Kozlovic <ivan@synadia.com>
Date: Wed, 22 May 2019 12:31:16 -0600
Subject: [PATCH] Check inbound GW connection connected state in parser

If the first protocol for an inbound gateway connection is not
CONNECT, reject with auth violation.

Fixes #1006

Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
---
 server/const.go      |  2 +-
 server/gateway.go    | 12 ++++++++++++
 server/parser.go     | 13 +++++++++++--
 test/gateway_test.go | 33 +++++++++++++++++++++++++++++++++
 4 files changed, 57 insertions(+), 3 deletions(-)

diff --git a/server/const.go b/server/const.go
index d3e31791..e1b74106 100644
--- a/server/const.go
+++ b/server/const.go
@@ -40,7 +40,7 @@ var (
 
 const (
 	// VERSION is the current version for the server.
-	VERSION = "2.0.0-RC12"
+	VERSION = "2.0.0-RC14"
 
 	// PROTO is the currently supported protocol.
 	// 0 was the original
diff --git a/server/gateway.go b/server/gateway.go
index 51d9b279..4ff2bf57 100644
--- a/server/gateway.go
+++ b/server/gateway.go
@@ -149,6 +149,9 @@ type gateway struct {
 	infoJSON   []byte            // Needed when sending INFO after receiving INFO from remote
 	outsim     *sync.Map         // Per-account subject interest (or no-interest) (outbound conn)
 	insim      map[string]*insie // Per-account subject no-interest sent or modeInterestOnly mode (inbound conn)
+
+	// Set/check in readLoop without lock. This is to know that an inbound has sent the CONNECT protocol first
+	connected bool
 }
 
 // Outbound subject interest entry.
@@ -809,6 +812,15 @@ func (c *client) processGatewayConnect(arg []byte) error {
 		return ErrWrongGateway
 	}
 
+	// For a gateway connection, c.gw is guaranteed not to be nil here
+	// (created in createGateway() and never set to nil).
+	// For inbound connections, it is important to know in the parser
+	// if the CONNECT was received first, so we use this boolean (as
+	// opposed to client.flags that require locking) to indicate that
+	// CONNECT was processed. Again, this boolean is set/read in the
+	// readLoop without locking.
+	c.gw.connected = true
+
 	return nil
 }
 
diff --git a/server/parser.go b/server/parser.go
index 6fabcbca..f03826b6 100644
--- a/server/parser.go
+++ b/server/parser.go
@@ -127,8 +127,17 @@ func (c *client) parse(buf []byte) error {
 
 		switch c.state {
 		case OP_START:
-			if b != 'C' && b != 'c' && authSet {
-				goto authErr
+			if b != 'C' && b != 'c' {
+				if authSet {
+					goto authErr
+				}
+				// If the connection is a gateway connection, make sure that
+				// if this is an inbound, it starts with a CONNECT.
+				if c.kind == GATEWAY && !c.gw.outbound && !c.gw.connected {
+					// Use auth violation since no CONNECT was sent.
+					// It could be a parseErr too.
+					goto authErr
+				}
 			}
 			switch b {
 			case 'P', 'p':
diff --git a/test/gateway_test.go b/test/gateway_test.go
index 8d427a20..668a7e70 100644
--- a/test/gateway_test.go
+++ b/test/gateway_test.go
@@ -446,3 +446,36 @@ func TestGatewaySendAllSubs(t *testing.T) {
 	clientSend("UNSUB 1\r\n")
 	gAExpect(runsubRe)
 }
+
+func TestGatewayNoPanicOnBadProtocol(t *testing.T) {
+	ob := testDefaultOptionsForGateway("B")
+	sb := runGatewayServer(ob)
+	defer sb.Shutdown()
+
+	for _, test := range []struct {
+		name  string
+		proto string
+	}{
+		{"sub", "SUB > 1\r\n"},
+		{"unsub", "UNSUB 1\r\n"},
+		{"rsub", "RS+ $foo foo 2\r\n"},
+		{"runsub", "RS- $foo foo 2\r\n"},
+		{"pub", "PUB foo 2\r\nok\r\n"},
+		{"anything", "xxxx\r\n"},
+	} {
+		t.Run(test.name, func(t *testing.T) {
+			// Create raw tcp connection to gateway port
+			client := createClientConn(t, ob.Gateway.Host, ob.Gateway.Port)
+			defer client.Close()
+			clientSend := sendCommand(t, client)
+			clientSend(test.proto)
+		})
+	}
+
+	// Server should not have crashed.
+	client := createClientConn(t, ob.Host, ob.Port)
+	defer client.Close()
+	clientSend, clientExpect := setupConn(t, client)
+	clientSend("PING\r\n")
+	clientExpect(pongRe)
+}