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
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
35 changes: 30 additions & 5 deletions utils/musig.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,40 @@ 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. 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 +17 to +18
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.
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
}
Comment on lines +26 to +35
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 +17 to +40
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 +99,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