onionmessage: drop onion messages cycling back to the sending peer

[gijswijs]

Block forwarding of an onion message when the resolved next hop is the same peer that delivered it. Such a forward would immediately bounce the message back over the connection it arrived on, which is never useful and can be abused to amplify traffic against a peer (or to have us unknowingly participate in a trivial loop between two malicious neighbours).

The check runs after the routing action has been resolved, so it catches both the direct next-node-ID case and the SCID-resolved case. When a cycle is detected a new sentinel ErrSamePeerCycle is returned and the drop is logged at warn level.

Note on HTLC cycle hardening

It may be worth applying a similar hardening to HTLC forwarding. The current HTLC cycle check prevents forwarding back over the same channel, but it would still allow a cycle if the return hop goes over a parallel channel to the same peer. LND supports multiple channels per peer, so a peer-level check (in addition to the existing channel-level one) would be a natural next step.

[gemini-code-assist]

Summary of Changes

Hello, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request introduces a security hardening measure for onion message routing. By detecting and blocking messages that would be immediately routed back to the originating peer, the system prevents trivial routing loops and potential traffic amplification attacks. The change ensures that the routing logic validates the next hop against the sender's identity before proceeding with the forward action.

Highlights

• Cycle Prevention: Implemented a check to prevent onion messages from being forwarded back to the same peer that delivered them, mitigating potential traffic amplification and routing loops.
• Error Handling: Introduced a new sentinel error 'ErrSamePeerCycle' to explicitly handle and log instances where a cyclic message is dropped.
• Testing: Added comprehensive test coverage in 'TestOnionPeerActorSamePeerCycle' to validate both direct next-node-ID and SCID-resolved routing paths.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[github-actions]

🟡 PR Severity: MEDIUM

Automated classification | 2 files (excl. tests) | 31 lines changed (excl. tests)

🟡 Medium (2 files)

• onionmessage/actor.go - onion message package; not in CRITICAL or HIGH category lists
• onionmessage/errors.go - onion message package; error definitions

🟢 Low / Excluded (1 file)

• onionmessage/actor_test.go - test file, excluded from severity classification

Analysis

This PR makes additions to the onionmessage/ package (actor.go and errors.go). The onion messaging package does not fall under any of the CRITICAL or HIGH severity package paths defined in the classification rules, so it is classified as MEDIUM — "other Go files not categorized above".

No severity bump is warranted: only 2 non-test files are changed (threshold: >20), and only 31 non-test lines are added (threshold: >500).

---

_{To override, add a severity-override-{critical,high,medium,low} label.}
<!-- pr-severity-bot -->

[gemini-code-assist]

Code Review

This pull request introduces logic to detect and block onion message cycles by preventing messages from being forwarded back to the peer that sent them. It includes the addition of the ErrSamePeerCycle error and a new test case, TestOnionPeerActorSamePeerCycle, which validates the cycle detection for both direct node IDs and SCID-resolved paths. The review feedback suggests optimizing the serialization of the next node ID to avoid redundant operations and updating the logging format to adhere to the repository's structured logging style guide.

[gemini-code-assist]

The next node ID is serialized and copied into a [33]byte array here for the cycle check, and then the exact same operation is performed again later in the ElimEither block (lines 193-197) during forwarding. To improve efficiency, consider serializing the key once and reusing the result.

[gemini-code-assist]

Adhere to the repository's structured logging style. Use key-value pairs for all attributes (including the error) and keep each attribute on a single line. Structured log lines are exempt from the 80-character limit.

			log.WarnS(logCtx, "Dropping cyclic onion message",
				slog.Any("err", ErrSamePeerCycle),
				lnutils.LogPubKey("next_node_id", fwdAction.nextNodeID))

References

1. Structured log lines should use key-value pairs and one line per key-value pair for multiple attributes. They are an exception to the 80-character rule. ^(link)

[ziggie1984]

Seems like a nice to have edge case but it doesn’t materially change the resource-exhaustion story for onion messages I would say.

[gijswijs]

Seems like a nice to have edge case but it doesn’t materially change the resource-exhaustion story for onion messages I would say.

No, this is just a tiny improvement, that aligns forwarding behavior of onion messages with our forwarding behavior for HTLCs.

But don't underestimate the power of cycles in routes. The Lockdown Attack depends on them: https://eprint.iacr.org/2019/1149

This is just a way to mitigate cycles of length two.

[ziggie1984]

LGTM, minor nits (by gemini)

[ziggie1984]

Nit: we usually have a blank line before the next case statement

[ziggie1984]

Minor readability nit: since this only applies to the forward branch, you could fold the same-peer check into the forwarding path instead of doing a separate WhenLeft(...) pass and then
converting nextNodeID to [33]byte again below. Current code is fine, but this would keep the forward-case validation and execution together.

  var payload *lnwire.OnionMessagePayload

  routingAction.WhenLeft(func(fwdAction forwardAction) {
  	var nextNodeIDBytes [33]byte
  	copy(nextNodeIDBytes[:], fwdAction.nextNodeID.SerializeCompressed())

  	if nextNodeIDBytes == a.peerPubKey {
  		err = ErrSamePeerCycle
  		return
  	}

  	log.DebugS(logCtx, "Forwarding onion message",
  		lnutils.LogPubKey("next_node_id", fwdAction.nextNodeID),
  	)

  	nextMsg := lnwire.NewOnionMessage(
  		fwdAction.nextPathKey,
  		fwdAction.nextPacket,
  	)

  	sendErr := a.peerSender.SendToPeer(nextNodeIDBytes, nextMsg)
  	if sendErr != nil {
  		log.ErrorS(logCtx, "Failed to forward onion message", sendErr)
  	}

  	payload = fwdAction.payload
  })

  routingAction.WhenRight(func(dlvrAction deliverAction) {
  	log.DebugS(logCtx, "Delivering onion message to self")
  	payload = dlvrAction.payload
  })

  if err != nil {
  	return fn.Err[*Response](err)
  }

[Roasbeef]

LGTM 🛻

[github-actions]

Successfully created backport PR for v0.21.x-branch:

• [v0.21.x-branch] Backport #10754: onionmessage: drop onion messages cycling back to the sending peer #10760