diff --git a/store/keychain/internal/go-keychain/secretservice/dh_ietf1024_sha256_aes128_cbc_pkcs7.go b/store/keychain/internal/go-keychain/secretservice/dh_ietf1024_sha256_aes128_cbc_pkcs7.go index cf727ce4..8ba12e20 100644 --- a/store/keychain/internal/go-keychain/secretservice/dh_ietf1024_sha256_aes128_cbc_pkcs7.go +++ b/store/keychain/internal/go-keychain/secretservice/dh_ietf1024_sha256_aes128_cbc_pkcs7.go @@ -66,7 +66,17 @@ func (group *dhGroup) keygenHKDFSHA256AES128(theirPublic, myPrivate *big.Int) ([ if err != nil { return nil, err } - sharedSecretBytes := sharedSecret.Bytes() + // The Secret Service peer derives the key over the shared secret encoded as + // a fixed-length big-endian value matching the group prime size (128 bytes + // for the 1024-bit Second Oakley Group). big.Int.Bytes() returns the + // minimal encoding and strips leading zero bytes, so when the shared secret + // happens to have one or more leading zero bytes (~1/256 of sessions per + // byte) the HKDF input would be shorter than the peer's, deriving a + // different AES key. That mismatch makes the keyring reject the item with + // "the secret was transferred or encrypted in an invalid way". FillBytes + // left-pads with zeros so both sides agree on the input. + sharedSecretBytes := make([]byte, (group.p.BitLen()+7)/8) + sharedSecret.FillBytes(sharedSecretBytes) defer clear(sharedSecretBytes) r := hkdf.New(sha256.New, sharedSecretBytes, nil, nil) diff --git a/store/keychain/internal/go-keychain/secretservice/dh_ietf1024_sha256_aes128_cbc_pkcs7_test.go b/store/keychain/internal/go-keychain/secretservice/dh_ietf1024_sha256_aes128_cbc_pkcs7_test.go index be186333..9f796816 100644 --- a/store/keychain/internal/go-keychain/secretservice/dh_ietf1024_sha256_aes128_cbc_pkcs7_test.go +++ b/store/keychain/internal/go-keychain/secretservice/dh_ietf1024_sha256_aes128_cbc_pkcs7_test.go @@ -1,9 +1,13 @@ package secretservice import ( + "crypto/sha256" + "io" + "math/big" "testing" "github.com/stretchr/testify/require" + "golang.org/x/crypto/hkdf" ) func TestNewKeypair(t *testing.T) { @@ -32,6 +36,46 @@ func TestKeygen(t *testing.T) { require.Equal(t, myKey, theirKey) } +// TestKeygenPadsSharedSecretWithLeadingZero is a regression test for the +// intermittent "secret was transferred or encrypted in an invalid way" failure. +// The Secret Service peer derives the AES key over the DH shared secret encoded +// as a fixed-length 128-byte big-endian value; big.Int.Bytes() drops leading +// zero bytes, so a shared secret with a leading zero byte (~1/256 of sessions) +// used to yield a shorter HKDF input and a mismatched key. +// +// Using myPrivate = 1 makes the shared secret equal to theirPublic (theirPublic^1 +// mod p), and 2^1016-1 is only 127 bytes wide, so its group encoding has a +// leading zero byte. The derived key must match HKDF over the zero-padded +// 128-byte encoding, not the stripped 127-byte one. +func TestKeygenPadsSharedSecretWithLeadingZero(t *testing.T) { + group := rfc2409SecondOakleyGroup() + + theirPublic := new(big.Int).Sub(new(big.Int).Lsh(big.NewInt(1), 1016), big.NewInt(1)) + require.Len(t, theirPublic.Bytes(), 127, "test vector must have a leading zero byte in its 128-byte encoding") + + got, err := group.keygenHKDFSHA256AES128(theirPublic, big.NewInt(1)) + require.NoError(t, err) + + primeLen := (group.p.BitLen() + 7) / 8 + require.Equal(t, 128, primeLen) + padded := make([]byte, primeLen) + theirPublic.FillBytes(padded) + + require.Equal(t, hkdfAESKey(t, padded), got, "key must derive from the zero-padded shared secret") + require.NotEqual(t, hkdfAESKey(t, theirPublic.Bytes()), got, "stripped-leading-zero encoding must derive a different (wrong) key") +} + +// hkdfAESKey derives a 16-byte AES key from ikm the same way +// keygenHKDFSHA256AES128 does, so tests can assert against the expected input. +func hkdfAESKey(t *testing.T, ikm []byte) []byte { + t.Helper() + r := hkdf.New(sha256.New, ikm, nil, nil) + key := make([]byte, 16) + _, err := io.ReadFull(r, key) + require.NoError(t, err) + return key +} + func TestEncryption(t *testing.T) { key := []byte("YELLOW SUBMARINE") plaintext := []byte("hello world") diff --git a/store/keychain/internal/go-keychain/secretservice/secretservice.go b/store/keychain/internal/go-keychain/secretservice/secretservice.go index 25538245..af4579c3 100644 --- a/store/keychain/internal/go-keychain/secretservice/secretservice.go +++ b/store/keychain/internal/go-keychain/secretservice/secretservice.go @@ -343,7 +343,7 @@ func (s *SecretService) GetSecret(item dbus.ObjectPath, session Session) (secret case AuthenticationDHAES: plaintext, err := unauthenticatedAESCBCDecrypt(secret.Parameters, secret.Value, session.AESKey) if err != nil { - return nil, nil + return nil, fmt.Errorf("failed to decrypt secret: %w", err) } secretPlaintext = plaintext default: