ssh: fix parsing of GSSAPI payloads offering multiple mechanisms

parseGSSAPIPayload overwrote the remaining payload with the trailing
bytes returned by asn1.Unmarshal after decoding each OID, so any
request offering more than one mechanism failed to parse after the
first one. Track the remaining payload across iterations instead, and
be strict: reject payloads with trailing bytes after an OID or after
the mechanism list.

Add a test with a two-mechanism payload (Kerberos V5 followed by
SPNEGO) that failed before this change.

Change-Id: If0984efcf103da76323d5d03ad7443d8ec9d3aed
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/803180
Reviewed-by: Carlos Amedee <carlos@golang.org>
Reviewed-by: Nicola Murino <nicola.murino@gmail.com>
Reviewed-by: David Chase <drchase@google.com>
LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

ssh/ssh_gss.go

@@ -118,24 +118,28 @@ func parseGSSAPIPayload(payload []byte) (*userAuthRequestGSSAPI, error) {
 		OIDS: make([]asn1.ObjectIdentifier, n),
 	}
 	for i := 0; i < int(n); i++ {
-		var (
-			desiredMech []byte
-			err         error
-		)
+		var desiredMech []byte
 		desiredMech, rest, ok = parseString(rest)
 		if !ok {
 			return nil, errors.New("parse string failed")
 		}
-		if rest, err = asn1.Unmarshal(desiredMech, &s.OIDS[i]); err != nil {
+		trailing, err := asn1.Unmarshal(desiredMech, &s.OIDS[i])
+		if err != nil {
 			return nil, err
 		}
+		if len(trailing) != 0 {
+			return nil, errors.New("trailing bytes after OID")
+		}
+	}
+	if len(rest) != 0 {
+		return nil, errors.New("trailing bytes after mechanisms")
 	}
 	return s, nil
 }
 
 // See RFC 4462 section 3.6.
 func buildMIC(sessionID string, username string, service string, authMethod string) []byte {
-	out := make([]byte, 0, 0)
+	out := make([]byte, 0)
 	out = appendString(out, sessionID)
 	out = append(out, msgUserAuthRequest)
 	out = appendString(out, username)

ssh/ssh_gss_test.go

@@ -1,6 +1,7 @@
 package ssh
 
 import (
+	"encoding/asn1"
 	"fmt"
 	"testing"
 )
@@ -17,6 +18,34 @@ func TestParseGSSAPIPayload(t *testing.T) {
 	}
 }
 
+func TestParseGSSAPIPayloadMultipleMechs(t *testing.T) {
+	// A payload offering two mechanisms: Kerberos V5 (1.2.840.113554.1.2.2)
+	// followed by SPNEGO (1.3.6.1.5.5.2). The parser must track the
+	// remaining payload across loop iterations, not the trailing bytes
+	// returned by asn1.Unmarshal.
+	payload := []byte{
+		0x00, 0x00, 0x00, 0x02, // two mechanisms
+		0x00, 0x00, 0x00, 0x0b, // length of the first OID
+		0x06, 0x09, 0x2a, 0x86, 0x48, 0x86, 0xf7, 0x12, 0x01, 0x02, 0x02,
+		0x00, 0x00, 0x00, 0x08, // length of the second OID
+		0x06, 0x06, 0x2b, 0x06, 0x01, 0x05, 0x05, 0x02,
+	}
+	res, err := parseGSSAPIPayload(payload)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(res.OIDS) != 2 {
+		t.Fatalf("got %d OIDs, want 2", len(res.OIDS))
+	}
+	if !res.OIDS[0].Equal(krb5Mesh) {
+		t.Errorf("OIDS[0]: got %v, want %v", res.OIDS[0], krb5Mesh)
+	}
+	spnego := asn1.ObjectIdentifier{1, 3, 6, 1, 5, 5, 2}
+	if !res.OIDS[1].Equal(spnego) {
+		t.Errorf("OIDS[1]: got %v, want %v", res.OIDS[1], spnego)
+	}
+}
+
 func TestParseDubiousGSSAPIPayload(t *testing.T) {
 	for _, tc := range []struct {
 		name    string