Use NewGCMTLS (when using experiment boringcrypto) #803
Conversation
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
noiseutil/boring_test.go
Outdated
|
|
||
| // Ensure NewGCMTLS validates the nonce is non-repeating | ||
| func TestNewGCMTLS(t *testing.T) { | ||
| // Test Case 16 from http://csrc.nist.gov/groups/ST/toolkit/BCM/documents/proposedmodes/gcm/gcm-spec.pdf |
There was a problem hiding this comment.
This link is now a redirect to https://csrc.nist.gov/projects/block-cipher-techniques/bcm, which eventually links to https://nvlpubs.nist.gov/nistpubs/Legacy/SP/nistspecialpublication800-38d.pdf for GCM. But I didn't see any test cases listed therein – is there a way I see the case referenced here?
There was a problem hiding this comment.
hmmm, let me try to track it down. I actually cribbed this from the boringssl tests IIRC, so maybe I should just link there.
wadey
added
the
needs-defined-net-review
| //ci.writeLock.Lock() | ||
| if noiseutil.EncryptLockNeeded { | ||
| // NOTE: for goboring AESGCMTLS we need to lock because of the nonce check | ||
| ci.writeLock.Lock() |
There was a problem hiding this comment.
IIUC the Lock guarantees that the ci.messageCounter.Add(1) line just below, which increments the message counter, is kept in lock step with the Seal() call on the actual encryption keys, called through EncryptDanger just before the lock is released.
If that's correct, then I think this lock is also required in the SendVia() implementation, which also calls EncryptDanger (and therefore Seal()) to authenticate a relayed packet.
SendVia() is the only other active invocation of EncryptDanger() that I see in Nebula code.
* 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
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)