laantungir/fips
1
0
Fork 0
mirror of https://github.com/jmcorgan/fips.git synced 2026-07-22 07:48:26 +00:00
Code Issues Packages Projects Releases Wiki Activity

Fix session state poisoning panic, harden session state access

Browse Source 
handle_session_ack() had a bug where take_state() followed by
a non-Initiating match would re-insert the entry with state=None,
causing panics on subsequent .state()/.state_mut() calls. Fix by
checking is_initiating() before take_state().

Add safe is_initiating()/is_responding() to SessionEntry (matching
existing is_established()) and convert all production .state() callers
to use these None-safe methods. Gate .state() with #[cfg(test)].
This commit is contained in:
Johnathan Corgan
2026-02-19 20:56:00 +00:00
parent a8d3627072
commit 12db6f561a
2 changed files with 47 additions and 40 deletions
Download Patch File Download Diff File Expand all files Collapse all files

76
src/node/handlers/session.rs
View File

@@ -141,7 +141,7 @@ impl Node {
}
};
if entry.state().is_responding() {
if entry.is_responding() {
let old_state = entry.take_state();
let handshake = match old_state {
Some(EndToEndState::Responding(hs)) => hs,
@@ -299,42 +299,38 @@ impl Node {
// Check for existing session with this remote
if let Some(existing) = self.sessions.get(src_addr) {
match existing.state() {
EndToEndState::Initiating(_) => {
// Simultaneous initiation: smaller NodeAddr wins as initiator
if self.identity.node_addr() < src_addr {
// We win — drop their setup, they'll process ours
debug!(
src = %self.peer_display_name(src_addr),
"Simultaneous session initiation: we win (smaller addr), dropping their setup"
);
return;
}
// We lose — discard our pending handshake, become responder below
if existing.is_initiating() {
// Simultaneous initiation: smaller NodeAddr wins as initiator
if self.identity.node_addr() < src_addr {
// We win — drop their setup, they'll process ours
debug!(
src = %self.peer_display_name(src_addr),
"Simultaneous session initiation: we lose, becoming responder"
"Simultaneous session initiation: we win (smaller addr), dropping their setup"
);
}
EndToEndState::Responding(_) => {
// Duplicate setup while we already responded — resend stored ack
if let Some(payload) = existing.handshake_payload() {
debug!(src = %self.peer_display_name(src_addr), "Duplicate SessionSetup, resending SessionAck");
let my_addr = *self.node_addr();
let mut datagram = SessionDatagram::new(my_addr, *src_addr, payload.to_vec())
.with_ttl(self.config.node.session.default_ttl);
if let Err(e) = self.send_session_datagram(&mut datagram).await {
debug!(error = %e, dest = %self.peer_display_name(src_addr), "Failed to resend SessionAck");
}
} else {
debug!(src = %self.peer_display_name(src_addr), "Duplicate SessionSetup, no stored ack to resend");
}
return;
}
EndToEndState::Established(_) => {
// Re-establishment: replace existing session below
debug!(src = %self.peer_display_name(src_addr), "Session re-establishment from peer");
// We lose — discard our pending handshake, become responder below
debug!(
src = %self.peer_display_name(src_addr),
"Simultaneous session initiation: we lose, becoming responder"
);
} else if existing.is_responding() {
// Duplicate setup while we already responded — resend stored ack
if let Some(payload) = existing.handshake_payload() {
debug!(src = %self.peer_display_name(src_addr), "Duplicate SessionSetup, resending SessionAck");
let my_addr = *self.node_addr();
let mut datagram = SessionDatagram::new(my_addr, *src_addr, payload.to_vec())
.with_ttl(self.config.node.session.default_ttl);
if let Err(e) = self.send_session_datagram(&mut datagram).await {
debug!(error = %e, dest = %self.peer_display_name(src_addr), "Failed to resend SessionAck");
}
} else {
debug!(src = %self.peer_display_name(src_addr), "Duplicate SessionSetup, no stored ack to resend");
}
return;
} else if existing.is_established() {
// Re-establishment: replace existing session below
debug!(src = %self.peer_display_name(src_addr), "Session re-establishment from peer");
}
}
@@ -422,15 +418,15 @@ impl Node {
}
};
// Must be in Initiating state
// Must be in Initiating state — check before take to avoid poisoning
if !entry.is_initiating() {
debug!(src = %self.peer_display_name(src_addr), "SessionAck but session not in Initiating state");
self.sessions.insert(*src_addr, entry);
return;
}
let handshake = match entry.take_state() {
Some(EndToEndState::Initiating(hs)) => hs,
_ => {
debug!(src = %self.peer_display_name(src_addr), "SessionAck but session not in Initiating state");
// Put it back
self.sessions.insert(*src_addr, entry);
return;
}
_ => unreachable!("checked is_initiating above"),
};
// Complete the handshake
@@ -720,7 +716,7 @@ impl Node {
) -> Result<(), NodeError> {
// Check for existing session
if let Some(existing) = self.sessions.get(&dest_addr)
&& (existing.state().is_established() || existing.state().is_initiating())
&& (existing.is_established() || existing.is_initiating())
{
return Ok(());
}
@@ -1281,7 +1277,7 @@ impl Node {
// Skip if a session already exists
if let Some(existing) = self.sessions.get(&dest_addr)
&& (existing.state().is_established() || existing.state().is_initiating())
&& (existing.is_established() || existing.is_initiating())
{
return;
}

11
src/node/session.rs
View File

@@ -112,6 +112,7 @@ impl SessionEntry {
}
/// Get the current session state.
#[cfg(test)]
pub(crate) fn state(&self) -> &EndToEndState {
self.state.as_ref().expect("session state taken but not restored")
}
@@ -147,6 +148,16 @@ impl SessionEntry {
self.state.as_ref().is_some_and(|s| s.is_established())
}
/// Check if we are the initiator (waiting for ack).
pub(crate) fn is_initiating(&self) -> bool {
self.state.as_ref().is_some_and(|s| s.is_initiating())
}
/// Check if we are the responder (sent ack, waiting for data).
pub(crate) fn is_responding(&self) -> bool {
self.state.as_ref().is_some_and(|s| s.is_responding())
}
/// Get creation time.
#[cfg(test)]
pub(crate) fn created_at(&self) -> u64 {