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 redirected 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 Sep 29, 2023
1 parent fb697fc commit 1850dd1
Show file tree
Hide file tree
Showing 5 changed files with 33 additions and 33 deletions.
14 changes: 6 additions & 8 deletions talpid-core/src/split_tunnel/windows/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -728,6 +728,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 Expand Up @@ -840,14 +846,6 @@ fn split_tunnel_default_route_change_handler(
},
Ok(None) => {
log::warn!("Failed to obtain default route interface address");
match address_family {
AddressFamily::Ipv4 => {
ctx.addresses.internet_ipv4 = None;
}
AddressFamily::Ipv6 => {
ctx.addresses.internet_ipv6 = None;
}
}
}
Err(error) => {
log::error!(
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 @@ -406,6 +406,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 @@ -550,18 +552,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
15 changes: 9 additions & 6 deletions talpid-core/src/tunnel_state_machine/disconnected_state.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,15 @@ impl DisconnectedState {
shared_values: &mut SharedTunnelStateValues,
should_reset_firewall: bool,
) {
if should_reset_firewall && !shared_values.block_when_disconnected {
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 reset addresses in split tunnel driver")
);
}
} else if should_reset_firewall {
if let Err(error) = shared_values.split_tunnel.clear_tunnel_addresses() {
log::error!(
"{}",
Expand All @@ -60,11 +68,6 @@ impl DisconnectedState {
)
);
}
} else 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")
);
}
}

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 @@ -111,6 +111,11 @@ impl ErrorStateCause {
pub fn prevents_filtering_resolver(&self) -> bool {
matches!(self, Self::SetDnsError)
}

#[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 1850dd1

Please sign in to comment.