Open Dirbaio opened 3 years ago
@Dirbaio
I am currently running into needing to fix this issue for my application.
My approach would absolutely be to implement
Check in TcpSocket that emitting failed due to wrong source addr and change to Closed? Seems better because it allows the application to immediately notice and reconnect, but i'm not sure if there are any disadvantages
I am quite new to the smoltcp
codebase. Where would be the most correct place to compare src_addr
for the interface vs packets?
I think I've managed to check for a mismatched src addr in Interface
, in order to not emit garbage, and instead return an error from emit
(respond
closure in interface).
Next up is catching this in TcpSocket
to call set_state(State::Closed)
.. But how to go about that? The error type on emit
is generic, so I can't match on the result of emit
in TcpSocket
, and set_state()
is private, so I can't call that in Interface
..
Which way would be the most correct to go about this?
This diff essentially does what I want it to functionally, but I would suspect this is not the proper way to solve it? @Dirbaio ?
diff --git a/src/iface/interface/mod.rs b/src/iface/interface/mod.rs
index 7d6bdc8..235d9dc 100644
--- a/src/iface/interface/mod.rs
+++ b/src/iface/interface/mod.rs
@@ -599,6 +599,7 @@ impl Interface {
enum EgressError {
Exhausted,
+ MismatchingSrcIp,
Dispatch(DispatchError),
}
@@ -614,6 +615,12 @@ impl Interface {
let mut neighbor_addr = None;
let mut respond = |inner: &mut InterfaceInner, meta: PacketMeta, response: Packet| {
neighbor_addr = Some(response.ip_repr().dst_addr());
+
+ if !inner.has_ip_addr(response.ip_repr().src_addr()) {
+ net_debug!("failed to transmit IP: mismatched src addr");
+ return Err(EgressError::MismatchingSrcIp);
+ }
+
let t = device.transmit(inner.now).ok_or_else(|| {
net_debug!("failed to transmit IP: device exhausted");
EgressError::Exhausted
@@ -663,13 +670,19 @@ impl Interface {
})
}
#[cfg(feature = "socket-tcp")]
- Socket::Tcp(socket) => socket.dispatch(&mut self.inner, |inner, (ip, tcp)| {
+ Socket::Tcp(socket) => match socket.dispatch(&mut self.inner, |inner, (ip, tcp)| {
respond(
inner,
PacketMeta::default(),
Packet::new(ip, IpPayload::Tcp(tcp)),
)
- }),
+ }) {
+ Err(EgressError::MismatchingSrcIp) => {
+ socket.set_state(tcp::State::Closed);
+ Err(EgressError::MismatchingSrcIp)
+ }
+ r => r,
+ },
#[cfg(feature = "socket-dhcpv4")]
Socket::Dhcpv4(socket) => {
socket.dispatch(&mut self.inner, |inner, (ip, udp, dhcp)| {
@@ -702,6 +715,7 @@ impl Interface {
neighbor_addr.expect("non-IP response packet"),
);
}
+ Err(EgressError::MismatchingSrcIp) => {}
Ok(()) => {}
}
}
diff --git a/src/socket/tcp.rs b/src/socket/tcp.rs
index d7b85ab..cf6f42d 100644
--- a/src/socket/tcp.rs
+++ b/src/socket/tcp.rs
@@ -1204,7 +1204,7 @@ impl<'a> Socket<'a> {
self.rx_buffer.len()
}
- fn set_state(&mut self, state: State) {
+ pub fn set_state(&mut self, state: State) {
if self.state != state {
tcp_trace!("state={}=>{}", self.state, state);
}
If there's a TCP socket open, and then the smoltcp changes IP address (due to eg being plugged into a different Ethernet network), the socket stays established and still transmits packets with the old source IP address.
No matter what, a host should never transmit packets with a source IP that's not its own :)
In the packet capture below you can see the DHCP server gives the address
10.42.0.59
to the smoltcp host, but it continues transmitting packets for the old192.168.1.37
address. Eventually the socket times out and the application establishes a new one, which then gets the right source IP and communication returns.I think the same bug can be triggered by explicitly calling
tcp_socket.connect
with a wrong source address. Maybe with UDP and other sockets too.Not sure what the best solution is.
Interface
that we have the source address, and fail emitting if not? The socket would still have to timeout, but at least we don't emit garbage into the network.This is somewhat related with #458