firewall rule: remove DNS rule (was only needed in Qubes 3)

[hannesm]

//cc @palainp would you mind to try this out?

fixes #63

[hannesm]

hmm, I'm even more curious that rules.ml still has some default_dns_servers hardcoded -- is this and the special dns rule still a thing with Qubes 4.x?

[palainp]

This is compiling and running fine (with docker the sha256 is 1855cb82fce272d93c43ecf06d175ae82e07c309855fe8cd5fdd6313a7f54a0f).

I managed to run a speedtest and this was ok (the bandwidth is still less than a linux sys-firewall, around 60%, the cpu seems to be a possible bottleneck, I have to investigate that later).

The IPs (10.139.1.1 and 10.139.1.2) are those provided by Qubes and seems to be translated to the next AppVM in the network chain:

$ cat /etc/resolv.conf 
nameserver 10.139.1.1
nameserver 10.139.1.2

I'm not sure if we can get them automatically in uplink.ml?

[palainp]

Thinking back, and as the fw shouldn't have to resolve something, I tried to remove that part of the code:

diff --git a/rules.ml b/rules.ml
index f72d6c0..541d621 100644
--- a/rules.ml
+++ b/rules.ml
@@ -10,12 +10,6 @@ module Q = Pf_qubes.Parse_qubes
 let src = Logs.Src.create "rules" ~doc:"Firewall rules"
 module Log = (val Logs.src_log src : Logs.LOG)

-(* the upstream NetVM will redirect TCP and UDP port 53 traffic with
-   these destination IPs to its upstream nameserver. *)
-let default_dns_servers = [
-  Ipaddr.V4.of_string_exn "10.139.1.1";
-  Ipaddr.V4.of_string_exn "10.139.1.2";
-]
 let dns_port = 53

 module Classifier = struct
@@ -26,14 +20,6 @@ module Classifier = struct

   let matches_proto rule packet = match rule.Q.proto, rule.Q.specialtarget with
     | None, None -> true
-    | None, Some `dns when List.mem packet.ipv4_header.Ipv4_packet.dst default_dns_servers -> begin
-      (* specialtarget=dns applies only to the specialtarget destination IPs, and
-         specialtarget=dns is also implicitly tcp/udp port 53 *)
-      match packet.transport_header with
-        | `TCP header -> header.Tcp.Tcp_packet.dst_port = dns_port
-        | `UDP header -> header.Udp_packet.dst_port = dns_port
-        | _ -> false
-      end
    (* DNS rules can only match traffic headed to the specialtarget hosts, so any other destination
    isn't a match for DNS rules *)
     | None, Some `dns -> false

and it still works (but probably needs a bit more testing to make sure you don't miss any corner cases).

[hannesm]

Thanks @palainp. So from my understanding, the specialtarget `dns should match for dns packets: (a) in doa.ml (read_network_config) we extract the /qubes-primary-dns already (but there's eventually a /qubes-secondary-dns?) (b) we should use these value instead of Rules.default_dns_servers to check..

Certainly the entire `dns can be removed -- especially if you don't have any special firewall rules for `dns in your setup. But the semantics in the qubes firewall ruleset is likely that it should respect the QubesDB values.

[hannesm]

I added a commit here which removes the hardcoded IPv4 addresses, and uses those from QubesDB (read once at startup). Could you give this a try, @palainp?

[palainp]

It compiles and starts like before, but I somehow got:

[2022-09-07 17:11:30] Fatal error: exception Failure("Netif: ERROR response")
[2022-09-07 17:11:30] Raised at Netchannel__Frontend.Make.write.(fun) in file "duniverse/mirage-net-xen/lib/frontend.ml", line 377, characters 16-24
[2022-09-07 17:11:30] Called from Lwt.Resolution_loop.handle_with_async_exception_hook in file "duniverse/lwt/src/core/lwt.ml", line 1144, characters 8-11
[2022-09-07 17:11:30] Solo5: solo5_exit(2) called

Not sure if that is related, but I never got that. I try to reproduce.

[hannesm]

hmm, strange...

[palainp]

It's certainly not related (see another appearance of Netif failure https://github.com/mirage/qubes-mirage-firewall/issues/120#issuecomment-932711576 ). I can't reproduce it and it works as before now. It's ok for me to be merged!

[hannesm]

merged as part of #149