Merge branch 'win-fix-winfw-deinit'

10 files changed, 116 insertions, 42 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index f6d0665b47..9375d30ccc 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -43,6 +43,11 @@ Line wrap the file at 100 chars.                                              Th
   button.
 - Show when the app failed to block all connections after an error.
 
+### Security
+#### Windows
+- Fix issue in daemon where the `block_when_disconnected` setting was sometimes not honored when
+  stopping the daemon. I.e. traffic could flow freely after the daemon was stopped.
+
 
 ## [2020.3] - 2020-02-20
 This release is identical to 2020.3-beta1
diff --git a/talpid-core/src/firewall/windows.rs b/talpid-core/src/firewall/windows.rs
index 89e880da7e..807bcd1167 100644
--- a/talpid-core/src/firewall/windows.rs
+++ b/talpid-core/src/firewall/windows.rs
@@ -113,7 +113,11 @@ impl FirewallT for Firewall {
 
 impl Drop for Firewall {
     fn drop(&mut self) {
-        if unsafe { WinFw_Deinitialize().into_result().is_ok() } {
+        if unsafe {
+            WinFw_Deinitialize(WinFwCleanupPolicy::ContinueBlocking)
+                .into_result()
+                .is_ok()
+        } {
             trace!("Successfully deinitialized windows firewall module");
         } else {
             error!("Failed to deinitialize windows firewall module");
@@ -280,6 +284,14 @@ mod winfw {
         pub num_addresses: usize,
     }
 
+    #[allow(dead_code)]
+    #[repr(u8)]
+    #[derive(Clone, Copy)]
+    pub enum WinFwCleanupPolicy {
+        ContinueBlocking = 0u8,
+        ResetFirewall = 1u8,
+    }
+
     ffi_error!(InitializationResult, Error::Initialization);
     ffi_error!(DeinitializationResult, Error::Deinitialization);
     ffi_error!(ApplyConnectingResult, Error::ApplyingConnectingPolicy);
@@ -304,7 +316,7 @@ mod winfw {
         ) -> InitializationResult;
 
         #[link_name = "WinFw_Deinitialize"]
-        pub fn WinFw_Deinitialize() -> DeinitializationResult;
+        pub fn WinFw_Deinitialize(cleanupPolicy: WinFwCleanupPolicy) -> DeinitializationResult;
 
         #[link_name = "WinFw_ApplyPolicyConnecting"]
         pub fn WinFw_ApplyPolicyConnecting(
diff --git a/windows/winfw/src/extras/cli/commands/winfw/deinit.cpp b/windows/winfw/src/extras/cli/commands/winfw/deinit.cpp
index 5fa2c91278..013159f062 100644
--- a/windows/winfw/src/extras/cli/commands/winfw/deinit.cpp
+++ b/windows/winfw/src/extras/cli/commands/winfw/deinit.cpp
@@ -28,7 +28,7 @@ void Deinit::handleRequest(const std::vector<std::wstring> &arguments)
 		THROW_ERROR("Invalid argument(s). Cannot complete request.");
 	}
 
-	m_messageSink((WinFw_Deinitialize()
+	m_messageSink((WinFw_Deinitialize(WINFW_CLEANUP_POLICY_RESET_FIREWALL)
 		? L"Deinitialization completed successfully."
 		: L"Deinitialization failed. See above for details, if any."));
 }
diff --git a/windows/winfw/src/extras/cli/commands/winfw/policy.cpp b/windows/winfw/src/extras/cli/commands/winfw/policy.cpp
index ca84187e98..a722501832 100644
--- a/windows/winfw/src/extras/cli/commands/winfw/policy.cpp
+++ b/windows/winfw/src/extras/cli/commands/winfw/policy.cpp
@@ -111,8 +111,8 @@ void Policy::processConnecting(const KeyValuePairs &arguments)
 
 	auto success = WinFw_ApplyPolicyConnecting
 	(
-		settings,
-		relay,
+		&settings,
+		&relay,
 		nullptr
 	);
 
@@ -140,8 +140,8 @@ void Policy::processConnected(const KeyValuePairs &arguments)
 
 	auto success = WinFw_ApplyPolicyConnected
 	(
-		settings,
-		relay,
+		&settings,
+		&relay,
 		GetArgumentValue(arguments, L"tunnel").c_str(),
 		GetArgumentValue(arguments, L"dns").c_str(),
 		nullptr
@@ -160,7 +160,7 @@ void Policy::processBlocked(const KeyValuePairs &arguments)
 		GetArgumentValue(arguments, L"lan")
 	);
 
-	auto success = WinFw_ApplyPolicyBlocked(settings);
+	auto success = WinFw_ApplyPolicyBlocked(&settings);
 
 	m_messageSink((success
 		? L"Successfully applied policy."
diff --git a/windows/winfw/src/winfw/fwcontext.cpp b/windows/winfw/src/winfw/fwcontext.cpp
index 54a7797a69..4883e6f6d8 100644
--- a/windows/winfw/src/winfw/fwcontext.cpp
+++ b/windows/winfw/src/winfw/fwcontext.cpp
@@ -116,6 +116,7 @@ CreateRelayDnsExclusion(const WinFwRelay &relay)
 
 FwContext::FwContext(uint32_t timeout)
 	: m_baseline(0)
+	, m_activePolicy(Policy::None)
 {
 	auto engine = wfp::FilterEngine::StandardSession(timeout);
 
@@ -130,10 +131,12 @@ FwContext::FwContext(uint32_t timeout)
 	}
 
 	m_baseline = m_sessionController->checkpoint();
+	m_activePolicy = Policy::None;
 }
 
 FwContext::FwContext(uint32_t timeout, const WinFwSettings &settings)
 	: m_baseline(0)
+	, m_activePolicy(Policy::None)
 {
 	auto engine = wfp::FilterEngine::StandardSession(timeout);
 
@@ -150,6 +153,7 @@ FwContext::FwContext(uint32_t timeout, const WinFwSettings &settings)
 	}
 
 	m_baseline = checkpoint;
+	m_activePolicy = Policy::Blocked;
 }
 
 bool FwContext::applyPolicyConnecting
@@ -183,7 +187,14 @@ bool FwContext::applyPolicyConnecting
 		));
 	}
 
-	return applyRuleset(ruleset);
+	const auto status = applyRuleset(ruleset);
+
+	if (status)
+	{
+		m_activePolicy = Policy::Connecting;
+	}
+
+	return status;
 }
 
 bool FwContext::applyPolicyConnected
@@ -221,20 +232,46 @@ bool FwContext::applyPolicyConnected
 		tunnelInterfaceAlias
 	));
 
-	return applyRuleset(ruleset);
+	const auto status = applyRuleset(ruleset);
+
+	if (status)
+	{
+		m_activePolicy = Policy::Connected;
+	}
+
+	return status;
 }
 
 bool FwContext::applyPolicyBlocked(const WinFwSettings &settings)
 {
-	return applyRuleset(composePolicyBlocked(settings));
+	const auto status = applyRuleset(composePolicyBlocked(settings));
+
+	if (status)
+	{
+		m_activePolicy = Policy::Blocked;
+	}
+
+	return status;
 }
 
 bool FwContext::reset()
 {
-	return m_sessionController->executeTransaction([this](SessionController &controller, wfp::FilterEngine &)
+	const auto status = m_sessionController->executeTransaction([this](SessionController &controller, wfp::FilterEngine &)
 	{
 		return controller.revert(m_baseline), true;
 	});
+
+	if (status)
+	{
+		m_activePolicy = Policy::None;
+	}
+
+	return status;
+}
+
+FwContext::Policy FwContext::activePolicy() const
+{
+	return m_activePolicy;
 }
 
 FwContext::Ruleset FwContext::composePolicyBlocked(const WinFwSettings &settings)
diff --git a/windows/winfw/src/winfw/fwcontext.h b/windows/winfw/src/winfw/fwcontext.h
index 6e2bc590e4..6bdb398b16 100644
--- a/windows/winfw/src/winfw/fwcontext.h
+++ b/windows/winfw/src/winfw/fwcontext.h
@@ -43,6 +43,16 @@ public:
 
 	bool reset();
 
+	enum class Policy
+	{
+		Connecting,
+		Connected,
+		Blocked,
+		None,
+	};
+
+	Policy activePolicy() const;
+
 	using Ruleset = std::vector<std::unique_ptr<rules::IFirewallRule> >;
 
 private:
@@ -62,4 +72,5 @@ private:
 	std::unique_ptr<SessionController> m_sessionController;
 
 	uint32_t m_baseline;
+	Policy m_activePolicy;
 };
diff --git a/windows/winfw/src/winfw/sessioncontroller.cpp b/windows/winfw/src/winfw/sessioncontroller.cpp
index c5342fce78..83f6863c91 100644
--- a/windows/winfw/src/winfw/sessioncontroller.cpp
+++ b/windows/winfw/src/winfw/sessioncontroller.cpp
@@ -1,10 +1,10 @@
 #include "stdafx.h"
 #include "sessioncontroller.h"
 #include "wfpobjecttype.h"
-#include "libwfp/objectinstaller.h"
-#include "libwfp/objectdeleter.h"
-#include "libwfp/transaction.h"
-#include "libcommon/memory.h"
+#include <libwfp/objectinstaller.h>
+#include <libwfp/objectdeleter.h>
+#include <libwfp/transaction.h>
+#include <libcommon/memory.h>
 #include <libcommon/error.h>
 #include <utility>
 
@@ -65,26 +65,6 @@ SessionController::SessionController(std::unique_ptr<wfp::FilterEngine> &&engine
 {
 }
 
-SessionController::~SessionController()
-{
-	//
-	// TODO: Review destruction of this instance and its owner.
-	//
-
-	try
-	{
-		executeTransaction([this](SessionController &, wfp::FilterEngine &)
-		{
-			reset();
-			return true;
-		});
-	}
-	catch (...)
-	{
-		return;
-	}
-}
-
 bool SessionController::addProvider(wfp::ProviderBuilder &providerBuilder)
 {
 	if (false == m_activeTransaction)
diff --git a/windows/winfw/src/winfw/sessioncontroller.h b/windows/winfw/src/winfw/sessioncontroller.h
index 4300791660..f82563ce0d 100644
--- a/windows/winfw/src/winfw/sessioncontroller.h
+++ b/windows/winfw/src/winfw/sessioncontroller.h
@@ -3,8 +3,8 @@
 #include "iobjectinstaller.h"
 #include "sessionrecord.h"
 #include "mullvadguids.h"
-#include "libwfp/filterengine.h"
-#include "libwfp/iidentifiable.h"
+#include <libwfp/filterengine.h>
+#include <libwfp/iidentifiable.h>
 #include <functional>
 #include <atomic>
 #include <memory>
@@ -15,7 +15,6 @@ class SessionController : public IObjectInstaller
 public:
 
 	SessionController(std::unique_ptr<wfp::FilterEngine> &&engine);
-	~SessionController();
 
 	bool addProvider(wfp::ProviderBuilder &providerBuilder) override;
 	bool addSublayer(wfp::SublayerBuilder &sublayerBuilder) override;
diff --git a/windows/winfw/src/winfw/winfw.cpp b/windows/winfw/src/winfw/winfw.cpp
index 0af40994e6..6c1cc7cd5a 100644
--- a/windows/winfw/src/winfw/winfw.cpp
+++ b/windows/winfw/src/winfw/winfw.cpp
@@ -144,17 +144,35 @@ WinFw_InitializeBlocked(
 WINFW_LINKAGE
 bool
 WINFW_API
-WinFw_Deinitialize()
+WinFw_Deinitialize(WINFW_CLEANUP_POLICY cleanupPolicy)
 {
 	if (nullptr == g_fwContext)
 	{
 		return true;
 	}
 
+	const auto activePolicy = g_fwContext->activePolicy();
+
+	//
+	// Do not use FwContext::reset() here because it just
+	// removes the current policy but leaves sublayers etc.
+	//
+
 	delete g_fwContext;
 	g_fwContext = nullptr;
 
-	return true;
+	//
+	// Only skip clean-up if this is what the caller requested
+	// and if the current policy is "(net) blocked".
+	//
+
+	if (WINFW_CLEANUP_POLICY_CONTINUE_BLOCKING == cleanupPolicy
+		&& FwContext::Policy::Blocked == activePolicy)
+	{
+		return true;
+	}
+
+	return WinFw_Reset();
 }
 
 WINFW_LINKAGE
diff --git a/windows/winfw/src/winfw/winfw.h b/windows/winfw/src/winfw/winfw.h
index e989d69f57..8f418c333b 100644
--- a/windows/winfw/src/winfw/winfw.h
+++ b/windows/winfw/src/winfw/winfw.h
@@ -92,6 +92,16 @@ WinFw_InitializeBlocked(
 	void *logSinkContext
 );
 
+enum WINFW_CLEANUP_POLICY
+{
+	// Continue blocking if this happens to be the active policy
+	// otherwise reset the firewall.
+	WINFW_CLEANUP_POLICY_CONTINUE_BLOCKING = 0,
+
+	// Remove all objects that have been registered with WFP.
+	WINFW_CLEANUP_POLICY_RESET_FIREWALL = 1,
+};
+
 //
 // Deinitialize:
 //
@@ -101,7 +111,9 @@ extern "C"
 WINFW_LINKAGE
 bool
 WINFW_API
-WinFw_Deinitialize();
+WinFw_Deinitialize(
+	WINFW_CLEANUP_POLICY cleanupPolicy
+);
 
 //
 // PingableHosts: