[crypto] ssh: ignore MAC if AEAD ciphers negotiated

Roland Shoemaker (Gerrit)

unread,

Mar 2, 2022, 11:25:56 AM3/2/22

to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com

Roland Shoemaker has uploaded this change for review.

ssh: ignore MAC if AEAD ciphers negotiated

If the server/client cipher chosen is one of the two AEAD ciphers that
we support (aes12...@openssh.com and chacha20...@openssh.com),
don't attempt to find a common MAC algorithm in findAgreedAlgorithms.
Similarly in newPacketCipher, don't attempt to generate a MAC key if we
are using a AEAD cipher.

Fixes golang/go#51406

Change-Id: Id48ae72f052cb0a0c597b32e9901a0f218e4161f
---
M ssh/common.go
M ssh/handshake_test.go
M ssh/transport.go
3 files changed, 62 insertions(+), 9 deletions(-)

diff --git a/ssh/common.go b/ssh/common.go
index 5ae2275..ec1f839 100644
--- a/ssh/common.go
+++ b/ssh/common.go
@@ -152,6 +152,11 @@
 	return 1 << 30
 }
 
+var aeadCiphers = map[string]bool{
+	gcmCipherID:        true,
+	chacha20Poly1305ID: true,
+}
+
 type algorithms struct {
 	kex     string
 	hostKey string
@@ -187,14 +192,18 @@
 		return
 	}
 
-	ctos.MAC, err = findCommon("client to server MAC", clientKexInit.MACsClientServer, serverKexInit.MACsClientServer)
-	if err != nil {
-		return
+	if !aeadCiphers[ctos.Cipher] {
+		ctos.MAC, err = findCommon("client to server MAC", clientKexInit.MACsClientServer, serverKexInit.MACsClientServer)
+		if err != nil {
+			return
+		}
 	}
 
-	stoc.MAC, err = findCommon("server to client MAC", clientKexInit.MACsServerClient, serverKexInit.MACsServerClient)
-	if err != nil {
-		return
+	if !aeadCiphers[stoc.Cipher] {
+		stoc.MAC, err = findCommon("server to client MAC", clientKexInit.MACsServerClient, serverKexInit.MACsServerClient)
+		if err != nil {
+			return
+		}
 	}
 
 	ctos.Compression, err = findCommon("client to server compression", clientKexInit.CompressionClientServer, serverKexInit.CompressionClientServer)
diff --git a/ssh/handshake_test.go b/ssh/handshake_test.go
index 02fbe83..46bfd6d 100644
--- a/ssh/handshake_test.go
+++ b/ssh/handshake_test.go
@@ -560,3 +560,26 @@
 		t.Errorf("got rekey after %dG write, want 64G", wgb)
 	}
 }
+
+func TestHandshakeAEADCipherNoMAC(t *testing.T) {
+	for _, cipher := range []string{chacha20Poly1305ID, gcmCipherID} {
+		checker := &syncChecker{
+			called: make(chan int, 1),
+		}
+		clientConf := &ClientConfig{
+			Config: Config{
+				Ciphers: []string{cipher},
+				MACs:    []string{},
+			},
+			HostKeyCallback: checker.Check,
+		}
+		trC, trS, err := handshakePair(clientConf, "addr", false)
+		if err != nil {
+			t.Fatalf("handshakePair: %v", err)
+		}
+		defer trC.Close()
+		defer trS.Close()
+
+		<-checker.called
+	}
+}
diff --git a/ssh/transport.go b/ssh/transport.go
index 49ddc2e..acf5a21 100644
--- a/ssh/transport.go
+++ b/ssh/transport.go
@@ -238,15 +238,19 @@
 // (to setup server->client keys) or clientKeys (for client->server keys).
 func newPacketCipher(d direction, algs directionAlgorithms, kex *kexResult) (packetCipher, error) {
 	cipherMode := cipherModes[algs.Cipher]
-	macMode := macModes[algs.MAC]
 
 	iv := make([]byte, cipherMode.ivSize)
 	key := make([]byte, cipherMode.keySize)
-	macKey := make([]byte, macMode.keySize)
 
 	generateKeyMaterial(iv, d.ivTag, kex)
 	generateKeyMaterial(key, d.keyTag, kex)
-	generateKeyMaterial(macKey, d.macKeyTag, kex)
+
+	var macKey []byte
+	if !aeadCiphers[algs.Cipher] {
+		macMode := macModes[algs.MAC]
+		macKey = make([]byte, macMode.keySize)
+		generateKeyMaterial(macKey, d.macKeyTag, kex)
+	}
 
 	return cipherModes[algs.Cipher].create(key, iv, macKey, algs)
 }

To view, visit change 389214. To unsubscribe, or for help writing mail filters, visit settings.

Roland Shoemaker (Gerrit)

unread,

Mar 2, 2022, 11:26:08 AM3/2/22

to goph...@pubsubhelper.golang.org, Filippo Valsorda, golang-co...@googlegroups.com

Attention is currently required from: Filippo Valsorda.

Patch set 1:Run-TryBot +1Trust +1

View Change

To view, visit change 389214. To unsubscribe, or for help writing mail filters, visit settings.

Filippo Valsorda (Gerrit)

unread,

Mar 12, 2022, 8:11:40 AM3/12/22

to Roland Shoemaker, goph...@pubsubhelper.golang.org, Filippo Valsorda, Gopher Robot, golang-co...@googlegroups.com

Attention is currently required from: Roland Shoemaker.

Patch set 1:Code-Review +2

View Change

1 comment:

Patchset:
- Patch Set #1:
  From PROTOCOL.chacha20poly1305
  If the chacha20...@openssh.com cipher is selected in key exchange, the offered MAC algorithms are ignored and no MAC is required to be negotiated.

To view, visit change 389214. To unsubscribe, or for help writing mail filters, visit settings.

Filippo Valsorda (Gerrit)

unread,

Mar 12, 2022, 8:11:47 AM3/12/22

to Roland Shoemaker, Filippo Valsorda, goph...@pubsubhelper.golang.org, golang-...@googlegroups.com, Gopher Robot, golang-co...@googlegroups.com

Filippo Valsorda submitted this change.

View Change

Approvals:
  Filippo Valsorda: Looks good to me, approved
  Roland Shoemaker: Trusted; Run TryBots
  Gopher Robot: TryBots succeeded

ssh: ignore MAC if AEAD ciphers negotiated

If the server/client cipher chosen is one of the two AEAD ciphers that
we support (aes12...@openssh.com and chacha20...@openssh.com),
don't attempt to find a common MAC algorithm in findAgreedAlgorithms.
Similarly in newPacketCipher, don't attempt to generate a MAC key if we
are using a AEAD cipher.

Fixes golang/go#51406

Change-Id: Id48ae72f052cb0a0c597b32e9901a0f218e4161f

Reviewed-on: https://go-review.googlesource.com/c/crypto/+/389214
Trust: Roland Shoemaker <rol...@golang.org>
Run-TryBot: Roland Shoemaker <rol...@golang.org>
TryBot-Result: Gopher Robot <go...@golang.org>
Reviewed-by: Filippo Valsorda <fil...@golang.org>


---
M ssh/common.go
M ssh/handshake_test.go
M ssh/transport.go

3 files changed, 67 insertions(+), 9 deletions(-)

To view, visit change 389214. To unsubscribe, or for help writing mail filters, visit settings.

Reply all

Reply to author

Forward