refactor: bcrypt key derivation to aead (#509)

CHANGELOG.md

@@ -69,6 +69,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ### Improvements
 
+* (crypto) [#3129](https://github.com/cosmos/cosmos-sdk/pull/3129) New armor and keyring key derivation uses aead and encryption uses chacha20poly
 * (x/slashing) [#15580](https://github.com/cosmos/cosmos-sdk/pull/15580) Refactor the validator's missed block signing window to be a chunked bitmap instead of a "logical" bitmap, significantly reducing the storage footprint.
 * [#15448](https://github.com/cosmos/cosmos-sdk/pull/15448) Automatically populate the block timestamp for historical queries. In contexts where the block timestamp is needed for previous states, the timestamp will now be set. Note, when querying against a node it must be re-synced in order to be able to automatically populate the block timestamp. Otherwise, the block timestamp will be populated for heights going forward once upgraded.
 * (x/gov) [#15554](https://github.com/cosmos/cosmos-sdk/pull/15554) Add proposal result log in `active_proposal` event. When a proposal passes but fails to execute, the proposal result is logged in the `active_proposal` event.

crypto/armor.go

@@ -7,9 +7,11 @@ import (
 	"io"
 
 	"github.com/cometbft/cometbft/crypto"
-	"golang.org/x/crypto/openpgp/armor" //nolint:staticcheck // TODO: remove this dependency
+	"golang.org/x/crypto/argon2"
+	"golang.org/x/crypto/openpgp/armor" //nolint:staticcheck //TODO: remove this dependency
 
 	errorsmod "cosmossdk.io/errors"
+	"golang.org/x/crypto/chacha20poly1305"
 
 	"github.com/cosmos/cosmos-sdk/codec/legacy"
 	"github.com/cosmos/cosmos-sdk/crypto/keys/bcrypt"
@@ -29,6 +31,18 @@ const (
 	headerType    = "type"
 )
 
+var (
+	kdfHeader = "kdf"
+	kdfBcrypt = "bcrypt"
+	kdfArgon2 = "argon2"
+)
+
+const (
+	argon2Time    = 1
+	argon2Memory  = 64 * 1024
+	argon2Threads = 4
+)
+
 // BcryptSecurityParameter is security parameter var, and it can be changed within the lcd test.
 // Making the bcrypt security parameter a var shouldn't be a security issue:
 // One can't verify an invalid key by maliciously changing the bcrypt
@@ -131,8 +145,8 @@ func unarmorBytes(armorStr, blockType string) (bz []byte, header map[string]stri
 func EncryptArmorPrivKey(privKey cryptotypes.PrivKey, passphrase, algo string) string {
 	saltBytes, encBytes := encryptPrivKey(privKey, passphrase)
 	header := map[string]string{
-		"kdf":  "bcrypt",
-		"salt": fmt.Sprintf("%X", saltBytes),
+		kdfHeader: kdfArgon2,
+		"salt":    fmt.Sprintf("%X", saltBytes),
 	}
 
 	if algo != "" {
@@ -144,20 +158,22 @@ func EncryptArmorPrivKey(privKey cryptotypes.PrivKey, passphrase, algo string) s
 	return armorStr
 }
 
-// encrypt the given privKey with the passphrase using a randomly
-// generated salt and the xsalsa20 cipher. returns the salt and the
-// encrypted priv key.
 func encryptPrivKey(privKey cryptotypes.PrivKey, passphrase string) (saltBytes, encBytes []byte) {
 	saltBytes = crypto.CRandBytes(16)
-	key, err := bcrypt.GenerateFromPassword(saltBytes, []byte(passphrase), BcryptSecurityParameter)
+
+	key := argon2.IDKey([]byte(passphrase), saltBytes, argon2Time, argon2Memory, argon2Threads, chacha20poly1305.KeySize)
+	privKeyBytes := legacy.Cdc.MustMarshal(privKey)
+
+	aead, err := chacha20poly1305.New(key)
 	if err != nil {
-		panic(errorsmod.Wrap(err, "error generating bcrypt key from passphrase"))
+		panic(errorsmod.Wrap(err, "error generating cypher from key"))
 	}
 
-	key = crypto.Sha256(key) // get 32 bytes
-	privKeyBytes := legacy.Cdc.MustMarshal(privKey)
+	nonce := make([]byte, aead.NonceSize(), aead.NonceSize()+len(privKeyBytes)+aead.Overhead()) // Nonce is fixed to maintain consistency, each key is generated  at every encryption using a random salt.
+
+	encBytes = aead.Seal(nil, nonce, privKeyBytes, nil)
 
-	return saltBytes, xsalsa20symmetric.EncryptSymmetric(privKeyBytes, key)
+	return saltBytes, encBytes
 }
 
 // UnarmorDecryptPrivKey returns the privkey byte slice, a string of the algo type, and an error
@@ -171,8 +187,8 @@ func UnarmorDecryptPrivKey(armorStr, passphrase string) (privKey cryptotypes.Pri
 		return privKey, "", fmt.Errorf("unrecognized armor type: %v", blockType)
 	}
 
-	if header["kdf"] != "bcrypt" {
-		return privKey, "", fmt.Errorf("unrecognized KDF type: %v", header["kdf"])
+	if header[kdfHeader] != kdfBcrypt && header[kdfHeader] != kdfArgon2 {
+		return privKey, "", fmt.Errorf("unrecognized KDF type: %v", header[kdfHeader])
 	}
 
 	if header["salt"] == "" {
@@ -184,7 +200,7 @@ func UnarmorDecryptPrivKey(armorStr, passphrase string) (privKey cryptotypes.Pri
 		return privKey, "", fmt.Errorf("error decoding salt: %v", err.Error())
 	}
 
-	privKey, err = decryptPrivKey(saltBytes, encBytes, passphrase)
+	privKey, err = decryptPrivKey(saltBytes, encBytes, passphrase, header[kdfHeader])
 
 	if header[headerType] == "" {
 		header[headerType] = defaultAlgo
@@ -193,18 +209,45 @@ func UnarmorDecryptPrivKey(armorStr, passphrase string) (privKey cryptotypes.Pri
 	return privKey, header[headerType], err
 }
 
-func decryptPrivKey(saltBytes, encBytes []byte, passphrase string) (privKey cryptotypes.PrivKey, err error) {
-	key, err := bcrypt.GenerateFromPassword(saltBytes, []byte(passphrase), BcryptSecurityParameter)
-	if err != nil {
-		return privKey, errorsmod.Wrap(err, "error generating bcrypt key from passphrase")
-	}
+func decryptPrivKey(saltBytes, encBytes []byte, passphrase, kdf string) (privKey cryptotypes.PrivKey, err error) {
+	// Key derivation
+	var (
+		key          []byte
+		privKeyBytes []byte
+	)
+
+	// Since the argon2 key derivation and chacha encryption was implemented together, it is not possible to have mixed kdf and encryption algorithms
+	switch kdf {
+	case kdfArgon2:
+		key = argon2.IDKey([]byte(passphrase), saltBytes, argon2Time, argon2Memory, argon2Threads, chacha20poly1305.KeySize)
+
+		aead, err := chacha20poly1305.New(key)
+		if err != nil {
+			return privKey, errorsmod.Wrap(err, "Error generating aead cypher for key.")
+		} else if len(encBytes) < aead.NonceSize() {
+			return privKey, errorsmod.Wrap(nil, "Encrypted bytes length is smaller than aead nonce size.")
+		}
+		nonce := make([]byte, aead.NonceSize(), aead.NonceSize()+len(privKeyBytes)+aead.Overhead())
+		privKeyBytes, err = aead.Open(nil, nonce, encBytes, nil) // Decrypt the message and check it wasn't tampered with.
+		if err != nil {
+			return privKey, sdkerrors.ErrWrongPassword
+		}
+	case kdfBcrypt:
+		key, err = bcrypt.GenerateFromPassword(saltBytes, []byte(passphrase), BcryptSecurityParameter)
+		if err != nil {
+			return privKey, errorsmod.Wrap(err, "Error generating bcrypt cypher for key.")
+		}
+		key = crypto.Sha256(key) // Get 32 bytes
+		privKeyBytes, err = xsalsa20symmetric.DecryptSymmetric(encBytes, key)
 
-	key = crypto.Sha256(key) // Get 32 bytes
+		if err == xsalsa20symmetric.ErrCiphertextDecrypt {
+			return privKey, sdkerrors.ErrWrongPassword
+		}
+	default:
+		return privKey, errorsmod.Wrap(nil, fmt.Sprintf("Unrecognized key derivation function (kdf) header: %s.", kdf))
+	}
 
-	privKeyBytes, err := xsalsa20symmetric.DecryptSymmetric(encBytes, key)
-	if err != nil && err == xsalsa20symmetric.ErrCiphertextDecrypt {
-		return privKey, sdkerrors.ErrWrongPassword
-	} else if err != nil {
+	if err != nil {
 		return privKey, err
 	}
 

crypto/armor_test.go

@@ -8,7 +8,9 @@ import (
 	"testing"
 
 	cmtcrypto "github.com/cometbft/cometbft/crypto"
-	"github.com/cometbft/cometbft/crypto/xsalsa20symmetric"
+	"github.com/cosmos/cosmos-sdk/crypto/xsalsa20symmetric"
+
+	_ "github.com/cosmos/cosmos-sdk/runtime"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
@@ -23,7 +25,6 @@ import (
 	"github.com/cosmos/cosmos-sdk/crypto/keys/bcrypt"
 	"github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1"
 	cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
-	_ "github.com/cosmos/cosmos-sdk/runtime"
 	"github.com/cosmos/cosmos-sdk/testutil/configurator"
 	"github.com/cosmos/cosmos-sdk/types"
 )
@@ -200,3 +201,55 @@ func TestArmor(t *testing.T) {
 	assert.Equal(t, blockType, blockType2)
 	assert.Equal(t, data, data2)
 }
+
+func TestBcryptLegacyEncryption(t *testing.T) {
+	privKey := secp256k1.GenPrivKey()
+	saltBytes := cmtcrypto.CRandBytes(16)
+	passphrase := "passphrase"
+	privKeyBytes := legacy.Cdc.MustMarshal(privKey)
+
+	// Bcrypt + Aead
+	headerBcrypt := map[string]string{
+		"kdf":  "bcrypt",
+		"salt": fmt.Sprintf("%X", saltBytes),
+	}
+	keyBcrypt, _ := bcrypt.GenerateFromPassword(saltBytes, []byte(passphrase), 12) // Legacy key generation
+	keyBcrypt = cmtcrypto.Sha256(keyBcrypt)
+
+	// bcrypt + xsalsa20symmetric
+	encBytesBcryptXsalsa20symetric := xsalsa20symmetric.EncryptSymmetric(privKeyBytes, keyBcrypt)
+
+	type testCase struct {
+		description string
+		armor       string
+	}
+
+	for _, scenario := range []testCase{
+		{
+			description: "Argon2 + Aead",
+			armor:       crypto.EncryptArmorPrivKey(privKey, "passphrase", ""),
+		},
+		{
+			description: "Bcrypt + xsalsa20symmetric",
+			armor:       crypto.EncodeArmor("TENDERMINT PRIVATE KEY", headerBcrypt, encBytesBcryptXsalsa20symetric),
+		},
+	} {
+		t.Run(scenario.description, func(t *testing.T) {
+			_, _, err := crypto.UnarmorDecryptPrivKey(scenario.armor, "wrongpassphrase")
+			require.Error(t, err)
+			decryptedPrivKey, _, err := crypto.UnarmorDecryptPrivKey(scenario.armor, "passphrase")
+			require.NoError(t, err)
+			require.True(t, privKey.Equals(decryptedPrivKey))
+		})
+	}
+
+	// Test wrong kdf header
+	headerWithoutKdf := map[string]string{
+		"kdf":  "wrongKdf",
+		"salt": fmt.Sprintf("%X", saltBytes),
+	}
+
+	_, _, err := crypto.UnarmorDecryptPrivKey(crypto.EncodeArmor("TENDERMINT PRIVATE KEY", headerWithoutKdf, encBytesBcryptXsalsa20symetric), "passphrase")
+	require.Error(t, err)
+	require.Equal(t, "unrecognized KDF type: wrongKdf", err.Error())
+}