Repository: fujiapple852/trippy (language: rust)

Issue:
Tracer debug_asset failure for UDP protocol
### Error

Tracing with UDP protocol failed with:

```
  2024-02-03T11:06:08.569220Z DEBUG trippy::tracing::net::platform::unix: buf: "45 20 42 00 55 db 00 00 35 01 f4 f9 b3 3d c5 b7 c0 a8 01 15 03 03 37 e7 00 00 00 00 45 48 3a 00 6c 95 00 00 2c 11 e7 23 c0 a8 01 15 b3 3d c5 b7 c8 d5 f5 9a 00 26 77 47 41 02 e7 18 c0 dd a3 e5 00 00 00 00 00 04 00 00 02 4d 00 00 00 08 00 00 00 00 00 00 00 00", bytes_read: 86
    at src/tracing/net/platform/unix.rs:417
    in trippy::tracing::net::platform::unix::read
    in trippy::tracing::net::ipv4::recv_icmp_probe with protocol: Udp, icmp_extension_mode: Enabled
    in trippy::tracing::net::channel::recv_icmp_probe
    in trippy::tracing::net::channel::recv_probe
    in trippy::tracing::tracer::recv_response
    in trippy::tracing::tracer::trace

  2024-02-03T11:06:08.569304Z  INFO trippy::tracing::net::platform::unix: return: Ok(86)
    at src/tracing/net/platform/unix.rs:411
    in trippy::tracing::net::platform::unix::read
    in trippy::tracing::net::ipv4::recv_icmp_probe with protocol: Udp, icmp_extension_mode: Enabled
    in trippy::tracing::net::channel::recv_icmp_probe
    in trippy::tracing::net::channel::recv_probe
    in trippy::tracing::tracer::recv_response
    in trippy::tracing::tracer::trace

  2024-02-03T11:06:08.569512Z DEBUG trippy::tracing::net::channel: resp: DestinationUnreachable(ProbeResponseData { recv: SystemTime { tv_sec: 1706958368, tv_nsec: 569449000 }, addr: 179.61.197.183, resp_seq: Udp(ProbeResponseSeqUdp { identifier: 27797, src_port: 51413, dest_port: 62874, checksum: 30535 }) }, None)
    at src/tracing/net/channel.rs:99
    in trippy::tracing::net::channel::recv_probe
    in trippy::tracing::tracer::recv_response
    in trippy::tracing::tracer::trace

thread 'tracer-34052' panicked at src/tracing/tracer.rs:658:17:
completed probe was not in Awaited state (seq=62874)
```

### Packet Debug

Ipv4 packet debug:

```
Ipv4Packet {
    version: 4,
    header_length: 5,
    dscp: 8,
    ecn: 0,
    total_length: 16896,
    identification: 21979,
    flags_and_fragment_offset: 0,
    ttl: 53,
    protocol: Icmp,
    checksum: 62713,
    source: 179.61.197.183,
    destination: 192.168.1.21,
    options_raw: [],
    payload: "03 03 37 e7 00 00 00 00 45 48 3a 00 6c 95 00 00 2c 11 e7 23 c0 a8 01 15 b3 3d c5 b7 c8 d5 f5 9a 00 26 77 47 41 02 e7 18 c0 dd a3 e5 00 00 00 00 00 04 00 00 02 4d 00 00 00 08 00 00 00 00 00 00 00 00",
}
```

Ipv4 payload (icmp) packet debug:

```
IcmpPacket {
    icmp_type: DestinationUnreachable,
    icmp_code: IcmpCode(
        3,
    ),
    checksum: 14311,
}
```

Ipv4 payload (icmp DestinationUnreachable) packet debug:

```
DestinationUnreachablePacket {
    icmp_type: DestinationUnreachable,
    icmp_code: IcmpCode(
        3,
    ),
    checksum: 14311,
    length: 0,
    next_hop_mtu: 0,
    payload: "45 48 3a 00 6c 95 00 00 2c 11 e7 23 c0 a8 01 15 b3 3d c5 b7 c8 d5 f5 9a 00 26 77 47 41 02 e7 18 c0 dd a3 e5 00 00 00 00 00 04 00 00 02 4d 00 00 00 08 00 00 00 00 00 00 00 00",
}
```

Icmp DestinationUnreachable payload (original ipv4 datagram) packet debug: 

```
Ipv4Packet {
    version: 4,
    header_length: 5,
    dscp: 18,
    ecn: 0,
    total_length: 14848,
    identification: 27797,
    flags_and_fragment_offset: 0,
    ttl: 44,
    protocol: Udp,
    checksum: 59171,
    source: 192.168.1.21,
    destination: 179.61.197.183,
    options_raw: [],
    payload: "c8 d5 f5 9a 00 26 77 47 41 02 e7 18 c0 dd a3 e5 00 00 00 00 00 04 00 00 02 4d 00 00 00 08 00 00 00 00 00 00 00 00",
}
```

### Analysis

1. Focusing on the `ProbeResponseSeqUdp` above:

```
ProbeResponseSeqUdp { identifier: 27797, src_port: 51413, dest_port: 62874, checksum: 30535 }
```

The `src_port` (51413) should match previous packets (34052) but does not.  Note that for UDP the dest port is incremented by default.  

2. The `destination` of the original datagram (179.61.197.183) does not match the actual destination of this trace (151.101.0.81).

3. The `sequence` (62874) is extracted from the UDP `dest_port`.  

The last `sequence` _sent_ before the crash was 62148:

```
in trippy::tracing::net::channel::send_probe with probe: Probe { sequence: Sequence(62148), identifier: TraceId(0), src_port: Port(34052), dest_port: Port(62148), ttl: TimeToLive(1), round: Round(973), sent: SystemTime { tv_sec: 1706958368, tv_nsec: 559814000 } }
```

The `in_round` function checks:

```rust
pub fn in_round(&self, sequence: Sequence) -> bool {
    sequence >= self.round_sequence && sequence.0 - self.round_sequence.0 < BUFFER_SIZE
}
```

We know `sequence` was 62874.  
We know that the last sequence we sent was 62148.  We do not know the exact value for `round_sequence` here, but it should be no more than 62148 - 24 (the max inflight) so we can assume `round_sequence` was roughly 62124.

Therefore:
sequence >= self.round_sequence evaluates to true (62874 >= 62124)
sequence.0 - self.round_sequence.0 < BUFFER_SIZE evaluates to true (750 < 1024)

### Conclusion

An unrelated UDP `DestinationUnreachable` ICMP packet, which was generated in response to a UDP packet sent by our host with a `dest_port` that happens to be close to the range of the sequence number accepted by the current round, was received and incorrectly processed.  As the probe with the sequence number was not one we had dispatched we fail with a `debug_assert`.

Note that this `debug_assert` was recently added (in #969) and so the underlying issue may not be new.


### Solution

Add additional filtering for ICMP responses (of type `DestinationUnreachable` and `TimeExceeded` for UDP and TCP original datagrams) such that the src & dest port and the dest address are checked and if they do not match the expected values the packet is discarded.


Discussion / hints:

Fix patch:
diff --git a/src/tracing/net/ipv4.rs b/src/tracing/net/ipv4.rs
index 2549af5a0..a90e66584 100644
--- a/src/tracing/net/ipv4.rs
+++ b/src/tracing/net/ipv4.rs
@@ -427,14 +427,21 @@ fn extract_probe_resp_seq(
         }
         (Protocol::Udp, IpProtocol::Udp) => {
             let (src_port, dest_port, checksum, identifier) = extract_udp_packet(ipv4)?;
+
             Some(ProbeResponseSeq::Udp(ProbeResponseSeqUdp::new(
-                identifier, src_port, dest_port, checksum,
+                identifier,
+                IpAddr::V4(ipv4.get_destination()),
+                src_port,
+                dest_port,
+                checksum,
             )))
         }
         (Protocol::Tcp, IpProtocol::Tcp) => {
             let (src_port, dest_port) = extract_tcp_packet(ipv4)?;
             Some(ProbeResponseSeq::Tcp(ProbeResponseSeqTcp::new(
-                src_port, dest_port,
+                IpAddr::V4(ipv4.get_destination()),
+                src_port,
+                dest_port,
             )))
         }
         _ => None,
diff --git a/src/tracing/net/ipv6.rs b/src/tracing/net/ipv6.rs
index 2923e1a98..a80d8765a 100644
--- a/src/tracing/net/ipv6.rs
+++ b/src/tracing/net/ipv6.rs
@@ -375,13 +375,19 @@ fn extract_probe_resp_seq(
         (Protocol::Udp, IpProtocol::Udp) => {
             let (src_port, dest_port, checksum) = extract_udp_packet(ipv6)?;
             Some(ProbeResponseSeq::Udp(ProbeResponseSeqUdp::new(
-                0, src_port, dest_port, checksum,
+                0,
+                IpAddr::V6(ipv6.get_destination_address()),
+                src_port,
+                dest_port,
+                checksum,
             )))
         }
         (Protocol::Tcp, IpProtocol::Tcp) => {
             let (src_port, dest_port) = extract_tcp_packet(ipv6)?;
             Some(ProbeResponseSeq::Tcp(ProbeResponseSeqTcp::new(
-                src_port, dest_port,
+                IpAddr::V6(ipv6.get_destination_address()),
+                src_port,
+                dest_port,
             )))
         }
         _ => None,
diff --git a/src/tracing/probe.rs b/src/tracing/probe.rs
index b0a20441e..358ffb302 100644
--- a/src/tracing/probe.rs
+++ b/src/tracing/probe.rs
@@ -221,15 +221,23 @@ impl ProbeResponseSeqIcmp {
 #[derive(Debug, Clone)]
 pub struct ProbeResponseSeqUdp {
     pub identifier: u16,
+    pub dest_addr: IpAddr,
     pub src_port: u16,
     pub dest_port: u16,
     pub checksum: u16,
 }
 
 impl ProbeResponseSeqUdp {
-    pub fn new(identifier: u16, src_port: u16, dest_port: u16, checksum: u16) -> Self {
+    pub fn new(
+        identifier: u16,
+        dest_addr: IpAddr,
+        src_port: u16,
+        dest_port: u16,
+        checksum: u16,
+    ) -> Self {
         Self {
             identifier,
+            dest_addr,
             src_port,
             dest_port,
             checksum,
@@ -239,13 +247,15 @@ impl ProbeResponseSeqUdp {
 
 #[derive(Debug, Clone)]
 pub struct ProbeResponseSeqTcp {
+    pub dest_addr: IpAddr,
     pub src_port: u16,
     pub dest_port: u16,
 }
 
 impl ProbeResponseSeqTcp {
-    pub fn new(src_port: u16, dest_port: u16) -> Self {
+    pub fn new(dest_addr: IpAddr, src_port: u16, dest_port: u16) -> Self {
         Self {
+            dest_addr,
             src_port,
             dest_port,
         }
diff --git a/src/tracing/tracer.rs b/src/tracing/tracer.rs
index 2732cec3b..158a7c01b 100644
--- a/src/tracing/tracer.rs
+++ b/src/tracing/tracer.rs
@@ -152,7 +152,7 @@ impl<F: Fn(&TracerRound<'_>)> Tracer<F> {
             Some(ProbeResponse::TimeExceeded(data, extensions)) => {
                 let (trace_id, sequence, received, host) = self.extract(&data);
                 let is_target = host == self.config.target_addr;
-                if self.check_trace_id(trace_id) && st.in_round(sequence) {
+                if self.check_trace_id(trace_id) && st.in_round(sequence) && self.validate(&data) {
                     st.complete_probe_time_exceeded(
                         sequence, host, received, is_target, extensions,
                     );
@@ -160,19 +160,19 @@ impl<F: Fn(&TracerRound<'_>)> Tracer<F> {
             }
             Some(ProbeResponse::DestinationUnreachable(data, extensions)) => {
                 let (trace_id, sequence, received, host) = self.extract(&data);
-                if self.check_trace_id(trace_id) && st.in_round(sequence) {
+                if self.check_trace_id(trace_id) && st.in_round(sequence) && self.validate(&data) {
                     st.complete_probe_unreachable(sequence, host, received, extensions);
                 }
             }
             Some(ProbeResponse::EchoReply(data)) => {
                 let (trace_id, sequence, received, host) = self.extract(&data);
-                if self.check_trace_id(trace_id) && st.in_round(sequence) {
+                if self.check_trace_id(trace_id) && st.in_round(sequence) && self.validate(&data) {
                     st.complete_probe_echo_reply(sequence, host, received);
                 }
             }
             Some(ProbeResponse::TcpReply(data) | ProbeResponse::TcpRefused(data)) => {
                 let (trace_id, sequence, received, host) = self.extract(&data);
-                if self.check_trace_id(trace_id) && st.in_round(sequence) {
+                if self.check_trace_id(trace_id) && st.in_round(sequence) && self.validate(&data) {
                     st.complete_probe_other(sequence, host, received);
                 }
             }
@@ -238,6 +238,49 @@ impl<F: Fn(&TracerRound<'_>)> Tracer<F> {
         self.config.trace_identifier == trace_id || trace_id == TraceId(0)
     }
 
+    /// Validate the probe response data.
+    ///
+    /// Carries out specific check for UDP/TCP probe responses.  This is
+    /// required as the network layer may receive incoming ICMP
+    /// `DestinationUnreachable` (and other types) packets with a UDP/TCP
+    /// original datagram which does not correspond to a probe sent by the
+    /// tracer and must therefore be ignored.
+    ///
+    /// For UDP and TCP probe responses, check that the src/dest ports and
+    /// dest address match the expected values.
+    ///
+    /// For ICMP probe responses no additional checks are required.
+    fn validate(&self, resp: &ProbeResponseData) -> bool {
+        fn validate_ports(port_direction: PortDirection, src_port: u16, dest_port: u16) -> bool {
+            match port_direction {
+                PortDirection::FixedSrc(src) if src.0 == src_port => true,
+                PortDirection::FixedDest(dest) if dest.0 == dest_port => true,
+                PortDirection::FixedBoth(src, dest) if src.0 == src_port && dest.0 == dest_port => {
+                    true
+                }
+                _ => false,
+            }
+        }
+        match resp.resp_seq {
+            ProbeResponseSeq::Icmp(_) => true,
+            ProbeResponseSeq::Udp(ProbeResponseSeqUdp {
+                dest_addr,
+                src_port,
+                dest_port,
+                ..
+            })
+            | ProbeResponseSeq::Tcp(ProbeResponseSeqTcp {
+                dest_addr,
+                src_port,
+                dest_port,
+            }) => {
+                let check_ports = validate_ports(self.config.port_direction, src_port, dest_port);
+                let check_dest_addr = self.config.target_addr == dest_addr;
+                check_dest_addr && check_ports
+            }
+        }
+    }
+
     /// Extract the `TraceId`, `Sequence`, `SystemTime` and `IpAddr` from the `ProbeResponseData` in
     /// a protocol specific way.
     #[instrument(skip(self))]
@@ -257,6 +300,7 @@ impl<F: Fn(&TracerRound<'_>)> Tracer<F> {
                 src_port,
                 dest_port,
                 checksum,
+                ..
             }) => {
                 let sequence = match (self.config.multipath_strategy, self.config.port_direction) {
                     (MultipathStrategy::Classic, PortDirection::FixedDest(_)) => src_port,
@@ -269,6 +313,7 @@ impl<F: Fn(&TracerRound<'_>)> Tracer<F> {
             ProbeResponseSeq::Tcp(ProbeResponseSeqTcp {
                 src_port,
                 dest_port,
+                ..
             }) => {
                 let sequence = match self.config.port_direction {
                     PortDirection::FixedSrc(_) => dest_port,


Test patch: