ssh: ignore invalid MACs and KEXs just like we do for ciphers
Tighter validation could cause backwards incompatibility issues, eg
configurations with valid and invalid MACs, KEXs, ciphers currently work
if a supported algorithm is negotiated and that's also the scenario of
removing support for an existing algorithm.
Fixes golang/go#39397
Change-Id: If90253ba89e1d8f732cc1e1c3d24fe0a1e2dac71
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/512175
Run-TryBot: Han-Wen Nienhuys <hanwen@google.com>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Han-Wen Nienhuys <hanwen@google.com>
Auto-Submit: Filippo Valsorda <filippo@golang.org>
Reviewed-by: David Chase <drchase@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
diff --git a/ssh/client_test.go b/ssh/client_test.go
index 2814755..c114573 100644
--- a/ssh/client_test.go
+++ b/ssh/client_test.go
@@ -254,3 +254,93 @@
})
}
}
+
+func TestUnsupportedAlgorithm(t *testing.T) {
+ for _, tt := range []struct {
+ name string
+ config Config
+ wantError string
+ }{
+ {
+ "unsupported KEX",
+ Config{
+ KeyExchanges: []string{"unsupported"},
+ },
+ "no common algorithm",
+ },
+ {
+ "unsupported and supported KEXs",
+ Config{
+ KeyExchanges: []string{"unsupported", kexAlgoCurve25519SHA256},
+ },
+ "",
+ },
+ {
+ "unsupported cipher",
+ Config{
+ Ciphers: []string{"unsupported"},
+ },
+ "no common algorithm",
+ },
+ {
+ "unsupported and supported ciphers",
+ Config{
+ Ciphers: []string{"unsupported", chacha20Poly1305ID},
+ },
+ "",
+ },
+ {
+ "unsupported MAC",
+ Config{
+ MACs: []string{"unsupported"},
+ // MAC is used for non AAED ciphers.
+ Ciphers: []string{"aes256-ctr"},
+ },
+ "no common algorithm",
+ },
+ {
+ "unsupported and supported MACs",
+ Config{
+ MACs: []string{"unsupported", "hmac-sha2-256-etm@openssh.com"},
+ // MAC is used for non AAED ciphers.
+ Ciphers: []string{"aes256-ctr"},
+ },
+ "",
+ },
+ } {
+ t.Run(tt.name, func(t *testing.T) {
+ c1, c2, err := netPipe()
+ if err != nil {
+ t.Fatalf("netPipe: %v", err)
+ }
+ defer c1.Close()
+ defer c2.Close()
+
+ serverConf := &ServerConfig{
+ Config: tt.config,
+ PasswordCallback: func(conn ConnMetadata, password []byte) (*Permissions, error) {
+ return &Permissions{}, nil
+ },
+ }
+ serverConf.AddHostKey(testSigners["rsa"])
+ go NewServerConn(c1, serverConf)
+
+ clientConf := &ClientConfig{
+ User: "testuser",
+ Config: tt.config,
+ Auth: []AuthMethod{
+ Password("testpw"),
+ },
+ HostKeyCallback: InsecureIgnoreHostKey(),
+ }
+ _, _, _, err = NewClientConn(c2, "", clientConf)
+ if err != nil {
+ if tt.wantError == "" || !strings.Contains(err.Error(), tt.wantError) {
+ t.Errorf("%s: got error %q, missing %q", tt.name, err.Error(), tt.wantError)
+ }
+ } else if tt.wantError != "" {
+ t.Errorf("%s: succeeded, but want error string %q", tt.name, tt.wantError)
+ }
+ })
+ }
+}
diff --git a/ssh/common.go b/ssh/common.go
index 44f71de..3862ec8 100644
--- a/ssh/common.go
+++ b/ssh/common.go
@@ -269,16 +269,16 @@
// unspecified, a size suitable for the chosen cipher is used.
RekeyThreshold uint64
- // The allowed key exchanges algorithms. If unspecified then a
- // default set of algorithms is used.
+ // The allowed key exchanges algorithms. If unspecified then a default set
+ // of algorithms is used. Unsupported values are silently ignored.
KeyExchanges []string
- // The allowed cipher algorithms. If unspecified then a sensible
- // default is used.
+ // The allowed cipher algorithms. If unspecified then a sensible default is
+ // used. Unsupported values are silently ignored.
Ciphers []string
- // The allowed MAC algorithms. If unspecified then a sensible default
- // is used.
+ // The allowed MAC algorithms. If unspecified then a sensible default is
+ // used. Unsupported values are silently ignored.
MACs []string
}
@@ -295,7 +295,7 @@
var ciphers []string
for _, c := range c.Ciphers {
if cipherModes[c] != nil {
- // reject the cipher if we have no cipherModes definition
+ // Ignore the cipher if we have no cipherModes definition.
ciphers = append(ciphers, c)
}
}
@@ -304,10 +304,26 @@
if c.KeyExchanges == nil {
c.KeyExchanges = preferredKexAlgos
}
+ var kexs []string
+ for _, k := range c.KeyExchanges {
+ if kexAlgoMap[k] != nil {
+ // Ignore the KEX if we have no kexAlgoMap definition.
+ kexs = append(kexs, k)
+ }
+ }
+ c.KeyExchanges = kexs
if c.MACs == nil {
c.MACs = supportedMACs
}
+ var macs []string
+ for _, m := range c.MACs {
+ if macModes[m] != nil {
+ // Ignore the MAC if we have no macModes definition.
+ macs = append(macs, m)
+ }
+ }
+ c.MACs = macs
if c.RekeyThreshold == 0 {
// cipher specific default
