Skip to content
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

TCP sockets can transmit packets with wrong source IP #466

Open
Dirbaio opened this issue Apr 13, 2021 · 3 comments · May be fixed by #970
Open

TCP sockets can transmit packets with wrong source IP #466

Dirbaio opened this issue Apr 13, 2021 · 3 comments · May be fixed by #970
Labels

Comments

@Dirbaio
Copy link
Member

Dirbaio commented Apr 13, 2021

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 old 192.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.

screenshot-2021-04-13_21-33-31

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.

  • Simply check in 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.
  • 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.

This is somewhat related with #458

@MathiasKoch
Copy link

@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?

@MathiasKoch
Copy link

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?

@MathiasKoch
Copy link

MathiasKoch commented Jul 8, 2024

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);
         }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging a pull request may close this issue.

2 participants