Trail of bits 018 (#9674)

* TOB-018 remediation

* Make key derivation an optional config flag, off by default, for backwards compatibility

* Fix unit tests

* Address some feedback

* Set config on unit test

* Fix another test failure

* One more conf fail

* Switch one of the test cases to not use a derive dkey

* wip

* comments

command/agent.go

@@ -349,6 +349,7 @@ func (c *AgentCommand) Run(args []string) int {
 					Client:  client,
 					WrapTTL: sc.WrapTTL,
 					DHType:  sc.DHType,
+					DeriveKey: sc.DeriveKey,
 					DHPath:  sc.DHPath,
 					AAD:     sc.AAD,
 				}

command/agent/cert_with_name_end_to_end_test.go

@@ -11,16 +11,16 @@ import (
 	hclog "github.com/hashicorp/go-hclog"
 
 	"github.com/hashicorp/vault/api"
+	"github.com/hashicorp/vault/sdk/helper/jsonutil"
+	"github.com/hashicorp/vault/sdk/helper/logging"
+	"github.com/hashicorp/vault/sdk/logical"
 	vaultcert "github.com/hashicorp/vault/builtin/credential/cert"
 	"github.com/hashicorp/vault/command/agent/auth"
 	agentcert "github.com/hashicorp/vault/command/agent/auth/cert"
 	"github.com/hashicorp/vault/command/agent/sink"
 	"github.com/hashicorp/vault/command/agent/sink/file"
 	"github.com/hashicorp/vault/helper/dhutil"
 	vaulthttp "github.com/hashicorp/vault/http"
-	"github.com/hashicorp/vault/sdk/helper/jsonutil"
-	"github.com/hashicorp/vault/sdk/helper/logging"
-	"github.com/hashicorp/vault/sdk/logical"
 	"github.com/hashicorp/vault/vault"
 )
 
@@ -137,6 +137,7 @@ func testCertWithNameEndToEnd(t *testing.T, ahWrapping bool) {
 		AAD:    "foobar",
 		DHType: "curve25519",
 		DHPath: dhpath,
+		DeriveKey: true,
 		Config: map[string]interface{}{
 			"path": out,
 		},
@@ -186,14 +187,17 @@ func testCertWithNameEndToEnd(t *testing.T, ahWrapping bool) {
 					continue
 				}
 
-				aesKey, err := dhutil.GenerateSharedKey(pri, resp.Curve25519PublicKey)
+				shared, err := dhutil.GenerateSharedSecret(pri, resp.Curve25519PublicKey)
+				if err != nil {
+					t.Fatal(err)
+				}
+				aesKey, err := dhutil.DeriveSharedKey(shared, pub, resp.Curve25519PublicKey)
 				if err != nil {
 					t.Fatal(err)
 				}
 				if len(aesKey) == 0 {
 					t.Fatal("got empty aes key")
 				}
-
 				val, err = dhutil.DecryptAES(aesKey, resp.EncryptedPayload, resp.Nonce, []byte("foobar"))
 				if err != nil {
 					t.Fatalf("error: %v\nresp: %v", err, string(val))

command/agent/cert_with_no_name_end_to_end_test.go

@@ -183,7 +183,7 @@ func testCertWithNoNAmeEndToEnd(t *testing.T, ahWrapping bool) {
 					continue
 				}
 
-				aesKey, err := dhutil.GenerateSharedKey(pri, resp.Curve25519PublicKey)
+				aesKey, err := dhutil.GenerateSharedSecret(pri, resp.Curve25519PublicKey)
 				if err != nil {
 					t.Fatal(err)
 				}

command/agent/config/config.go

@@ -75,6 +75,7 @@ type Sink struct {
 	WrapTTLRaw interface{}   `hcl:"wrap_ttl"`
 	WrapTTL    time.Duration `hcl:"-"`
 	DHType     string        `hcl:"dh_type"`
+	DeriveKey  bool          `hcl:"derive_key"`
 	DHPath     string        `hcl:"dh_path"`
 	AAD        string        `hcl:"aad"`
 	AADEnvVar  string        `hcl:"aad_env_var"`
@@ -395,6 +396,9 @@ func parseSinks(result *Config, list *ast.ObjectList) error {
 			if s.AAD != "" {
 				return multierror.Prefix(errors.New("specifying AAD data without 'dh_type' does not make sense"), fmt.Sprintf("sink.%s", s.Type))
 			}
+			if s.DeriveKey {
+				return multierror.Prefix(errors.New("specifying 'derive_key' data without 'dh_type' does not make sense"), fmt.Sprintf("sink.%s", s.Type))
+			}
 		case s.DHPath != "" && s.DHType != "":
 		default:
 			return multierror.Prefix(errors.New("'dh_type' and 'dh_path' must be specified together"), fmt.Sprintf("sink.%s", s.Type))

command/agent/config/config_test.go

@@ -143,6 +143,7 @@ func TestLoadConfigFile(t *testing.T) {
 					DHType:  "curve25519",
 					DHPath:  "/tmp/file-foo-dhpath2",
 					AAD:     "aad",
+					DeriveKey: true,
 					Config: map[string]interface{}{
 						"path": "/tmp/file-bar",
 					},

command/agent/config/test-fixtures/config-embedded-type.hcl

@@ -23,6 +23,7 @@ auto_auth {
 		aad_env_var = "TEST_AAD_ENV"
 		dh_type = "curve25519"
 		dh_path = "/tmp/file-foo-dhpath2"
+		derive_key = true
 		config = {
 			path = "/tmp/file-bar"
 		}

command/agent/config/test-fixtures/config.hcl

@@ -25,6 +25,7 @@ auto_auth {
 		aad_env_var = "TEST_AAD_ENV"
 		dh_type = "curve25519"
 		dh_path = "/tmp/file-foo-dhpath2"
+		derive_key = true
 		config = {
 			path = "/tmp/file-bar"
 		}

command/agent/jwt_end_to_end_test.go

@@ -158,6 +158,7 @@ func testJWTEndToEnd(t *testing.T, ahWrapping bool) {
 		AAD:    "foobar",
 		DHType: "curve25519",
 		DHPath: dhpath,
+		DeriveKey: true,
 		Config: map[string]interface{}{
 			"path": out,
 		},
@@ -231,7 +232,11 @@ func testJWTEndToEnd(t *testing.T, ahWrapping bool) {
 					continue
 				}
 
-				aesKey, err := dhutil.GenerateSharedKey(pri, resp.Curve25519PublicKey)
+				shared, err := dhutil.GenerateSharedSecret(pri, resp.Curve25519PublicKey)
+				if err != nil {
+					t.Fatal(err)
+				}
+				aesKey, err := dhutil.DeriveSharedKey(shared, pub, resp.Curve25519PublicKey)
 				if err != nil {
 					t.Fatal(err)
 				}

command/agent/sink/sink.go

@@ -32,6 +32,7 @@ type SinkConfig struct {
 	WrapTTL            time.Duration
 	DHType             string
 	DHPath             string
+	DeriveKey          bool
 	AAD                string
 	cachedRemotePubKey []byte
 	cachedPubKey       []byte
@@ -205,7 +206,16 @@ func (s *SinkConfig) encryptToken(token string) (string, error) {
 		resp.Curve25519PublicKey = s.cachedPubKey
 	}
 
-	aesKey, err = dhutil.GenerateSharedKey(s.cachedPriKey, s.cachedRemotePubKey)
+	secret, err := dhutil.GenerateSharedSecret(s.cachedPriKey, s.cachedRemotePubKey)
+	if err != nil {
+		return "", errwrap.Wrapf("error calculating shared key: {{err}}", err)
+	}
+	if s.DeriveKey {
+		aesKey, err = dhutil.DeriveSharedKey(secret, s.cachedPubKey, s.cachedRemotePubKey)
+	} else {
+		aesKey = secret
+	}
+
 	if err != nil {
 		return "", errwrap.Wrapf("error deriving shared key: {{err}}", err)
 	}

helper/dhutil/dhutil.go

@@ -1,11 +1,15 @@
 package dhutil
 
 import (
+	"bytes"
 	"crypto/aes"
 	"crypto/cipher"
 	"crypto/rand"
+	"crypto/sha256"
+	"encoding/hex"
 	"errors"
 	"fmt"
+	"golang.org/x/crypto/hkdf"
 	"io"
 
 	"golang.org/x/crypto/curve25519"
@@ -34,23 +38,60 @@ func GeneratePublicPrivateKey() ([]byte, []byte, error) {
 	return public[:], scalar[:], nil
 }
 
-// generateSharedKey uses the private key and the other party's public key to
+// GenerateSharedSecret uses the private key and the other party's public key to
 // generate the shared secret.
-func GenerateSharedKey(ourPrivate, theirPublic []byte) ([]byte, error) {
+func GenerateSharedSecret(ourPrivate, theirPublic []byte) ([]byte, error) {
 	if len(ourPrivate) != 32 {
 		return nil, fmt.Errorf("invalid private key length: %d", len(ourPrivate))
 	}
 	if len(theirPublic) != 32 {
 		return nil, fmt.Errorf("invalid public key length: %d", len(theirPublic))
 	}
 
-	var scalar, pub, secret [32]byte
-	copy(scalar[:], ourPrivate)
-	copy(pub[:], theirPublic)
+	return curve25519.X25519(ourPrivate, theirPublic)
+}
+
+// DeriveSharedKey uses HKDF to derive a key from a shared secret and public keys
+func DeriveSharedKey(secret, ourPublic, theirPublic []byte) ([]byte, error) {
+	// Derive the final key from the HKDF of the secret and public keys.
+
+/*
+	Internally, HKDF hashes the secret and two public keys. If Alice and Bob are doing DH key exchange, Alice calculates:
+
+	HKDF(secret, A, B) since ourPublic is A.
+
+		Bob calculates HKDF(secret, B, A), since Bob's ours is B. That produces a different value. Now we only care
+	    that both public keys participate in the derivation, so simply sorting them so they are in a consistent
+	    numerical order (either one would do) arrives at an agreed value.
+*/
+
+	var pub1 []byte
+	var pub2 []byte
+	switch bytes.Compare(ourPublic, theirPublic) {
+	case 0:
+		return nil, errors.New("same public key supplied for both participants")
+	case -1:
+		pub1 = ourPublic
+		pub2 = theirPublic
+	case 1:
+		pub1 = theirPublic
+		pub2 = ourPublic
+	}
+
+	kio := hkdf.New(sha256.New, secret, pub1, pub2)
 
-	curve25519.ScalarMult(&secret, &scalar, &pub)
+	var key [32]byte
+	n, err := io.ReadFull(kio, key[:])
+	if err != nil {
+		// Don't return the key along with the error to prevent misuse
+		return nil, err
+	}
+	if n != 32 {
+		return nil, errors.New("short read from hkdf")
+	}
+	fmt.Printf("Key: %s\n", hex.EncodeToString(key[:]))
 
-	return secret[:], nil
+	return key[:], nil
 }
 
 // Use AES256-GCM to encrypt some plaintext with a provided key. The returned values are

helper/dhutil/dhutil_test.go

@@ -0,0 +1 @@
+package dhutil

website/pages/docs/agent/autoauth/index.mdx

@@ -155,6 +155,11 @@ These configuration values are common to all Sinks:
   agent should read the client's initial parameters (e.g. curve25519 public
   key).
 
+- `derive_key` `(bool: false)` - If specified, the final encryption key is
+  calculated by using HKDF-SHA256 to derive a key from the calculated shared
+  secret and the two public keys for enhanced security.  This is recommended
+  if backward compatibility isn't a concern.
+
 - `aad` `(string: optional)` - If specified, additional authenticated data to
   use with the AES-GCM encryption of the token. Can be any string, including
   serialized data.