From 19b2236570350a1ca5bdf1518ecb21a2439a68c2 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Tue, 8 Mar 2022 14:35:22 +0800 Subject: [PATCH 1/3] lnwire: add length validation in NewSigFromRawSignature --- lnwire/signature.go | 61 ++++++++++--- lnwire/signature_test.go | 180 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 229 insertions(+), 12 deletions(-) diff --git a/lnwire/signature.go b/lnwire/signature.go index 794076847..f0bed72cb 100644 --- a/lnwire/signature.go +++ b/lnwire/signature.go @@ -1,6 +1,7 @@ package lnwire import ( + "errors" "fmt" "github.com/btcsuite/btcd/btcec/v2/ecdsa" @@ -13,23 +14,61 @@ import ( // signature (raw bytes and *ecdsa.Signature). type Sig [64]byte +var ( + errSigTooShort = errors.New("malformed signature: too short") + errBadLength = errors.New("malformed signature: bad length") + errBadRLength = errors.New("malformed signature: bogus R length") + errBadSLength = errors.New("malformed signature: bogus S length") + errRTooLong = errors.New("R is over 32 bytes long without padding") + errSTooLong = errors.New("S is over 32 bytes long without padding") +) + // NewSigFromRawSignature returns a Sig from a Bitcoin raw signature encoded in // the canonical DER encoding. func NewSigFromRawSignature(sig []byte) (Sig, error) { var b Sig - if len(sig) == 0 { - return b, fmt.Errorf("cannot decode empty signature") + // Check the total length is above the minimal. + if len(sig) < ecdsa.MinSigLen { + return b, errSigTooShort } - // Extract lengths of R and S. The DER representation is laid out as - // 0x30 0x02 r 0x02 s - // which means the length of R is the 4th byte and the length of S - // is the second byte after R ends. 0x02 signifies a length-prefixed, + // The DER representation is laid out as: + // 0x30 0x02 r 0x02 s + // which means the length of R is the 4th byte and the length of S is + // the second byte after R ends. 0x02 signifies a length-prefixed, // zero-padded, big-endian bigint. 0x30 signifies a DER signature. // See the Serialize() method for ecdsa.Signature for details. - rLen := sig[3] - sLen := sig[5+rLen] + + // Reading , remaining: [0x02 r 0x02 s] + sigLen := int(sig[1]) + + // siglen should be less than the entire message and greater than + // the minimal message size. + if sigLen+2 > len(sig) || sigLen+2 < ecdsa.MinSigLen { + return b, errBadLength + } + + // Reading , remaining: [r 0x02 s] + rLen := int(sig[3]) + + // rLen must be positive and must be able to fit in other elements. + // Assuming s is one byte, then we have 0x30, , 0x20, + // , 0x20, , s, a total of 7 bytes. + if rLen <= 0 || rLen+7 > len(sig) { + return b, errBadRLength + } + + // Reading , remaining: [s] + sLen := int(sig[5+rLen]) + + // S should be the rest of the string. + // sLen must be positive and must be able to fit in other elements. + // We know r is rLen bytes, and we have 0x30, , 0x20, + // , 0x20, , a total of rLen+6 bytes. + if sLen <= 0 || sLen+rLen+6 > len(sig) { + return b, errBadSLength + } // Check to make sure R and S can both fit into their intended buffers. // We check S first because these code blocks decrement sLen and rLen @@ -39,8 +78,7 @@ func NewSigFromRawSignature(sig []byte) (Sig, error) { // check S first. if sLen > 32 { if (sLen > 33) || (sig[6+rLen] != 0x00) { - return b, fmt.Errorf("S is over 32 bytes long " + - "without padding") + return b, errSTooLong } sLen-- copy(b[64-sLen:], sig[7+rLen:]) @@ -51,8 +89,7 @@ func NewSigFromRawSignature(sig []byte) (Sig, error) { // Do the same for R as we did for S if rLen > 32 { if (rLen > 33) || (sig[4] != 0x00) { - return b, fmt.Errorf("R is over 32 bytes long " + - "without padding") + return b, errRTooLong } rLen-- copy(b[32-rLen:], sig[5:5+rLen]) diff --git a/lnwire/signature_test.go b/lnwire/signature_test.go index a8deaf03a..48ce212a0 100644 --- a/lnwire/signature_test.go +++ b/lnwire/signature_test.go @@ -7,6 +7,7 @@ import ( "github.com/btcsuite/btcd/btcec/v2" "github.com/btcsuite/btcd/btcec/v2/ecdsa" + "github.com/stretchr/testify/require" ) func TestSignatureSerializeDeserialize(t *testing.T) { @@ -92,3 +93,182 @@ func TestSignatureSerializeDeserialize(t *testing.T) { // the signature from ModNScalar values which don't allow setting a // value larger than N (hence the name mod n). } + +var ( + // signatures from bitcoin blockchain tx + // 0437cd7f8525ceed2324359c2d0ba26006d92d85. + normalSig = []byte{ + 0x30, 0x44, 0x02, 0x20, + // r value + 0x4e, 0x45, 0xe1, 0x69, 0x32, 0xb8, 0xaf, 0x51, + 0x49, 0x61, 0xa1, 0xd3, 0xa1, 0xa2, 0x5f, 0xdf, + 0x3f, 0x4f, 0x77, 0x32, 0xe9, 0xd6, 0x24, 0xc6, + 0xc6, 0x15, 0x48, 0xab, 0x5f, 0xb8, 0xcd, 0x41, + + 0x02, 0x20, + // s value + 0x18, 0x15, 0x22, 0xec, 0x8e, 0xca, 0x07, 0xde, + 0x48, 0x60, 0xa4, 0xac, 0xdd, 0x12, 0x90, 0x9d, + 0x83, 0x1c, 0xc5, 0x6c, 0xbb, 0xac, 0x46, 0x22, + 0x08, 0x22, 0x21, 0xa8, 0x76, 0x8d, 0x1d, 0x09, + } + + // minimal length with 1 byte r and 1 byte s. + minSig = []byte{ + 0x30, 0x06, 0x02, 0x01, 0x00, 0x02, 0x01, 0x00, + } + + // sig length is below 6. + smallLenSig = []byte{ + 0x30, 0x05, 0x02, 0x01, 0x00, 0x02, 0x01, 0x00, + } + + // sig length is above 6. + largeLenSig = []byte{ + 0x30, 0x07, 0x02, 0x01, 0x00, 0x02, 0x01, 0x00, + } + + // r length is 2. + largeRSig = []byte{ + 0x30, 0x06, 0x02, 0x02, 0x00, 0x02, 0x01, 0x00, + } + + // r length is 0. + smallRSig = []byte{ + 0x30, 0x06, 0x02, 0x00, 0x00, 0x02, 0x01, 0x00, + } + + // s length is 2. + largeSSig = []byte{ + 0x30, 0x06, 0x02, 0x01, 0x00, 0x02, 0x02, 0x00, + } + + // s length is 0. + smallSSig = []byte{ + 0x30, 0x06, 0x02, 0x01, 0x00, 0x02, 0x00, 0x00, + } + + // r length is 33. + missPaddingRSig = []byte{ + 0x30, 0x25, 0x02, 0x21, + // r value with a wrong padding. + 0xff, + 0x4e, 0x45, 0xe1, 0x69, 0x32, 0xb8, 0xaf, 0x51, + 0x49, 0x61, 0xa1, 0xd3, 0xa1, 0xa2, 0x5f, 0xdf, + 0x3f, 0x4f, 0x77, 0x32, 0xe9, 0xd6, 0x24, 0xc6, + 0xc6, 0x15, 0x48, 0xab, 0x5f, 0xb8, 0xcd, 0x41, + // s value is 0. + 0x02, 0x01, 0x00, + } + + // s length is 33. + missPaddingSSig = []byte{ + // r value is 0. + 0x30, 0x25, 0x02, 0x01, 0x00, + 0x02, 0x21, + // s value with a wrong padding. + 0xff, + 0x18, 0x15, 0x22, 0xec, 0x8e, 0xca, 0x07, 0xde, + 0x48, 0x60, 0xa4, 0xac, 0xdd, 0x12, 0x90, 0x9d, + 0x83, 0x1c, 0xc5, 0x6c, 0xbb, 0xac, 0x46, 0x22, + 0x08, 0x22, 0x21, 0xa8, 0x76, 0x8d, 0x1d, 0x09, + } +) + +func TestNewSigFromRawSignature(t *testing.T) { + t.Parallel() + testCases := []struct { + name string + rawSig []byte + expectedErr error + expectedSig Sig + }{ + { + name: "valid signature", + rawSig: normalSig, + expectedErr: nil, + expectedSig: Sig{ + // r value + 0x4e, 0x45, 0xe1, 0x69, 0x32, 0xb8, 0xaf, 0x51, + 0x49, 0x61, 0xa1, 0xd3, 0xa1, 0xa2, 0x5f, 0xdf, + 0x3f, 0x4f, 0x77, 0x32, 0xe9, 0xd6, 0x24, 0xc6, + 0xc6, 0x15, 0x48, 0xab, 0x5f, 0xb8, 0xcd, 0x41, + // s value + 0x18, 0x15, 0x22, 0xec, 0x8e, 0xca, 0x07, 0xde, + 0x48, 0x60, 0xa4, 0xac, 0xdd, 0x12, 0x90, 0x9d, + 0x83, 0x1c, 0xc5, 0x6c, 0xbb, 0xac, 0x46, 0x22, + 0x08, 0x22, 0x21, 0xa8, 0x76, 0x8d, 0x1d, 0x09, + }, + }, + { + name: "minimal length signature", + rawSig: minSig, + expectedErr: nil, + // NOTE: r and s are both 0x00 here. + expectedSig: Sig{}, + }, + { + name: "signature length too short", + rawSig: []byte{0x30}, + expectedErr: errSigTooShort, + expectedSig: Sig{}, + }, + { + name: "sig length too large", + rawSig: largeLenSig, + expectedErr: errBadLength, + expectedSig: Sig{}, + }, + { + name: "sig length too small", + rawSig: smallLenSig, + expectedErr: errBadLength, + expectedSig: Sig{}, + }, + { + name: "r length too large", + rawSig: largeRSig, + expectedErr: errBadRLength, + expectedSig: Sig{}, + }, + { + name: "r length too small", + rawSig: smallRSig, + expectedErr: errBadRLength, + expectedSig: Sig{}, + }, + { + name: "s length too large", + rawSig: largeSSig, + expectedErr: errBadSLength, + expectedSig: Sig{}, + }, + { + name: "s length too small", + rawSig: smallSSig, + expectedErr: errBadSLength, + expectedSig: Sig{}, + }, + { + name: "missing padding in r", + rawSig: missPaddingRSig, + expectedErr: errRTooLong, + expectedSig: Sig{}, + }, + { + name: "missing padding in s", + rawSig: missPaddingSSig, + expectedErr: errSTooLong, + expectedSig: Sig{}, + }, + } + + for _, tc := range testCases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + result, err := NewSigFromRawSignature(tc.rawSig) + require.Equal(t, tc.expectedErr, err) + require.Equal(t, tc.expectedSig, result) + }) + } +} From 0e242bae825afa4a7834401d55c3ce6ed3702922 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Tue, 8 Mar 2022 17:47:31 +0800 Subject: [PATCH 2/3] netann: create testSigBytes for unit test --- netann/chan_status_manager_test.go | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/netann/chan_status_manager_test.go b/netann/chan_status_manager_test.go index 4e8d97281..19ada7ba6 100644 --- a/netann/chan_status_manager_test.go +++ b/netann/chan_status_manager_test.go @@ -21,6 +21,11 @@ import ( var ( testKeyLoc = keychain.KeyLocator{Family: keychain.KeyFamilyNodeKey} + + // testSigBytes specifies a testing signature with the minimal length. + testSigBytes = []byte{ + 0x30, 0x06, 0x02, 0x01, 0x00, 0x02, 0x01, 0x00, + } ) // randOutpoint creates a random wire.Outpoint. @@ -105,13 +110,13 @@ func createEdgePolicies(t *testing.T, channel *channeldb.OpenChannel, ChannelID: channel.ShortChanID().ToUint64(), ChannelFlags: dir1, LastUpdate: time.Now(), - SigBytes: make([]byte, 64), + SigBytes: testSigBytes, }, &channeldb.ChannelEdgePolicy{ ChannelID: channel.ShortChanID().ToUint64(), ChannelFlags: dir2, LastUpdate: time.Now(), - SigBytes: make([]byte, 64), + SigBytes: testSigBytes, } } @@ -209,7 +214,7 @@ func (g *mockGraph) ApplyChannelUpdate(update *lnwire.ChannelUpdate) error { ChannelID: update.ShortChannelID.ToUint64(), ChannelFlags: update.ChannelFlags, LastUpdate: timestamp, - SigBytes: make([]byte, 64), + SigBytes: testSigBytes, } if update1 { From 74c44da315ad2cf820d1de6de74ff19a64a2c97e Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Tue, 8 Mar 2022 14:35:44 +0800 Subject: [PATCH 3/3] docs: update release notes for sig length check --- docs/release-notes/release-notes-0.15.0.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/docs/release-notes/release-notes-0.15.0.md b/docs/release-notes/release-notes-0.15.0.md index 6a6e5a5a5..2358cc8e9 100644 --- a/docs/release-notes/release-notes-0.15.0.md +++ b/docs/release-notes/release-notes-0.15.0.md @@ -44,6 +44,11 @@ arbitrator relying on htlcswitch to be started first](https://github.com/lightningnetwork/lnd/pull/6214). +* [Added signature length + validation](https://github.com/lightningnetwork/lnd/pull/6314) when calling + `NewSigFromRawSignature`. + + ## Misc * [An example systemd service file](https://github.com/lightningnetwork/lnd/pull/6033)