Make algorithms more robust to unsupported hashes (#185)

* make algorithms more robust to unsupported hashes

* fix TestTLS1PRFUnsupportedHash

* Apply suggestions from code review

Co-authored-by: Davis Goodin <[email protected]>

* preserve hashFuncHash error

* add continue in TestHKDF

---------

Co-authored-by: Davis Goodin <[email protected]>

Author: qmuntal

evp.go

@@ -16,6 +16,30 @@ import (
 // cacheMD is a cache of crypto.Hash to GO_EVP_MD_PTR.
 var cacheMD sync.Map
 
+// hashFuncHash calls fn() and returns its result.
+// If fn() panics, the panic is recovered and returned as an error.
+// This is used to avoid aborting the program when calling
+// an unsupported hash function. It is the caller's responsibility
+// to check the returned value.
+func hashFuncHash(fn func() hash.Hash) (h hash.Hash, err error) {
+	defer func() {
+		r := recover()
+		if r == nil {
+			return
+		}
+		h = nil
+		switch e := r.(type) {
+		case error:
+			err = e
+		case string:
+			err = errors.New(e)
+		default:
+			err = errors.New("unsupported panic")
+		}
+	}()
+	return fn(), nil
+}
+
 // hashToMD converts a hash.Hash implementation from this package to a GO_EVP_MD_PTR.
 func hashToMD(h hash.Hash) C.GO_EVP_MD_PTR {
 	var ch crypto.Hash

hash_test.go

@@ -369,3 +369,16 @@ func BenchmarkSHA256(b *testing.B) {
 		openssl.SHA256(buf)
 	}
 }
+
+// stubHash is a hash.Hash implementation that does nothing.
+type stubHash struct{}
+
+func newStubHash() hash.Hash {
+	return new(stubHash)
+}
+
+func (h *stubHash) Write(p []byte) (int, error) { return 0, nil }
+func (h *stubHash) Sum(in []byte) []byte        { return in }
+func (h *stubHash) Reset()                      {}
+func (h *stubHash) Size() int                   { return 0 }
+func (h *stubHash) BlockSize() int              { return 0 }

hkdf.go

@@ -32,13 +32,16 @@ func SupportsHKDF() bool {
 	}
 }
 
-func newHKDF(h func() hash.Hash, mode C.int) (*hkdf, error) {
+func newHKDF(fh func() hash.Hash, mode C.int) (*hkdf, error) {
 	if !SupportsHKDF() {
 		return nil, errUnsupportedVersion()
 	}
 
-	ch := h()
-	md := hashToMD(ch)
+	h, err := hashFuncHash(fh)
+	if err != nil {
+		return nil, err
+	}
+	md := hashToMD(h)
 	if md == nil {
 		return nil, errors.New("unsupported hash function")
 	}
@@ -75,7 +78,7 @@ func newHKDF(h func() hash.Hash, mode C.int) (*hkdf, error) {
 		}
 	}
 
-	c := &hkdf{ctx: ctx, hashLen: ch.Size()}
+	c := &hkdf{ctx: ctx, hashLen: h.Size()}
 	ctx = nil
 
 	runtime.SetFinalizer(c, (*hkdf).finalize)

hkdf_test.go

@@ -289,16 +289,16 @@ var hkdfTests = []hkdfTest{
 	},
 }
 
-func newHKDF(hash func() hash.Hash, secret, salt, info []byte) io.Reader {
+func newHKDF(hash func() hash.Hash, secret, salt, info []byte) (io.Reader, error) {
 	prk, err := openssl.ExtractHKDF(hash, secret, salt)
 	if err != nil {
-		panic(err)
+		return nil, err
 	}
 	r, err := openssl.ExpandHKDF(hash, prk, info)
 	if err != nil {
-		panic(err)
+		return nil, err
 	}
-	return r
+	return r, nil
 }
 
 func TestHKDF(t *testing.T) {
@@ -314,7 +314,11 @@ func TestHKDF(t *testing.T) {
 			t.Errorf("test %d: incorrect PRK: have %v, need %v.", i, prk, tt.prk)
 		}
 
-		hkdf := newHKDF(tt.hash, tt.master, tt.salt, tt.info)
+		hkdf, err := newHKDF(tt.hash, tt.master, tt.salt, tt.info)
+		if err != nil {
+			t.Errorf("test %d: error creating HKDF: %v.", i, err)
+			continue
+		}
 		out := make([]byte, len(tt.out))
 
 		n, err := io.ReadFull(hkdf, out)
@@ -347,7 +351,10 @@ func TestHKDFMultiRead(t *testing.T) {
 		t.Skip("HKDF is not supported")
 	}
 	for i, tt := range hkdfTests {
-		hkdf := newHKDF(tt.hash, tt.master, tt.salt, tt.info)
+		hkdf, err := newHKDF(tt.hash, tt.master, tt.salt, tt.info)
+		if err != nil {
+			t.Errorf("test %d: error creating HKDF: %v.", i, err)
+		}
 		out := make([]byte, len(tt.out))
 
 		for b := range len(tt.out) {
@@ -371,7 +378,10 @@ func TestHKDFLimit(t *testing.T) {
 	master := []byte{0x00, 0x01, 0x02, 0x03}
 	info := []byte{}
 
-	hkdf := newHKDF(hash, master, nil, info)
+	hkdf, err := newHKDF(hash, master, nil, info)
+	if err != nil {
+		t.Fatalf("error creating HKDF: %v.", err)
+	}
 	limit := hash().Size() * 255
 	out := make([]byte, limit)
 
@@ -387,3 +397,11 @@ func TestHKDFLimit(t *testing.T) {
 		t.Errorf("key expansion overflowed: n = %d, err = %v", n, err)
 	}
 }
+
+func TestHKDFUnsupportedHash(t *testing.T) {
+	// Test that newHKDF returns an error instead of panicking.
+	_, err := newHKDF(newStubHash, []byte{0x00, 0x01, 0x02, 0x03}, nil, []byte{})
+	if err == nil {
+		t.Error("expected error for unsupported hash")
+	}
+}

hmac.go

@@ -17,9 +17,9 @@ var OSSL_MAC_PARAM_DIGEST = C.CString("digest")
 // The function h must return a hash implemented by
 // OpenSSL (for example, h could be openssl.NewSHA256).
 // If h is not recognized, NewHMAC returns nil.
-func NewHMAC(h func() hash.Hash, key []byte) hash.Hash {
-	ch := h()
-	md := hashToMD(ch)
+func NewHMAC(fh func() hash.Hash, key []byte) hash.Hash {
+	h, _ := hashFuncHash(fh)
+	md := hashToMD(h)
 	if md == nil {
 		return nil
 	}
@@ -34,8 +34,8 @@ func NewHMAC(h func() hash.Hash, key []byte) hash.Hash {
 	}
 
 	hmac := &opensslHMAC{
-		size:      ch.Size(),
-		blockSize: ch.BlockSize(),
+		size:      h.Size(),
+		blockSize: h.BlockSize(),
 	}
 
 	switch vMajor {

hmac_test.go

@@ -1,40 +1,42 @@
-package openssl
+package openssl_test
 
 import (
 	"bytes"
 	"hash"
 	"testing"
+
+	"github.com/golang-fips/openssl/v2"
 )
 
 func TestHMAC(t *testing.T) {
 	var tests = []struct {
 		name string
 		fn   func() hash.Hash
 	}{
-		{"sha1", NewSHA1},
-		{"sha224", NewSHA224},
-		{"sha256", NewSHA256},
-		{"sha384", NewSHA384},
-		{"sha512", NewSHA512},
+		{"sha1", openssl.NewSHA1},
+		{"sha224", openssl.NewSHA224},
+		{"sha256", openssl.NewSHA256},
+		{"sha384", openssl.NewSHA384},
+		{"sha512", openssl.NewSHA512},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			t.Parallel()
-			h := NewHMAC(tt.fn, nil)
+			h := openssl.NewHMAC(tt.fn, nil)
 			if h == nil {
 				t.Skip("digest not supported")
 			}
 			h.Write([]byte("hello"))
 			sumHello := h.Sum(nil)
 
-			h = NewHMAC(tt.fn, nil)
+			h = openssl.NewHMAC(tt.fn, nil)
 			h.Write([]byte("hello world"))
 			sumHelloWorld := h.Sum(nil)
 
 			// Test that Sum has no effect on future Sum or Write operations.
 			// This is a bit unusual as far as usage, but it's allowed
 			// by the definition of Go hash.Hash, and some clients expect it to work.
-			h = NewHMAC(tt.fn, nil)
+			h = openssl.NewHMAC(tt.fn, nil)
 			h.Write([]byte("hello"))
 			if sum := h.Sum(nil); !bytes.Equal(sum, sumHello) {
 				t.Fatalf("1st Sum after hello = %x, want %x", sum, sumHello)
@@ -60,11 +62,20 @@ func TestHMAC(t *testing.T) {
 	}
 }
 
+func TestHMACUnsupportedHash(t *testing.T) {
+	// Test that NewHMAC returns nil for unsupported hashes
+	// instead of panicking.
+	h := openssl.NewHMAC(newStubHash, nil)
+	if h != nil {
+		t.Errorf("returned non-nil for unsupported hash")
+	}
+}
+
 func BenchmarkHMACSHA256_32(b *testing.B) {
 	b.StopTimer()
 	key := make([]byte, 32)
 	buf := make([]byte, 32)
-	h := NewHMAC(NewSHA256, key)
+	h := openssl.NewHMAC(openssl.NewSHA256, key)
 	b.SetBytes(int64(len(buf)))
 	b.StartTimer()
 	b.ReportAllocs()
@@ -83,7 +94,7 @@ func BenchmarkHMACNewWriteSum(b *testing.B) {
 	b.StartTimer()
 	b.ReportAllocs()
 	for i := 0; i < b.N; i++ {
-		h := NewHMAC(NewSHA256, make([]byte, 32))
+		h := openssl.NewHMAC(openssl.NewSHA256, make([]byte, 32))
 		h.Write(buf)
 		mac := h.Sum(nil)
 		buf[0] = mac[0]

pbkdf2.go

@@ -9,8 +9,12 @@ import (
 	"hash"
 )
 
-func PBKDF2(password, salt []byte, iter, keyLen int, h func() hash.Hash) ([]byte, error) {
-	md := hashToMD(h())
+func PBKDF2(password, salt []byte, iter, keyLen int, fh func() hash.Hash) ([]byte, error) {
+	h, err := hashFuncHash(fh)
+	if err != nil {
+		return nil, err
+	}
+	md := hashToMD(h)
 	if md == nil {
 		return nil, errors.New("unsupported hash function")
 	}

pbkdf2_test.go

@@ -158,6 +158,14 @@ func TestWithHMACSHA256(t *testing.T) {
 	testHash(t, openssl.NewSHA256, "SHA256", sha256TestVectors)
 }
 
+func TestWithUnsupportedHash(t *testing.T) {
+	// Test that PBKDF2 returns an error for unsupported hashes instead of panicking.
+	_, err := openssl.PBKDF2([]byte{1, 2}, []byte{3, 4}, 0, 2, newStubHash)
+	if err == nil {
+		t.Fatal("expected error")
+	}
+}
+
 var sink uint8
 
 func benchmark(b *testing.B, h func() hash.Hash) {

tls1prf.go

@@ -19,17 +19,21 @@ func SupportsTLS1PRF() bool {
 // TLS1PRF implements the TLS 1.0/1.1 pseudo-random function if h is nil,
 // else it implements the TLS 1.2 pseudo-random function.
 // The pseudo-random number will be written to result and will be of length len(result).
-func TLS1PRF(result, secret, label, seed []byte, h func() hash.Hash) error {
+func TLS1PRF(result, secret, label, seed []byte, fh func() hash.Hash) error {
 	var md C.GO_EVP_MD_PTR
-	if h == nil {
+	if fh == nil {
 		// TLS 1.0/1.1 PRF doesn't allow to specify the hash function,
 		// it always uses MD5SHA1. If h is nil, then assume
 		// that the caller wants to use TLS 1.0/1.1 PRF.
 		// OpenSSL detects this case by checking if the hash
 		// function is MD5SHA1.
 		md = cryptoHashToMD(crypto.MD5SHA1)
 	} else {
-		md = hashToMD(h())
+		h, err := hashFuncHash(fh)
+		if err != nil {
+			return err
+		}
+		md = hashToMD(h)
 	}
 	if md == nil {
 		return errors.New("unsupported hash function")

tls1prf_test.go

@@ -165,3 +165,17 @@ func TestTLS1PRF(t *testing.T) {
 		})
 	}
 }
+
+func TestTLS1PRFUnsupportedHash(t *testing.T) {
+	if !openssl.SupportsTLS1PRF() {
+		t.Skip("TLS PRF is not supported")
+	}
+
+	tt := tls1prfTests[0]
+	result := make([]byte, len(tt.out))
+	// Test that TLS1PRF returns an error for unsupported hashes instead of panicking.
+	err := openssl.TLS1PRF(result, tt.secret, tt.label, tt.seed, newStubHash)
+	if err == nil {
+		t.Errorf("expected an error for unsupported hash")
+	}
+}