From 11a73bfa0b43a94daef2080518abbb66a684649c Mon Sep 17 00:00:00 2001 From: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> Date: Tue, 2 Jun 2026 17:02:35 +0200 Subject: [PATCH] fix(keychain): pad DH shared secret to fixed length on Linux The Secret Service session encryption mode (dh-ietf1024-sha256-aes128-cbc-pkcs7) derived the HKDF AES key from sharedSecret.Bytes(), which returns the minimal big-endian encoding and strips leading zero bytes. When the Diffie-Hellman shared secret had one or more leading zero bytes (~1/256 of sessions per byte), our HKDF input was shorter than the 128-byte zero-padded value the keyring peer uses, so the two sides derived different AES keys. The keyring then rejected CreateItem with "the secret was transferred or encrypted in an invalid way", surfacing as intermittent CI failures on the Linux keychain tests. Encode the shared secret into a fixed-length buffer sized to the group prime (128 bytes for the 1024-bit Second Oakley Group) with big.Int.FillBytes so both sides agree on the HKDF input. Also surface the decrypt error in GetSecret instead of silently returning (nil, nil), and add a deterministic regression test that pins the zero-padded encoding. Fixes #547 Co-Authored-By: Claude Opus 4.8 (1M context) --- .../dh_ietf1024_sha256_aes128_cbc_pkcs7.go | 12 ++++- ...h_ietf1024_sha256_aes128_cbc_pkcs7_test.go | 44 +++++++++++++++++++ .../secretservice/secretservice.go | 2 +- 3 files changed, 56 insertions(+), 2 deletions(-) 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: