Check if privddrop was successful (#345)

Currently, if nzbget is daemonized as root, and an invalid `DaemonUsername` is set, privileges are not dropped. Consequence is that nzbget runs with elevated privileges, which is undesirable for any software with network-facing interfaces.
nzbgetcom · Aug 9, 2024 · 61585fa · 61585fa
1 parent f89978f
commit 61585fa
Show file tree

Hide file tree

Showing 5 changed files with 30 additions and 12 deletions.
diff --git a/daemon/main/Options.cpp b/daemon/main/Options.cpp
@@ -656,7 +656,7 @@ void Options::CheckDir(CString& dir, const char* optionName,
 	}
 }
 
-void Options::InitOptions()
+void Options::CheckDirs()
 {
 	const char* mainDir = GetOption(OPTION_MAINDIR);
 
@@ -667,7 +667,10 @@ void Options::InitOptions()
 	CheckDir(m_webDir, OPTION_WEBDIR, nullptr, true, false);
 	CheckDir(m_scriptDir, OPTION_SCRIPTDIR, mainDir, true, false);
 	CheckDir(m_nzbDir, OPTION_NZBDIR, mainDir, false, true);
+}
 
+void Options::InitOptions()
+{
 	m_requiredDir = GetOption(OPTION_REQUIREDDIR);
 
 	m_configTemplate		= GetOption(OPTION_CONFIGTEMPLATE);

diff --git a/daemon/main/Options.h b/daemon/main/Options.h
@@ -196,6 +196,7 @@ class Options
 	GuardedOptEntries GuardOptEntries() { return GuardedOptEntries(&m_optEntries, &m_optEntriesMutex); }
 	void CreateSchedulerTask(int id, const char* time, const char* weekDays,
 		ESchedulerCommand command, const char* param);
+	void CheckDirs();
 
 	// Options
 	const char* GetConfigFilename() const { return m_configFilename; }

diff --git a/daemon/main/nzbget.cpp b/daemon/main/nzbget.cpp
@@ -327,6 +327,8 @@ void NZBGet::Init()
 		info("nzbget %s remote-mode", Util::VersionRevision());
 	}
 
+	m_options->CheckDirs();
+
 	info("using %s", m_options->GetConfigFilename());
 	info("nzbget runs on %s:%i", m_options->GetControlIp(), m_options->GetControlPort());
 
@@ -1010,18 +1012,24 @@ void NZBGet::Daemonize()
 	if (getuid() == 0 || geteuid() == 0)
 	{
 		struct passwd *pw = getpwnam(m_options->GetDaemonUsername());
-		if (pw)
+		if (pw == nullptr)
 		{
-			// Change owner of lock file
-			fchown(lfp, pw->pw_uid, pw->pw_gid);
-			// Set aux groups to null.
-			setgroups(0, (const gid_t*)0);
-			// Set primary group.
-			setgid(pw->pw_gid);
-			// Try setting aux groups correctly - not critical if this fails.
-			initgroups(m_options->GetDaemonUsername(), pw->pw_gid);
-			// Finally, set uid.
-			setuid(pw->pw_uid);
+			error("Starting daemon failed: invalid DaemonUsername");
+			exit(1);
+		}
+
+		// Change owner of lock- and logfile
+		chown(m_options->GetLockFile(), pw->pw_uid, pw->pw_gid);
+		chown(m_options->GetLogFile(), pw->pw_uid, pw->pw_gid);
+
+		// Set aux groups to null, configure primary and aux groups, and then assign uid
+		if (setgroups(0, (const gid_t*)0) ||
+				setgid(pw->pw_gid) ||
+				initgroups(m_options->GetDaemonUsername(), pw->pw_gid) ||
+				setuid(pw->pw_uid))
+		{
+			error("Starting daemon failed: could not drop privileges");
+			exit(1);
 		}
 	}
 

diff --git a/tests/extension/ExtensionManagerTest.cpp b/tests/extension/ExtensionManagerTest.cpp
@@ -52,6 +52,8 @@ BOOST_AUTO_TEST_CASE(LoadExtesionsTest)
 	Options options(&cmdOpts, nullptr);
 	g_Options = &options;
 
+	options.CheckDirs();
+
 	std::vector<std::string> correctOrder = { "Extension2", "Extension1", "email" };
 	ExtensionManager::Manager manager;
 
@@ -78,6 +80,8 @@ BOOST_AUTO_TEST_CASE(ShouldNotDeleteExtensionIfExtensionIsBusyTest)
 	g_Options = &options;
 	ExtensionManager::Manager manager;
 
+	options.CheckDirs();
+
 	BOOST_REQUIRE(manager.LoadExtensions() == std::nullopt);
 
 	const auto busyExt = manager.GetExtensions()[0];

diff --git a/tests/main/OptionsTest.cpp b/tests/main/OptionsTest.cpp
@@ -58,6 +58,8 @@ BOOST_AUTO_TEST_CASE(OptionsInitWithoutConfigurationFileTest)
 {
 	Options options(nullptr, nullptr);
 
+	options.CheckDirs();
+
 	BOOST_CHECK(options.GetConfigFilename() == nullptr);
 #ifdef WIN32
 	BOOST_CHECK(strcmp(options.GetTempDir(), "nzbget/tmp") == 0);