From 20cfcac7dac83dfd5a9c8e551d30a1c5d9c30c9e Mon Sep 17 00:00:00 2001 From: Thomas Eizinger Date: Thu, 30 May 2024 11:41:55 +1000 Subject: [PATCH] test(connlib): don't route packets from IPs other than the client's (#5161) Currently, the gateway has a piece of functionality to ensure we only ever route packets that actually originate from the client. This is important because a gateway connects to multiple clients and thus - without this check - client A could send a packet through the tunnel that gets interpreted as traffic from client B by mangling the source IP of their packets. The portal assigns these source IPs when the clients sign in and passes them to the gateway whenever a client connects. We can thus drop all traffic on the gateway side from IPs that we don't recognise. Currently, a client will still trigger a connection intent for an IP packet, even if it doesn't have the tunnel's source IP set. We may want to consider changing this behaviour in the future. --- .../tunnel/proptest-regressions/tests.txt | 1 + rust/connlib/tunnel/src/client.rs | 4 +- rust/connlib/tunnel/src/gateway.rs | 6 + rust/connlib/tunnel/src/tests.rs | 167 ++++++++++-------- 4 files changed, 98 insertions(+), 80 deletions(-) diff --git a/rust/connlib/tunnel/proptest-regressions/tests.txt b/rust/connlib/tunnel/proptest-regressions/tests.txt index 42d641714..90ec6b0a4 100644 --- a/rust/connlib/tunnel/proptest-regressions/tests.txt +++ b/rust/connlib/tunnel/proptest-regressions/tests.txt @@ -17,3 +17,4 @@ cc 4855625684cfe850e8b965097038ac8fed979cd21abd5284eb25e48136fcbb3e cc bbc56c960f18a6115b3c114f2c5280a813fa2a12f21af383ad4154863fb944bc cc 71165314a457a9f39928d882ffcc2ae02aedc2a35ce55b387c990b1bc5022a38 # shrinks to (initial_state, transitions, seen_counter) = (ReferenceState { now: Instant { tv_sec: 18548, tv_nsec: 387490499 }, utc_now: 2024-05-22T05:00:47.375132041Z, client: SimNode { id: ClientId(a89380c1-5379-5a2b-c5da-47fdeb956f75), state: PrivateKey("d348d4c5fa2ad05dfc686f289fb7084a16a095e208000844bc0ef7566b25addb"), ip4_socket: Some(127.0.0.1:41241), ip6_socket: Some([6e67:9b39:4a3f:a896:9e02:dc56:353e:65b3]:58740), tunnel_ip4: 100.111.145.90, tunnel_ip6: ::ffff:63.59.65.126 }, gateway: SimNode { id: GatewayId(d680e99a-31a7-9d8b-1052-6b85edfe42bc), state: PrivateKey("d39211badb6305b5b583574ee303b6a112b66311c38b4c22d68d69cb8d917bae"), ip4_socket: Some(82.215.208.98:18290), ip6_socket: Some([f122:b9a6:b3fb:bf35:319d:f921:d911:d012]:57327), tunnel_ip4: 100.81.19.26, tunnel_ip6: 3bfa:312f:c388:91b9:f6ce:95a6:a07:8784 }, relay: SimRelay { id: RelayId(a908052e-8823-7892-b1e2-e2c64deab06a), ip_stack: Dual { ip4: 75.190.170.204, ip6: ::ffff:0.235.25.126 }, allocations: {} }, client_cidr_resources: {}, connected_resources: {}, gateway_received_icmp_packets: [] }, [AddCidrResource(ResourceDescriptionCidr { id: ResourceId(00000000-0000-0000-0000-000000000000), address: V4(Ipv4Network { network_address: 0.0.0.0, netmask: 28 }), name: "aaaa", address_description: "bmuf", sites: [Site { name: "zvkyrxzs", id: SiteId(67566938-ef2b-b442-64e0-f0f6fd05634d) }, Site { name: "hvzhiban", id: SiteId(4b7a5400-0fb2-2e42-444a-bc94fbaeed2e) }, Site { name: "fjmyjoh", id: SiteId(13340020-2717-880f-ba45-c823725c2234) }, Site { name: "fgrhxuvi", id: SiteId(8c260658-d7fb-c49c-c9f0-769cd8ced648) }] }), SendICMPPacketToIp4Resource { r_idx: Index(0) }, AddCidrResource(ResourceDescriptionCidr { id: ResourceId(00000000-0000-0000-0000-000000000001), address: V4(Ipv4Network { network_address: 0.0.0.0, netmask: 28 }), name: "aaaa", address_description: "clkriftb", sites: [Site { name: "fgfg", id: SiteId(6b8d5470-3636-4879-2a32-94790962e6fd) }] }), SendICMPPacketToIp4Resource { r_idx: Index(11791212112709260679) }], None) cc c9b812a8526dad4b00e366ea6b6d60054a4dd64c58849f1244f6fb2ba9d3686b # shrinks to (initial_state, transitions, seen_counter) = (ReferenceState { now: Instant { tv_sec: 20170, tv_nsec: 301610515 }, utc_now: 2024-05-22T05:27:49.998877399Z, client: SimNode { id: ClientId(00000000-0000-0000-0000-000000000000), state: PrivateKey("0000000000000000000000000000000000000000000000000000000000000000"), ip4_socket: Some(127.0.0.1:1), ip6_socket: None, tunnel_ip4: 100.64.0.1, tunnel_ip6: ::ffff:0.0.0.0 }, gateway: SimNode { id: GatewayId(00000000-0000-0000-0000-000000000000), state: PrivateKey("0000000000000000000000000000000000000000000000000000000000000001"), ip4_socket: None, ip6_socket: Some([::ffff:0.0.0.0]:12607), tunnel_ip4: 100.66.167.161, tunnel_ip6: ::ffff:155.93.248.155 }, relay: SimRelay { id: RelayId(c4e3332a-6f75-3a59-3ed8-15fbf26de134), ip_stack: Dual { ip4: 115.115.97.13, ip6: ::ffff:196.17.152.128 }, allocations: {} }, client_cidr_resources: {}, connected_resources: {}, gateway_received_icmp_packets: [] }, [AddCidrResource(ResourceDescriptionCidr { id: ResourceId(00000000-0000-0000-0000-000000000000), address: V4(Ipv4Network { network_address: 127.0.0.1, netmask: 32 }), name: "aaaa", address_description: "mgnx", sites: [Site { name: "njmblcn", id: SiteId(b6f995cd-2c8e-a35d-117f-1e7affe3bc7b) }, Site { name: "angxwaw", id: SiteId(c1772dc0-60c6-ebfc-bcb5-978b288bbabf) }, Site { name: "vblitlg", id: SiteId(f41e2663-0254-e054-2353-0855dabea706) }, Site { name: "plcfnotvz", id: SiteId(72f3a8cc-8015-bf96-adcd-b40df5f25383) }, Site { name: "lflx", id: SiteId(214a47c8-5ce9-3e33-4738-da1a36d79d68) }, Site { name: "shfamjx", id: SiteId(1f2c00bf-3437-9ac9-c9d3-e66ebf3a1558) }, Site { name: "offgdto", id: SiteId(edb8c103-1dda-01f7-d648-47ccfad1eb97) }, Site { name: "gzss", id: SiteId(e2058be3-2fe7-5459-b8b3-eaee2c434828) }, Site { name: "ovougquk", id: SiteId(1410820a-7236-ac4d-e649-f276f3418e51) }] }), SendICMPPacketToIp4Resource { r_idx: Index(0) }, AddCidrResource(ResourceDescriptionCidr { id: ResourceId(00000000-0000-0000-0000-000000000001), address: V4(Ipv4Network { network_address: 127.0.0.1, netmask: 32 }), name: "aaaa", address_description: "llgt", sites: [Site { name: "jmcxcphun", id: SiteId(190023c3-f08f-0b3c-ca68-ac5d45127341) }, Site { name: "mxofshkeo", id: SiteId(c39f0553-11d6-ddb0-1bc2-57e841e038fc) }, Site { name: "kaasxisn", id: SiteId(7c30df2b-95f9-0a6c-ebc7-92fd0a131376) }, Site { name: "hhitinvik", id: SiteId(39c252f6-4848-e5b7-1fe3-a4b886258e9a) }] }), SendICMPPacketToIp4Resource { r_idx: Index(0) }, SendICMPPacketToRandomIp { dst: 127.0.0.1 }], None) +cc d4d65c807696fb5a36f012090ef5bbcf3cc1917ddbbbe41d273387173b7b9044 # shrinks to (initial_state, transitions, seen_counter) = (ReferenceState { now: Instant { tv_sec: 4234, tv_nsec: 954499181 }, utc_now: 2024-05-30T01:02:43.851812081Z, client: SimNode { id: ClientId(00000000-0000-0000-0000-000000000000), state: PrivateKey("0000000000000000000000000000000000000000000000000000000000000000"), ip4_socket: None, ip6_socket: Some([::ffff:0.0.0.0]:1), tunnel_ip4: 100.64.0.1, tunnel_ip6: ::ffff:0.0.0.0 }, gateway: SimNode { id: GatewayId(b77d4d5f-3cf6-1b07-2957-bd465e5c2d9d), state: PrivateKey("498b2d67afe3cf56af8c010ec13ba4e185368077090cfbbb694c7a5cf03fb687"), ip4_socket: None, ip6_socket: Some([::ffff:85.37.17.81]:4098), tunnel_ip4: 100.109.173.208, tunnel_ip6: ::ffff:158.40.153.102 }, relay: SimRelay { id: RelayId(dcc26607-f113-9649-8ea5-2cd6366168cb), ip_stack: Dual { ip4: 127.0.0.1, ip6: ::ffff:217.253.220.229 }, allocations: {} }, system_dns_resolvers: [127.0.0.1, 199.164.14.248, 141.37.3.107], upstream_dns_resolvers: [IpPort(IpDnsServer { address: [::ffff:127.0.0.1]:53 }), IpPort(IpDnsServer { address: [cb46:599c:647e:cea8:ec5a:2437:92ac:d449]:53 })], client_cidr_resources: {}, connected_resources: {}, expected_icmp_handshakes: [] }, [AddCidrResource(ResourceDescriptionCidr { id: ResourceId(00000000-0000-0000-0000-000000000000), address: V6(Ipv6Network { network_address: ::ffff:0.0.0.0, netmask: 128 }), name: "aaaa", address_description: "xzthwhrin", sites: [Site { name: "ryjnkomlk", id: SiteId(cf128543-edc0-205e-06de-4c002a19df60) }, Site { name: "mvuvzltgn", id: SiteId(aafd15f6-eeb2-1396-81be-bc106f25bcec) }] }), SendICMPPacketToCidrResource { r_idx: Index(0), seq: 0, identifier: 0, src: Some(::ffff:0.0.0.0) }, SendICMPPacketToCidrResource { r_idx: Index(0), seq: 0, identifier: 0, src: Some(::ffff:127.0.0.1) }], None) diff --git a/rust/connlib/tunnel/src/client.rs b/rust/connlib/tunnel/src/client.rs index d9deb8198..5c6f9cdbb 100644 --- a/rust/connlib/tunnel/src/client.rs +++ b/rust/connlib/tunnel/src/client.rs @@ -418,7 +418,7 @@ impl ClientState { self.node.public_key() } - #[tracing::instrument(level = "trace", skip_all, fields(dest))] + #[tracing::instrument(level = "trace", skip_all, fields(dst))] pub(crate) fn encapsulate<'s>( &'s mut self, packet: MutableIpPacket<'_>, @@ -432,7 +432,7 @@ impl ClientState { Err(non_dns_packet) => non_dns_packet, }; - tracing::Span::current().record("dest", tracing::field::display(dest)); + tracing::Span::current().record("dst", tracing::field::display(dest)); if is_definitely_not_a_resource(dest) { return None; diff --git a/rust/connlib/tunnel/src/gateway.rs b/rust/connlib/tunnel/src/gateway.rs index cd262b3d8..65e479742 100644 --- a/rust/connlib/tunnel/src/gateway.rs +++ b/rust/connlib/tunnel/src/gateway.rs @@ -175,6 +175,7 @@ impl GatewayState { Some(transmit) } + #[tracing::instrument(level = "trace", skip_all, fields(src, dst))] pub(crate) fn decapsulate<'b>( &mut self, local: SocketAddr, @@ -193,6 +194,9 @@ impl GatewayState { .inspect_err(|e| tracing::warn!(%local, %from, num_bytes = %packet.len(), "Failed to decapsulate incoming packet: {e}")) .ok()??; + tracing::Span::current().record("src", tracing::field::display(packet.source())); + tracing::Span::current().record("dst", tracing::field::display(packet.destination())); + let Some(peer) = self.peers.get_mut(&conn_id) else { tracing::error!(%conn_id, %local, %from, "Couldn't find connection"); @@ -207,6 +211,8 @@ impl GatewayState { return None; } + tracing::trace!("Decapsulated packet"); + Some(packet.into_immutable()) } diff --git a/rust/connlib/tunnel/src/tests.rs b/rust/connlib/tunnel/src/tests.rs index a75762fa5..e5361ff2a 100644 --- a/rust/connlib/tunnel/src/tests.rs +++ b/rust/connlib/tunnel/src/tests.rs @@ -106,17 +106,17 @@ enum Transition { seq: u16, identifier: u16, }, - /// Send a ICMP packet to an IPv4 resource. - SendICMPPacketToIp4Resource { - r_idx: sample::Index, - seq: u16, - identifier: u16, - }, - /// Send a ICMP packet to an IPv6 resource. - SendICMPPacketToIp6Resource { + /// Send an ICMP packet to a CIDR resource. + SendICMPPacketToCidrResource { r_idx: sample::Index, seq: u16, identifier: u16, + + /// An optional source address. + /// + /// Packets are only allowed to come from the IP assigned by the portal. + /// All others MUST be dropped by the gateway. + src: Option, }, /// The system's DNS servers changed. UpdateSystemDnsServers { servers: Vec }, @@ -245,14 +245,15 @@ impl StateMachineTest for TunnelTest { buffered_transmits.extend(state.send_ip_packet_client_to_gateway(packet)); } - Transition::SendICMPPacketToIp4Resource { + Transition::SendICMPPacketToCidrResource { r_idx, seq, identifier, + src, } => { - let dst = ref_state.sample_ipv4_cidr_resource_dst(&r_idx); + let dst = ref_state.sample_cidr_resource_dst(&r_idx); let packet = ip_packet::make::icmp_request_packet( - state.client.tunnel_ip(dst), + src.unwrap_or(state.client.tunnel_ip(dst)), dst, seq, identifier, @@ -260,21 +261,6 @@ impl StateMachineTest for TunnelTest { buffered_transmits.extend(state.send_ip_packet_client_to_gateway(packet)); } - Transition::SendICMPPacketToIp6Resource { - r_idx, - seq, - identifier, - } => { - let dst = ref_state.sample_ipv6_cidr_resource_dst(&r_idx); - let packet = ip_packet::make::icmp_request_packet( - state.client.tunnel_ip(dst), - dst, - seq, - identifier, - ); - - buffered_transmits.extend(state.send_ip_packet_client_to_gateway(packet)) - } Transition::Tick { millis } => { state.now += Duration::from_millis(millis); } @@ -290,20 +276,21 @@ impl StateMachineTest for TunnelTest { } }; state.advance(ref_state, &mut buffered_transmits); + assert!(buffered_transmits.is_empty()); // Sanity check to ensure we handled all packets. // Assert our properties: Check that our actual state is equivalent to our expectation (the reference state). for (resource_dst, seq, identifier) in ref_state.expected_icmp_handshakes.iter() { let client_sent_request = &state .client_sent_icmp_requests - .get(&(*seq, *identifier)) + .remove(&(*seq, *identifier)) .unwrap(); let client_received_reply = &state .client_received_icmp_replies - .get(&(*seq, *identifier)) + .remove(&(*seq, *identifier)) .unwrap(); let gateway_received_request = &state .gateway_received_requests - .get(&(*seq, *identifier)) + .remove(&(*seq, *identifier)) .unwrap(); assert_eq!( @@ -329,14 +316,23 @@ impl StateMachineTest for TunnelTest { "ICMP request source == ICMP reply destination" ); } + assert_eq!( + state.gateway_received_requests, + HashMap::new(), + "Unexpected ICMP requests on gateway" + ); + assert_eq!( + state.client_received_icmp_replies, + HashMap::new(), + "Unexpected ICMP replies on client" + ); + assert_eq!( state.effective_dns_servers(), ref_state.expected_dns_servers(), "Effective DNS servers should match either system or upstream DNS" ); - assert!(buffered_transmits.is_empty()); // Sanity check to ensure we handled all packets. - state } } @@ -422,8 +418,6 @@ impl ReferenceStateMachine for ReferenceState { let set_upstream_dns_servers = upstream_dns_servers() .prop_map(|servers| Transition::UpdateUpstreamDnsServers { servers }); - let (num_ip4_resources, num_ip6_resources) = state.client_cidr_resources.len(); - let mut strategies = vec![ (1, add_cidr_resource.boxed()), (1, tick.boxed()), @@ -432,12 +426,12 @@ impl ReferenceStateMachine for ReferenceState { (1, icmp_to_random_ip().boxed()), ]; - if num_ip4_resources > 0 { - strategies.push((3, icmp_to_ipv4_cidr_resource().boxed())); - } + if !state.client_cidr_resources.is_empty() { + strategies.push((10, icmp_to_cidr_resource_from_client().boxed())); - if num_ip6_resources > 0 { - strategies.push((3, icmp_to_ipv6_cidr_resource().boxed())); + // Packets from random source addresses are tested less frequently. + // Those are dropped by the gateway so this transition only ensures we have this safe-guard. + strategies.push((1, icmp_to_cidr_resource_from_random_src().boxed())); } Union::new_weighted(strategies).boxed() @@ -447,6 +441,8 @@ impl ReferenceStateMachine for ReferenceState { /// /// Here is where we implement the "expected" logic. fn apply(mut state: Self::State, transition: &Self::Transition) -> Self::State { + state.expected_icmp_handshakes.clear(); + match transition { Transition::AddCidrResource(r) => { state.client_cidr_resources.insert(r.address, r.clone()); @@ -456,23 +452,16 @@ impl ReferenceStateMachine for ReferenceState { seq, identifier, } => { - state.on_icmp_packet(*dst, *seq, *identifier); + state.on_icmp_packet(None, *dst, *seq, *identifier); } - Transition::SendICMPPacketToIp4Resource { + Transition::SendICMPPacketToCidrResource { r_idx, seq, identifier, + src, } => { - let dst = state.sample_ipv4_cidr_resource_dst(r_idx); - state.on_icmp_packet(dst, *seq, *identifier); - } - Transition::SendICMPPacketToIp6Resource { - r_idx, - seq, - identifier, - } => { - let dst = state.sample_ipv6_cidr_resource_dst(r_idx); - state.on_icmp_packet(dst, *seq, *identifier); + let dst = state.sample_cidr_resource_dst(r_idx); + state.on_icmp_packet(*src, dst, *seq, *identifier); } Transition::Tick { millis } => state.now += Duration::from_millis(*millis), Transition::UpdateSystemDnsServers { servers } => { @@ -508,31 +497,21 @@ impl ReferenceStateMachine for ReferenceState { seq, identifier, } => state.is_valid_icmp_packet(dst, seq, identifier), - Transition::SendICMPPacketToIp4Resource { + Transition::SendICMPPacketToCidrResource { r_idx, seq, identifier, + src, } => { - if state.client_cidr_resources.len().0 == 0 { + if state.client_cidr_resources.is_empty() { return false; } - let dst = state.sample_ipv4_cidr_resource_dst(r_idx); + let dst = state.sample_cidr_resource_dst(r_idx); + let is_valid_icmp_packet = state.is_valid_icmp_packet(&dst, seq, identifier); - state.is_valid_icmp_packet(&dst.into(), seq, identifier) - } - Transition::SendICMPPacketToIp6Resource { - r_idx, - seq, - identifier, - } => { - if state.client_cidr_resources.len().1 == 0 { - return false; - } - - let dst = state.sample_ipv6_cidr_resource_dst(r_idx); - - state.is_valid_icmp_packet(&dst.into(), seq, identifier) + is_valid_icmp_packet + && (src.is_none() || src.is_some_and(|s| s.is_ipv4() == dst.is_ipv4())) } Transition::UpdateSystemDnsServers { .. } => true, Transition::UpdateUpstreamDnsServers { .. } => true, @@ -972,8 +951,16 @@ impl TunnelTest { /// Several helper functions to make the reference state more readable. impl ReferenceState { #[tracing::instrument(level = "debug", skip_all, fields(dst, resource))] - fn on_icmp_packet(&mut self, dst: impl Into, seq: u16, identifier: u16) { + fn on_icmp_packet( + &mut self, + src: Option, + dst: impl Into, + seq: u16, + identifier: u16, + ) { let dst = dst.into(); + let packet_originates_from_client = + src.is_none() || src.is_some_and(|src| src == self.client.tunnel_ip(src)); // Packets from IPs other than the client's are dropped by the gateway. We still create connections for them though. tracing::Span::current().record("dst", tracing::field::display(dst)); // Second, if we are not yet connected, check if we have a resource for this IP. @@ -982,7 +969,7 @@ impl ReferenceState { return; }; - if self.connected_resources.contains(&resource.id) { + if self.connected_resources.contains(&resource.id) && packet_originates_from_client { tracing::debug!("Connected to resource, expecting packet to be routed"); self.expected_icmp_handshakes .push_back((dst, seq, identifier)); @@ -994,6 +981,22 @@ impl ReferenceState { self.connected_resources.insert(resource.id); } + fn sample_cidr_resource_dst(&self, idx: &sample::Index) -> IpAddr { + let (num_ip4_resources, num_ip6_resources) = self.client_cidr_resources.len(); + + let mut ips = Vec::new(); + + if num_ip4_resources > 0 { + ips.push(self.sample_ipv4_cidr_resource_dst(idx).into()) + } + + if num_ip6_resources > 0 { + ips.push(self.sample_ipv6_cidr_resource_dst(idx).into()) + } + + *idx.get(&ips) + } + /// Samples an [`Ipv4Addr`] from _any_ of our IPv4 CIDR resources. fn sample_ipv4_cidr_resource_dst(&self, idx: &sample::Index) -> Ipv4Addr { let num_ip4_resources = self.client_cidr_resources.len().0; @@ -1435,24 +1438,32 @@ fn icmp_to_random_ip() -> impl Strategy { }) } -fn icmp_to_ipv4_cidr_resource() -> impl Strategy { +fn icmp_to_cidr_resource_from_client() -> impl Strategy { (any::(), any::(), any::()).prop_map( - move |(r_idx, seq, identifier)| Transition::SendICMPPacketToIp4Resource { + move |(r_idx, seq, identifier)| Transition::SendICMPPacketToCidrResource { r_idx, seq, identifier, + src: None, }, ) } -fn icmp_to_ipv6_cidr_resource() -> impl Strategy { - (any::(), any::(), any::()).prop_map( - move |(r_idx, seq, identifier)| Transition::SendICMPPacketToIp6Resource { - r_idx, - seq, - identifier, - }, +fn icmp_to_cidr_resource_from_random_src() -> impl Strategy { + ( + any::(), + any::(), + any::(), + any::(), ) + .prop_map(move |(r_idx, seq, identifier, src)| { + Transition::SendICMPPacketToCidrResource { + r_idx, + seq, + identifier, + src: Some(src), + } + }) } fn client_id() -> impl Strategy {