Update RestrictDns in WinFw

5 files changed, 56 insertions, 83 deletions

diff --git a/windows/winfw/src/winfw/fwcontext.cpp b/windows/winfw/src/winfw/fwcontext.cpp
index bd89b3cf65..931ced34a9 100644
--- a/windows/winfw/src/winfw/fwcontext.cpp
+++ b/windows/winfw/src/winfw/fwcontext.cpp
@@ -166,9 +166,10 @@ bool FwContext::applyPolicyConnected
 
 	ruleset.emplace_back(std::make_unique<rules::RestrictDns>(
 		tunnelInterfaceAlias,
-		wfp::IpAddress(v4DnsHost),
-		(v6DnsHost != nullptr) ? std::make_unique<wfp::IpAddress>(v6DnsHost) : nullptr,
-		(relay.port == 53) ? std::make_unique<wfp::IpAddress>(relay.ip) : nullptr
+		wfp::IpAddress(std::wstring(v4DnsHost)),
+		nullptr != v6DnsHost ? std::make_optional<wfp::IpAddress>(std::wstring(v6DnsHost)) : std::nullopt,
+		std::wstring(relay.ip),
+		relay.port
 	));
 
 	return applyRuleset(ruleset);
diff --git a/windows/winfw/src/winfw/mullvadguids.cpp b/windows/winfw/src/winfw/mullvadguids.cpp
index 770c81f7db..e73fac26ed 100644
--- a/windows/winfw/src/winfw/mullvadguids.cpp
+++ b/windows/winfw/src/winfw/mullvadguids.cpp
@@ -52,7 +52,6 @@ DetailedWfpObjectRegistry MullvadGuids::BuildDetailedRegistry()
 	registry.insert(std::make_pair(WfpObjectType::Filter, FilterPermitVpnTunnel_Outbound_Ipv6()));
 	registry.insert(std::make_pair(WfpObjectType::Filter, FilterRestrictDns_Outbound_Ipv4()));
 	registry.insert(std::make_pair(WfpObjectType::Filter, FilterRestrictDns_Outbound_Tunnel_Ipv4()));
-	registry.insert(std::make_pair(WfpObjectType::Filter, FilterRestrictDns_HACK_TO_ALLOW_RELAY_ON_PORT_53()));
 	registry.insert(std::make_pair(WfpObjectType::Filter, FilterRestrictDns_Outbound_Ipv6()));
 	registry.insert(std::make_pair(WfpObjectType::Filter, FilterRestrictDns_Outbound_Tunnel_Ipv6()));
 	registry.insert(std::make_pair(WfpObjectType::Filter, FilterPermitVpnTunnelService_Ipv4()));
@@ -474,20 +473,6 @@ const GUID &MullvadGuids::FilterRestrictDns_Outbound_Tunnel_Ipv4()
 }
 
 //static
-const GUID& MullvadGuids::FilterRestrictDns_HACK_TO_ALLOW_RELAY_ON_PORT_53()
-{
-	static const GUID g =
-	{
-		0x6a613b73,
-		0x7308,
-		0x4ae4,
-		{ 0x91, 0x7d, 0xd2, 0xa2, 0x29, 0x17, 0xcc, 0x3f }
-	};
-
-	return g;
-}
-
-//static
 const GUID &MullvadGuids::FilterRestrictDns_Outbound_Ipv6()
 {
 	static const GUID g =
diff --git a/windows/winfw/src/winfw/mullvadguids.h b/windows/winfw/src/winfw/mullvadguids.h
index a2001a2bdb..3c3ca9702b 100644
--- a/windows/winfw/src/winfw/mullvadguids.h
+++ b/windows/winfw/src/winfw/mullvadguids.h
@@ -58,7 +58,6 @@ public:
 
 	static const GUID &FilterRestrictDns_Outbound_Ipv4();
 	static const GUID &FilterRestrictDns_Outbound_Tunnel_Ipv4();
-	static const GUID &FilterRestrictDns_HACK_TO_ALLOW_RELAY_ON_PORT_53();
 	static const GUID &FilterRestrictDns_Outbound_Ipv6();
 	static const GUID &FilterRestrictDns_Outbound_Tunnel_Ipv6();
 
diff --git a/windows/winfw/src/winfw/rules/restrictdns.cpp b/windows/winfw/src/winfw/rules/restrictdns.cpp
index 2eb560d973..efa4c8421b 100644
--- a/windows/winfw/src/winfw/rules/restrictdns.cpp
+++ b/windows/winfw/src/winfw/rules/restrictdns.cpp
@@ -12,14 +12,16 @@ using namespace wfp::conditions;
 namespace rules
 {
 
-RestrictDns::RestrictDns(const std::wstring& tunnelInterfaceAlias,
+RestrictDns::RestrictDns(const std::wstring &tunnelInterfaceAlias,
 	const wfp::IpAddress v4DnsHost,
-	std::unique_ptr<wfp::IpAddress> v6DnsHost,
-	std::unique_ptr<wfp::IpAddress> relay)
+	std::optional<wfp::IpAddress> v6DnsHost,
+	wfp::IpAddress relay,
+	uint16_t relayPort)
 	: m_tunnelInterfaceAlias(tunnelInterfaceAlias)
 	, m_v4DnsHost(v4DnsHost)
-	, m_v6DnsHost(std::move(v6DnsHost))
-	, m_relayHost(std::move(relay))
+	, m_v6DnsHost(v6DnsHost)
+	, m_relayHost(relay)
+	, m_relayPort(relayPort)
 
 {
 }
@@ -37,23 +39,21 @@ bool RestrictDns::apply(IObjectInstaller &objectInstaller)
 	// TODO: Have each rule specify requirements?
 	//
 
-	if (nullptr != m_relayHost) {
-
-		filterBuilder
-			.key(MullvadGuids::FilterRestrictDns_Outbound_Ipv4())
-			.name(L"Permit relay connection over port 53 (IPv4)")
-			.key(MullvadGuids::FilterRestrictDns_HACK_TO_ALLOW_RELAY_ON_PORT_53())
-			.description(L"This filter is part of a rule that restricts DNS traffic")
-			.provider(MullvadGuids::Provider())
-			.layer(FWPM_LAYER_ALE_AUTH_CONNECT_V4)
-			.sublayer(MullvadGuids::SublayerBlacklist())
-			.weight(wfp::FilterBuilder::WeightClass::Max)
-			.permit();
+	filterBuilder
+		.provider(MullvadGuids::Provider())
+		.description(L"This filter is part of a rule that restricts DNS traffic")
+		.sublayer(MullvadGuids::SublayerBlacklist())
+		.key(MullvadGuids::FilterRestrictDns_Outbound_Tunnel_Ipv4())
+		.name(L"Restrict DNS requests inside the VPN tunnel (IPv4)")
+		.layer(FWPM_LAYER_ALE_AUTH_CONNECT_V4)
+		.weight(MAXUINT16)
+		.permit();
 
+	{
 		wfp::ConditionBuilder conditionBuilder(FWPM_LAYER_ALE_AUTH_CONNECT_V4);
 
-		conditionBuilder.add_condition(ConditionPort::Remote(53));
-		conditionBuilder.add_condition(ConditionIp::Remote(*m_relayHost, CompareEq()));
+		conditionBuilder.add_condition(ConditionInterface::Alias(m_tunnelInterfaceAlias, CompareEq()));
+		conditionBuilder.add_condition(ConditionIp::Remote(m_v4DnsHost, CompareEq()));
 
 		if (!objectInstaller.addFilter(filterBuilder, conditionBuilder))
 		{
@@ -64,35 +64,21 @@ bool RestrictDns::apply(IObjectInstaller &objectInstaller)
 	filterBuilder
 		.key(MullvadGuids::FilterRestrictDns_Outbound_Ipv4())
 		.name(L"Block DNS requests outside the VPN tunnel (IPv4)")
-		.description(L"This filter is part of a rule that restricts DNS traffic")
-		.provider(MullvadGuids::Provider())
 		.layer(FWPM_LAYER_ALE_AUTH_CONNECT_V4)
-		.sublayer(MullvadGuids::SublayerBlacklist())
-		.weight(wfp::FilterBuilder::WeightClass::Max)
+		.weight(MAXUINT16 - 1)
 		.block();
 
 	{
 		wfp::ConditionBuilder conditionBuilder(FWPM_LAYER_ALE_AUTH_CONNECT_V4);
-
 		conditionBuilder.add_condition(ConditionPort::Remote(53));
-		conditionBuilder.add_condition(ConditionInterface::Alias(m_tunnelInterfaceAlias, CompareNeq()));
 
-		if (!objectInstaller.addFilter(filterBuilder, conditionBuilder))
+		if (53 == m_relayPort)
 		{
-			return false;
+			//
+			// Allow relay traffic over port 53
+			//
+			conditionBuilder.add_condition(ConditionIp::Remote(m_relayHost, CompareNeq()));
 		}
-	}
-
-	filterBuilder
-		.name(L"Restrict DNS requests inside the VPN tunnel (IPv4)")
-		.key(MullvadGuids::FilterRestrictDns_Outbound_Tunnel_Ipv4())
-		.layer(FWPM_LAYER_ALE_AUTH_CONNECT_V4);
-
-	{
-		wfp::ConditionBuilder conditionBuilder(FWPM_LAYER_ALE_AUTH_CONNECT_V4);
-
-		conditionBuilder.add_condition(ConditionPort::Remote(53));
-		conditionBuilder.add_condition(ConditionIp::Remote(m_v4DnsHost, CompareNeq()));
 
 		if (!objectInstaller.addFilter(filterBuilder, conditionBuilder))
 		{
@@ -104,38 +90,38 @@ bool RestrictDns::apply(IObjectInstaller &objectInstaller)
 	// IPv6 also
 	//
 
-	filterBuilder
-		.key(MullvadGuids::FilterRestrictDns_Outbound_Ipv6())
-		.name(L"Block DNS requests outside the VPN tunnel (IPv6)")
-		.layer(FWPM_LAYER_ALE_AUTH_CONNECT_V6);
-
+	if (m_v6DnsHost.has_value())
 	{
-		wfp::ConditionBuilder conditionBuilder(FWPM_LAYER_ALE_AUTH_CONNECT_V6);
-
-		conditionBuilder.add_condition(ConditionPort::Remote(53));
-		conditionBuilder.add_condition(ConditionInterface::Alias(m_tunnelInterfaceAlias, CompareNeq()));
+		filterBuilder
+			.key(MullvadGuids::FilterRestrictDns_Outbound_Tunnel_Ipv6())
+			.name(L"Restrict DNS requests inside the VPN tunnel (IPv6)")
+			.layer(FWPM_LAYER_ALE_AUTH_CONNECT_V6)
+			.weight(MAXUINT16)
+			.permit();
 
-		if (!objectInstaller.addFilter(filterBuilder, conditionBuilder))
 		{
-			return false;
+			wfp::ConditionBuilder conditionBuilder(FWPM_LAYER_ALE_AUTH_CONNECT_V6);
+
+			conditionBuilder.add_condition(ConditionInterface::Alias(m_tunnelInterfaceAlias, CompareEq()));
+			conditionBuilder.add_condition(ConditionIp::Remote(m_v6DnsHost.value(), CompareEq()));
+
+			if (!objectInstaller.addFilter(filterBuilder, conditionBuilder))
+			{
+				return false;
+			}
 		}
 	}
 
 	filterBuilder
-		.key(MullvadGuids::FilterRestrictDns_Outbound_Tunnel_Ipv6())
-		.name(L"Restrict DNS requests inside the VPN tunnel (IPv6)")
-		.layer(FWPM_LAYER_ALE_AUTH_CONNECT_V6);
+		.key(MullvadGuids::FilterRestrictDns_Outbound_Ipv6())
+		.name(L"Block DNS requests outside the VPN tunnel (IPv6)")
+		.layer(FWPM_LAYER_ALE_AUTH_CONNECT_V6)
+		.weight(MAXUINT16 - 1)
+		.block();
 
 	{
 		wfp::ConditionBuilder conditionBuilder(FWPM_LAYER_ALE_AUTH_CONNECT_V6);
-
 		conditionBuilder.add_condition(ConditionPort::Remote(53));
-
-		if (m_v6DnsHost != nullptr)
-		{
-			conditionBuilder.add_condition(ConditionIp::Remote(*m_v6DnsHost, CompareNeq()));
-		}
-
 		return objectInstaller.addFilter(filterBuilder, conditionBuilder);
 	}
 }
diff --git a/windows/winfw/src/winfw/rules/restrictdns.h b/windows/winfw/src/winfw/rules/restrictdns.h
index 0b54a6465e..cdfe1f4697 100644
--- a/windows/winfw/src/winfw/rules/restrictdns.h
+++ b/windows/winfw/src/winfw/rules/restrictdns.h
@@ -2,7 +2,9 @@
 
 #include "ifirewallrule.h"
 #include "libwfp/ipaddress.h"
+#include <optional>
 #include <string>
+#include <cstdint>
 
 namespace rules
 {
@@ -11,7 +13,7 @@ class RestrictDns : public IFirewallRule
 {
 public:
 
-	RestrictDns(const std::wstring &tunnelInterfaceAlias, const wfp::IpAddress v4DnsHost, std::unique_ptr<wfp::IpAddress> v6DnsHost, std::unique_ptr<wfp::IpAddress> relay);
+	RestrictDns(const std::wstring &tunnelInterfaceAlias, const wfp::IpAddress v4DnsHost, std::optional<wfp::IpAddress> v6DnsHost, wfp::IpAddress relay, uint16_t relayPort);
 
 	bool apply(IObjectInstaller &objectInstaller) override;
 
@@ -19,9 +21,9 @@ private:
 
 	const std::wstring m_tunnelInterfaceAlias;
 	const wfp::IpAddress m_v4DnsHost;
-	const std::unique_ptr<wfp::IpAddress> m_v6DnsHost;
-	// If connecting to relay on port 53, the traffic to port 53 should be allowed.
-	const std::unique_ptr<wfp::IpAddress> m_relayHost;
+	const std::optional<wfp::IpAddress> m_v6DnsHost;
+	const uint16_t m_relayPort;
+	const wfp::IpAddress m_relayHost;
 
 };