From a7001a70d2f1f929323b7b53101b1638b60a6bc0 Mon Sep 17 00:00:00 2001 From: Thomas Leonard Date: Sun, 25 Sep 2016 14:38:17 +0100 Subject: [PATCH] Allow clients to have any IP address We previously assumed that Qubes would always give clients IP addresses on a particular network. However, it is not required to do this and in fact uses a different network for disposable VMs. With this change: - We no longer reject clients with unknown IP addresses - The `Unknown_client` classification is gone; we have no way to tell the difference between a client that isn't connected and an external address. - We now consider every client to be on a point-to-point link and do not answer ARP requests on behalf of other clients. Clients should assume their netmask is 255.255.255.255 (and ignore /qubes-netmask). This is a partial fix for #9. It allows disposable VMs to connect to the firewall but for some reason they don't process any frames we send them (we get their ARP requests but they don't get our replies). Taking eth0 down in the disp VM, then bringing it back up (and re-adding the routes) allows it to work. --- _tags | 1 - client_eth.ml | 19 +++++++++---------- client_eth.mli | 24 ++++++++++++------------ dao.ml | 8 +------- dao.mli | 1 - firewall.ml | 5 +---- packet.ml | 2 +- router.ml | 11 +++-------- rules.ml | 1 - unikernel.ml | 5 +---- 10 files changed, 28 insertions(+), 49 deletions(-) diff --git a/_tags b/_tags index 69adb29..7441bd2 100644 --- a/_tags +++ b/_tags @@ -1,3 +1,2 @@ not : warn(A-4), strict_sequence : package(cstruct.syntax) -true: -syntax(camlp4o) diff --git a/client_eth.ml b/client_eth.ml index af0f299..d027134 100644 --- a/client_eth.ml +++ b/client_eth.ml @@ -1,32 +1,28 @@ -(* Copyright (C) 2015, Thomas Leonard +(* Copyright (C) 2016, Thomas Leonard See the README file for details. *) open Utils -let src = Logs.Src.create "client_eth" ~doc:"Ethernet for NetVM clients" +let src = Logs.Src.create "client_eth" ~doc:"Ethernet networks for NetVM clients" module Log = (val Logs.src_log src : Logs.LOG) type t = { mutable iface_of_ip : client_link IpMap.t; - prefix : Ipaddr.V4.Prefix.t; client_gw : Ipaddr.V4.t; (* The IP that clients are given as their default gateway. *) } type host = [ `Client of client_link - | `Unknown_client of Ipaddr.t | `Client_gateway | `External of Ipaddr.t ] -let create ~prefix ~client_gw = - { iface_of_ip = IpMap.empty; client_gw; prefix } +let create ~client_gw = + { iface_of_ip = IpMap.empty; client_gw } -let prefix t = t.prefix let client_gw t = t.client_gw let add_client t iface = let ip = iface#other_ip in - assert (Ipaddr.V4.Prefix.mem ip t.prefix); (* TODO: Should probably wait for the previous client to disappear. *) (* assert (not (IpMap.mem ip t.iface_of_ip)); *) t.iface_of_ip <- t.iface_of_ip |> IpMap.add ip iface @@ -45,13 +41,11 @@ let classify t ip = if ip4 = t.client_gw then `Client_gateway else match lookup t ip4 with | Some client_link -> `Client client_link - | None when Ipaddr.V4.Prefix.mem ip4 t.prefix -> `Unknown_client ip | None -> `External ip let resolve t : host -> Ipaddr.t = function | `Client client_link -> Ipaddr.V4 client_link#other_ip | `Client_gateway -> Ipaddr.V4 t.client_gw - | `Unknown_client addr | `External addr -> addr module ARP = struct @@ -62,9 +56,14 @@ module ARP = struct let lookup t ip = if ip = t.net.client_gw then Some t.client_link#my_mac + else None + (* We're now treating client networks as point-to-point links, + so we no longer respond on behalf of other clients. *) + (* else match IpMap.find ip t.net.iface_of_ip with | Some client_iface -> Some client_iface#other_mac | None -> None + *) let create ~net client_link = {net; client_link} diff --git a/client_eth.mli b/client_eth.mli index 45203ae..cd8ccfe 100644 --- a/client_eth.mli +++ b/client_eth.mli @@ -1,34 +1,36 @@ -(* Copyright (C) 2015, Thomas Leonard +(* Copyright (C) 2016, Thomas Leonard See the README file for details. *) -(** The ethernet network our client AppVMs are on. *) +(** The ethernet networks connecting us to our client AppVMs. + Note: each AppVM is on a point-to-point link, each link being considered to be a separate Ethernet network. *) open Utils type t -(** A network for client AppVMs to join. *) +(** A collection of clients. *) type host = [ `Client of client_link - | `Unknown_client of Ipaddr.t | `Client_gateway | `External of Ipaddr.t ] +(* Note: Qubes does not allow us to distinguish between an external address and a + disconnected client. + See: https://github.com/talex5/qubes-mirage-firewall/issues/9#issuecomment-246956850 *) -val create : prefix:Ipaddr.V4.Prefix.t -> client_gw:Ipaddr.V4.t -> t -(** [create ~prefix ~client_gw] is a network of client machines. - Their IP addresses all start with [prefix] and they are configured to - use [client_gw] as their default gateway. *) +val create : client_gw:Ipaddr.V4.t -> t +(** [create ~client_gw] is a network of client machines. + Qubes will have configured the client machines to use [client_gw] as their default gateway. *) val add_client : t -> client_link -> unit val remove_client : t -> client_link -> unit -val prefix : t -> Ipaddr.V4.Prefix.t val client_gw : t -> Ipaddr.V4.t val classify : t -> Ipaddr.t -> host val resolve : t -> host -> Ipaddr.t val lookup : t -> Ipaddr.V4.t -> client_link option +(** [lookup t addr] is the client with IP address [addr], if connected. *) module ARP : sig (** We already know the correct mapping of IP addresses to MAC addresses, so we never @@ -40,9 +42,7 @@ module ARP : sig val create : net:t -> client_link -> arp (** [create ~net client_link] is an ARP responder for [client_link]. - It answers on behalf of other clients in [net] (but not for the client - itself, since the client might be trying to check that its own address is - free). It also answers for the client's gateway address. *) + It answers only for the client's gateway address. *) val input : arp -> Cstruct.t -> Cstruct.t option (** Process one ethernet frame containing an ARP message. diff --git a/dao.ml b/dao.ml index 972d2e9..f0ab65b 100644 --- a/dao.ml +++ b/dao.ml @@ -44,7 +44,6 @@ type network_config = { uplink_netvm_ip : Ipaddr.V4.t; (* The IP address of NetVM (our gateway) *) uplink_our_ip : Ipaddr.V4.t; (* The IP address of our interface to NetVM *) - clients_prefix : Ipaddr.V4.Prefix.t; (* The network connecting our client VMs to us *) clients_our_ip : Ipaddr.V4.t; (* The IP address of our interface to our client VMs (their gateway) *) } @@ -56,12 +55,7 @@ let read_network_config qubesDB = | Some value -> value in let uplink_our_ip = get "/qubes-ip" |> Ipaddr.V4.of_string_exn in let uplink_netvm_ip = get "/qubes-gateway" |> Ipaddr.V4.of_string_exn in - let clients_prefix = - (* This is oddly named: seems to be the network we provide to our clients *) - let client_network = get "/qubes-netvm-network" |> Ipaddr.V4.of_string_exn in - let client_netmask = get "/qubes-netvm-netmask" |> Ipaddr.V4.of_string_exn in - Ipaddr.V4.Prefix.of_netmask client_netmask client_network in let clients_our_ip = get "/qubes-netvm-gateway" |> Ipaddr.V4.of_string_exn in - { uplink_netvm_ip; uplink_our_ip; clients_prefix; clients_our_ip } + { uplink_netvm_ip; uplink_our_ip; clients_our_ip } let set_iptables_error db = Qubes.DB.write db "/qubes-iptables-error" diff --git a/dao.mli b/dao.mli index adf036a..c0f2862 100644 --- a/dao.mli +++ b/dao.mli @@ -22,7 +22,6 @@ type network_config = { uplink_netvm_ip : Ipaddr.V4.t; (* The IP address of NetVM (our gateway) *) uplink_our_ip : Ipaddr.V4.t; (* The IP address of our interface to NetVM *) - clients_prefix : Ipaddr.V4.Prefix.t; (* The network connecting our client VMs to us *) clients_our_ip : Ipaddr.V4.t; (* The IP address of our interface to our client VMs (their gateway) *) } diff --git a/firewall.ml b/firewall.ml index 4b98302..cdfd977 100644 --- a/firewall.ml +++ b/firewall.ml @@ -155,9 +155,6 @@ let apply_rules t rules info = match rules info, info.dst with | `Accept, `Client client_link -> transmit ~frame client_link | `Accept, (`External _ | `NetVM) -> transmit ~frame t.Router.uplink - | `Accept, `Unknown_client _ -> - Log.warn (fun f -> f "Dropping packet to unknown client %a" pp_packet info); - return () | `Accept, (`Firewall_uplink | `Client_gateway) -> Log.warn (fun f -> f "Bad rule: firewall can't accept packets %a" pp_packet info); return () @@ -196,7 +193,7 @@ let ipv4_from_netvm t frame = | None -> return () | Some info -> match info.src with - | `Client _ | `Unknown_client _ | `Firewall_uplink | `Client_gateway -> + | `Client _ | `Firewall_uplink | `Client_gateway -> Log.warn (fun f -> f "Frame from NetVM has internal source IP address! %a" pp_packet info); return () | `External _ | `NetVM -> diff --git a/packet.ml b/packet.ml index bf9f062..a359e16 100644 --- a/packet.ml +++ b/packet.ml @@ -11,7 +11,7 @@ type ports = { } type host = - [ `Client of client_link | `Unknown_client of Ipaddr.t | `Client_gateway | `Firewall_uplink | `NetVM | `External of Ipaddr.t ] + [ `Client of client_link | `Client_gateway | `Firewall_uplink | `NetVM | `External of Ipaddr.t ] type info = { frame : Cstruct.t; diff --git a/router.ml b/router.ml index e86d38b..8e1dc44 100644 --- a/router.ml +++ b/router.ml @@ -21,14 +21,9 @@ let create ~client_eth ~uplink = let target t buf = let open Wire_structs.Ipv4_wire in let dst_ip = get_ipv4_dst buf |> Ipaddr.V4.of_int32 in - if Ipaddr.V4.Prefix.mem dst_ip (Client_eth.prefix t.client_eth) then ( - match Client_eth.lookup t.client_eth dst_ip with - | Some client_link -> Some (client_link :> interface) - | None -> - Log.warn (fun f -> f "Packet to unknown internal client %a - dropping" - Ipaddr.V4.pp_hum dst_ip); - None - ) else Some t.uplink + match Client_eth.lookup t.client_eth dst_ip with + | Some client_link -> Some (client_link :> interface) + | None -> Some t.uplink let add_client t = Client_eth.add_client t.client_eth let remove_client t = Client_eth.remove_client t.client_eth diff --git a/rules.ml b/rules.ml index a2e86ae..7e62790 100644 --- a/rules.ml +++ b/rules.ml @@ -32,7 +32,6 @@ let from_client = function | { dst = `Client_gateway; proto = `UDP { dport = 53 } } -> `NAT_to (`NetVM, 53) | { dst = (`Client_gateway | `Firewall_uplink) } -> `Drop "packet addressed to firewall itself" | { dst = `Client _ } -> `Drop "prevent communication between client VMs" - | { dst = `Unknown_client _ } -> `Drop "target client not running" (** Decide what to do with a packet received from the outside world. Note: If the packet matched an existing NAT rule then this isn't called. *) diff --git a/unikernel.ml b/unikernel.ml index d64274f..e03380b 100644 --- a/unikernel.ml +++ b/unikernel.ml @@ -14,16 +14,13 @@ module Main (Clock : V1.CLOCK) = struct let network qubesDB = (* Read configuration from QubesDB *) let config = Dao.read_network_config qubesDB in - Logs.info (fun f -> f "Client (internal) network is %a" - Ipaddr.V4.Prefix.pp_hum config.Dao.clients_prefix); (* Initialise connection to NetVM *) Uplink.connect config >>= fun uplink -> (* Report success *) Dao.set_iptables_error qubesDB "" >>= fun () -> (* Set up client-side networking *) let client_eth = Client_eth.create - ~client_gw:config.Dao.clients_our_ip - ~prefix:config.Dao.clients_prefix in + ~client_gw:config.Dao.clients_our_ip in (* Set up routing between networks and hosts *) let router = Router.create ~client_eth