Skip to content

Commit

Permalink
Don't unregister tunnel IP in ST driver when reconnecting
Browse files Browse the repository at this point in the history
Similarly, if we enter the error state due to being offline, don't
send an update

Since the IP is likely to be identical after reconnecting, this should
lower the number of IOCTLs to 0 from 2. There is a small risk that
some other network interface will take over the previous tunnel IP and
be incorrectly rebound to the default interface, though this should not
be dangerous from a security standpoint, since it only applies to
excluded processes
  • Loading branch information
dlon committed Dec 14, 2022
1 parent f42ee25 commit 0a389e8
Show file tree
Hide file tree
Showing 5 changed files with 33 additions and 28 deletions.
6 changes: 6 additions & 0 deletions talpid-core/src/split_tunnel/windows/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -783,6 +783,12 @@ impl SplitTunnel {
self.send_request(Request::RegisterIps(InterfaceAddresses::default()))
}

/// Returns whether connections are being redirected.
pub fn has_tunnel_addresses(&self) -> bool {
// NOTE: Relying on assumption that `set_tunnel_addresses` was used here.
self._route_change_callback.is_some()
}

/// Returns a handle used for interacting with the split tunnel module.
pub fn handle(&self) -> SplitTunnelHandle {
SplitTunnelHandle {
Expand Down
14 changes: 2 additions & 12 deletions talpid-core/src/tunnel_state_machine/connecting_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -404,6 +404,8 @@ impl ConnectingState {
AfterDisconnect::Block(ErrorStateCause::AuthFailed(reason)),
),
Some((TunnelEvent::InterfaceUp(metadata, allowed_tunnel_traffic), _done_tx)) => {
// NOTE: It is crucial to set the correct tunnel IP before allowing any traffic into
// the tunnel, as leaks into the tunnel are possible otherwise.
#[cfg(windows)]
if let Err(error) = shared_values
.split_tunnel
Expand Down Expand Up @@ -544,18 +546,6 @@ impl TunnelState for ConnectingState {
ErrorState::enter(shared_values, ErrorStateCause::TunnelParameterError(err))
}
Ok(tunnel_parameters) => {
#[cfg(windows)]
if let Err(error) = shared_values.split_tunnel.set_tunnel_addresses(None) {
log::error!(
"{}",
error.display_chain_with_msg(
"Failed to reset addresses in split tunnel driver"
)
);

return ErrorState::enter(shared_values, ErrorStateCause::SplitTunnelError);
}

if let Err(error) = Self::set_firewall_policy(
shared_values,
&tunnel_parameters,
Expand Down
18 changes: 9 additions & 9 deletions talpid-core/src/tunnel_state_machine/disconnected_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,21 +51,21 @@ impl DisconnectedState {
shared_values: &mut SharedTunnelStateValues,
should_reset_firewall: bool,
) {
if should_reset_firewall && !shared_values.block_when_disconnected {
if let Err(error) = shared_values.split_tunnel.clear_tunnel_addresses() {
if shared_values.block_when_disconnected {
if let Err(error) = shared_values.split_tunnel.set_tunnel_addresses(None) {
log::error!(
"{}",
error.display_chain_with_msg(
"Failed to unregister addresses with split tunnel driver"
)
error
.display_chain_with_msg("Failed to reset addresses in split tunnel driver")
);
}
} else {
if let Err(error) = shared_values.split_tunnel.set_tunnel_addresses(None) {
} else if should_reset_firewall {
if let Err(error) = shared_values.split_tunnel.clear_tunnel_addresses() {
log::error!(
"{}",
error
.display_chain_with_msg("Failed to reset addresses in split tunnel driver")
error.display_chain_with_msg(
"Failed to unregister addresses with split tunnel driver"
)
);
}
}
Expand Down
18 changes: 11 additions & 7 deletions talpid-core/src/tunnel_state_machine/error_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -85,13 +85,17 @@ impl TunnelState for ErrorState {
block_reason: Self::Bootstrap,
) -> (TunnelStateWrapper, TunnelStateTransition) {
#[cfg(windows)]
if let Err(error) = shared_values.split_tunnel.set_tunnel_addresses(None) {
log::error!(
"{}",
error.display_chain_with_msg(
"Failed to register addresses with split tunnel driver"
)
);
if !block_reason.prevents_split_tunneling()
&& !shared_values.split_tunnel.has_tunnel_addresses()
{
if let Err(error) = shared_values.split_tunnel.set_tunnel_addresses(None) {
log::error!(
"{}",
error.display_chain_with_msg(
"Failed to register addresses with split tunnel driver"
)
);
}
}

#[cfg(target_os = "macos")]
Expand Down
5 changes: 5 additions & 0 deletions talpid-types/src/tunnel.rs
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,11 @@ impl ErrorStateCause {
_ => false,
}
}

#[cfg(target_os = "windows")]
pub fn prevents_split_tunneling(&self) -> bool {
matches!(self, Self::SplitTunnelError | Self::IsOffline)
}
}

/// Errors that can occur when generating tunnel parameters.
Expand Down

0 comments on commit 0a389e8

Please sign in to comment.