Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
@@ -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) {
Expand Down Expand Up @@ -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")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
Loading