From 04e33c6e15b350cf45af437f8ba72646bdcbe78e Mon Sep 17 00:00:00 2001 From: Dmitry Dodzin Date: Mon, 14 Oct 2024 14:31:20 +0300 Subject: [PATCH] Disallow using IPv6 sockets with mirrord (#2837) * Revert "4f82caadbf6b00a6c51e36289021aff4526dfa6c" * Typo * Revert "5606f9226b520b19533308dbda0b2df64d375991" --- Cargo.lock | 2 +- changelog.d/2836.fixed.md | 1 + mirrord/layer/src/debugger_ports.rs | 2 +- mirrord/layer/src/error.rs | 7 +++++++ mirrord/layer/src/socket.rs | 19 ++++++++----------- mirrord/layer/src/socket/dns_selector.rs | 2 +- mirrord/layer/src/socket/ops.rs | 23 ++++++++++------------- mirrord/protocol/Cargo.toml | 2 +- mirrord/protocol/src/outgoing.rs | 5 ----- 9 files changed, 30 insertions(+), 33 deletions(-) create mode 100644 changelog.d/2836.fixed.md diff --git a/Cargo.lock b/Cargo.lock index 33a753ca13c..d9e130083b7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4352,7 +4352,7 @@ dependencies = [ [[package]] name = "mirrord-protocol" -version = "1.11.1" +version = "1.11.2" dependencies = [ "actix-codec", "bincode", diff --git a/changelog.d/2836.fixed.md b/changelog.d/2836.fixed.md new file mode 100644 index 00000000000..b02c553ce0b --- /dev/null +++ b/changelog.d/2836.fixed.md @@ -0,0 +1 @@ +Disallow using IPv6 sockets with mirrord. diff --git a/mirrord/layer/src/debugger_ports.rs b/mirrord/layer/src/debugger_ports.rs index fdabac20859..2b2feb0aed6 100644 --- a/mirrord/layer/src/debugger_ports.rs +++ b/mirrord/layer/src/debugger_ports.rs @@ -248,7 +248,7 @@ impl DebuggerPorts { /// Return whether the given [SocketAddr] is used by the debugger. pub fn contains(&self, addr: &SocketAddr) -> bool { let is_localhost = matches!( - addr.ip().to_canonical(), + addr.ip(), IpAddr::V4(Ipv4Addr::LOCALHOST) | IpAddr::V6(Ipv6Addr::LOCALHOST) ); if !is_localhost { diff --git a/mirrord/layer/src/error.rs b/mirrord/layer/src/error.rs index 406dd28ffdc..5bd0dd1b062 100644 --- a/mirrord/layer/src/error.rs +++ b/mirrord/layer/src/error.rs @@ -75,6 +75,9 @@ pub(crate) enum HookError { #[error("mirrord-layer: SIP patch failed with error `{0}`!")] FailedSipPatch(#[from] SipError), + #[error("mirrord-layer: IPv6 can't be used with mirrord")] + SocketUnsuportedIpv6, + // `From` implemented below, not with `#[from]` so that when new variants of // `SerializationError` are added, they are mapped into different variants of // `LayerError`. @@ -218,6 +221,9 @@ impl From for i64 { HookError::FileNotFound => { info!("mirrord file not found triggered") } + HookError::SocketUnsuportedIpv6 => { + info!("{fail}") + } HookError::ProxyError(ref err) => { graceful_exit!( r"Proxy error, connectivity issue or a bug. @@ -286,6 +292,7 @@ impl From for i64 { HookError::LocalFileCreation(_) => libc::EINVAL, #[cfg(target_os = "macos")] HookError::FailedSipPatch(_) => libc::EACCES, + HookError::SocketUnsuportedIpv6 => libc::EAFNOSUPPORT, HookError::UnsupportedSocketType => libc::EAFNOSUPPORT, HookError::BadPointer => libc::EFAULT, HookError::AddressAlreadyBound(_) => libc::EADDRINUSE, diff --git a/mirrord/layer/src/socket.rs b/mirrord/layer/src/socket.rs index 189c71e2152..945391c9bc2 100644 --- a/mirrord/layer/src/socket.rs +++ b/mirrord/layer/src/socket.rs @@ -1,7 +1,5 @@ //! We implement each hook function in a safe function as much as possible, having the unsafe do the //! absolute minimum -//! Note the case of IPv6 in IPv4 which requires special care to do right -//! use std::{ collections::HashMap, net::{SocketAddr, ToSocketAddrs}, @@ -384,13 +382,13 @@ impl OutgoingSelector { // https://github.com/metalbear-co/mirrord/issues/2389 fixed and I don't have time to // fully understand or refactor, and the logic is sound (if it's loopback, just connect to // it) - if address.ip().to_canonical().is_loopback() { + if address.ip().is_loopback() { return Ok(address); } let cached = REMOTE_DNS_REVERSE_MAPPING .lock()? - .get(&address.ip().to_canonical()) + .get(&address.ip()) .cloned(); let Some(hostname) = cached else { return Ok(address); @@ -460,7 +458,7 @@ impl ProtocolAndAddressFilterExt for ProtocolAndAddressFilter { let _guard = DetourGuard::new(); match (name.as_str(), *port).to_socket_addrs() { - Ok(addresses) => addresses.map(|addr| addr.ip().to_canonical()).collect(), + Ok(addresses) => addresses.map(|addr| addr.ip()).collect(), Err(e) => { let as_string = e.to_string(); if as_string.contains("Temporary failure in name resolution") @@ -479,13 +477,12 @@ impl ProtocolAndAddressFilterExt for ProtocolAndAddressFilter { } }; - Ok(resolved_ips - .into_iter() - .any(|ip| ip == address.ip().to_canonical())) + Ok(resolved_ips.into_iter().any(|ip| ip == address.ip())) } - AddressFilter::Socket(addr) => Ok(addr.ip().to_canonical().is_unspecified() - || addr.ip().to_canonical() == address.ip().to_canonical()), - AddressFilter::Subnet(net, _) => Ok(net.contains(&address.ip().to_canonical())), + AddressFilter::Socket(addr) => { + Ok(addr.ip().is_unspecified() || addr.ip() == address.ip()) + } + AddressFilter::Subnet(net, _) => Ok(net.contains(&address.ip())), AddressFilter::Port(..) => Ok(true), } } diff --git a/mirrord/layer/src/socket/dns_selector.rs b/mirrord/layer/src/socket/dns_selector.rs index 20c202ec8ae..e83dbba2d9f 100644 --- a/mirrord/layer/src/socket/dns_selector.rs +++ b/mirrord/layer/src/socket/dns_selector.rs @@ -33,7 +33,7 @@ impl DnsSelector { AddressFilter::Port(..) => true, AddressFilter::Name(filter_name, _) => filter_name == node, AddressFilter::Socket(filter_socket) => { - filter_socket.ip().to_canonical().is_unspecified() + filter_socket.ip().is_unspecified() || Some(filter_socket.ip()) == node.parse().ok() } AddressFilter::Subnet(filter_subnet, _) => { diff --git a/mirrord/layer/src/socket/ops.rs b/mirrord/layer/src/socket/ops.rs index e6f92ff492d..3be081f42d3 100644 --- a/mirrord/layer/src/socket/ops.rs +++ b/mirrord/layer/src/socket/ops.rs @@ -129,6 +129,10 @@ pub(super) fn socket(domain: c_int, type_: c_int, protocol: c_int) -> Detour Detour let addr = requested_address.ip(); let port = requested_address.port(); - let canonical_address = addr.to_canonical(); - - let address = if canonical_address.is_loopback() || canonical_address.is_unspecified() { + let address = if addr.is_loopback() || addr.is_unspecified() { *requested_address } else if addr.is_ipv4() { SocketAddr::new(IpAddr::V4(Ipv4Addr::LOCALHOST), port) @@ -255,7 +257,7 @@ pub(super) fn bind( // we don't use `is_localhost` here since unspecified means to listen // on all IPs. - if incoming_config.ignore_localhost && requested_address.ip().to_canonical().is_loopback() { + if incoming_config.ignore_localhost && requested_address.ip().is_loopback() { return Detour::Bypass(Bypass::IgnoreLocalhost(requested_port)); } @@ -566,11 +568,7 @@ pub(super) fn connect( ) -> Detour { let remote_address = SockAddr::try_from_raw(raw_address, address_length)?; let optional_ip_address = remote_address.as_socket(); - let is_ipv4_in_ipv6 = remote_address - .as_socket() - .as_ref() - .map(|addr| addr.ip().to_canonical().is_ipv6()) - .unwrap_or(false); + let unix_streams = crate::setup().remote_unix_streams(); trace!("in connect {:#?}", SOCKETS); @@ -587,7 +585,7 @@ pub(super) fn connect( .family() .map(|family| family as i32) .unwrap_or(-1); - if domain != libc::AF_INET && domain != libc::AF_UNIX && !is_ipv4_in_ipv6 { + if domain != libc::AF_INET && domain != libc::AF_UNIX { return Detour::Bypass(Bypass::Domain(domain)); } // I really hate it, but nix seems to really make this API bad :() @@ -609,9 +607,8 @@ pub(super) fn connect( return Detour::Success(connect_result); } - let canonical_ip = ip_address.ip().to_canonical(); - - if canonical_ip.is_loopback() || canonical_ip.is_unspecified() { + let ip = ip_address.ip(); + if ip.is_loopback() || ip.is_unspecified() { if let Some(result) = connect_to_local_address(sockfd, &user_socket_info, ip_address)? { // `result` here is always a success, as error and bypass are returned on the `?` // above. diff --git a/mirrord/protocol/Cargo.toml b/mirrord/protocol/Cargo.toml index c6e954f0f38..f05fc0835e3 100644 --- a/mirrord/protocol/Cargo.toml +++ b/mirrord/protocol/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "mirrord-protocol" -version = "1.11.1" +version = "1.11.2" authors.workspace = true description.workspace = true documentation.workspace = true diff --git a/mirrord/protocol/src/outgoing.rs b/mirrord/protocol/src/outgoing.rs index 2614aac0052..dedfa099b7b 100644 --- a/mirrord/protocol/src/outgoing.rs +++ b/mirrord/protocol/src/outgoing.rs @@ -83,11 +83,6 @@ impl TryFrom for SocketAddress { fn try_from(addr: OsSockAddr) -> Result { addr.as_socket() - .map(|mut socket_addr| { - // convert ipv4 in ipv6 to ipv4 - socket_addr.set_ip(socket_addr.ip().to_canonical()); - socket_addr - }) .map(SocketAddress::Ip) .or_else(|| { addr.as_pathname()