[bug]: incorrect signature verification edge case

[morehouse]

BOLT 7 states in multiple places that gossip message signatures are to be verified over the actual message bytes. For example, for node_announcement:

The receiving node:

• if signature is NOT a valid signature (using node_id of the double-SHA256 of the entire message following the signature field, including any future fields appended to the end):
  • SHOULD send a warning.
  • MAY close the connection.
  • MUST NOT process the message further.

But LND actually re-encodes messages through its own codec before verifying signatures, and for one particular field the round trip can change the underlying bytes:

• features bitvectors are changed to minimal form

When this occurs, LND verifies signatures incorrectly and may reject gossip that other implementations accept, or vice versa.

Implications

While a spec-conforming node should never create gossip that triggers this bug, a buggy or malicious node could do it and cause LND's view of the LN network to diverge from other nodes' views.

A more concerning network isolation vector could arise if any implementation ever decides to ban peers that repeatedly send gossip with incorrect signatures. While no implementation does this currently, Eclair does have a TODO indicating the intention to do it in the future:

https://github.com/ACINQ/eclair/blob/2dda79468a8b69a2acf7962cdac63245f7cc3ee8/eclair-core/src/main/scala/fr/acinq/eclair/io/PeerConnection.scala#L459-L468

Reproducing

0001-Demonstrate-signature-verification-bug.patch

Discovery

This issue was surfaced while fuzzing node_announcements using Smite.

[Roasbeef]

features bitvectors are changed to minimal form

Glancing a the spec, I think it's a bit inconsistent here.

Eg:

• https://github.com/lightning/bolts/blob/master/01-messaging.md#:~:text=SHOULD%20use%20the%20minimum%20length%20required%20to%20represent%20the%20features%20field.

• SHOULD use the minimum length required to represent the features field.

If they spec says SHOULD, but an implementation doesn't, then is the preferred behavior to accept or reject?

The nature of things right now also is that the view of two nodes of the same implementation may diverge depending on things like zombie chan pruning, etc.

I think we'd likely seek to gain clarity in this area in the spec before switching to banning or rejection, etc.

[Roasbeef]

For ref, here's where we re-encode:

lnd/lnwire/channel_announcement.go

Lines 162 to 202 in cc49b37

	// DataToSign is used to retrieve part of the announcement message which should
	// be signed.
	func (a *ChannelAnnouncement1) DataToSign() ([]byte, error) {
	// We should not include the signatures itself.
	b := make([]byte, 0, MaxMsgBody)
	buf := bytes.NewBuffer(b)
	if err := WriteRawFeatureVector(buf, a.Features); err != nil {
	return nil, err
	}
	if err := WriteBytes(buf, a.ChainHash[:]); err != nil {
	return nil, err
	}
	if err := WriteShortChannelID(buf, a.ShortChannelID); err != nil {
	return nil, err
	}
	if err := WriteBytes(buf, a.NodeID1[:]); err != nil {
	return nil, err
	}
	if err := WriteBytes(buf, a.NodeID2[:]); err != nil {
	return nil, err
	}
	if err := WriteBytes(buf, a.BitcoinKey1[:]); err != nil {
	return nil, err
	}
	if err := WriteBytes(buf, a.BitcoinKey2[:]); err != nil {
	return nil, err
	}
	if err := WriteBytes(buf, a.ExtraOpaqueData); err != nil {
	return nil, err
	}
	return buf.Bytes(), nil
	}

All other gossip messages have a similar DataToSign() method that takes a based message, then spits out fresh bytes to sign/verify against.

[erickcestari]

Here is where the features are decoded into a RawFeatureVector, causing the padding bytes to be lost. Later, when re-encoded, the result becomes minimally encoded.

lnd/lnwire/features.go

Lines 681 to 699 in cc49b37

	func (fv *RawFeatureVector) decode(r io.Reader, length, width int) error {
	// Read the feature vector data.
	data := make([]byte, length)
	if _, err := io.ReadFull(r, data); err != nil {
	return err
	}
	// Set feature bits from parsed data.
	bitsNumber := len(data) * width
	for i := 0; i < bitsNumber; i++ {
	byteIndex := int(i / width)
	bitIndex := uint(i % width)
	if (data[length-byteIndex-1]>>bitIndex)&1 == 1 {
	fv.Set(FeatureBit(i))
	}
	}
	return nil
	}