[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?