Skip to content

Commit

Permalink
ssh: ignore invalid MACs and KEXs just like we do for ciphers
Browse files Browse the repository at this point in the history
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 <[email protected]>
Reviewed-by: Filippo Valsorda <[email protected]>
Reviewed-by: Han-Wen Nienhuys <[email protected]>
Auto-Submit: Filippo Valsorda <[email protected]>
Reviewed-by: David Chase <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
  • Loading branch information
drakkan authored and gopherbot committed Jul 31, 2023
1 parent d08e19b commit ddfa821
Show file tree
Hide file tree
Showing 2 changed files with 113 additions and 7 deletions.
90 changes: 90 additions & 0 deletions ssh/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -254,3 +254,93 @@ func TestNewClientConn(t *testing.T) {
})
}
}

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", "[email protected]"},
// 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)
}
})
}
}
30 changes: 23 additions & 7 deletions ssh/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -269,16 +269,16 @@ type Config struct {
// 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
}

Expand All @@ -295,7 +295,7 @@ func (c *Config) SetDefaults() {
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)
}
}
Expand All @@ -304,10 +304,26 @@ func (c *Config) SetDefaults() {
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
Expand Down

0 comments on commit ddfa821

Please sign in to comment.