Skip to content

Add onion failure packet length check to prevent out of bounds error #3686

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 26, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions lightning/src/ln/onion_route_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -692,6 +692,13 @@ fn test_onion_failure() {
}, || nodes[2].node.fail_htlc_backwards(&payment_hash), false, None,
Some(NetworkUpdate::NodeFailure { node_id: route.paths[0].hops[1].pubkey, is_permanent: true }),
Some(channels[1].0.contents.short_channel_id), None);

run_onion_failure_test_with_fail_intercept("bogus err packet that is too short for an hmac", 200, &nodes,
&route, &payment_hash, &payment_secret, |_msg| {}, |msg| {
msg.reason = vec![1, 2, 3];
}, || nodes[2].node.fail_htlc_backwards(&payment_hash), false, None,
None, None, None);

run_onion_failure_test_with_fail_intercept("0-length channel update in intermediate node UPDATE onion failure",
100, &nodes, &route, &payment_hash, &payment_secret, |msg| {
msg.amount_msat -= 1;
Expand Down
23 changes: 23 additions & 0 deletions lightning/src/ln/onion_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -991,6 +991,29 @@ where
_ => unreachable!(),
};

// Check that there is at least enough data for an hmac, otherwise none of the checking that we may do makes sense.
// Also prevent slice out of bounds further down.
if encrypted_packet.data.len() < 32 {
log_warn!(
logger,
"Non-attributable failure encountered on route {}",
path.hops.iter().map(|h| h.pubkey.to_string()).collect::<Vec<_>>().join("->")
);

// Signal that we failed permanently. Without a valid hmac, we can't identify the failing node and we can't
// apply a penalty. Therefore there is nothing more we can do other than failing the payment.
return DecodedOnionFailure {
network_update: None,
short_channel_id: None,
payment_failed_permanently: true,
failed_within_blinded_path: false,
#[cfg(any(test, feature = "_test_utils"))]
onion_error_code: None,
#[cfg(any(test, feature = "_test_utils"))]
onion_error_data: None,
};
}

// Learnings from the HTLC failure to inform future payment retries and scoring.
struct FailureLearnings {
network_update: Option<NetworkUpdate>,
Expand Down
Loading