crypto/internal/fips/hkdf: correctly set the service indicator for short salts

For #69536

Change-Id: Ibe2623311c8be5fb3e7411b33e61bf66d026e14d
Reviewed-on: https://go-review.googlesource.com/c/go/+/626877
Reviewed-by: Michael Knyszek <mknyszek@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Russ Cox <rsc@golang.org>
Auto-Submit: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Daniel McCarney <daniel@binaryparadox.net>

diff --git a/src/crypto/internal/fips/hkdf/hkdf.go b/src/crypto/internal/fips/hkdf/hkdf.go
index 914c955fd2f7d70763702cc96530416698d5e1a4..745a0525bbda5c52a50cd97e6771a21ac79810d8 100644 (file)
--- a/src/crypto/internal/fips/hkdf/hkdf.go
+++ b/src/crypto/internal/fips/hkdf/hkdf.go
@@ -17,6 +17,7 @@ func Extract[H fips.Hash](h func() H, secret, salt []byte) []byte {
                salt = make([]byte, h().Size())
        }
        extractor := hmac.New(h, salt)
+       hmac.MarkAsUsedInHKDF(extractor)
        extractor.Write(secret)
        return extractor.Sum(nil)
 }
@@ -24,6 +25,7 @@ func Extract[H fips.Hash](h func() H, secret, salt []byte) []byte {
 func Expand[H fips.Hash](h func() H, pseudorandomKey, info []byte, keyLen int) []byte {
        out := make([]byte, 0, keyLen)
        expander := hmac.New(h, pseudorandomKey)
+       hmac.MarkAsUsedInHKDF(expander)
        var counter uint8
        var buf []byte
 

diff --git a/src/crypto/internal/fips/hkdf/hkdf_test.go b/src/crypto/internal/fips/hkdf/hkdf_test.go
index f78d1e7af35b64d958a31dccaffe73cb9e3a420f..6bb2c6bc4ad39e5a2b55cd36a23d4f1ad12b050a 100644 (file)
--- a/src/crypto/internal/fips/hkdf/hkdf_test.go
+++ b/src/crypto/internal/fips/hkdf/hkdf_test.go
@@ -5,6 +5,8 @@ package hkdf_test
 
 import (
        "bytes"
+       "crypto/internal/boring"
+       "crypto/internal/fips"
        "crypto/internal/fips/hkdf"
        "crypto/md5"
        "crypto/sha1"
@@ -331,6 +333,32 @@ func TestHKDFLimit(t *testing.T) {
        hkdf.Key(hash, master, nil, info, limit+1)
 }
 
+func TestFIPSServiceIndicator(t *testing.T) {
+       if boring.Enabled {
+               t.Skip("in BoringCrypto mode HMAC is not from the Go FIPS module")
+       }
+
+       fips.ResetServiceIndicator()
+       hkdf.Key(sha256.New, []byte("YELLOW SUBMARINE"), nil, nil, 32)
+       if !fips.ServiceIndicator() {
+               t.Error("FIPS service indicator should be set")
+       }
+
+       // Key too short.
+       fips.ResetServiceIndicator()
+       hkdf.Key(sha256.New, []byte("key"), nil, nil, 32)
+       if fips.ServiceIndicator() {
+               t.Error("FIPS service indicator should not be set")
+       }
+
+       // Salt and info are short, which is ok, but translates to a short HMAC key.
+       fips.ResetServiceIndicator()
+       hkdf.Key(sha256.New, []byte("YELLOW SUBMARINE"), []byte("salt"), []byte("info"), 32)
+       if !fips.ServiceIndicator() {
+               t.Error("FIPS service indicator should be set")
+       }
+}
+
 func Benchmark16ByteMD5Single(b *testing.B) {
        benchmarkHKDFSingle(md5.New, 16, b)
 }

diff --git a/src/crypto/internal/fips/hmac/hmac.go b/src/crypto/internal/fips/hmac/hmac.go
index ef6136e15501ac1ceb859cc8d31dd432ea05d243..e47de385df2729aaba6aaed235b88f65ccadc03f 100644 (file)
--- a/src/crypto/internal/fips/hmac/hmac.go
+++ b/src/crypto/internal/fips/hmac/hmac.go
@@ -35,9 +35,25 @@ type HMAC struct {
        // copy of the key, but rather the marshaled state of outer/inner after
        // opad/ipad has been fed into it.
        marshaled bool
+
+       // forHKDF and keyLen are stored to inform the service indicator decision.
+       forHKDF bool
+       keyLen  int
 }
 
 func (h *HMAC) Sum(in []byte) []byte {
+       // Per FIPS 140-3 IG C.M, key lengths below 112 bits are only allowed for
+       // legacy use (i.e. verification only) and we don't support that. However,
+       // HKDF uses the HMAC key for the salt, which is allowed to be shorter.
+       if h.keyLen < 112/8 && !h.forHKDF {
+               fips.RecordNonApproved()
+       }
+       switch h.inner.(type) {
+       case *sha256.Digest, *sha512.Digest, *sha3.Digest:
+       default:
+               fips.RecordNonApproved()
+       }
+
        origLen := len(in)
        in = h.inner.Sum(in)
 
@@ -113,7 +129,7 @@ func (h *HMAC) Reset() {
 
 // New returns a new HMAC hash using the given [fips.Hash] type and key.
 func New[H fips.Hash](h func() H, key []byte) *HMAC {
-       hm := new(HMAC)
+       hm := &HMAC{keyLen: len(key)}
        hm.outer = h()
        hm.inner = h()
        unique := true
@@ -129,7 +145,6 @@ func New[H fips.Hash](h func() H, key []byte) *HMAC {
        if !unique {
                panic("crypto/hmac: hash generation function does not produce unique values")
        }
-       setServiceIndicator(hm.outer, key)
        blocksize := hm.inner.BlockSize()
        hm.ipad = make([]byte, blocksize)
        hm.opad = make([]byte, blocksize)
@@ -151,17 +166,7 @@ func New[H fips.Hash](h func() H, key []byte) *HMAC {
        return hm
 }
 
-func setServiceIndicator(h fips.Hash, key []byte) {
-       // Per FIPS 140-3 IG C.M, key lengths below 112 bits are only allowed for
-       // legacy use (i.e. verification only) and we don't support that.
-       if len(key) < 112/8 {
-               fips.RecordNonApproved()
-       }
-
-       switch h.(type) {
-       case *sha256.Digest, *sha512.Digest, *sha3.Digest:
-               fips.RecordApproved()
-       default:
-               fips.RecordNonApproved()
-       }
+// MarkAsUsedInHKDF records that this HMAC instance is used as part of HKDF.
+func MarkAsUsedInHKDF(h *HMAC) {
+       h.forHKDF = true
 }

diff --git a/src/crypto/internal/fips/indicator.go b/src/crypto/internal/fips/indicator.go
index 9e4f3c7845cce60cea65fb8981347bec81c77355..984b39ad2e4b51d8899b5fbe8fc97662334cd463 100644 (file)
--- a/src/crypto/internal/fips/indicator.go
+++ b/src/crypto/internal/fips/indicator.go
@@ -44,6 +44,11 @@ func ServiceIndicator() bool {
 
 // RecordApproved is an internal function that records the use of an approved
 // service. It does not override RecordNonApproved calls in the same span.
+//
+// It should be called by exposed functions that perform a whole cryptographic
+// alrgorithm (e.g. by Sum, not by New, unless a cryptographic Instantiate
+// algorithm is performed) and should be called after any checks that may cause
+// the function to error out or panic.
 func RecordApproved() {
        if getIndicator() == indicatorUnset {
                setIndicator(indicatorTrue)