ssh: don't skip the source-address critical option in CheckCert

CertChecker.CheckCert ignored source-address on the assumption that
serverAuthenticate would enforce it, but that only happens in the
server-side user authentication path. Nothing enforced it in the host key
path, so CheckHostKey accepted CA-signed host certificates carrying
source-address regardless of the server's address, and the same applied to
applications calling CheckCert directly.

Drop the special case: every critical option must be listed in
SupportedCriticalOptions, host and user certificates alike. Authenticate,
the only path where serverAuthenticate does enforce source-address, checks
against a copy of the CertChecker with that option appended.

Fixes golang/go#80872

Change-Id: I3c3554b71ea2a4ce4b17696a9596b973b7d74b00
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/816840
Reviewed-by: David Chase <drchase@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
Auto-Submit: Nicola Murino <nicola.murino@gmail.com>

ssh/certs.go

@@ -10,6 +10,7 @@ import (
 	"fmt"
 	"io"
 	"net"
+	"slices"
 	"sort"
 	"time"
 )
@@ -305,8 +306,11 @@ const sourceAddressCriticalOption = "source-address"
 // minimally, the IsAuthority callback should be set.
 type CertChecker struct {
 	// SupportedCriticalOptions lists the CriticalOptions that the
-	// server application layer understands. These are only used
-	// for user certificates.
+	// application layer understands. A certificate carrying a critical
+	// option that is not listed here is rejected.
+	// CertChecker.Authenticate additionally accepts the source-address
+	// option, which the server enforces on the Permissions that
+	// Authenticate returns.
 	SupportedCriticalOptions []string
 
 	// IsUserAuthority should return true if the key is recognized as an
@@ -369,8 +373,9 @@ func (c *CertChecker) CheckHostKey(addr string, remote net.Addr, key PublicKey)
 	return c.CheckCert(hostname, cert)
 }
 
-// Authenticate checks a user certificate. Authenticate can be used as
-// a value for ServerConfig.PublicKeyCallback.
+// Authenticate checks a user certificate. Authenticate can be used as a value
+// for ServerConfig.PublicKeyCallback. The source-address critical option is
+// allowed, as it will be enforced by the server.
 func (c *CertChecker) Authenticate(conn ConnMetadata, pubKey PublicKey) (*Permissions, error) {
 	cert, ok := pubKey.(*Certificate)
 	if !ok {
@@ -389,8 +394,11 @@ func (c *CertChecker) Authenticate(conn ConnMetadata, pubKey PublicKey) (*Permis
 	if !c.IsUserAuthority(cert.SignatureKey) {
 		return nil, fmt.Errorf("ssh: certificate signed by unrecognized authority")
 	}
-
-	if err := c.CheckCert(conn.User(), cert); err != nil {
+	// The source-address critical option is enforced by serverAuthenticate,
+	// so it is supported regardless of SupportedCriticalOptions
+	cc := *c
+	cc.SupportedCriticalOptions = append(slices.Clip(cc.SupportedCriticalOptions), sourceAddressCriticalOption)
+	if err := cc.CheckCert(conn.User(), cert); err != nil {
 		return nil, err
 	}
 
@@ -398,27 +406,15 @@ func (c *CertChecker) Authenticate(conn ConnMetadata, pubKey PublicKey) (*Permis
 }
 
 // CheckCert checks CriticalOptions, ValidPrincipals, revocation, timestamp and
-// the signature of the certificate.
+// the signature of the certificate. Critical options that are not listed in
+// SupportedCriticalOptions are rejected.
 func (c *CertChecker) CheckCert(principal string, cert *Certificate) error {
 	if c.IsRevoked != nil && c.IsRevoked(cert) {
 		return fmt.Errorf("ssh: certificate serial %d revoked", cert.Serial)
 	}
 
 	for opt := range cert.CriticalOptions {
-		// sourceAddressCriticalOption will be enforced by
-		// serverAuthenticate
-		if opt == sourceAddressCriticalOption {
-			continue
-		}
-
-		found := false
-		for _, supp := range c.SupportedCriticalOptions {
-			if supp == opt {
-				found = true
-				break
-			}
-		}
-		if !found {
+		if !slices.Contains(c.SupportedCriticalOptions, opt) {
 			return fmt.Errorf("ssh: unsupported critical option %q in certificate", opt)
 		}
 	}

ssh/certs_test.go

@@ -385,6 +385,82 @@ func TestHostKeyCert(t *testing.T) {
 	}
 }
 
+type testConnMeta struct {
+	ConnMetadata
+	user string
+}
+
+func (c testConnMeta) User() string { return c.user }
+
+func TestCertCriticalOptions(t *testing.T) {
+	appended := make([]string, 2)
+	appended[0], appended[1] = "supported-option", "appended-option"
+	supported := appended[:1]
+
+	checker := &CertChecker{
+		SupportedCriticalOptions: supported,
+		IsHostAuthority: func(p PublicKey, addr string) bool {
+			return bytes.Equal(testPublicKeys["ecdsa"].Marshal(), p.Marshal())
+		},
+		IsUserAuthority: func(p PublicKey) bool {
+			return bytes.Equal(testPublicKeys["ecdsa"].Marshal(), p.Marshal())
+		},
+	}
+
+	for _, test := range []struct {
+		name string
+		opts map[string]string
+		// succeed is the expected outcome of CheckHostKey, for a host
+		// certificate, and of CheckCert, for a user certificate.
+		succeed bool
+		// authSucceed is the expected outcome of Authenticate.
+		authSucceed bool
+	}{
+		{name: "no critical options", opts: nil, succeed: true, authSucceed: true},
+		{name: "source-address", opts: map[string]string{sourceAddressCriticalOption: "192.168.1.0/24"}, authSucceed: true},
+		{name: "unknown option", opts: map[string]string{"unknown-option": ""}},
+		{name: "supported option", opts: map[string]string{"supported-option": ""}, succeed: true, authSucceed: true},
+	} {
+		hostCert := &Certificate{
+			ValidPrincipals: []string{"hostname"},
+			Key:             testPublicKeys["rsa"],
+			ValidBefore:     CertTimeInfinity,
+			CertType:        HostCert,
+			Permissions:     Permissions{CriticalOptions: test.opts},
+		}
+		if err := hostCert.SignCert(rand.Reader, testSigners["ecdsa"]); err != nil {
+			t.Fatalf("SignCert: %v", err)
+		}
+
+		if err := checker.CheckHostKey("hostname:22", nil, hostCert); (err == nil) != test.succeed {
+			t.Errorf("CheckHostKey(%s): got %v, want success=%v", test.name, err, test.succeed)
+		}
+
+		userCert := &Certificate{
+			ValidPrincipals: []string{"user"},
+			Key:             testPublicKeys["rsa"],
+			ValidBefore:     CertTimeInfinity,
+			CertType:        UserCert,
+			Permissions:     Permissions{CriticalOptions: test.opts},
+		}
+		if err := userCert.SignCert(rand.Reader, testSigners["ecdsa"]); err != nil {
+			t.Fatalf("SignCert: %v", err)
+		}
+
+		if err := checker.CheckCert("user", userCert); (err == nil) != test.succeed {
+			t.Errorf("CheckCert(%s): got %v, want success=%v", test.name, err, test.succeed)
+		}
+
+		if _, err := checker.Authenticate(testConnMeta{user: "user"}, userCert); (err == nil) != test.authSucceed {
+			t.Errorf("Authenticate(%s): got %v, want success=%v", test.name, err, test.authSucceed)
+		}
+	}
+
+	if appended[1] != "appended-option" {
+		t.Errorf("Authenticate overwrote the caller's slice: got %q, want %q", appended[1], "appended-option")
+	}
+}
+
 type legacyRSASigner struct {
 	Signer
 }