Compatibility issue due to #738 - bad signature for announcement ... ChannelUpdates

[n1bor]

When an pre #738 node connects to a newer node and opens a channel the channel opens but when 1st channel update is sent from new-> old version all channels are closed due to invalid sig error.
New versions sends this:
00:24:59.612 DEBUG fr.acinq.eclair.Diagnostics - OUT msg=ChannelUpdate(3045022100fa18a60f87a6e9553c66e902bb4ea7dffd345746c0cf982d49baea59d3a6a3b9022034c00ba7cd3a2d37e72dab8e5e38d84a6260617b0ebbff87e59d785b8750941101,6fe28c0ab6f1b372c1a6a246ae63f74f931e8365e15a089c68d6190000000000,86243000a5c0000,1541808455, 1,0,144,1,2,2,Some(1000000000))

Old version sees this:
00:24:59.613 WARN akka://eclair-node/user/$g/router fr.acinq.eclair.router.Router - bad signature for announcement shortChannelId=86243000a5c0000 ChannelUpdate(3045022100fa18a60f87a6e9553c66e902bb4ea7dffd345746c0cf982d49baea59d3a6a3b9022034c00ba7cd3a2d37e72dab8e5e38d84a6260617b0ebbff87e59d785b8750941101,6fe28c0ab6f1b372c1a6a246ae63f74f931e8365e15a089c68d6190000000000,86243000a5c0000,1541808455,0100, 144,1,2,2)

and fails this and all other channels.

I checked and the new version is correct in thinking there is a valid sig.

  test("channel update announcement check") {
    val ann = ChannelUpdate("3045022100fa18a60f87a6e9553c66e902bb4ea7dffd345746c0cf982d49baea59d3a6a3b9022034c00ba7cd3a2d37e72dab8e5e38d84a6260617b0ebbff87e59d785b8750941101",
        "6fe28c0ab6f1b372c1a6a246ae63f74f931e8365e15a089c68d6190000000000",ShortChannelId("86243000a5c0000"),1541808455,    1,0,144,1,2,2,Some(1000000000))
    
    assert(checkSig(ann, PublicKey("02d11c4c4f986ff547288d873f6a33758aab744927355b01272ee560a3ba44d01e")))
    //assert(checkSig(ann, PublicKey("03c436af41160a355fc1ed230a64f6a64bcbd2ae50f12171d1318f9782602be601")))
    assert(checkSig(ann, randomKey.publicKey) === false)
    import fr.acinq.eclair.wire.LightningMessageCodecs
    val encoded = LightningMessageCodecs.lightningMessageCodec.encode(ann).require
    
    println(encoded.toHex)
  }

[n1bor]

encodes to:
0102fa18a60f87a6e9553c66e902bb4ea7dffd345746c0cf982d49baea59d3a6a3b934c00ba7cd3a2d37e72dab8e5e38d84a6260617b0ebbff87e59d785b875094116fe28c0ab6f1b372c1a6a246ae63f74f931e8365e15a089c68d6190000000000086243000a5c00005be621470100009000000000000000010000000200000002000000003b9aca00

[n1bor]

Is this a general compatibility issue?

Looking at network:

eclair-cli allupdates | grep htlcMaximumMsat | wc -l
1464
eclair-cli allupdates | grep signature | wc -l
17201

Most nodes do not support this or at least are not sending it.
But are newer versions of eclair sending it to nodes that do not yet understand it?
So failing the signatures? I have a node that has a channel but most explorers are not showing the fees implying that the ChannelUpdate is not being processed?

[n1bor]

Looking at spec it states we should read whole message and hash that - but this is not what code does.
if you add an extra "00" to the end of the test data in the test "decode channel_update with htlc_maximum_msat" it still matches the signature when it should fail.

So if yet another another new field was added to end would have same issue in current version.
ChannelUpdate needs a remainingMessage: BinaryData field at the end I think?
Do other messages need similar?

[pm47]

There is not much we can do: older versions are not spec compliant, the bug was hidden as long as there weren't new fields in `channel_update`. This bug has been fixed in #738 and people should upgrade to 0.2-beta8. Le jeu. 15 nov. 2018 à 00:13, n1bor <notifications@github.com> a écrit :

…

Looking at spec it states we should read whole message and hash that - but this is not what code does. if you add an extra "00" to the end of the test data in the test "decode channel_update with htlc_maximum_msat" it still matches the signature when it should fail. So if yet another another new field was added to end would have same issue in current version. ChannelUpdate needs a remainingMessage: BinaryData field at the end I think? Do other messages need similar? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#751 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB7yvqIwWPHdGw0dAD6rSjpLK8ae7LgYks5uvKOFgaJpZM4YX_j3> .

[n1bor]

But #738 still has the bug? I.e. when the next field is added the current version will have issues.
And do you know if the other implementations have the same issue? As if so maybe we should not use htlcMaximumMsat till more of network has upgraded.
Just issues like this split the network and make payments very unreliable.

[pm47]

But #738 still has the bug? I.e. when the next field is added the current version will have issues.

No, #738 fixes the bug, it even includes a compatibility test with c-lightning to be sure, see below. What do you mean by current version?

eclair/eclair-core/src/test/scala/fr/acinq/eclair/wire/LightningMessageCodecsSpec.scala

Lines 294 to 303 in 93464aa

	test("decode channel_update with htlc_maximum_msat") {
	// this was generated by c-lightning
	val bin = BinaryData("010258fff7d0e987e2cdd560e3bb5a046b4efe7b26c969c2f51da1dceec7bcb8ae1b634790503d5290c1a6c51d681cf8f4211d27ed33a257dcc1102862571bf1792306226e46111a0b59caaf126043eb5bbf28c34f3a5e332a1fc7b2b73cf188910f0005a100000200005bc75919010100060000000000000001000000010000000a000000003a699d00")
	val update = LightningMessageCodecs.lightningMessageCodec.decode(BitVector(bin.toArray)).require.value.asInstanceOf[ChannelUpdate]
	assert(update === ChannelUpdate("3044022058fff7d0e987e2cdd560e3bb5a046b4efe7b26c969c2f51da1dceec7bcb8ae1b0220634790503d5290c1a6c51d681cf8f4211d27ed33a257dcc1102862571bf1792301", "06226e46111a0b59caaf126043eb5bbf28c34f3a5e332a1fc7b2b73cf188910f", ShortChannelId(0x5a10000020000L), 1539791129, 1, 1, 6, 1, 1, 10, Some(980000000L)))
	val nodeId = PublicKey("03370c9bac836e557eb4f017fe8f9cc047f44db39c1c4e410ff0f7be142b817ae4")
	assert(Announcements.checkSig(update, nodeId))
	val bin2 = BinaryData(LightningMessageCodecs.lightningMessageCodec.encode(update).require.toByteArray)
	assert(bin === bin2)
	}

[n1bor]

The last assert below is failing. I think it should pass.
i.e. when the spec is next changed and more data is sent in the ChannelUpdate message it should be included in the hash and so signature computation.

  test("decode channel_update with htlc_maximum_msat") {
    // this was generated by c-lightning
    val bin = BinaryData("010258fff7d0e987e2cdd560e3bb5a046b4efe7b26c969c2f51da1dceec7bcb8ae1b634790503d5290c1a6c51d681cf8f4211d27ed33a257dcc1102862571bf1792306226e46111a0b59caaf126043eb5bbf28c34f3a5e332a1fc7b2b73cf188910f0005a100000200005bc75919010100060000000000000001000000010000000a000000003a699d00")
    val update = LightningMessageCodecs.lightningMessageCodec.decode(BitVector(bin.toArray)).require.value.asInstanceOf[ChannelUpdate]
    assert(update === ChannelUpdate("3044022058fff7d0e987e2cdd560e3bb5a046b4efe7b26c969c2f51da1dceec7bcb8ae1b0220634790503d5290c1a6c51d681cf8f4211d27ed33a257dcc1102862571bf1792301", "06226e46111a0b59caaf126043eb5bbf28c34f3a5e332a1fc7b2b73cf188910f", ShortChannelId(0x5a10000020000L), 1539791129, 1, 1, 6, 1, 1, 10, Some(980000000L)))
    val nodeId = PublicKey("03370c9bac836e557eb4f017fe8f9cc047f44db39c1c4e410ff0f7be142b817ae4")
    assert(Announcements.checkSig(update, nodeId))
    val bin2 = BinaryData(LightningMessageCodecs.lightningMessageCodec.encode(update).require.toByteArray)
    assert(bin === bin2)
    
    //Additional test that I think needs to pass (i.e. sig check should fail.
    val bin3=BinaryData(bin.toString() + "0000") //some new field added to spec in future that we should ignore
    val update2 = LightningMessageCodecs.lightningMessageCodec.decode(BitVector(bin2.toArray)).require.value.asInstanceOf[ChannelUpdate]
    assert(!Announcements.checkSig(update2, nodeId)) //We added some data to the packet so the hash should change so this sig check should now fail
  }

[pm47]

The last assert below is failing. I think it should pass.

You are completely right: indeed we haven't fixed the more general issue of signing the rest of the message, including unknown fields, which means that we may run into the same issue again. A (weak) reason for this is that we are going to switch to TLV in BOLT 1.1 anyway and all formats will change; there probably won't be any new fields in this old format.

[t-bast]

Closing this issue as old nodes have probably upgraded since then.