crypto/ssh: fix encoding of ssh certs with critical options

Attention - BREAKING change for the certificates generated with
the previous versions of crypto/ssh!  Need to regenerate
certificates with a version of crypto/ssh library including
this fix.

[PROTOCOL.cerkeys] requires two length fields for non-empty
values of critical options (or extensions - but those are
currently always empty)  - see
https://bugzilla.mindrot.org/show_bug.cgi?id=2389.
Add SSH-conform handling of such composite values in marshalTuples
and parseTuples and related test (TestParseCertWithOptions) parsing
a certificate created with ssh-keygen which includes critical options.

Fixes #10569

Change-Id: Iecbfca67a66668880635141c72bc5fc370a9c112
Reviewed-on: https://go-review.googlesource.com/9375
Reviewed-by: Adam Langley <agl@golang.org>
Run-TryBot: Adam Langley <agl@golang.org>

ssh/certs.go

@@ -85,46 +85,73 @@ func marshalStringList(namelist []string) []byte {
 	return to
 }
 
+type optionsTuple struct {
+	Key   string
+	Value []byte
+}
+
+type optionsTupleValue struct {
+	Value string
+}
+
+// serialize a map of critical options or extensions
+// issue #10569 - per [PROTOCOL.certkeys] and SSH implementation,
+// we need two length prefixes for a non-empty string value
 func marshalTuples(tups map[string]string) []byte {
 	keys := make([]string, 0, len(tups))
-	for k := range tups {
-		keys = append(keys, k)
+	for key := range tups {
+		keys = append(keys, key)
 	}
 	sort.Strings(keys)
 
-	var r []byte
-	for _, k := range keys {
-		s := struct{ K, V string }{k, tups[k]}
-		r = append(r, Marshal(&s)...)
+	var ret []byte
+	for _, key := range keys {
+		s := optionsTuple{Key: key}
+		if value := tups[key]; len(value) > 0 {
+			s.Value = Marshal(&optionsTupleValue{value})
+		}
+		ret = append(ret, Marshal(&s)...)
 	}
-	return r
+	return ret
 }
 
+// issue #10569 - per [PROTOCOL.certkeys] and SSH implementation,
+// we need two length prefixes for a non-empty option value
 func parseTuples(in []byte) (map[string]string, error) {
 	tups := map[string]string{}
 	var lastKey string
 	var haveLastKey bool
 
 	for len(in) > 0 {
-		nameBytes, rest, ok := parseString(in)
-		if !ok {
-			return nil, errShortRead
-		}
-		data, rest, ok := parseString(rest)
-		if !ok {
+		var key, val, extra []byte
+		var ok bool
+
+		if key, in, ok = parseString(in); !ok {
 			return nil, errShortRead
 		}
-		name := string(nameBytes)
-
+		keyStr := string(key)
 		// according to [PROTOCOL.certkeys], the names must be in
 		// lexical order.
-		if haveLastKey && name <= lastKey {
+		if haveLastKey && keyStr <= lastKey {
 			return nil, fmt.Errorf("ssh: certificate options are not in lexical order")
 		}
-		lastKey, haveLastKey = name, true
-
-		tups[name] = string(data)
-		in = rest
+		lastKey, haveLastKey = keyStr, true
+		// the next field is a data field, which if non-empty has a string embedded
+		if val, in, ok = parseString(in); !ok {
+			return nil, errShortRead
+		}
+		if len(val) > 0 {
+			val, extra, ok = parseString(val)
+			if !ok {
+				return nil, errShortRead
+			}
+			if len(extra) > 0 {
+				return nil, fmt.Errorf("ssh: unexpected trailing data after certificate option value")
+			}
+			tups[keyStr] = string(val)
+		} else {
+			tups[keyStr] = ""
+		}
 	}
 	return tups, nil
 }

ssh/certs_test.go

@@ -7,13 +7,14 @@ package ssh
 import (
 	"bytes"
 	"crypto/rand"
+	"reflect"
 	"testing"
 	"time"
 )
 
 // Cert generated by ssh-keygen 6.0p1 Debian-4.
 // % ssh-keygen -s ca-key -I test user-key
-var exampleSSHCert = `ssh-rsa-cert-v01@openssh.com AAAAHHNzaC1yc2EtY2VydC12MDFAb3BlbnNzaC5jb20AAAAgb1srW/W3ZDjYAO45xLYAwzHBDLsJ4Ux6ICFIkTjb1LEAAAADAQABAAAAYQCkoR51poH0wE8w72cqSB8Sszx+vAhzcMdCO0wqHTj7UNENHWEXGrU0E0UQekD7U+yhkhtoyjbPOVIP7hNa6aRk/ezdh/iUnCIt4Jt1v3Z1h1P+hA4QuYFMHNB+rmjPwAcAAAAAAAAAAAAAAAEAAAAEdGVzdAAAAAAAAAAAAAAAAP//////////AAAAAAAAAIIAAAAVcGVybWl0LVgxMS1mb3J3YXJkaW5nAAAAAAAAABdwZXJtaXQtYWdlbnQtZm9yd2FyZGluZwAAAAAAAAAWcGVybWl0LXBvcnQtZm9yd2FyZGluZwAAAAAAAAAKcGVybWl0LXB0eQAAAAAAAAAOcGVybWl0LXVzZXItcmMAAAAAAAAAAAAAAHcAAAAHc3NoLXJzYQAAAAMBAAEAAABhANFS2kaktpSGc+CcmEKPyw9mJC4nZKxHKTgLVZeaGbFZOvJTNzBspQHdy7Q1uKSfktxpgjZnksiu/tFF9ngyY2KFoc+U88ya95IZUycBGCUbBQ8+bhDtw/icdDGQD5WnUwAAAG8AAAAHc3NoLXJzYQAAAGC8Y9Z2LQKhIhxf52773XaWrXdxP0t3GBVo4A10vUWiYoAGepr6rQIoGGXFxT4B9Gp+nEBJjOwKDXPrAevow0T9ca8gZN+0ykbhSrXLE5Ao48rqr3zP4O1/9P7e6gp0gw8=`
+const exampleSSHCert = `ssh-rsa-cert-v01@openssh.com AAAAHHNzaC1yc2EtY2VydC12MDFAb3BlbnNzaC5jb20AAAAgb1srW/W3ZDjYAO45xLYAwzHBDLsJ4Ux6ICFIkTjb1LEAAAADAQABAAAAYQCkoR51poH0wE8w72cqSB8Sszx+vAhzcMdCO0wqHTj7UNENHWEXGrU0E0UQekD7U+yhkhtoyjbPOVIP7hNa6aRk/ezdh/iUnCIt4Jt1v3Z1h1P+hA4QuYFMHNB+rmjPwAcAAAAAAAAAAAAAAAEAAAAEdGVzdAAAAAAAAAAAAAAAAP//////////AAAAAAAAAIIAAAAVcGVybWl0LVgxMS1mb3J3YXJkaW5nAAAAAAAAABdwZXJtaXQtYWdlbnQtZm9yd2FyZGluZwAAAAAAAAAWcGVybWl0LXBvcnQtZm9yd2FyZGluZwAAAAAAAAAKcGVybWl0LXB0eQAAAAAAAAAOcGVybWl0LXVzZXItcmMAAAAAAAAAAAAAAHcAAAAHc3NoLXJzYQAAAAMBAAEAAABhANFS2kaktpSGc+CcmEKPyw9mJC4nZKxHKTgLVZeaGbFZOvJTNzBspQHdy7Q1uKSfktxpgjZnksiu/tFF9ngyY2KFoc+U88ya95IZUycBGCUbBQ8+bhDtw/icdDGQD5WnUwAAAG8AAAAHc3NoLXJzYQAAAGC8Y9Z2LQKhIhxf52773XaWrXdxP0t3GBVo4A10vUWiYoAGepr6rQIoGGXFxT4B9Gp+nEBJjOwKDXPrAevow0T9ca8gZN+0ykbhSrXLE5Ao48rqr3zP4O1/9P7e6gp0gw8=`
 
 func TestParseCert(t *testing.T) {
 	authKeyBytes := []byte(exampleSSHCert)
@@ -27,9 +28,63 @@ func TestParseCert(t *testing.T) {
 	}
 
 	if _, ok := key.(*Certificate); !ok {
-		t.Fatalf("got %#v, want *Certificate", key)
+		t.Fatalf("got %v (%T), want *Certificate", key, key)
+	}
+
+	marshaled := MarshalAuthorizedKey(key)
+	// Before comparison, remove the trailing newline that
+	// MarshalAuthorizedKey adds.
+	marshaled = marshaled[:len(marshaled)-1]
+	if !bytes.Equal(authKeyBytes, marshaled) {
+		t.Errorf("marshaled certificate does not match original: got %q, want %q", marshaled, authKeyBytes)
 	}
+}
+
+// Cert generated by ssh-keygen OpenSSH_6.8p1 OS X 10.10.3
+// % ssh-keygen -s ca -I testcert -O source-address=192.168.1.0/24 -O force-command=/bin/sleep user.pub
+// user.pub key: ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDACh1rt2DXfV3hk6fszSQcQ/rueMId0kVD9U7nl8cfEnFxqOCrNT92g4laQIGl2mn8lsGZfTLg8ksHq3gkvgO3oo/0wHy4v32JeBOHTsN5AL4gfHNEhWeWb50ev47hnTsRIt9P4dxogeUo/hTu7j9+s9lLpEQXCvq6xocXQt0j8MV9qZBBXFLXVT3cWIkSqOdwt/5ZBg+1GSrc7WfCXVWgTk4a20uPMuJPxU4RQwZW6X3+O8Pqo8C3cW0OzZRFP6gUYUKUsTI5WntlS+LAxgw1mZNsozFGdbiOPRnEryE3SRldh9vjDR3tin1fGpA5P7+CEB/bqaXtG3V+F2OkqaMN
+// Critical Options:
+//         force-command /bin/sleep
+//         source-address 192.168.1.0/24
+// Extensions:
+//         permit-X11-forwarding
+//         permit-agent-forwarding
+//         permit-port-forwarding
+//         permit-pty
+//         permit-user-rc
+const exampleSSHCertWithOptions = `ssh-rsa-cert-v01@openssh.com AAAAHHNzaC1yc2EtY2VydC12MDFAb3BlbnNzaC5jb20AAAAgDyysCJY0XrO1n03EeRRoITnTPdjENFmWDs9X58PP3VUAAAADAQABAAABAQDACh1rt2DXfV3hk6fszSQcQ/rueMId0kVD9U7nl8cfEnFxqOCrNT92g4laQIGl2mn8lsGZfTLg8ksHq3gkvgO3oo/0wHy4v32JeBOHTsN5AL4gfHNEhWeWb50ev47hnTsRIt9P4dxogeUo/hTu7j9+s9lLpEQXCvq6xocXQt0j8MV9qZBBXFLXVT3cWIkSqOdwt/5ZBg+1GSrc7WfCXVWgTk4a20uPMuJPxU4RQwZW6X3+O8Pqo8C3cW0OzZRFP6gUYUKUsTI5WntlS+LAxgw1mZNsozFGdbiOPRnEryE3SRldh9vjDR3tin1fGpA5P7+CEB/bqaXtG3V+F2OkqaMNAAAAAAAAAAAAAAABAAAACHRlc3RjZXJ0AAAAAAAAAAAAAAAA//////////8AAABLAAAADWZvcmNlLWNvbW1hbmQAAAAOAAAACi9iaW4vc2xlZXAAAAAOc291cmNlLWFkZHJlc3MAAAASAAAADjE5Mi4xNjguMS4wLzI0AAAAggAAABVwZXJtaXQtWDExLWZvcndhcmRpbmcAAAAAAAAAF3Blcm1pdC1hZ2VudC1mb3J3YXJkaW5nAAAAAAAAABZwZXJtaXQtcG9ydC1mb3J3YXJkaW5nAAAAAAAAAApwZXJtaXQtcHR5AAAAAAAAAA5wZXJtaXQtdXNlci1yYwAAAAAAAAAAAAABFwAAAAdzc2gtcnNhAAAAAwEAAQAAAQEAwU+c5ui5A8+J/CFpjW8wCa52bEODA808WWQDCSuTG/eMXNf59v9Y8Pk0F1E9dGCosSNyVcB/hacUrc6He+i97+HJCyKavBsE6GDxrjRyxYqAlfcOXi/IVmaUGiO8OQ39d4GHrjToInKvExSUeleQyH4Y4/e27T/pILAqPFL3fyrvMLT5qU9QyIt6zIpa7GBP5+urouNavMprV3zsfIqNBbWypinOQAw823a5wN+zwXnhZrgQiHZ/USG09Y6k98y1dTVz8YHlQVR4D3lpTAsKDKJ5hCH9WU4fdf+lU8OyNGaJ/vz0XNqxcToe1l4numLTnaoSuH89pHryjqurB7lJKwAAAQ8AAAAHc3NoLXJzYQAAAQCaHvUIoPL1zWUHIXLvu96/HU1s/i4CAW2IIEuGgxCUCiFj6vyTyYtgxQxcmbfZf6eaITlS6XJZa7Qq4iaFZh75C1DXTX8labXhRSD4E2t//AIP9MC1rtQC5xo6FmbQ+BoKcDskr+mNACcbRSxs3IL3bwCfWDnIw2WbVox9ZdcthJKk4UoCW4ix4QwdHw7zlddlz++fGEEVhmTbll1SUkycGApPFBsAYRTMupUJcYPIeReBI/m8XfkoMk99bV8ZJQTAd7OekHY2/48Ff53jLmyDjP7kNw1F8OaPtkFs6dGJXta4krmaekPy87j+35In5hFj7yoOqvSbmYUkeX70/GGQ`
+
+func TestParseCertWithOptions(t *testing.T) {
+	opts := map[string]string{
+		"source-address": "192.168.1.0/24",
+		"force-command": "/bin/sleep",
+	}
+	exts := map[string]string{
+		"permit-X11-forwarding":   "",
+		"permit-agent-forwarding": "",
+		"permit-port-forwarding":  "",
+		"permit-pty":              "",
+		"permit-user-rc":          "",
+	}
+	authKeyBytes := []byte(exampleSSHCertWithOptions)
 
+	key, _, _, rest, err := ParseAuthorizedKey(authKeyBytes)
+	if err != nil {
+		t.Fatalf("ParseAuthorizedKey: %v", err)
+	}
+	if len(rest) > 0 {
+		t.Errorf("rest: got %q, want empty", rest)
+	}
+	cert, ok := key.(*Certificate)
+	if !ok {
+		t.Fatalf("got %v (%T), want *Certificate", key, key)
+	}
+	if !reflect.DeepEqual(cert.CriticalOptions, opts) {
+		t.Errorf("unexpected critical options - got %v, want %v", cert.CriticalOptions, opts)
+	}
+	if !reflect.DeepEqual(cert.Extensions, exts) {
+		t.Errorf("unexpected Extensions - got %v, want %v", cert.Extensions, exts)
+	}
 	marshaled := MarshalAuthorizedKey(key)
 	// Before comparison, remove the trailing newline that
 	// MarshalAuthorizedKey adds.