Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TLS 1.3 HKDF-Extract with empty salt fails when using the KeyPair FIPS Provider #253

Open
samiponkanenssh opened this issue Feb 21, 2025 · 5 comments · May be fixed by #260
Open

TLS 1.3 HKDF-Extract with empty salt fails when using the KeyPair FIPS Provider #253

samiponkanenssh opened this issue Feb 21, 2025 · 5 comments · May be fixed by #260

Comments

@samiponkanenssh
Copy link

As instructed by @qmuntal I am opening this issue here in golang-fips/openssl.

The issue is that TLS 1.3 does not work in binaries built with microsoft/go and -tags goexperiment.opensslcrypto when using the KeyPair FIPS Provider for OpenSSL 3 (https://csrc.nist.gov/projects/cryptographic-module-validation-program/certificate/4724).

I have been in contact with Keypair and the root cause is that Keypair FIPS provider enforces a minimum HMAC key length of 112 bits. However, when crypto/tls calls HKDF Extract() it sometimes does this with nil salt and consequently when that salt is used as the HMAC key, the FIPS provider's enforcement kicks in and triggers a panic.

The issue can be reproduced with this simple app:

package main

import (
	"crypto/tls"
	"fmt"
	"os"
)

func main() {
	if len(os.Args) <= 1 {
		panic("missing dst address")
	}

	c, err := tls.Dial("tcp", os.Args[1],
		&tls.Config{
			MinVersion: tls.VersionTLS13,
		})
	if err != nil {
		panic(err)
	}
	fmt.Printf("successfully connected to %s (%s)\n", os.Args[1], c.RemoteAddr().String())
	c.Close()
}

The panic looks following on microsoft/go1.23.6-20250211.6:

$ LD_LIBRARY_PATH=~/src/keypair/openssl-3.0.10-install/lib64 ./test www.google.com:443
panic: tls: HKDF-Extract invocation failed unexpectedly: EVP_PKEY_derive
        openssl error(s):

goroutine 1 [running]:
crypto/tls.(*cipherSuiteTLS13).extract(0x80b460?, {0x0?, 0x0?, 0x0?}, {0x0?, 0x0?, 0x0?})
        /usr/local/lib/microsoftgo/go1.23.6-20250211.6/src/crypto/tls/key_schedule.go:97 +0x193
crypto/tls.(*clientHandshakeStateTLS13).establishHandshakeKeys(0xc0001d9a08)
        /usr/local/lib/microsoftgo/go1.23.6-20250211.6/src/crypto/tls/handshake_client_tls13.go:514 +0x2ce
crypto/tls.(*clientHandshakeStateTLS13).handshake(0xc0001d9a08)
        /usr/local/lib/microsoftgo/go1.23.6-20250211.6/src/crypto/tls/handshake_client_tls13.go:132 +0x725
crypto/tls.(*Conn).clientHandshake(0xc000004708, {0x6af338, 0xc0000be140})
        /usr/local/lib/microsoftgo/go1.23.6-20250211.6/src/crypto/tls/handshake_client.go:375 +0x845
crypto/tls.(*Conn).handshakeContext(0xc000004708, {0x6af178, 0x833040})
        /usr/local/lib/microsoftgo/go1.23.6-20250211.6/src/crypto/tls/conn.go:1568 +0x3a6
crypto/tls.(*Conn).HandshakeContext(...)
        /usr/local/lib/microsoftgo/go1.23.6-20250211.6/src/crypto/tls/conn.go:1508
crypto/tls.dial({0x6af178?, 0x833040?}, 0xc000085e30, {0x645d46, 0x3}, {0x7ffc4884e085, 0x12}, 0xc00019a000)
        /usr/local/lib/microsoftgo/go1.23.6-20250211.6/src/crypto/tls/tls.go:159 +0x3a5
crypto/tls.DialWithDialer(...)
        /usr/local/lib/microsoftgo/go1.23.6-20250211.6/src/crypto/tls/tls.go:119
crypto/tls.Dial({0x645d46?, 0x100451089?}, {0x7ffc4884e085?, 0xc000085f40?}, 0x40f36b?)
        /usr/local/lib/microsoftgo/go1.23.6-20250211.6/src/crypto/tls/tls.go:173 +0x7a
main.main()
        /home/xxxxx/tls.go:14 +0x6f

Similar panic is triggered in microsoft/go1.24.0-20250212.5:

$ LD_LIBRARY_PATH=~/src/keypair/openssl-3.0.10-install/lib64 ./test www.google.com:443
panic: EVP_KDF_derive
        openssl error(s):

goroutine 1 [running]:
crypto/tls/internal/tls13.extract[...](0xc000290110?, {0x0?, 0xc00002f6b8?, 0x50d39b?}, {0x0?, 0x0?, 0x0?})
        /usr/local/lib/microsoftgo/go1.24.0-20250212.5/src/crypto/tls/internal/tls13/tls13.go:52 +0xbd
crypto/tls/internal/tls13.NewEarlySecret[...](0xc000290110, {0x0, 0x0?, 0x0?})
        /usr/local/lib/microsoftgo/go1.24.0-20250212.5/src/crypto/tls/internal/tls13/tls13.go:83 +0x35
crypto/tls.(*clientHandshakeStateTLS13).establishHandshakeKeys(0xc00002fa00)
        /usr/local/lib/microsoftgo/go1.24.0-20250212.5/src/crypto/tls/handshake_client_tls13.go:519 +0x2ac
crypto/tls.(*clientHandshakeStateTLS13).handshake(0xc00002fa00)
        /usr/local/lib/microsoftgo/go1.24.0-20250212.5/src/crypto/tls/handshake_client_tls13.go:134 +0x73e
crypto/tls.(*Conn).clientHandshake(0xc00028a008, {0x6e4b40, 0xc00028e000})
        /usr/local/lib/microsoftgo/go1.24.0-20250212.5/src/crypto/tls/handshake_client.go:379 +0x810
crypto/tls.(*Conn).handshakeContext(0xc00028a008, {0x6e4898, 0x8b2000})
        /usr/local/lib/microsoftgo/go1.24.0-20250212.5/src/crypto/tls/conn.go:1568 +0x39a
crypto/tls.(*Conn).HandshakeContext(...)
        /usr/local/lib/microsoftgo/go1.24.0-20250212.5/src/crypto/tls/conn.go:1508
crypto/tls.dial({0x6e4898?, 0x8b2000?}, 0xc00007be30, {0x686261, 0x3}, {0x7fff1e8f1085, 0x12}, 0xc000138000)
        /usr/local/lib/microsoftgo/go1.24.0-20250212.5/src/crypto/tls/tls.go:159 +0x3a5
crypto/tls.DialWithDialer(...)
        /usr/local/lib/microsoftgo/go1.24.0-20250212.5/src/crypto/tls/tls.go:119
crypto/tls.Dial({0x686261?, 0x457b49?}, {0x7fff1e8f1085?, 0xc00007bf40?}, 0x416cc8?)
        /usr/local/lib/microsoftgo/go1.24.0-20250212.5/src/crypto/tls/tls.go:173 +0x7a
main.main()
        /home/xxxxx/tls.go:14 +0x6f

According to RFC 5869 Section 2.2, the salt "if not provided, it is set to a string of HashLen zeros.".

Patching the OpenSSL HKDF bindings in the golang-fips/openssl/v2/hkdf.go to convert nil salt to zeroed buffer does fix the issue.

For microsoft/go1.23.6-20250211.6 the patch looks like following:

diff -ruN go1.23.6-20250211.6.orig/src/vendor/github.com/golang-fips/openssl/v2/hkdf.go go1.23.6-20250211.6/src/vendor/github.com/golang-fips/openssl/v2/hkdf.go
--- go1.23.6-20250211.6.orig/src/vendor/github.com/golang-fips/openssl/v2/hkdf.go       2025-02-21 11:58:44.705750417 +0200
+++ go1.23.6-20250211.6/src/vendor/github.com/golang-fips/openssl/v2/hkdf.go    2025-02-21 12:00:59.960965835 +0200
@@ -110,6 +110,11 @@
        if err != nil {
                return nil, err
        }
+
+       if salt == nil {
+               salt = make([]byte, h().Size())
+       }
+
        switch vMajor {
        case 3:
                if C.go_openssl_EVP_PKEY_CTX_set1_hkdf_key(c.ctx,

For microsoft/go1.24.0-20250212.5 the patch looks like following, though I have not done very extensive testing on go1.24:

diff -ruN go1.24.0-20250212.5.orig/src/vendor/github.com/golang-fips/openssl/v2/hkdf.go go1.24.0-20250212.5/src/vendor/github.com/golang-fips/openssl/v2/hkdf.go
--- go1.24.0-20250212.5.orig/src/vendor/github.com/golang-fips/openssl/v2/hkdf.go       2025-02-21 12:20:33.666148363 +0200
+++ go1.24.0-20250212.5/src/vendor/github.com/golang-fips/openssl/v2/hkdf.go    2025-02-21 12:22:19.831310298 +0200
@@ -119,6 +119,10 @@
                return nil, err
        }
 
+       if salt == nil {
+               salt = make([]byte, h().Size())
+       }
+
        switch vMajor {
        case 1:
                ctx, err := newHKDFCtx1(md, C.GO_EVP_KDF_HKDF_MODE_EXTRACT_ONLY, secret, salt, nil, nil)
@qmuntal
Copy link
Collaborator

qmuntal commented Feb 21, 2025

Thanks for reporting and for the detailed reproducer.

According to RFC 5869 Section 2.2, the salt "if not provided, it is set to a string of HashLen zeros.".

Our HKDF test suite contains at least on test that checks openssl.ExtractHKDF can be called with an empty salt. CI exercises these tests with OpenSSL 1 and OpenSSL 3 (with default, fips, and SCOSSL providers). All these seems to accept empty HKDF salts. I suspect that what they are doing is internally allocating a zeroed string with the required size.

In fact, OpenSSL TLS 1.3 implementation behavior is similar to ours, it only sets the salt if it is not nil: https://github.com/openssl/openssl/blob/3240427a8530f5aa6070f135e954e20e591fa132/ssl/tls13_enc.c#L206-L208

On the other hand, the OpenSSL HKDF documentation says the following, contradicting the implemented behavior:

The digest, key and salt values must be set before a key is derived otherwise an error will occur.

Having said this, I would prefer not to force an allocation on every OpenSSL user just because one provider is not compatible (unless I'm wrong) with the OpenSSL behavior. Let me push back a bit on this. Wouldn't it make more sense for the Keypair FIPS provider to follow suite with OpenSSL built-in providers?

@simo5 @derekparker you are more involved with the OpenSSL project. What do you think?

@qmuntal qmuntal changed the title TLS 1.3 HKDF non-standard behavior TLS 1.3 HKDF-Extract with empty salt fails when using the KeyPair FIPS Provider Feb 21, 2025
@samiponkanenssh
Copy link
Author

If you decide this issue if worth fixing, then I can open PRs.

I believe the performance concerns can be handled with taking a slice from a preallocated buffer instead of allocating a new buffer per call.

@qmuntal
Copy link
Collaborator

qmuntal commented Feb 25, 2025

My gut feeling is that the Keypair FIPS provider should accept empty salts regardless of what we do here, if it wants to be compatible with the rest of the OpenSSL ecosystem.

On the other hand, the OpenSSL HKDF docs do mention that the salt is mandatory even when that is not enforced, so we better comply with what it says on the tin. @samiponkanenssh please send that PR 😄.

@samiponkanenssh
Copy link
Author

@qmuntal The PR I just opened fixes the nil salt issue, which I am seeing with microsoft/go TLS 1.3.

I noticed that there is a slightly different yet similar problem in the hkdf_test.go. A few test vectors use salt = []byte{}, which triggers the same error in the Keypair FIPS provider. However, it seems the microsoft/go crypt/tls package would never pass such params to golang-fips/openssl.

There is also a separate problem in hkdf_test.go as it uses test vectors with SHA1 hash.

Would you prefer me changing the PR to handle also the len(salt) == 0 case, or changing the unit tests to pass salt = nil?

@qmuntal
Copy link
Collaborator

qmuntal commented Mar 3, 2025

Would you prefer me changing the PR to handle also the len(salt) == 0 case, or changing the unit tests to pass salt = nil?

Better treat len(salt) == 0 as salt == nil. The Go standard library does that too, so we should follow suite.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants