From a8588c4e9c0a7b03de9888eb75be564db3fcad80 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Sun, 6 Dec 2020 12:55:51 -0500 Subject: [PATCH 1/8] Purge stale connection tracking entries This ensures that a VM cannot use connection tracking entries created by another VM. --- network/vif-route-qubes | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/network/vif-route-qubes b/network/vif-route-qubes index 1cf43d2..d61f736 100755 --- a/network/vif-route-qubes +++ b/network/vif-route-qubes @@ -47,6 +47,14 @@ network_hooks() { fi } +conntrack_purge () { + local 'n=(0|[1-9][0-9]*)' output deleted msg + msg='flow entries have been deleted\.$' + deleted="^conntrack v$n\\.$n\\.$n \\(conntrack-tools\\): $n $msg" + output=$(LC_ALL=C exec conntrack -D "$@" 2>&1 >/dev/null) || : + [[ "$output" =~ $deleted ]] +} + ipt_arg= if "iptables-restore" --help 2>&1 | grep -q wait=; then # 'wait' must be last on command line if secs not specified @@ -146,6 +154,10 @@ if [ "${ip}" ]; then ip -- neighbour "${ipcmd}" to "${addr}" \ dev "${vif}" lladdr "$mac" nud permanent fi + if ! conntrack_purge -s "$addr" || ! conntrack_purge -d "$addr"; then + printf 'Cannot purge stale conntrack entries for %q\n' "$addr">&2 + exit 1 + fi done # if no IPv6 is assigned, block all IPv6 traffic on that interface if ! [[ "$ip" = *:* ]]; then From 9840953f5fb2417a2586f95fcaf7a103a1efdebe Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Mon, 7 Dec 2020 14:08:32 -0500 Subject: [PATCH 2/8] vif-route-qubes: Check that the -e flag is set --- network/vif-route-qubes | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/network/vif-route-qubes b/network/vif-route-qubes index d61f736..055fac0 100755 --- a/network/vif-route-qubes +++ b/network/vif-route-qubes @@ -134,6 +134,11 @@ if [ -n "$appvm_ip" ] && [ -n "$appvm_gw_ip" ] && [ "$appvm_ip" != "$netvm_ip" ] . "$dir/vif-qubes-nat.sh" fi +case $- in +(*e*) :;; +(*) echo '-e not set'>&2; exit 1;; +esac + # add anti-spoofing rules before enabling the interface if [ "${ip}" ]; then # If we’ve been given a list of IP addresses, then add routes from us to From 70253edeab13f42d36e36e3b1fef637e32d5efab Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Mon, 7 Dec 2020 14:11:12 -0500 Subject: [PATCH 3/8] Remove spurious line continuation; add quotes. Pipelines can extend over multiple lines without needing line continuation. --- network/vif-route-qubes | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/network/vif-route-qubes b/network/vif-route-qubes index 055fac0..91404c9 100755 --- a/network/vif-route-qubes +++ b/network/vif-route-qubes @@ -153,8 +153,8 @@ if [ "${ip}" ]; then printf '%s\n' "*raw" \ "$iptables_cmd -i ${vif} ! -s ${addr} -j DROP" \ "$iptables_cmd ! -i vif+ -s ${addr} -j DROP" \ - "COMMIT" | \ - ${cmdprefix} $ipt --noflush $ipt_arg + "COMMIT" | + ${cmdprefix} "$ipt" --noflush $ipt_arg if [[ "$command" = 'online' ]]; then ip -- neighbour "${ipcmd}" to "${addr}" \ dev "${vif}" lladdr "$mac" nud permanent @@ -166,7 +166,7 @@ if [ "${ip}" ]; then done # if no IPv6 is assigned, block all IPv6 traffic on that interface if ! [[ "$ip" = *:* ]]; then - echo -e "*raw\\n$iptables_cmd -i ${vif} -j DROP\\nCOMMIT" | \ + echo -e "*raw\\n$iptables_cmd -i ${vif} -j DROP\\nCOMMIT" | ${cmdprefix} ip6tables-restore --noflush $ipt_arg fi fi From d960f7af8561a420a68d61fab52f09ff0c5e1dd4 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Mon, 7 Dec 2020 14:12:01 -0500 Subject: [PATCH 4/8] Stop disabling checksum offload We now have a newer qemu in the stubdomain, so checksum offloading should work. --- network/vif-route-qubes | 2 -- 1 file changed, 2 deletions(-) diff --git a/network/vif-route-qubes b/network/vif-route-qubes index 91404c9..5624ca6 100755 --- a/network/vif-route-qubes +++ b/network/vif-route-qubes @@ -198,7 +198,5 @@ fi log debug "Successful vif-route-qubes $command for $vif." if [ "$command" = "online" ]; then - # disable tx checksumming offload, apparently it doesn't work with our ancient qemu in stubdom - do_without_error ethtool -K "$vif" tx off success fi From 44b3c12d946ec758e8b569f3348b216826a59df3 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Mon, 7 Dec 2020 14:57:03 -0500 Subject: [PATCH 5/8] Keep shellcheck from complaining MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The code was correct, but shellcheck didn’t recognize that ‘n’ had been assigned as a local variable. --- network/vif-route-qubes | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/network/vif-route-qubes b/network/vif-route-qubes index 5624ca6..51bf47e 100755 --- a/network/vif-route-qubes +++ b/network/vif-route-qubes @@ -48,8 +48,8 @@ network_hooks() { } conntrack_purge () { - local 'n=(0|[1-9][0-9]*)' output deleted msg - msg='flow entries have been deleted\.$' + local n output deleted msg + n='(0|[1-9][0-9]*)' msg='flow entries have been deleted\.$' deleted="^conntrack v$n\\.$n\\.$n \\(conntrack-tools\\): $n $msg" output=$(LC_ALL=C exec conntrack -D "$@" 2>&1 >/dev/null) || : [[ "$output" =~ $deleted ]] From 6565facec3e86b90acd698c4320fca957dd2098e Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Wed, 16 Dec 2020 01:45:23 -0500 Subject: [PATCH 6/8] Add conntrack-tools dependency to qubes-core-agent-networking Otherwise no vif-* interfaces come up. --- archlinux/PKGBUILD | 1 + debian/control | 1 + rpm_spec/core-agent.spec.in | 1 + 3 files changed, 3 insertions(+) diff --git a/archlinux/PKGBUILD b/archlinux/PKGBUILD index bfe90e2..49f3784 100644 --- a/archlinux/PKGBUILD +++ b/archlinux/PKGBUILD @@ -117,6 +117,7 @@ package_qubes-vm-networking() { pkgdesc="Qubes OS tools allowing to use a Qubes VM as a NetVM/ProxyVM" depends=(qubes-vm-core qubes-vm-utils python ethtool net-tools qubes-db-vm networkmanager iptables tinyproxy nftables + conntrack-tools ) install=PKGBUILD-networking.install diff --git a/debian/control b/debian/control index d02dd12..d028177 100644 --- a/debian/control +++ b/debian/control @@ -125,6 +125,7 @@ Depends: iptables, net-tools, ethtool, + conntrack, socat, tinyproxy, iproute2, diff --git a/rpm_spec/core-agent.spec.in b/rpm_spec/core-agent.spec.in index 17720ab..262a71b 100644 --- a/rpm_spec/core-agent.spec.in +++ b/rpm_spec/core-agent.spec.in @@ -235,6 +235,7 @@ Scripts required to handle dom0 updates. Summary: Networking support for Qubes VM Requires: ethtool Requires: iptables +Requires: conntrack-tools Requires: net-tools Requires: iproute Requires: nftables From c09909c7022f17b578cd9a102639a4bdb87325e6 Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Thu, 17 Dec 2020 23:07:12 -0500 Subject: [PATCH 7/8] =?UTF-8?q?Don=E2=80=99t=20assume=20dom0=20will=20neve?= =?UTF-8?q?r=20have=20a=20network=20connection?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In test setups, this actually happens! --- network/vif-route-qubes | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/network/vif-route-qubes b/network/vif-route-qubes index 51bf47e..1c3a854 100755 --- a/network/vif-route-qubes +++ b/network/vif-route-qubes @@ -88,8 +88,16 @@ fi readonly max_domid=32752 -# if domid is 0 something is seriously wrong, so don’t check for that case -if ! [[ $vif =~ ^vif([1-9][0-9]{,4})\.(0|[1-9][0-9]*)$ ]]; then +# This comment used to say, “if domid is 0 something is seriously wrong, so +# don’t check for that case”. Indeed, dom0 should never have an Ethernet +# connection in a production QubesOS system. +# +# However, giving dom0 an Ethernet connection can be extremely useful in +# insecure test environments, where there is simply no data worth compromising. +# In fact, some test setups, including OpenQA, actually do this. Therefore, we +# now handle this case correctly, even though it is by definition a security +# risk. +if ! [[ $vif =~ ^vif(0|[1-9][0-9]{,4})\.(0|[1-9][0-9]*)$ ]]; then printf 'Bad interface name %q\n' "$vif">&2 exit 1 fi From e5b56b96c45e6687948fd4d8bacc18ec213865ce Mon Sep 17 00:00:00 2001 From: Demi Marie Obenour Date: Thu, 17 Dec 2020 23:39:19 -0500 Subject: [PATCH 8/8] =?UTF-8?q?Don=E2=80=99t=20rely=20on=20an=20arbitrary?= =?UTF-8?q?=20length=20limit?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We can check for overlong domids without hardcoding the length in a regex. Just check if the length is longer than that of the max XID. --- network/vif-route-qubes | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/network/vif-route-qubes b/network/vif-route-qubes index 1c3a854..041cff2 100755 --- a/network/vif-route-qubes +++ b/network/vif-route-qubes @@ -97,7 +97,7 @@ readonly max_domid=32752 # In fact, some test setups, including OpenQA, actually do this. Therefore, we # now handle this case correctly, even though it is by definition a security # risk. -if ! [[ $vif =~ ^vif(0|[1-9][0-9]{,4})\.(0|[1-9][0-9]*)$ ]]; then +if ! [[ $vif =~ ^vif(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)$ ]]; then printf 'Bad interface name %q\n' "$vif">&2 exit 1 fi @@ -106,7 +106,8 @@ domid=${BASH_REMATCH[1]} sub=${BASH_REMATCH[2]} # metric must be positive, but prefer later interface # 32752 is max XID aka domid -if (( domid > max_domid )); then +# the length check ensures there is no overflow +if (( "${#domid}" > "${#max_domid}" || domid > max_domid )); then printf %s\\n "domid $domid too large" exit 1 fi