Merge branch 'win-fw-restrict-relay-access'

10 files changed, 210 insertions, 105 deletions

diff --git a/windows/winfw/src/winfw/fwcontext.cpp b/windows/winfw/src/winfw/fwcontext.cpp
index 4883e6f6d8..7661fe95d4 100644
--- a/windows/winfw/src/winfw/fwcontext.cpp
+++ b/windows/winfw/src/winfw/fwcontext.cpp
@@ -11,14 +11,13 @@
 #include "rules/baseline/permitlan.h"
 #include "rules/baseline/permitlanservice.h"
 #include "rules/baseline/permitloopback.h"
-#include "rules/baseline/permitvpnrelay.h"
 #include "rules/baseline/permitvpntunnel.h"
 #include "rules/baseline/permitvpntunnelservice.h"
 #include "rules/baseline/permitping.h"
 #include "rules/baseline/permitdns.h"
 #include "rules/dns/blockall.h"
-#include "rules/dns/permitnontunnel.h"
 #include "rules/dns/permittunnel.h"
+#include "rules/multi/permitvpnrelay.h"
 #include <libwfp/transaction.h>
 #include <libwfp/filterengine.h>
 #include <libcommon/error.h>
@@ -30,12 +29,12 @@ using namespace rules;
 namespace
 {
 
-baseline::PermitVpnRelay::Protocol TranslateProtocol(WinFwProtocol protocol)
+multi::PermitVpnRelay::Protocol TranslateProtocol(WinFwProtocol protocol)
 {
 	switch (protocol)
 	{
-		case Tcp: return baseline::PermitVpnRelay::Protocol::Tcp;
-		case Udp: return baseline::PermitVpnRelay::Protocol::Udp;
+		case Tcp: return multi::PermitVpnRelay::Protocol::Tcp;
+		case Udp: return multi::PermitVpnRelay::Protocol::Udp;
 		default:
 		{
 			THROW_ERROR("Missing case handler in switch clause");
@@ -48,16 +47,20 @@ baseline::PermitVpnRelay::Protocol TranslateProtocol(WinFwProtocol protocol)
 // a local resolver to leave the machine. From the local resolver the request will either be
 // resolved from cache, or forwarded out onto the Internet.
 //
-// Therefore, whenever the PermitLan rule might be activated, we must also do proper DNS management
-// to prevent leaks.
+// Therefore, we're unconditionally lifting all DNS traffic out of the baseline sublayer and restricting
+// it in the DNS sublayer instead. The PermitDNS rule in the baseline sublayer accomplishes this.
+//
+// This has implications for the way the relay access is configured. In the regular case there
+// is no issue: The PermitVpnRelay rule can be installed in the baseline sublayer.
+//
+// However, if the relay is running on the DNS port (53), it would be blocked unless the DNS
+// sublayer permits this traffic. For this reason, whenever the relay is on port 53, the
+// PermitVpnRelay rule has to be installed to the DNS sublayer instead of the baseline sublayer.
 //
 void AppendSettingsRules
 (
 	FwContext::Ruleset &ruleset,
-	const WinFwSettings &settings,
-	std::optional<std::wstring> tunnelInterfaceAlias = std::nullopt,
-	std::optional<std::vector<wfp::IpAddress> > nonTunnelDnsServers = std::nullopt,
-	std::optional<std::vector<wfp::IpAddress> > tunnelDnsServers = std::nullopt
+	const WinFwSettings &settings
 )
 {
 	if (settings.permitDhcp)
@@ -79,18 +82,32 @@ void AppendSettingsRules
 
 	ruleset.emplace_back(std::make_unique<baseline::PermitDns>());
 	ruleset.emplace_back(std::make_unique<dns::BlockAll>());
+}
 
-	if (nonTunnelDnsServers.has_value())
-	{
-		ruleset.emplace_back(std::make_unique<dns::PermitNonTunnel>(
-			tunnelInterfaceAlias, nonTunnelDnsServers.value()));
-	}
+//
+// Refer comment on `AppendSettingsRules`.
+//
+void AppendRelayRules
+(
+	FwContext::Ruleset &ruleset,
+	const WinFwRelay &relay,
+	const std::vector<std::wstring> &approvedApplications
+)
+{
+	auto sublayer =
+	(
+		DNS_SERVER_PORT == relay.port
+		? rules::multi::PermitVpnRelay::Sublayer::Dns
+		: rules::multi::PermitVpnRelay::Sublayer::Baseline
+	);
 
-	if (tunnelInterfaceAlias.has_value() && tunnelDnsServers.has_value())
-	{
-		ruleset.emplace_back(std::make_unique<dns::PermitTunnel>(
-			tunnelInterfaceAlias.value(), tunnelDnsServers.value()));
-	}
+	ruleset.emplace_back(std::make_unique<multi::PermitVpnRelay>(
+		wfp::IpAddress(relay.ip),
+		relay.port,
+		TranslateProtocol(relay.protocol),
+		approvedApplications,
+		sublayer
+	));
 }
 
 void AppendNetBlockedRules(FwContext::Ruleset &ruleset)
@@ -99,23 +116,15 @@ void AppendNetBlockedRules(FwContext::Ruleset &ruleset)
 	ruleset.emplace_back(std::make_unique<baseline::PermitLoopback>());
 }
 
-std::optional<std::vector<wfp::IpAddress> >
-CreateRelayDnsExclusion(const WinFwRelay &relay)
-{
-	if (relay.port != DNS_SERVER_PORT)
-	{
-		return std::nullopt;
-	}
-
-	std::vector<wfp::IpAddress> result = { wfp::IpAddress(relay.ip) };
-
-	return std::move(result);
-}
-
 } // anonymous namespace
 
-FwContext::FwContext(uint32_t timeout)
-	: m_baseline(0)
+FwContext::FwContext
+(
+	uint32_t timeout,
+	const std::vector<std::wstring> &approvedApplications
+)
+	: m_approvedApplications(approvedApplications)
+	, m_baseline(0)
 	, m_activePolicy(Policy::None)
 {
 	auto engine = wfp::FilterEngine::StandardSession(timeout);
@@ -134,8 +143,14 @@ FwContext::FwContext(uint32_t timeout)
 	m_activePolicy = Policy::None;
 }
 
-FwContext::FwContext(uint32_t timeout, const WinFwSettings &settings)
-	: m_baseline(0)
+FwContext::FwContext
+(
+	uint32_t timeout,
+	const WinFwSettings &settings,
+	const std::vector<std::wstring> &approvedApplications
+)
+	: m_approvedApplications(approvedApplications)
+	, m_baseline(0)
 	, m_activePolicy(Policy::None)
 {
 	auto engine = wfp::FilterEngine::StandardSession(timeout);
@@ -166,13 +181,8 @@ bool FwContext::applyPolicyConnecting
 	Ruleset ruleset;
 
 	AppendNetBlockedRules(ruleset);
-	AppendSettingsRules(ruleset, settings, std::nullopt, CreateRelayDnsExclusion(relay));
-
-	ruleset.emplace_back(std::make_unique<baseline::PermitVpnRelay>(
-		wfp::IpAddress(relay.ip),
-		relay.port,
-		TranslateProtocol(relay.protocol)
-	));
+	AppendSettingsRules(ruleset, settings);
+	AppendRelayRules(ruleset, relay, m_approvedApplications);
 
 	//
 	// Permit pinging the gateway inside the tunnel.
@@ -208,20 +218,11 @@ bool FwContext::applyPolicyConnected
 	Ruleset ruleset;
 
 	AppendNetBlockedRules(ruleset);
+	AppendSettingsRules(ruleset, settings);
+	AppendRelayRules(ruleset, relay, m_approvedApplications);
 
-	AppendSettingsRules
-	(
-		ruleset,
-		settings,
-		std::make_optional<>(tunnelInterfaceAlias),
-		CreateRelayDnsExclusion(relay),
-		std::make_optional<>(tunnelDnsServers)
-	);
-
-	ruleset.emplace_back(std::make_unique<baseline::PermitVpnRelay>(
-		wfp::IpAddress(relay.ip),
-		relay.port,
-		TranslateProtocol(relay.protocol)
+	ruleset.emplace_back(std::make_unique<dns::PermitTunnel>(
+		tunnelInterfaceAlias, tunnelDnsServers
 	));
 
 	ruleset.emplace_back(std::make_unique<baseline::PermitVpnTunnel>(
diff --git a/windows/winfw/src/winfw/fwcontext.h b/windows/winfw/src/winfw/fwcontext.h
index 6bdb398b16..fd8871e26b 100644
--- a/windows/winfw/src/winfw/fwcontext.h
+++ b/windows/winfw/src/winfw/fwcontext.h
@@ -13,10 +13,15 @@ class FwContext
 {
 public:
 
-	FwContext(uint32_t timeout);
+	FwContext(uint32_t timeout, const std::vector<std::wstring> &approvedApplications);
 
 	// This ctor applies the "blocked" policy.
-	FwContext(uint32_t timeout, const WinFwSettings &settings);
+	FwContext
+	(
+		uint32_t timeout,
+		const WinFwSettings &settings,
+		const std::vector<std::wstring> &approvedApplications
+	);
 
 	struct PingableHosts
 	{
@@ -69,6 +74,8 @@ private:
 	bool applyRuleset(const Ruleset &ruleset);
 	bool applyRulesetDirectly(const Ruleset &ruleset, SessionController &controller);
 
+	const std::vector<std::wstring> m_approvedApplications;
+
 	std::unique_ptr<SessionController> m_sessionController;
 
 	uint32_t m_baseline;
diff --git a/windows/winfw/src/winfw/rules/baseline/permitvpnrelay.h b/windows/winfw/src/winfw/rules/baseline/permitvpnrelay.h
deleted file mode 100644
index 8dd2c630f4..0000000000
--- a/windows/winfw/src/winfw/rules/baseline/permitvpnrelay.h
+++ /dev/null
@@ -1,30 +0,0 @@
-#pragma once
-
-#include <winfw/rules/ifirewallrule.h>
-#include <libwfp/ipaddress.h>
-
-namespace rules::baseline
-{
-
-class PermitVpnRelay : public IFirewallRule
-{
-public:
-
-	enum class Protocol
-	{
-		Tcp,
-		Udp
-	};
-
-	PermitVpnRelay(const wfp::IpAddress &relay, uint16_t relayPort, Protocol protocol);
-	
-	bool apply(IObjectInstaller &objectInstaller) override;
-
-private:
-
-	const wfp::IpAddress m_relay;
-	const uint16_t m_relayPort;
-	const Protocol m_protocol;
-};
-
-}
diff --git a/windows/winfw/src/winfw/rules/dns/permitnontunnel.h b/windows/winfw/src/winfw/rules/dns/permitnontunnel.h
index 07b67245c3..3d8fac5cf2 100644
--- a/windows/winfw/src/winfw/rules/dns/permitnontunnel.h
+++ b/windows/winfw/src/winfw/rules/dns/permitnontunnel.h
@@ -6,6 +6,11 @@
 #include <optional>
 #include <string>
 
+//
+// N.B. This rule must only be used for "custom DNS".
+// Connecting to a relay on port 53 is supported using a different rule.
+//
+
 namespace rules::dns
 {
 
@@ -14,8 +19,10 @@ class PermitNonTunnel : public IFirewallRule
 public:
 
 	//
-	// The alias argument has to be optional for when the relay is connected on port 53.
-	// At this point in time there's no tunnel yet.
+	// The tunnel alias is optional so this rule can be applied even
+	// when no tunnel exists.
+	//
+	// If a tunnel does exist, the alias must be provided.
 	//
 	PermitNonTunnel(std::optional<std::wstring> tunnelInterfaceAlias, const std::vector<wfp::IpAddress> &hosts);
 	
diff --git a/windows/winfw/src/winfw/rules/baseline/permitvpnrelay.cpp b/windows/winfw/src/winfw/rules/multi/permitvpnrelay.cpp
index daa21c3e35..db14ee4852 100644
--- a/windows/winfw/src/winfw/rules/baseline/permitvpnrelay.cpp
+++ b/windows/winfw/src/winfw/rules/multi/permitvpnrelay.cpp
@@ -6,11 +6,12 @@
 #include <libwfp/conditions/conditionprotocol.h>
 #include <libwfp/conditions/conditionip.h>
 #include <libwfp/conditions/conditionport.h>
+#include <libwfp/conditions/conditionapplication.h>
 #include <libcommon/error.h>
 
 using namespace wfp::conditions;
 
-namespace rules::baseline
+namespace rules::multi
 {
 
 namespace
@@ -42,13 +43,39 @@ std::unique_ptr<ConditionProtocol> CreateProtocolCondition(PermitVpnRelay::Proto
 	};
 }
 
+const GUID &TranslateSublayer(PermitVpnRelay::Sublayer sublayer)
+{
+	switch (sublayer)
+	{
+		case PermitVpnRelay::Sublayer::Baseline: return MullvadGuids::SublayerBaseline();
+		case PermitVpnRelay::Sublayer::Dns: return MullvadGuids::SublayerDns();
+		default:
+		{
+			THROW_ERROR("Missing case handler in switch clause");
+		}
+	};
+}
+
 } // anonymous namespace
 
-PermitVpnRelay::PermitVpnRelay(const wfp::IpAddress &relay, uint16_t relayPort, Protocol protocol)
+PermitVpnRelay::PermitVpnRelay
+(
+	const wfp::IpAddress &relay,
+	uint16_t relayPort,
+	Protocol protocol,
+	const std::vector<std::wstring> &approvedApplications,
+	Sublayer sublayer
+)
 	: m_relay(relay)
 	, m_relayPort(relayPort)
 	, m_protocol(protocol)
+	, m_approvedApplications(approvedApplications)
+	, m_sublayer(sublayer)
 {
+	if (m_approvedApplications.empty())
+	{
+		THROW_ERROR("Cannot configure relay access without list of approved applications");
+	}
 }
 
 bool PermitVpnRelay::apply(IObjectInstaller &objectInstaller)
@@ -65,7 +92,7 @@ bool PermitVpnRelay::apply(IObjectInstaller &objectInstaller)
 		.description(L"This filter is part of a rule that permits communication with a VPN relay")
 		.provider(MullvadGuids::Provider())
 		.layer(LayerFromIp(m_relay))
-		.sublayer(MullvadGuids::SublayerBaseline())
+		.sublayer(TranslateSublayer(m_sublayer))
 		.weight(wfp::FilterBuilder::WeightClass::Max)
 		.permit();
 
@@ -75,6 +102,11 @@ bool PermitVpnRelay::apply(IObjectInstaller &objectInstaller)
 	conditionBuilder.add_condition(ConditionPort::Remote(m_relayPort));
 	conditionBuilder.add_condition(CreateProtocolCondition(m_protocol));
 
+	for (const auto &app : m_approvedApplications)
+	{
+		conditionBuilder.add_condition(std::make_unique<ConditionApplication>(app));
+	}
+
 	return objectInstaller.addFilter(filterBuilder, conditionBuilder);
 }
 
diff --git a/windows/winfw/src/winfw/rules/multi/permitvpnrelay.h b/windows/winfw/src/winfw/rules/multi/permitvpnrelay.h
new file mode 100644
index 0000000000..e40fce159d
--- /dev/null
+++ b/windows/winfw/src/winfw/rules/multi/permitvpnrelay.h
@@ -0,0 +1,47 @@
+#pragma once
+
+#include <winfw/rules/ifirewallrule.h>
+#include <libwfp/ipaddress.h>
+#include <string>
+#include <vector>
+
+namespace rules::multi
+{
+
+class PermitVpnRelay : public IFirewallRule
+{
+public:
+
+	enum class Protocol
+	{
+		Tcp,
+		Udp
+	};
+
+	enum class Sublayer
+	{
+		Baseline,
+		Dns
+	};
+
+	PermitVpnRelay
+	(
+		const wfp::IpAddress &relay,
+		uint16_t relayPort,
+		Protocol protocol,
+		const std::vector<std::wstring> &approvedApplications,
+		Sublayer sublayer
+	);
+	
+	bool apply(IObjectInstaller &objectInstaller) override;
+
+private:
+
+	const wfp::IpAddress m_relay;
+	const uint16_t m_relayPort;
+	const Protocol m_protocol;
+	const std::vector<std::wstring> m_approvedApplications;
+	const Sublayer m_sublayer;
+};
+
+}
diff --git a/windows/winfw/src/winfw/winfw.cpp b/windows/winfw/src/winfw/winfw.cpp
index 6c1cc7cd5a..55587e03f9 100644
--- a/windows/winfw/src/winfw/winfw.cpp
+++ b/windows/winfw/src/winfw/winfw.cpp
@@ -42,6 +42,27 @@ std::optional<FwContext::PingableHosts> ConvertPingableHosts(const PingableHosts
 	return converted;
 }
 
+std::vector<std::wstring> ConvertApprovedApplications
+(
+	WinFwApprovedApplications *approvedApplications
+)
+{
+	if (nullptr == approvedApplications
+		|| 0 == approvedApplications->numApps)
+	{
+		THROW_ERROR("Invalid list of approved applications (empty list)");
+	}
+
+	std::vector<std::wstring> converted;
+
+	for (size_t i = 0; i < approvedApplications->numApps; ++i)
+	{
+		converted.emplace_back(std::wstring(approvedApplications->apps[i]));
+	}
+
+	return converted;
+}
+
 } // anonymous namespace
 
 WINFW_LINKAGE
@@ -49,6 +70,7 @@ bool
 WINFW_API
 WinFw_Initialize(
 	uint32_t timeout,
+	WinFwApprovedApplications *approvedApplications,
 	MullvadLogSink logSink,
 	void *logSinkContext
 )
@@ -70,7 +92,8 @@ WinFw_Initialize(
 		g_logSink = logSink;
 		g_logSinkContext = logSinkContext;
 
-		g_fwContext = new FwContext(timeout_ms);
+		g_fwContext = new FwContext(timeout_ms,
+			ConvertApprovedApplications(approvedApplications));
 	}
 	catch (std::exception &err)
 	{
@@ -96,6 +119,7 @@ WINFW_API
 WinFw_InitializeBlocked(
 	uint32_t timeout,
 	const WinFwSettings *settings,
+	WinFwApprovedApplications *approvedApplications,
 	MullvadLogSink logSink,
 	void *logSinkContext
 )
@@ -122,7 +146,8 @@ WinFw_InitializeBlocked(
 		g_logSink = logSink;
 		g_logSinkContext = logSinkContext;
 
-		g_fwContext = new FwContext(timeout_ms, *settings);
+		g_fwContext = new FwContext(timeout_ms, *settings,
+			ConvertApprovedApplications(approvedApplications));
 	}
 	catch (std::exception &err)
 	{
diff --git a/windows/winfw/src/winfw/winfw.h b/windows/winfw/src/winfw/winfw.h
index 8f418c333b..100c166d32 100644
--- a/windows/winfw/src/winfw/winfw.h
+++ b/windows/winfw/src/winfw/winfw.h
@@ -45,6 +45,17 @@ typedef struct tag_WinFwRelay
 }
 WinFwRelay;
 
+//
+// This structure is used to define the set of applications
+// that are allowed to communicate with the relay.
+//
+typedef struct tag_WinFwApprovedApplications
+{
+	const wchar_t **apps;
+	size_t numApps;
+}
+WinFwApprovedApplications;
+
 #pragma pack(pop)
 
 ///////////////////////////////////////////////////////////////////////////////
@@ -67,6 +78,7 @@ bool
 WINFW_API
 WinFw_Initialize(
 	uint32_t timeout,
+	WinFwApprovedApplications *approvedApplications,
 	MullvadLogSink logSink,
 	void *logSinkContext
 );
@@ -88,6 +100,7 @@ WINFW_API
 WinFw_InitializeBlocked(
 	uint32_t timeout,
 	const WinFwSettings *settings,
+	WinFwApprovedApplications *approvedApplications,
 	MullvadLogSink logSink,
 	void *logSinkContext
 );
diff --git a/windows/winfw/src/winfw/winfw.vcxproj b/windows/winfw/src/winfw/winfw.vcxproj
index c999f5aaca..85a6e0d0b4 100644
--- a/windows/winfw/src/winfw/winfw.vcxproj
+++ b/windows/winfw/src/winfw/winfw.vcxproj
@@ -32,12 +32,12 @@
     <ClCompile Include="rules\baseline\permitloopback.cpp" />
     <ClCompile Include="rules\baseline\permitndp.cpp" />
     <ClCompile Include="rules\baseline\permitping.cpp" />
-    <ClCompile Include="rules\baseline\permitvpnrelay.cpp" />
     <ClCompile Include="rules\baseline\permitvpntunnel.cpp" />
     <ClCompile Include="rules\baseline\permitvpntunnelservice.cpp" />
     <ClCompile Include="rules\dns\blockall.cpp" />
     <ClCompile Include="rules\dns\permitnontunnel.cpp" />
     <ClCompile Include="rules\dns\permittunnel.cpp" />
+    <ClCompile Include="rules\multi\permitvpnrelay.cpp" />
     <ClCompile Include="rules\shared.cpp" />
     <ClCompile Include="sessioncontroller.cpp" />
     <ClCompile Include="sessionrecord.cpp" />
@@ -65,12 +65,12 @@
     <ClInclude Include="rules\baseline\permitloopback.h" />
     <ClInclude Include="rules\baseline\permitndp.h" />
     <ClInclude Include="rules\baseline\permitping.h" />
-    <ClInclude Include="rules\baseline\permitvpnrelay.h" />
     <ClInclude Include="rules\baseline\permitvpntunnel.h" />
     <ClInclude Include="rules\baseline\permitvpntunnelservice.h" />
     <ClInclude Include="rules\dns\blockall.h" />
     <ClInclude Include="rules\dns\permitnontunnel.h" />
     <ClInclude Include="rules\dns\permittunnel.h" />
+    <ClInclude Include="rules\multi\permitvpnrelay.h" />
     <ClInclude Include="rules\ports.h" />
     <ClInclude Include="rules\shared.h" />
     <ClInclude Include="wfpobjecttype.h" />
diff --git a/windows/winfw/src/winfw/winfw.vcxproj.filters b/windows/winfw/src/winfw/winfw.vcxproj.filters
index 46c0594c10..9ac82e87fb 100644
--- a/windows/winfw/src/winfw/winfw.vcxproj.filters
+++ b/windows/winfw/src/winfw/winfw.vcxproj.filters
@@ -34,9 +34,6 @@
     <ClCompile Include="rules\baseline\permitping.cpp">
       <Filter>rules\baseline</Filter>
     </ClCompile>
-    <ClCompile Include="rules\baseline\permitvpnrelay.cpp">
-      <Filter>rules\baseline</Filter>
-    </ClCompile>
     <ClCompile Include="rules\baseline\permitvpntunnel.cpp">
       <Filter>rules\baseline</Filter>
     </ClCompile>
@@ -58,6 +55,9 @@
     <ClCompile Include="rules\shared.cpp">
       <Filter>rules</Filter>
     </ClCompile>
+    <ClCompile Include="rules\multi\permitvpnrelay.cpp">
+      <Filter>rules\multi</Filter>
+    </ClCompile>
   </ItemGroup>
   <ItemGroup>
     <ClInclude Include="stdafx.h" />
@@ -99,9 +99,6 @@
     <ClInclude Include="rules\baseline\permitping.h">
       <Filter>rules\baseline</Filter>
     </ClInclude>
-    <ClInclude Include="rules\baseline\permitvpnrelay.h">
-      <Filter>rules\baseline</Filter>
-    </ClInclude>
     <ClInclude Include="rules\baseline\permitvpntunnel.h">
       <Filter>rules\baseline</Filter>
     </ClInclude>
@@ -126,6 +123,9 @@
     <ClInclude Include="rules\shared.h">
       <Filter>rules</Filter>
     </ClInclude>
+    <ClInclude Include="rules\multi\permitvpnrelay.h">
+      <Filter>rules\multi</Filter>
+    </ClInclude>
   </ItemGroup>
   <ItemGroup>
     <Filter Include="rules">
@@ -137,6 +137,9 @@
     <Filter Include="rules\dns">
       <UniqueIdentifier>{9b35e8a4-84be-4ac3-9b6f-eb21cc02e065}</UniqueIdentifier>
     </Filter>
+    <Filter Include="rules\multi">
+      <UniqueIdentifier>{005cce7c-ed9d-4675-8e4f-759c9682b77e}</UniqueIdentifier>
+    </Filter>
   </ItemGroup>
   <ItemGroup>
     <None Include="winfw.def" />