From e0553822b0c8c7d48509482f4904fa5f0945feac Mon Sep 17 00:00:00 2001 From: Wade Simmons Date: Wed, 5 Apr 2023 11:08:23 -0400 Subject: [PATCH] Use NewGCMTLS (when using experiment boringcrypto) (#803) * Use NewGCMTLS (when using experiment boringcrypto) This change only affects builds built using `GOEXPERIMENT=boringcrypto`. When built with this experiment, we use the NewGCMTLS() method exposed by goboring, which validates that the nonce is strictly monotonically increasing. This is the TLS 1.2 specification for nonce generation (which also matches the method used by the Noise Protocol) - https://github.com/golang/go/blob/go1.19/src/crypto/tls/cipher_suites.go#L520-L522 - https://github.com/golang/go/blob/go1.19/src/crypto/internal/boring/aes.go#L235-L237 - https://github.com/golang/go/blob/go1.19/src/crypto/internal/boring/aes.go#L250 - https://github.com/google/boringssl/blob/ae223d6138807a13006342edfeef32e813246b39/include/openssl/aead.h#L379-L381 - https://github.com/google/boringssl/blob/ae223d6138807a13006342edfeef32e813246b39/crypto/fipsmodule/cipher/e_aes.c#L1082-L1093 * need to lock around EncryptDanger in SendVia * fix link to test vector --- connection_state.go | 3 +- inside.go | 23 +++++++++-- noiseutil/boring.go | 80 +++++++++++++++++++++++++++++++++++++ noiseutil/boring_test.go | 39 ++++++++++++++++++ noiseutil/notboring.go | 14 +++++++ noiseutil/notboring_test.go | 15 +++++++ 6 files changed, 169 insertions(+), 5 deletions(-) create mode 100644 noiseutil/boring.go create mode 100644 noiseutil/boring_test.go create mode 100644 noiseutil/notboring.go create mode 100644 noiseutil/notboring_test.go diff --git a/connection_state.go b/connection_state.go index 2a7be15..4f8a577 100644 --- a/connection_state.go +++ b/connection_state.go @@ -9,6 +9,7 @@ import ( "github.com/flynn/noise" "github.com/sirupsen/logrus" "github.com/slackhq/nebula/cert" + "github.com/slackhq/nebula/noiseutil" ) const ReplayWindow = 1024 @@ -28,7 +29,7 @@ type ConnectionState struct { } func (f *Interface) newConnectionState(l *logrus.Logger, initiator bool, pattern noise.HandshakePattern, psk []byte, pskStage int) *ConnectionState { - cs := noise.NewCipherSuite(noise.DH25519, noise.CipherAESGCM, noise.HashSHA256) + cs := noise.NewCipherSuite(noise.DH25519, noiseutil.CipherAESGCM, noise.HashSHA256) if f.cipher == "chachapoly" { cs = noise.NewCipherSuite(noise.DH25519, noise.CipherChaChaPoly, noise.HashSHA256) } diff --git a/inside.go b/inside.go index 21e2ab7..457fcac 100644 --- a/inside.go +++ b/inside.go @@ -6,6 +6,7 @@ import ( "github.com/slackhq/nebula/firewall" "github.com/slackhq/nebula/header" "github.com/slackhq/nebula/iputil" + "github.com/slackhq/nebula/noiseutil" "github.com/slackhq/nebula/udp" ) @@ -256,6 +257,11 @@ func (f *Interface) SendVia(viaIfc interface{}, ) { via := viaIfc.(*HostInfo) relay := relayIfc.(*Relay) + + if noiseutil.EncryptLockNeeded { + // NOTE: for goboring AESGCMTLS we need to lock because of the nonce check + via.ConnectionState.writeLock.Lock() + } c := via.ConnectionState.messageCounter.Add(1) out = header.Encode(out, header.Version, header.Message, header.MessageRelay, relay.RemoteIndex, c) @@ -264,6 +270,9 @@ func (f *Interface) SendVia(viaIfc interface{}, // Authenticate the header and payload, but do not encrypt for this message type. // The payload consists of the inner, unencrypted Nebula header, as well as the end-to-end encrypted payload. if len(out)+len(ad)+via.ConnectionState.eKey.Overhead() > cap(out) { + if noiseutil.EncryptLockNeeded { + via.ConnectionState.writeLock.Unlock() + } via.logger(f.l). WithField("outCap", cap(out)). WithField("payloadLen", len(ad)). @@ -285,6 +294,9 @@ func (f *Interface) SendVia(viaIfc interface{}, var err error out, err = via.ConnectionState.eKey.EncryptDanger(out, out, nil, c, nb) + if noiseutil.EncryptLockNeeded { + via.ConnectionState.writeLock.Unlock() + } if err != nil { via.logger(f.l).WithError(err).Info("Failed to EncryptDanger in sendVia") return @@ -313,8 +325,10 @@ func (f *Interface) sendNoMetrics(t header.MessageType, st header.MessageSubType out = out[header.Len:] } - //TODO: enable if we do more than 1 tun queue - //ci.writeLock.Lock() + if noiseutil.EncryptLockNeeded { + // NOTE: for goboring AESGCMTLS we need to lock because of the nonce check + ci.writeLock.Lock() + } c := ci.messageCounter.Add(1) //l.WithField("trace", string(debug.Stack())).Error("out Header ", &Header{Version, t, st, 0, hostinfo.remoteIndexId, c}, p) @@ -335,8 +349,9 @@ func (f *Interface) sendNoMetrics(t header.MessageType, st header.MessageSubType var err error out, err = ci.eKey.EncryptDanger(out, out, p, c, nb) - //TODO: see above note on lock - //ci.writeLock.Unlock() + if noiseutil.EncryptLockNeeded { + ci.writeLock.Unlock() + } if err != nil { hostinfo.logger(f.l).WithError(err). WithField("udpAddr", remote).WithField("counter", c). diff --git a/noiseutil/boring.go b/noiseutil/boring.go new file mode 100644 index 0000000..e9ad19b --- /dev/null +++ b/noiseutil/boring.go @@ -0,0 +1,80 @@ +//go:build boringcrypto +// +build boringcrypto + +package noiseutil + +import ( + "crypto/aes" + "crypto/cipher" + "encoding/binary" + + // unsafe needed for go:linkname + _ "unsafe" + + "github.com/flynn/noise" +) + +// EncryptLockNeeded indicates if calls to Encrypt need a lock +// This is true for boringcrypto because the Seal function verifies that the +// nonce is strictly increasing. +const EncryptLockNeeded = true + +// NewGCMTLS is no longer exposed in go1.19+, so we need to link it in +// See: https://github.com/golang/go/issues/56326 +// +// NewGCMTLS is the internal method used with boringcrypto that provices a +// validated mode of AES-GCM which enforces the nonce is strictly +// monotonically increasing. This is the TLS 1.2 specification for nonce +// generation (which also matches the method used by the Noise Protocol) +// +// - https://github.com/golang/go/blob/go1.19/src/crypto/tls/cipher_suites.go#L520-L522 +// - https://github.com/golang/go/blob/go1.19/src/crypto/internal/boring/aes.go#L235-L237 +// - https://github.com/golang/go/blob/go1.19/src/crypto/internal/boring/aes.go#L250 +// - https://github.com/google/boringssl/blob/ae223d6138807a13006342edfeef32e813246b39/include/openssl/aead.h#L379-L381 +// - https://github.com/google/boringssl/blob/ae223d6138807a13006342edfeef32e813246b39/crypto/fipsmodule/cipher/e_aes.c#L1082-L1093 +// +//go:linkname newGCMTLS crypto/internal/boring.NewGCMTLS +func newGCMTLS(c cipher.Block) (cipher.AEAD, error) + +type cipherFn struct { + fn func([32]byte) noise.Cipher + name string +} + +func (c cipherFn) Cipher(k [32]byte) noise.Cipher { return c.fn(k) } +func (c cipherFn) CipherName() string { return c.name } + +// CipherAESGCM is the AES256-GCM AEAD cipher (using NewGCMTLS when GoBoring is present) +var CipherAESGCM noise.CipherFunc = cipherFn{cipherAESGCMBoring, "AESGCM"} + +func cipherAESGCMBoring(k [32]byte) noise.Cipher { + c, err := aes.NewCipher(k[:]) + if err != nil { + panic(err) + } + gcm, err := newGCMTLS(c) + if err != nil { + panic(err) + } + return aeadCipher{ + gcm, + func(n uint64) []byte { + var nonce [12]byte + binary.BigEndian.PutUint64(nonce[4:], n) + return nonce[:] + }, + } +} + +type aeadCipher struct { + cipher.AEAD + nonce func(uint64) []byte +} + +func (c aeadCipher) Encrypt(out []byte, n uint64, ad, plaintext []byte) []byte { + return c.Seal(out, c.nonce(n), plaintext, ad) +} + +func (c aeadCipher) Decrypt(out []byte, n uint64, ad, ciphertext []byte) ([]byte, error) { + return c.Open(out, c.nonce(n), ciphertext, ad) +} diff --git a/noiseutil/boring_test.go b/noiseutil/boring_test.go new file mode 100644 index 0000000..bc5ff50 --- /dev/null +++ b/noiseutil/boring_test.go @@ -0,0 +1,39 @@ +//go:build boringcrypto +// +build boringcrypto + +package noiseutil + +import ( + "encoding/hex" + "testing" + + "github.com/stretchr/testify/assert" +) + +// Ensure NewGCMTLS validates the nonce is non-repeating +func TestNewGCMTLS(t *testing.T) { + // Test Case 16 from GCM Spec: + // - (now dead link): http://csrc.nist.gov/groups/ST/toolkit/BCM/documents/proposedmodes/gcm/gcm-spec.pdf + // - as listed in boringssl tests: https://github.com/google/boringssl/blob/fips-20220613/crypto/cipher_extra/test/cipher_tests.txt#L412-L418 + key, _ := hex.DecodeString("feffe9928665731c6d6a8f9467308308feffe9928665731c6d6a8f9467308308") + iv, _ := hex.DecodeString("cafebabefacedbaddecaf888") + plaintext, _ := hex.DecodeString("d9313225f88406e5a55909c5aff5269a86a7a9531534f7da2e4c303d8a318a721c3c0c95956809532fcf0e2449a6b525b16aedf5aa0de657ba637b39") + aad, _ := hex.DecodeString("feedfacedeadbeeffeedfacedeadbeefabaddad2") + expected, _ := hex.DecodeString("522dc1f099567d07f47f37a32a84427d643a8cdcbfe5c0c97598a2bd2555d1aa8cb08e48590dbb3da7b08b1056828838c5f61e6393ba7a0abcc9f662") + expectedTag, _ := hex.DecodeString("76fc6ece0f4e1768cddf8853bb2d551b") + + expected = append(expected, expectedTag...) + + var keyArray [32]byte + copy(keyArray[:], key) + c := CipherAESGCM.Cipher(keyArray) + aead := c.(aeadCipher).AEAD + + dst := aead.Seal([]byte{}, iv, plaintext, aad) + assert.Equal(t, expected, dst) + + // We expect this to fail since we are re-encrypting with a repeat IV + assert.PanicsWithError(t, "boringcrypto: EVP_AEAD_CTX_seal failed", func() { + dst = aead.Seal([]byte{}, iv, plaintext, aad) + }) +} diff --git a/noiseutil/notboring.go b/noiseutil/notboring.go new file mode 100644 index 0000000..be746f4 --- /dev/null +++ b/noiseutil/notboring.go @@ -0,0 +1,14 @@ +//go:build !boringcrypto +// +build !boringcrypto + +package noiseutil + +import ( + "github.com/flynn/noise" +) + +// EncryptLockNeeded indicates if calls to Encrypt need a lock +const EncryptLockNeeded = false + +// CipherAESGCM is the standard noise.CipherAESGCM when boringcrypto is not enabled +var CipherAESGCM noise.CipherFunc = noise.CipherAESGCM diff --git a/noiseutil/notboring_test.go b/noiseutil/notboring_test.go new file mode 100644 index 0000000..a27dbbd --- /dev/null +++ b/noiseutil/notboring_test.go @@ -0,0 +1,15 @@ +//go:build !boringcrypto +// +build !boringcrypto + +package noiseutil + +import ( + // NOTE: We have to force these imports here or boring_test.go fails to + // compile correctly. This seems to be a Go bug: + // + // $ GOEXPERIMENT=boringcrypto go test ./noiseutil + // # github.com/slackhq/nebula/noiseutil + // boring_test.go:10:2: cannot find package + + _ "github.com/stretchr/testify/assert" +)