Incorrect signature verification edge cases

[morehouse]

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

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 unknown fields following fee_proportional_millionths):
  • SHOULD send a warning and close the connection.
  • MUST NOT process the message further.

But Eclair actually re-encodes messages through its own codec before verifying signatures, and for certain fields the round trip can change the underlying bytes:

• characters after the first null byte are stripped from alias
• features bitvectors are changed to minimal form
• message_flags and channel_flags have the upper bits stripped

When this occurs, Eclair 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 these bugs, a buggy or malicious node could do it and cause Eclair'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:

eclair/eclair-core/src/main/scala/fr/acinq/eclair/io/PeerConnection.scala

Lines 459 to 468 in 2dda794

	case GossipDecision.InvalidSignature(r) =>
	val bin: String = LightningMessageCodecs.meteredLightningMessageCodec.encode(r) match {
	case Attempt.Successful(b) => b.toHex
	case _ => "unknown"
	}
	log.error(s"peer sent us a routing message with invalid sig: r=$r bin=$bin")
	// for now we just send a warning, maybe ban the peer in the future?
	// TODO: this doesn't actually disconnect the peer, once we introduce peer banning we should actively disconnect
	d.transport ! Warning(s"invalid announcement sig (bin=$bin)")
	d.behavior

Reproducing

0001-Demonstrate-signature-verification-bugs.patch

Discovery

This issue was surfaced while fuzzing node_announcements using Smite. Eclair failed to verify the signature when alias had trailing non-null bytes after the initial null byte, while other implementations succeeded in verifying the signature.

[t-bast]

Good catch, thanks! We fixed a related bug years ago, where we weren't correctly including unknown TLVs and thus forwarding a modified node_announcement for which the signature didn't match. We'll have the same issue for the fields you're highlighting, we need to fix that.

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

Just to make sure I'm not missing anything: this doesn't allow malicious nodes to get eclair to ignore valid announcements, right? The only announcements that eclair will end up ignoring (and messing up when forwarding to peers) are announcements that include these modified/invalid fields, which no sane implementation will create. So the worst thing that can happen is that we end up ignoring some nodes, that are malicious or buggy anyway, which should be fixed, but isn't critical?

[morehouse]

Just to make sure I'm not missing anything: this doesn't allow malicious nodes to get eclair to ignore valid announcements, right? The only announcements that eclair will end up ignoring (and messing up when forwarding to peers) are announcements that include these modified/invalid fields, which no sane implementation will create. So the worst thing that can happen is that we end up ignoring some nodes, that are malicious or buggy anyway, which should be fixed, but isn't critical?

Today a malicious node can get Eclair to ignore the malicious node's own announcements, but not other announcements. A malicious node could also get Eclair to accept the malicious nodes' own announcements that every other implementation would reject.

In the future, if Eclair implements the TODO mentioned above, or if any other implementation does something similar, a malicious node could produce gossip that would either cause Eclair to ban all its non-Eclair peers or would cause Eclair get banned by all its non-Eclair peers.

I also took a closer look, and it seems that even spec-conforming implementations can produce fields that will cause Eclair to incorrectly reject certain announcements.

• alias: "MUST set alias to a valid UTF-8 string, with any alias trailing-bytes equal to 0." UTF-8 technically allows null bytes within strings, and it appears that LND, LDK, and even Eclair will encode aliases containing null bytes if the node operator chooses such an alias.
• features in node_announcement: "SHOULD set flen to the minimum length required." It's a SHOULD, which means the sender can send non-minimal features if they want. AFAICT no implementation currently does this.
• features in channel_announcement: "MUST set len to the minimum length." Non-minimal features are not valid for this message.
• message_flags and channel_flags: "MUST set bits... not assigned a meaning to 0." Flag bytes with upper bits set are not valid.

Perhaps the spec should be updated to make the features requirements consistent across node_announcement and channel_announcement, especially since everyone already encodes minimally anyway. I'm less sure about changing the alias requirements to explicitly ban non-trailing null bytes

[t-bast]

Thanks, I agree with all of your points. I've opened a spec PR to ban unicode control characters entirely (I don't see any good reason to allow them) and fix the feature minimal encoding. If this gets a concept ACK, I'll make sure eclair follows this updated spec and will fix the message_flags and channel_flags encoding issue related to unknown upper bits.