[bug] channel announcement signatures must cover the tlv stream

[t-bast]

I've been noticing that our node receives a lot of gossip data that has invalid signatures.
This happens for node announcements, channel announcements and channel updates.

I've been able to track the reason behind the channel announcement issue: those channel announcements contain a TLV field (with tag 55555) and the signature doesn't cover that TLV field (it should!). Here is one example from mainnet:

// Encoded channel announcement:
01009fa1d5b5ee517104c5eae0c9962b513e89d6de5dad09366ff9b4f21f3299d4485dc54f1fa8add3272a9b61cc9c63619125c1b9500314a9add2d15cfcc6549d8a51e486f4e5f08f56a1b4000044dcdd920476c6a49d843057c2885e380e3d85ca33b734ba285bcf8290e15ded660095eb1f53b124ac73fffd3023a656b5e120fc356016a694fbb559e86da9e22f7ecfd1894df1618ad0289b7b6b44d6ce11334511539b07373b333527bd71605f11a7b24b8576b0999c995ea4edbe245c1d140c360961128f3d567be9959f0aa024d280d80942153b0846ff24c17cb9b93d0dd54e53dbc2a66f8158fa063ca110ca0f4c46ada5207b48d047604309372993139300006fe28c0ab6f1b372c1a6a246ae63f74f931e8365e15a089c68d61900000000000c8d520009a90001023e24602891c28a7872ea1ad5c1bb41abe4206ae1599bb981e3278a121e7895d603ad0188fd1c10f9b2f41349a7433072c3a350ad719bd1a5f3b8da4808f97b699603cde7e22d13a1b5b7625766240a38465324dee3eacda3a693866af37e68b8c7b502e92ec7ab9b3fbdd6a88ff547cf2899d4726297eea357e6219c7a42e5296095b4fdd903080000000000000000

val chanAnn = ChannelAnnouncement(
    nodeSignature1 = "9fa1d5b5ee517104c5eae0c9962b513e89d6de5dad09366ff9b4f21f3299d4485dc54f1fa8add3272a9b61cc9c63619125c1b9500314a9add2d15cfcc6549d8a",
    nodeSignature2 = "51e486f4e5f08f56a1b4000044dcdd920476c6a49d843057c2885e380e3d85ca33b734ba285bcf8290e15ded660095eb1f53b124ac73fffd3023a656b5e120fc",
    bitcoinSignature1 = "356016a694fbb559e86da9e22f7ecfd1894df1618ad0289b7b6b44d6ce11334511539b07373b333527bd71605f11a7b24b8576b0999c995ea4edbe245c1d140c",
    bitcoinSignature2 = "360961128f3d567be9959f0aa024d280d80942153b0846ff24c17cb9b93d0dd54e53dbc2a66f8158fa063ca110ca0f4c46ada5207b48d0476043093729931393",
    features = Features.empty,
    chainHash = "6fe28c0ab6f1b372c1a6a246ae63f74f931e8365e15a089c68d6190000000000",
    shortChannelId = "822610x2473x1",
    nodeId1 = "023e24602891c28a7872ea1ad5c1bb41abe4206ae1599bb981e3278a121e7895d6",
    nodeId2 = "03ad0188fd1c10f9b2f41349a7433072c3a350ad719bd1a5f3b8da4808f97b6996",
    bitcoinKey1 = "03cde7e22d13a1b5b7625766240a38465324dee3eacda3a693866af37e68b8c7b5",
    bitcoinKey2 = "02e92ec7ab9b3fbdd6a88ff547cf2899d4726297eea357e6219c7a42e5296095b4",
    tlvStream = TlvStream(Set.empty[ChannelAnnouncementTlv], Set(GenericTlv(tag = 55555, value = 0x0000000000000000)))
)

The signatures in this channel announcement don't cover the full channel announcement, as they don't sign the TLV stream. If you remove the TLV stream data, then the signatures are valid.

For node announcements, I'm not sure what the issue is. The following node announcement for example was relayed to our node and has an invalid signature:

// Encoded node announcement:
01012dde9e1f6fc92bc32286434ea0387898b87614608985dba49c8595d07ce4f7fc1549eb972f72eb2557c784dcb06e3b4cadc9aac12a5b62fc3991e0d54fae5ec600062000080aa2a266b0815303f27a6b17368dda34e01d183411d8350dfa8436a85a01cf325d52ebca1f13905800000000000000000000000000000000000000000000000000000000000000000000000000

NodeAnnouncement(
    signature = "2dde9e1f6fc92bc32286434ea0387898b87614608985dba49c8595d07ce4f7fc1549eb972f72eb2557c784dcb06e3b4cadc9aac12a5b62fc3991e0d54fae5ec6",
    features = Features(
      Features.Wumbo -> FeatureSupport.Optional,
      Features.UpfrontShutdownScript -> FeatureSupport.Optional,
      Features.DataLossProtect -> FeatureSupport.Optional,
      Features.VariableLengthOnion -> FeatureSupport.Optional,
      Features.StaticRemoteKey -> FeatureSupport.Optional,
      Features.PaymentSecret -> FeatureSupport.Optional,
      Features.ShutdownAnySegwit -> FeatureSupport.Optional,
      Features.ChannelType -> FeatureSupport.Optional,
      Features.BasicMultiPartPayment -> FeatureSupport.Optional,
      Features.ChannelRangeQueries -> FeatureSupport.Optional,
    ),
    timestamp = 1722843475 unixsec,
    nodeId = "03f27a6b17368dda34e01d183411d8350dfa8436a85a01cf325d52ebca1f139058",
    color = Color(0, 0, 0),
    alias = "",
    addresses = List(),
    tlvStream = TlvStream.empty
)

I'd like to know if lnd agrees that the signature is incorrect: if that's the case, why is lnd still relaying that invalid node announcement?

[bitromortac]

The signatures in this channel announcement don't cover the full channel announcement, as they don't sign the TLV stream. If you remove the TLV stream data, then the signatures are valid.

I could reproduce this, ValidateChannelAnn fails when the tlv data is set, but passes otherwise.

[Crypt-iQ]

How can both channel parties agree on the tlv data to sign?

[t-bast]

It depends on the spec for each specific TLV field (I don't know what this 55555 TLV is used for, it doesn't seem to be specified in any bLIP that I read, so I can't comment on this specific one), but most likely it should be "negotiated" in announcement_signatures to ensure that both peers produce the same channel_announcement. Otherwise, which one should readers use when they receive both? How are they expected to handle conflicts?

[Crypt-iQ]

LND sees the node ann as valid, not sure why the discrepancy. As far as the TLV fields, I agree that they should be negotiated in announce sig, but it's not spelled out in the spec afaict which is why prob why we dont do it.

[DerEwige]

It depends on the spec for each specific TLV field (I don't know what this 55555 TLV is used for, it doesn't seem to be specified in any bLIP that I read, so I can't comment on this specific one), but most likely it should be "negotiated" in announcement_signatures to ensure that both peers produce the same channel_announcement. Otherwise, which one should readers use when they receive both? How are they expected to handle conflicts?

55555 TLV carries the inbound fee information for that channel
afaik it is just a random high number TLV for the prototype
lightning/blips#18

[t-bast]

As far as the TLV fields, I agree that they should be negotiated in announce sig, but it's not spelled out in the spec afaict which is why prob why we dont do it.

Bolt 7 explicitly says that to produce the signatures, you "MUST compute the double-SHA256 hash h of the message, beginning at offset 256, up to the end of the message.". It thus requires signing the TLVs since they are part of the message bytes, do you think we should modify the spec to make this clearer?

LND sees the node ann as valid, not sure why the discrepancy.

Interesting, so for that node announcement, eclair sees it as invalid and lnd sees it as valid. Do we both encode it to the same value (01012dde9e1f6fc92bc32286434ea0387898b87614608985dba49c8595d07ce4f7fc1549eb972f72eb2557c784dcb06e3b4cadc9aac12a5b62fc3991e0d54fae5ec600062000080aa2a266b0815303f27a6b17368dda34e01d183411d8350dfa8436a85a01cf325d52ebca1f13905800000000000000000000000000000000000000000000000000000000000000000000000000)? I'd like to know if LDK and CLN consider this node announcement as valid or not, to see if this is an issue in eclair or in lnd. @rustyrussell @TheBlueMatt can you check?

55555 TLV carries the inbound fee information for that channel
afaik it is just a random high number TLV for the prototype
lightning/blips#18

I don't understand, that bLIP mentions setting this TLV in channel_update, never in channel_announcement. I don't see any good reason to include this in channel_announcement anyway, this seems to be a mistake?

[Crypt-iQ]

Bolt 7 explicitly says that to produce the signatures, you "MUST compute the double-SHA256 hash h of the message, beginning at offset 256, up to the end of the message.". It thus requires signing the TLVs since they are part of the message bytes, do you think we should modify the spec to make this clearer?

Both parties should sign the same data, right? If so, I think the spec needs to be explicit about how both parties can agree on the TLV data to sign; I don't think that's spelled out anywhere. On second thought, it can't be in announcement sigs because that's where we actually put the sig, so it needs to be prior to that.

[t-bast]

Both parties should sign the same data, right? If so, I think the spec needs to be explicit about how both parties can agree on the TLV data to sign; I don't think that's spelled out anywhere.

It's not spelled out anywhere yet because there is no TLV defined yet for channel_announcement. But it is spelled out that the signature must cover the whole message and thus include TLVs. I think that it's the responsibility of each feature that adds TLVs to channel_announcements to specify how nodes must come to agreement on the TLV data (and will probably depend on what that data contains).

We can add a sentence to the BOLTs to spell this out, and defer this negotiation to every feature that needs it.

I still don't understand why you chose to include a TLV in channel_announcement for inbound fees instead of only using channel_updates (or even better, a dedicated message exchanged only between channel peers), which would have avoided this issue entirely!

[TheBlueMatt]

Do you have a fully encoded version? Starting with 0101 doesn't look like its starting with a signature to me?

[t-bast]

It is a fully encoded version, 0101 is the lightning message type (257 is the message type for node announcements). But indeed the signature doesn't cover that message prefix, only what appears after those first two bytes!

[TheBlueMatt]

LDK says Invalid signature on node_announcement message (and we don't implement reading TLVs at the end of node_announcements at all because we don't have any fields that we care about yet, so I'm betting its not a TLV confusion isue).

[t-bast]

Thanks for checking! This node_announcement has an empty TLV stream, so this indeed shouldn't be the issue. So for those node_announcements, the result is currently that:

• Eclair says the signature is invalid
• LDK says the signature is invalid
• LND says the signature is valid
• CLN -> unknown yet

[Roasbeef]

I still don't understand why you chose to include a TLV in channel_announcement for inbound fees instead of only using

Perhaps someone else implemented it, but incorrectly? AFAIK, the field is only in the channel_update.

We also do include the extra TLV data in what we sign:

lnd/lnwire/channel_announcement.go

Lines 181 to 183 in c0420fe

	if err := WriteBytes(buf, a.ExtraOpaqueData); err != nil {
	return nil, err
	}

.

Here's where we make our channel announcement, we add no extra TLVs:

lnd/funding/manager.go

Lines 4149 to 4156 in c0420fe

	// The unconditional section of the announcement is the ShortChannelID
	// itself which compactly encodes the location of the funding output
	// within the blockchain.
	chanAnn := &lnwire.ChannelAnnouncement{
	ShortChannelID: shortChanID,
	Features: lnwire.NewRawFeatureVector(),
	ChainHash: chainHash,
	}

.

Could it be that new impl that has popped up maybe?

[t-bast]

Could it be that new impl that has popped up maybe?

Damn, it's probably someone who forked lnd, I think it's the only implementation with inbound fees support...the issue is that lnd does relay those invalid channel_announcements though, so they can't know they're doing it wrong. If by default lnd dropped those channel_announcements because their signatures are invalid, whoever wrote that fork would notice that they're not doing it right!

[Roasbeef]

Weird that we relay them, given we use that same DataToSign method when we go to validate: https://github.com/lightningnetwork/lnd/blob/master/graph/ann_validation.go#L15-L26. So if they're actually invalid (generated signature w/o extra data), then we should reject them.

We'll try more repro attempts....

[Crypt-iQ]

Oops, I forgot to actually validate the node announcement. LND sees it as invalid

[Roasbeef]

Ok, we might've found something here:

lnd/discovery/gossiper.go

Line 2235 in c0420fe

ExtraOpaqueData: edge.ExtraOpaqueData,

[Roasbeef]

narrator: turns out it was lnd all along 🤣

Fix is up here: #9002

TL;DR is that I introduced a bug here 6 years ago, but it went unnoticed as we didn't use TLVs in any gossip stuff yet. The fix itself is 1 line.

Once nodes update, they'll start to send the correct chan ann either once they issue a fee/policy update, or once the node rebroadcasts channels that are close to being considered zombies.

[t-bast]

Nice, thanks for the quick investigation!

[rustyrussell]

I just hacked some more logging into my CLN node, and seems like we have a bug:

2024-08-15T01:33:48.283Z INFO    gossipd: Channel announcement for 822610x2473x1
2024-08-15T01:33:48.283Z INFO    gossipd: ... sigcheck gave Bad node_signature_1 30450221009fa1d5b5ee517104c5eae0c9962b513e89d6de5dad09366ff9b4f21f3299d44802205dc54f1fa8add3272a9b61cc9c63619125c1b9500314a9add2d15cfcc6549d8a hash ed6b72ac35c7d737ae0a64b82e5c7b4d564969c624956edb18a881908cea0891 on channel_announcement 01009fa1d5b5ee517104c5eae0c9962b513e89d6de5dad09366ff9b4f21f3299d4485dc54f1fa8add3272a9b61cc9c63619125c1b9500314a9add2d15cfcc6549d8a51e486f4e5f08f56a1b4000044dcdd920476c6a49d843057c2885e380e3d85ca33b734ba285bcf8290e15ded660095eb1f53b124ac73fffd3023a656b5e120fc356016a694fbb559e86da9e22f7ecfd1894df1618ad0289b7b6b44d6ce11334511539b07373b333527bd71605f11a7b24b8576b0999c995ea4edbe245c1d140c360961128f3d567be9959f0aa024d280d80942153b0846ff24c17cb9b93d0dd54e53dbc2a66f8158fa063ca110ca0f4c46ada5207b48d047604309372993139300006fe28c0ab6f1b372c1a6a246ae63f74f931e8365e15a089c68d61900000000000c8d520009a90001023e24602891c28a7872ea1ad5c1bb41abe4206ae1599bb981e3278a121e7895d603ad0188fd1c10f9b2f41349a7433072c3a350ad719bd1a5f3b8da4808f97b699603cde7e22d13a1b5b7625766240a38465324dee3eacda3a693866af37e68b8c7b502e92ec7ab9b3fbdd6a88ff547cf2899d4726297eea357e6219c7a42e5296095b4fdd903080000000000000000

Will add testing and fix now, thanks!

[TheBlueMatt]

AFAIU you're right - this message is not validly signed so you should be failing.

[rustyrussell]

AFAIU you're right - this message is not validly signed so you should be failing.

Ah, right, I read too fast. Indeed...

[Roasbeef]

AFAIU you're right - this message is not validly signed so you should be failing.

Yeah the bug was updating the TLV in the chan ann, but not re-signing before we sent it out (to actually cover those fields).