device: fix races from changing private_key

Access keypair.sendNonce atomically.
Eliminate one unnecessary initialization to zero.

Mutate handshake.lastSentHandshake with the mutex held.

Co-authored-by: David Anderson <danderson@tailscale.com>
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>

diff --git a/device/device_test.go b/device/device_test.go
index e14391412ea72ad39b3bfc2d83d50229cb7ca069..6b7980bf47f2f0662b9b5f9103a6247b861d4bcd 100644 (file)
--- a/device/device_test.go
+++ b/device/device_test.go
@@ -215,6 +215,14 @@ func TestConcurrencySafety(t *testing.T) {
        }()
        warmup.Wait()
 
+       applyCfg := func(cfg io.ReadSeeker) {
+               cfg.Seek(0, io.SeekStart)
+               err := pair[0].dev.IpcSetOperation(cfg)
+               if err != nil {
+                       t.Fatal(err)
+               }
+       }
+
        // Change persistent_keepalive_interval concurrently with tunnel use.
        t.Run("persistentKeepaliveInterval", func(t *testing.T) {
                cfg := uapiCfg(
@@ -222,11 +230,24 @@ func TestConcurrencySafety(t *testing.T) {
                        "persistent_keepalive_interval", "1",
                )
                for i := 0; i < 1000; i++ {
-                       cfg.Seek(0, io.SeekStart)
-                       err := pair[0].dev.IpcSetOperation(cfg)
-                       if err != nil {
-                               t.Fatal(err)
-                       }
+                       applyCfg(cfg)
+               }
+       })
+
+       // Change private keys concurrently with tunnel use.
+       t.Run("privateKey", func(t *testing.T) {
+               bad := uapiCfg("private_key", "7777777777777777777777777777777777777777777777777777777777777777")
+               good := uapiCfg("private_key", "481eb0d8113a4a5da532d2c3e9c14b53c8454b34ab109676f6b58c2245e37b58")
+               // Set iters to a large number like 1000 to flush out data races quickly.
+               // Don't leave it large. That can cause logical races
+               // in which the handshake is interleaved with key changes
+               // such that the private key appears to be unchanging but
+               // other state gets reset, which can cause handshake failures like
+               // "Received packet with invalid mac1".
+               const iters = 1
+               for i := 0; i < iters; i++ {
+                       applyCfg(bad)
+                       applyCfg(good)
                }
        })
 

diff --git a/device/keypair.go b/device/keypair.go
index 254e69675a5677f690c4228e1c18c75983684f3a..27db779877b179824f232f68843cc954c8a9e019 100644 (file)
--- a/device/keypair.go
+++ b/device/keypair.go
@@ -23,7 +23,7 @@ import (
  */
 
 type Keypair struct {
-       sendNonce    uint64
+       sendNonce    uint64 // accessed atomically
        send         cipher.AEAD
        receive      cipher.AEAD
        replayFilter replay.Filter

diff --git a/device/noise-protocol.go b/device/noise-protocol.go
index 1dc854f464cba954543880d50382b483aa59f0ac..e34da8348d62f54f6fda855d7452b4870667e74e 100644 (file)
--- a/device/noise-protocol.go
+++ b/device/noise-protocol.go
@@ -566,7 +566,6 @@ func (peer *Peer) BeginSymmetricSession() error {
        setZero(recvKey[:])
 
        keypair.created = time.Now()
-       keypair.sendNonce = 0
        keypair.replayFilter.Reset()
        keypair.isInitiator = isInitiator
        keypair.localIndex = peer.handshake.localIndex

diff --git a/device/peer.go b/device/peer.go
index c0941603600295b8c37243880fc51889ae35f5c9..fe6de3345458c642df2ae12bd29cd6b088ba623b 100644 (file)
--- a/device/peer.go
+++ b/device/peer.go
@@ -249,16 +249,17 @@ func (peer *Peer) ExpireCurrentKeypairs() {
        handshake.mutex.Lock()
        peer.device.indexTable.Delete(handshake.localIndex)
        handshake.Clear()
-       handshake.mutex.Unlock()
        peer.handshake.lastSentHandshake = time.Now().Add(-(RekeyTimeout + time.Second))
+       handshake.mutex.Unlock()
 
        keypairs := &peer.keypairs
        keypairs.Lock()
        if keypairs.current != nil {
-               keypairs.current.sendNonce = RejectAfterMessages
+               atomic.StoreUint64(&keypairs.current.sendNonce, RejectAfterMessages)
        }
        if keypairs.next != nil {
-               keypairs.loadNext().sendNonce = RejectAfterMessages
+               next := keypairs.loadNext()
+               atomic.StoreUint64(&next.sendNonce, RejectAfterMessages)
        }
        keypairs.Unlock()
 }

diff --git a/device/send.go b/device/send.go
index 6b21708d7c9303f0b2c84bc6c0a4a6a85210938e..bc51fa6ba99ffd89fc41fdb0ba13e1656ca8eb68 100644 (file)
--- a/device/send.go
+++ b/device/send.go
@@ -403,7 +403,7 @@ NextPacket:
                                // check validity of newest key pair
 
                                keypair = peer.keypairs.Current()
-                               if keypair != nil && keypair.sendNonce < RejectAfterMessages {
+                               if keypair != nil && atomic.LoadUint64(&keypair.sendNonce) < RejectAfterMessages {
                                        if time.Since(keypair.created) < RejectAfterTime {
                                                break
                                        }