Skip to content
Open
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
5 changes: 4 additions & 1 deletion loopin.go
Original file line number Diff line number Diff line change
Expand Up @@ -1214,7 +1214,10 @@ func (s *loopInSwap) setState(state loopdb.SwapState) {
}

// sharedSecretFromHash derives the shared secret from the swap hash using the
// swap.KeyFamily family and zero as index.
// swap.KeyFamily family and zero as index. The swap hash is first interpreted
// with btcec.PrivKeyFromBytes semantics, so the derived ephemeral pubkey uses
// the same modulo-N normalization as the internal-key code paths that consume
// the resulting shared secret.
func sharedSecretFromHash(ctx context.Context, signer lndclient.SignerClient,
hash lntypes.Hash) ([32]byte, error) {

Expand Down
10 changes: 6 additions & 4 deletions staticaddr/script/script_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (
"fmt"
"testing"

"github.com/btcsuite/btcd/btcec/v2"
"github.com/btcsuite/btcd/txscript"
"github.com/btcsuite/btcd/wire"
"github.com/lightninglabs/loop/test"
Expand All @@ -28,9 +27,12 @@ func TestStaticAddressScript(t *testing.T) {
clientPrivKey, clientPubKey := test.CreateKey(1)
serverPrivKey, serverPubKey := test.CreateKey(2)

var clientKey, serverKey [32]byte
copy(clientKey[:], clientPrivKey.Serialize())
copy(serverKey[:], serverPrivKey.Serialize())

// Keys used for the Musig2 session.
privKeys := []*btcec.PrivateKey{clientPrivKey, serverPrivKey}
pubKeys := []*btcec.PublicKey{clientPubKey, serverPubKey}
privKeys := [][32]byte{clientKey, serverKey}

// Create a new static address.
staticAddress, err := NewStaticAddress(
Expand Down Expand Up @@ -91,7 +93,7 @@ func TestStaticAddressScript(t *testing.T) {
}

sig, err := utils.MuSig2Sign(
version, privKeys, pubKeys, tweak, msg,
version, privKeys, tweak, msg,
)
require.NoError(t, err)

Expand Down
39 changes: 34 additions & 5 deletions utils/musig.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,44 @@ import (
"fmt"

"github.com/btcsuite/btcd/btcec/v2"
"github.com/btcsuite/btcd/btcec/v2/schnorr"
"github.com/btcsuite/btcd/btcec/v2/schnorr/musig2"
"github.com/lightningnetwork/lnd/input"
)

// MuSig2Sign will create a MuSig2 signature for the passed message using the
// passed private keys.
func MuSig2Sign(version input.MuSig2Version, privKeys []*btcec.PrivateKey,
pubKeys []*btcec.PublicKey, tweaks *input.MuSig2Tweaks,
msg [32]byte) ([]byte, error) {
// passed raw private keys. Raw keys are interpreted with
// btcec.PrivKeyFromBytes semantics, which normalize 32-byte inputs modulo the
// secp256k1 group order instead of rejecting out-of-range values. It expects
// at least two signing keys.
func MuSig2Sign(version input.MuSig2Version, keys [][32]byte,
tweaks *input.MuSig2Tweaks, msg [32]byte) ([]byte, error) {

privKeys := make([]*btcec.PrivateKey, len(keys))
pubKeys := make([]*btcec.PublicKey, len(keys))
Comment on lines +20 to +21
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

It is more efficient to validate the input length before performing any cryptographic operations or memory allocations. Moving the check for at least two signing keys to the beginning of the function will allow it to fail fast.

	if len(keys) < 2 {
		return nil, fmt.Errorf("need at least two signing keys")
	}

	privKeys := make([]*btcec.PrivateKey, len(keys))
	pubKeys := make([]*btcec.PublicKey, len(keys))

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put the check after the translation to reduce the diff size. In the real use case the check never fires, so this has no performance impact.


// First parse the raw private keys and also create the corresponding
// public keys. This preserves the same normalization semantics used
// when these raw keys are turned into pubkeys elsewhere in the protocol.
for i, key := range keys {
privKeys[i], pubKeys[i] = btcec.PrivKeyFromBytes(key[:])
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

loop's new raw-key API accepts malformed private keys. btcec.PrivKeyFromBytes normalizes out-of-range bytes instead of validating them, and /tmp/loop-rawkey-check showed that an all-0xff key still signs successfully. That means callers can get a valid signature from a different scalar than the one they passed in.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I dug into this in more detail.

You are right about btcec.PrivKeyFromBytes: it is not a strict parser and it normalizes 32-byte inputs modulo the secp256k1 order. So an input like 0xff...ff does not fail; it aliases to the corresponding normalized scalar and can still produce a valid signature.

The important detail is that this is not a new signing semantic introduced by this change. The existing MuSig2/internal-key flow already relies on the same btcec.PrivKeyFromBytes behavior in the hash-derived key path, so for non-zero out-of-range inputs the protocol is already operating on the normalized scalar rather than on the literal 32-byte encoding.

So I do not think this is a signature-validity bug in the current protocol flow: the raw bytes may alias to a different scalar, but the resulting pubkey and the final signature are both computed from that same normalized scalar, so the signature still matches the key material actually used by the protocol. The bad case is zero, which does not survive this flow as a valid key.

I added a separate documentation-only commit, utils: document raw-key normalization semantics, to make the current modulo-N canonicalization explicit.


// MuSig2 v0.4 expects x-only public keys.
if version == input.MuSig2Version040 {
pubKey := pubKeys[i].SerializeCompressed()
xOnlyPubKey, err := schnorr.ParsePubKey(pubKey[1:])
if err != nil {
return nil, fmt.Errorf("error parsing x-only "+
"pubkey: %v", err)
}

pubKeys[i] = xOnlyPubKey
}
Comment on lines +30 to +39
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-critical critical

When using MuSig2 v0.4 with x-only public keys, if the original public key has an odd Y coordinate, the corresponding private key must be negated to match the x-only public key (which is assumed to have an even Y coordinate). Failing to do so will result in an invalid signature because the signer will be using a private key that does not correspond to the public key used in the MuSig2 aggregation. You should check the parity of the public key and negate the private key if necessary.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to add an extra private-key negation in our wrapper for review item 1.

For MuSig2Version040, our helper calls input.MuSig2CreateContext, which passes the private key into the v0.4 implementation.

Inside that implementation, musig2v040.NewContext already normalizes the local signer public key to x-only by calling schnorr.ParsePubKey(schnorr.SerializePubKey(signingKey.PubKey())).

Then, during signing, musig2v040.Sign checks whether the original local public key has odd Y, sets paritySignKey.Negate() when needed, and multiplies the private scalar by that parity factor before producing the partial signature.

So the odd-Y to x-only adjustment is already handled by lnd/input for the v0.4 path. Adding another manual negation in our helper would duplicate that logic and risks over-correcting the signer key.

I added a test case to make sure that odd Y keys work.

}

if len(privKeys) < 2 {
return nil, fmt.Errorf("need at least two signing keys")
}
Comment on lines +20 to +44
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The check for the minimum number of signing keys should be performed at the beginning of the function. This avoids unnecessary allocations and processing (such as parsing keys and performing x-only conversions) when the input is invalid.

	if len(keys) < 2 {
		return nil, fmt.Errorf("need at least two signing keys")
	}

	privKeys := make([]*btcec.PrivateKey, len(keys))
	pubKeys := make([]*btcec.PublicKey, len(keys))

	// First parse the raw private keys and also create the corresponding
	// public keys.
	for i, key := range keys {
		privKeys[i], pubKeys[i] = btcec.PrivKeyFromBytes(key[:])

		// MuSig2 v0.4 expects x-only public keys.
		if version == input.MuSig2Version040 {
			pubKey := pubKeys[i].SerializeCompressed()
			xOnlyPubKey, err := schnorr.ParsePubKey(pubKey[1:])
			if err != nil {
				return nil, fmt.Errorf("error parsing x-only "+
					"pubkey: %v", err)
			}

			pubKeys[i] = xOnlyPubKey
		}
	}


// Next we'll create MuSig2 sessions for each individual private
// signing key.
Expand Down Expand Up @@ -74,7 +103,7 @@ func MuSig2Sign(version input.MuSig2Version, privKeys []*btcec.PrivateKey,
}

if !haveAllSigs {
return nil, fmt.Errorf("combinging MuSig2 signatures " +
return nil, fmt.Errorf("combining MuSig2 signatures " +
"failed")
}

Expand Down
148 changes: 148 additions & 0 deletions utils/musig_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,148 @@
package utils

import (
"testing"

"github.com/btcsuite/btcd/btcec/v2"
"github.com/btcsuite/btcd/btcec/v2/schnorr"
"github.com/lightninglabs/loop/test"
"github.com/lightningnetwork/lnd/input"
"github.com/stretchr/testify/require"
)

// rawKeys returns serialized private keys for the given test seeds.
func rawKeys(seeds ...int32) [][32]byte {
keys := make([][32]byte, len(seeds))
for i, seed := range seeds {
privKey, _ := test.CreateKey(seed)
copy(keys[i][:], privKey.Serialize())
}

return keys
}

// signerPubKeys returns signer public keys derived from the given seeds in the
// format expected by the given MuSig2 version.
func signerPubKeys(t *testing.T, version input.MuSig2Version,
seeds ...int32) []*btcec.PublicKey {

t.Helper()

pubKeys := make([]*btcec.PublicKey, len(seeds))
for i, seed := range seeds {
_, pubKey := test.CreateKey(seed)

if version == input.MuSig2Version040 {
var err error
pubKey, err = schnorr.ParsePubKey(
schnorr.SerializePubKey(pubKey),
)
require.NoError(t, err)
}

pubKeys[i] = pubKey
}

return pubKeys
}

// hasOddY returns true if the compressed serialization of the public key uses
// the odd-Y prefix.
func hasOddY(pubKey *btcec.PublicKey) bool {
return pubKey.SerializeCompressed()[0] == 0x03
}

// TestMuSig2SignRejectsSingleSigner ensures the helper fails fast with a clear
// error instead of entering an invalid one-party MuSig2 flow.
func TestMuSig2SignRejectsSingleSigner(t *testing.T) {
_, err := MuSig2Sign(
input.MuSig2Version100RC2,
rawKeys(1),
&input.MuSig2Tweaks{},
[32]byte{},
)
require.ErrorContains(t, err, "need at least two signing keys")
}

// TestMuSig2SignSupportsVersions verifies the helper works with the supported
// MuSig2 versions used in Loop.
func TestMuSig2SignSupportsVersions(t *testing.T) {
t.Parallel()

tweaks := &input.MuSig2Tweaks{}
msg := [32]byte{1}

testCases := []struct {
name string
version input.MuSig2Version
seeds []int32
oddYSigner int32
}{
{
name: testVersionName(input.MuSig2Version040),
version: input.MuSig2Version040,
seeds: []int32{1, 2},
},
{
name: testVersionName(input.MuSig2Version100RC2),
version: input.MuSig2Version100RC2,
seeds: []int32{1, 2},
},
{
name: testVersionName(input.MuSig2Version040) +
" odd Y",
version: input.MuSig2Version040,
seeds: []int32{5, 1},
oddYSigner: 5,
},
}

for _, testCase := range testCases {
t.Run(testCase.name, func(t *testing.T) {
t.Parallel()

if testCase.oddYSigner != 0 {
_, oddYKey := test.CreateKey(
testCase.oddYSigner,
)
require.True(t, hasOddY(oddYKey))
}

keys := rawKeys(testCase.seeds...)
pubKeys := signerPubKeys(
t, testCase.version, testCase.seeds...,
)

sigBytes, err := MuSig2Sign(
testCase.version, keys, tweaks, msg,
)
require.NoError(t, err)
require.Len(t, sigBytes, 64)

sig, err := schnorr.ParseSignature(sigBytes)
require.NoError(t, err)

combinedKey, err := input.MuSig2CombineKeys(
testCase.version, pubKeys, true, tweaks,
)
require.NoError(t, err)
require.True(
t, sig.Verify(msg[:], combinedKey.FinalKey),
)
})
}
}

// testVersionName returns a stable subtest name for a MuSig2 version.
func testVersionName(version input.MuSig2Version) string {
switch version {
case input.MuSig2Version040:
return "MuSig2 0.4"

case input.MuSig2Version100RC2:
return "MuSig2 1.0RC2"

default:
return "unknown"
}
}
Loading