Fix handles locking, key wiping, and curve checks

.github/workflows/build.yml

@@ -120,6 +120,9 @@ jobs:
       - name: Run wolftls tests (full config)
         run: go test ./wolftls
 
+      - name: Run sha256 tests (full config)
+        run: go test ./sha256
+
       - name: Run wolfx509 tests (full config)
         run: go test ./wolfx509
 

handles/README.md

@@ -16,7 +16,8 @@ direct-wrapper layer; opinionated lifecycle/marshal helpers live here.
 
 ## Algorithm tagging
 
-`EccKey` reports `Algorithm()` returning `handles.AlgECDSAP256` so
+`EccKey` reports `Algorithm()` returning `handles.AlgECDSAP256` for P-256
+keys (`handles.AlgUnknown` for any other curve) so
 algorithm-polymorphic consumers (e.g. `wolfx509.KeyHandle`) can
 dispatch on the key class without type-asserting. The `handles.Algorithm`
 enum is the shared algorithm taxonomy across `handles` and `wolfx509`.

handles/aes_handles.go

@@ -23,14 +23,22 @@ package handles
 import (
 	"errors"
 	"fmt"
+	"sync"
 
 	wolfSSL "github.com/wolfssl/go-wolfssl"
 )
 
 // AesGcmAEAD implements crypto/cipher.AEAD using wolfCrypt AES-256-GCM.
-// Create one with NewAesGcmAEAD.
+// Create one with NewAesGcmAEAD and call Zero when it is no longer needed.
 type AesGcmAEAD struct {
-	key [wolfSSL.AES_256_KEY_SIZE]byte
+	st *aesGcmState
+}
+
+// aesGcmState is shared by copies of an AesGcmAEAD so Zero reaches them all.
+type aesGcmState struct {
+	mu     sync.RWMutex
+	key    [wolfSSL.AES_256_KEY_SIZE]byte
+	zeroed bool
 }
 
 // errAesGcmAuth is returned from Open for any decrypt/verify failure
@@ -40,7 +48,39 @@ var errAesGcmAuth = errors.New("wolfSSL: AES-GCM authentication failed")
 
 // NewAesGcmAEAD returns an AesGcmAEAD keyed with a 32-byte AES-256 key.
 func NewAesGcmAEAD(key [wolfSSL.AES_256_KEY_SIZE]byte) *AesGcmAEAD {
-	return &AesGcmAEAD{key: key}
+	return &AesGcmAEAD{st: &aesGcmState{key: key}}
+}
+
+// Zero wipes the key held by a (best effort: the Go runtime may already have
+// copied it). Zero waits for in-flight Seal and Open calls; afterwards Seal
+// panics and Open fails.
+func (a *AesGcmAEAD) Zero() {
+	st := a.st
+	if st == nil {
+		return
+	}
+	st.mu.Lock()
+	defer st.mu.Unlock()
+	zeroBytes(st.key[:])
+	st.zeroed = true
+}
+
+// use read-locks the key state for one operation, or returns nil if the
+// AEAD was not built by NewAesGcmAEAD or has been zeroed.
+func (a *AesGcmAEAD) use() *aesGcmState {
+	st := a.st
+	if st == nil {
+		return nil
+	}
+	st.mu.RLock()
+	if st.zeroed {
+		st.mu.RUnlock()
+		return nil
+	}
+	if testHookAcquired != nil {
+		testHookAcquired()
+	}
+	return st
 }
 
 func (a *AesGcmAEAD) NonceSize() int { return wolfSSL.GCM_NONCE_MID_SZ }
@@ -54,18 +94,21 @@ func (a *AesGcmAEAD) Seal(dst, nonce, plaintext, additionalData []byte) []byte {
 	if len(nonce) != a.NonceSize() {
 		panic("wolfSSL/handles: incorrect nonce length given to AEAD")
 	}
+	st := a.use()
+	if st == nil {
+		panic("wolfSSL/handles: AEAD used after Zero or without NewAesGcmAEAD")
+	}
+	defer st.mu.RUnlock()
 	aes := wolfSSL.Wc_AesAllocAligned()
 	if aes == nil {
 		panic("wolfSSL/handles: Wc_AesAllocAligned returned nil")
 	}
-	defer func() {
-		wolfSSL.Wc_AesFree(aes)
-		wolfSSL.Wc_AesFreeAllocAligned(aes)
-	}()
+	defer wolfSSL.Wc_AesFreeAllocAligned(aes)
 	if ret := wolfSSL.Wc_AesInit(aes, nil, wolfSSL.INVALID_DEVID); ret != 0 {
 		panic(fmt.Sprintf("wolfSSL/handles: wc_AesInit failed: %d", ret))
 	}
-	if ret := wolfSSL.Wc_AesGcmSetKey(aes, a.key[:], wolfSSL.AES_256_KEY_SIZE); ret != 0 {
+	defer wolfSSL.Wc_AesFree(aes)
+	if ret := wolfSSL.Wc_AesGcmSetKey(aes, st.key[:], wolfSSL.AES_256_KEY_SIZE); ret != 0 {
 		panic(fmt.Sprintf("wolfSSL/handles: wc_AesGcmSetKey failed: %d", ret))
 	}
 
@@ -92,18 +135,21 @@ func (a *AesGcmAEAD) Open(dst, nonce, ciphertext, additionalData []byte) ([]byte
 	if len(ciphertext) < a.Overhead() {
 		return nil, errAesGcmAuth
 	}
+	st := a.use()
+	if st == nil {
+		return nil, errAesGcmAuth
+	}
+	defer st.mu.RUnlock()
 	aes := wolfSSL.Wc_AesAllocAligned()
 	if aes == nil {
 		return nil, errAesGcmAuth
 	}
-	defer func() {
-		wolfSSL.Wc_AesFree(aes)
-		wolfSSL.Wc_AesFreeAllocAligned(aes)
-	}()
+	defer wolfSSL.Wc_AesFreeAllocAligned(aes)
 	if ret := wolfSSL.Wc_AesInit(aes, nil, wolfSSL.INVALID_DEVID); ret != 0 {
 		return nil, errAesGcmAuth
 	}
-	if ret := wolfSSL.Wc_AesGcmSetKey(aes, a.key[:], wolfSSL.AES_256_KEY_SIZE); ret != 0 {
+	defer wolfSSL.Wc_AesFree(aes)
+	if ret := wolfSSL.Wc_AesGcmSetKey(aes, st.key[:], wolfSSL.AES_256_KEY_SIZE); ret != 0 {
 		return nil, errAesGcmAuth
 	}
 
@@ -113,6 +159,7 @@ func (a *AesGcmAEAD) Open(dst, nonce, ciphertext, additionalData []byte) ([]byte
 
 	plaintext := make([]byte, ctLen)
 	if ret := wolfSSL.Wc_AesGcmDecrypt(aes, plaintext, ct, nonce, tag, additionalData); ret != 0 {
+		zeroBytes(plaintext)
 		return nil, errAesGcmAuth
 	}
 	return append(dst, plaintext...), nil

handles/aes_handles_test.go

@@ -23,6 +23,8 @@ package handles
 import (
 	"bytes"
 	"crypto/cipher"
+	"strings"
+	"sync"
 	"testing"
 
 	wolfSSL "github.com/wolfssl/go-wolfssl"
@@ -159,3 +161,139 @@ func TestAesGcmAEAD_DstAppend(t *testing.T) {
 		t.Fatalf("Open: got %q, want %q", decrypted[len(outPrefix):], plaintext)
 	}
 }
+
+func TestAesGcmAEADOpenFailureWipesPlaintext(t *testing.T) {
+	var key [wolfSSL.AES_256_KEY_SIZE]byte
+	key[2] = 0x5a
+	a := NewAesGcmAEAD(key)
+	nonce := make([]byte, a.NonceSize())
+	plaintext := []byte("plaintext that must not linger after a failed Open")
+	ciphertext := a.Seal(nil, nonce, plaintext, nil)
+	ciphertext[len(ciphertext)-1] ^= 0x01
+
+	wiped := recordZeroBytes(t)
+	if _, err := a.Open(nil, nonce, ciphertext, nil); err == nil {
+		t.Fatal("Open accepted a tampered tag")
+	}
+	for _, b := range wiped() {
+		if len(b) == len(plaintext) && allZero(b) {
+			return
+		}
+	}
+	t.Fatal("decrypt output buffer was not wiped after authentication failure")
+}
+
+func TestAesGcmAEADZeroWipesKey(t *testing.T) {
+	var key [wolfSSL.AES_256_KEY_SIZE]byte
+	for i := range key {
+		key[i] = byte(i + 1)
+	}
+	a := NewAesGcmAEAD(key)
+	nonce := make([]byte, a.NonceSize())
+	ciphertext := a.Seal(nil, nonce, []byte("data"), nil)
+
+	a.Zero()
+	if !allZero(a.st.key[:]) {
+		t.Fatal("Zero left key bytes in the AEAD")
+	}
+	if _, err := a.Open(nil, nonce, ciphertext, nil); err == nil {
+		t.Fatal("Open succeeded after Zero")
+	}
+	defer func() {
+		if recover() == nil {
+			t.Fatal("Seal did not panic after Zero")
+		}
+	}()
+	a.Seal(nil, nonce, []byte("data"), nil)
+}
+
+func TestAesGcmAEADZeroReachesCopies(t *testing.T) {
+	var key [wolfSSL.AES_256_KEY_SIZE]byte
+	key[0] = 0x11
+	a := NewAesGcmAEAD(key)
+	b := *a
+	a.Zero()
+	defer func() {
+		if recover() == nil {
+			t.Fatal("copy of a zeroed AEAD could still Seal")
+		}
+	}()
+	b.Seal(nil, make([]byte, b.NonceSize()), []byte("data"), nil)
+}
+
+func TestAesGcmAEADZeroConcurrentWithUse(t *testing.T) {
+	var key [wolfSSL.AES_256_KEY_SIZE]byte
+	for i := range key {
+		key[i] = byte(0x80 + i)
+	}
+	a := NewAesGcmAEAD(key)
+	zeroKeyAEAD := NewAesGcmAEAD([wolfSSL.AES_256_KEY_SIZE]byte{})
+	nonce := make([]byte, a.NonceSize())
+	msg := []byte("concurrent zero")
+	ct0 := a.Seal(nil, nonce, msg, nil)
+	var ready, wg sync.WaitGroup
+	for i := 0; i < 8; i++ {
+		ready.Add(1)
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+			signalled := false
+			defer func() {
+				if !signalled {
+					ready.Done()
+				}
+				if r := recover(); r != nil {
+					if s, ok := r.(string); !ok || !strings.Contains(s, "after Zero") {
+						t.Errorf("unexpected panic: %v", r)
+					}
+				}
+			}()
+			for j := 0; j < 50; j++ {
+				ct := a.Seal(nil, nonce, msg, nil)
+				if _, err := zeroKeyAEAD.Open(nil, nonce, ct, nil); err == nil {
+					t.Error("Seal used a wiped key")
+				}
+				_, _ = a.Open(nil, nonce, ct, nil)
+				if !signalled {
+					signalled = true
+					ready.Done()
+				}
+			}
+		}()
+	}
+	ready.Wait()
+	a.Zero()
+	wg.Wait()
+	if _, err := a.Open(nil, nonce, ct0, nil); err == nil {
+		t.Fatal("Open succeeded after Zero")
+	}
+}
+
+func TestAesGcmAEADZeroWaitsForInFlightSeal(t *testing.T) {
+	var key [wolfSSL.AES_256_KEY_SIZE]byte
+	for i := range key {
+		key[i] = byte(0x40 + i)
+	}
+	a := NewAesGcmAEAD(key)
+	nonce := make([]byte, a.NonceSize())
+	msg := []byte("in flight")
+
+	entered, release := holdFirstAcquire(t)
+	sealed := make(chan []byte, 1)
+	go func() { sealed <- a.Seal(nil, nonce, msg, nil) }()
+	<-entered
+	zeroed := make(chan struct{})
+	go func() {
+		a.Zero()
+		close(zeroed)
+	}()
+	assertBlocked(t, zeroed, release, "Zero")
+	ct := <-sealed
+	pt, err := NewAesGcmAEAD(key).Open(nil, nonce, ct, nil)
+	if err != nil || !bytes.Equal(pt, msg) {
+		t.Fatalf("in-flight Seal did not use the original key: %v", err)
+	}
+	if _, err := a.Open(nil, nonce, ct, nil); err == nil {
+		t.Fatal("Open succeeded after Zero")
+	}
+}