Harden Store administration and credential handling

src/agents/store/stored.c

@@ -368,7 +368,11 @@ _XplServiceMain(int argc, char *argv[])
 	XplMutexInit(StoreAgent.clients.lock);
 	XplMutexInit(StoreAgent.user_maintenance.lock);
 
-    StoreAgentReadConfiguration(NULL);
+	if (!StoreAgentReadConfiguration(NULL)) {
+		Log(LOG_FATAL, "Could not read Store credentials and configuration");
+		BongoAgentShutdown(&StoreAgent.agent);
+		return -1;
+	}
 
     /* setup the store guts: */
     if (StoreSetupCommands()) {

src/apps/admin/admin.c

@@ -41,6 +41,7 @@
 #define DEFAULT_EXTENSION_HOURS 48L
 #define MAX_EXTENSION_HOURS (24L * 31L)
 #define MAX_CONFIG_DOCUMENT_SIZE (1024U * 1024U)
+#define ADMIN_STORE_TIMEOUT_SECONDS 15U
 
 #ifndef BONGO_CONFIG_UI_PATH
 #define BONGO_CONFIG_UI_PATH "/usr/libexec/bongo/bongo-config-ui"
@@ -48,6 +49,22 @@
 
 static int ValidCommandName(const char *name);
 
+static int
+IsAdministrativeUser(void)
+{
+	const char *service_user;
+	uid_t service_uid;
+	gid_t service_gid;
+
+	if (geteuid() == 0)
+		return 1;
+	service_user = MsgGetUnprivilegedUser();
+	if (!service_user ||
+	    XplLookupUser(service_user, &service_uid, &service_gid) != 0)
+		return 0;
+	return geteuid() == service_uid;
+}
+
 static int
 ValidConfigurationName(const char *name)
 {
@@ -165,7 +182,7 @@ AccessConfiguration(const char *name, int replace)
 	}
 
 	XplInit();
-	if (!ConnStartup(15 * 60)) {
+	if (!ConnStartup(ADMIN_STORE_TIMEOUT_SECONDS)) {
 		fprintf(stderr, "bongo-admin: cannot initialize networking\n");
 		goto done;
 	}
@@ -361,12 +378,15 @@ ShowAgents(const char *requested)
 	int result = 1;
 
 	XplInit();
-	if (!ConnStartup(15 * 60))
+	if (!ConnStartup(ADMIN_STORE_TIMEOUT_SECONDS))
 		goto done;
 	conn_started = 1;
 	MsgInit();
 	msg_started = 1;
-	NMAPInitialize();
+	if (!NMAPInitialize()) {
+		fprintf(stderr, "bongo-admin: cannot load the Store credential\n");
+		goto done;
+	}
 	if (!NMAPReadConfigFile("manager", &config) || !config) {
 		fprintf(stderr, "bongo-admin: cannot read live manager configuration\n");
 		goto done;
@@ -468,12 +488,15 @@ RenameUser(const char *old_username, const char *new_username)
 		return 1;
 	}
 	XplInit();
-	if (!ConnStartup(15 * 60)) {
+	if (!ConnStartup(ADMIN_STORE_TIMEOUT_SECONDS)) {
 		fprintf(stderr, "bongo-admin: cannot initialize networking\n");
 		return 1;
 	}
 	MsgInit();
-	NMAPInitialize();
+	if (!NMAPInitialize()) {
+		fprintf(stderr, "bongo-admin: cannot load the Store credential\n");
+		goto done;
+	}
 	connection = NMAPConnect("127.0.0.1", NULL);
 	if (!connection ||
 	    !NMAPAuthenticateToStore(connection, response, sizeof(response))) {
@@ -621,8 +644,10 @@ main(int argc, char **argv)
 		Usage(stderr);
 		return 2;
 	}
-	if (geteuid() != 0) {
-		fprintf(stderr, "bongo-admin: this command must be run as root\n");
+	if (!IsAdministrativeUser()) {
+		fprintf(stderr,
+		        "bongo-admin: this command must be run as root or the Bongo "
+		        "service user\n");
 		return 1;
 	}
 	if (argc == 3 && !strcmp(argv[1], "__config-read"))

src/apps/config/tests/test_storage.py

@@ -23,6 +23,7 @@ from __future__ import annotations
 
 import copy
 import json
+import subprocess
 import sys
 import tempfile
 import unittest
@@ -122,6 +123,15 @@ class ConfigurationStorageTest(unittest.TestCase):
             self.store.save(configurations)
         self.assertEqual(self.store.live["smtp"], original_smtp)
 
+    def test_store_helper_timeout_names_operation_and_document(self):
+        store = ConfigurationStore(self.paths)
+        with mock.patch("subprocess.run", side_effect=subprocess.TimeoutExpired(
+                [str(self.paths.admin), "__config-read", "sieve"], 30)):
+            with self.assertRaisesRegex(
+                    ConfigurationError,
+                    r"timed out during __config-read for sieve"):
+                store.read_store_document("sieve")
+
     def test_scanner_ping_probes_require_expected_reply(self):
         clam = FakeSocket(b"PONG\0")
         spam = FakeSocket(b"SPAMD/1.5 0 PONG\r\n")

src/libs/connio/CMakeLists.txt

@@ -52,6 +52,19 @@ if(BUILD_TESTING)
 		Threads::Threads)
 	add_test(NAME connio-proxy-protocol COMMAND connio-proxy-protocol-test)
 
+	add_executable(connio-interface-list-test tests/interface-list-test.c)
+	connio_keep_test_dependencies(connio-interface-list-test)
+	target_link_libraries(connio-interface-list-test PRIVATE
+		bongoxpl
+		bongoconnio
+		bongoutil
+		bongojson
+		bongomsgapi
+		GnuTLS::GnuTLS
+		Threads::Threads)
+	add_test(NAME connio-interface-list COMMAND connio-interface-list-test)
+	set_tests_properties(connio-interface-list PROPERTIES TIMEOUT 10)
+
 	find_program(OPENSSL_EXECUTABLE openssl)
 	if(OPENSSL_EXECUTABLE)
 		set(TLS_TEST_KEY "${CMAKE_CURRENT_BINARY_DIR}/tls-peer-test.key")

src/libs/connio/nmap.c

@@ -1788,6 +1788,5 @@ BOOL
 NMAPInitialize(void)
 {
     // single cred for both store and queue atm...
-    MsgGetServerCredential(NMAPLibrary.access);
-    return TRUE;
+    return MsgGetServerCredential(NMAPLibrary.access);
 }

src/libs/connio/tests/interface-list-test.c

@@ -0,0 +1,64 @@
+/****************************************************************************
+ * <Novell-copyright>
+ * Copyright (c) 2001 Novell, Inc. All Rights Reserved.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of version 2 of the GNU General Public License
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, contact Novell, Inc.
+ *
+ * To contact Novell about this file by physical or electronic mail, you
+ * may find current contact information at www.novell.com.
+ * </Novell-copyright>
+ ****************************************************************************/
+
+#include <config.h>
+#include <fcntl.h>
+#include <stdio.h>
+
+#include <xpl.h>
+#include <connio.h>
+
+static int
+CountOpenDescriptors(void)
+{
+	int descriptor;
+	int count = 0;
+
+	for (descriptor = 0; descriptor < 256; descriptor++) {
+		if (fcntl(descriptor, F_GETFD) != -1)
+			count++;
+	}
+	return count;
+}
+
+int
+main(void)
+{
+	int before;
+	int after;
+	int iteration;
+
+	before = CountOpenDescriptors();
+	for (iteration = 0; iteration < 32; iteration++) {
+		if (XplGetInterfaceList() != 0) {
+			fprintf(stderr, "cannot enumerate network interfaces\n");
+			return 1;
+		}
+		XplDestroyInterfaceList();
+	}
+	after = CountOpenDescriptors();
+	if (after != before) {
+		fprintf(stderr, "interface enumeration leaked %d descriptor(s)\n",
+		        after - before);
+		return 1;
+	}
+	return 0;
+}

src/libs/connio/unix-ip.c

@@ -88,6 +88,8 @@ struct ifi_info {
 
 #endif /* __unp_ifi_h */
 
+static void free_ifi_info(struct ifi_info *ifihead);
+
 static struct ifi_info *
 get_ifi_info(int family, int doaliases)
 {
@@ -99,6 +101,9 @@ get_ifi_info(int family, int doaliases)
   struct sockaddr_in *sinptr;
   
   sockfd = socket(AF_INET, SOCK_DGRAM, 0);
+  if (sockfd < 0) {
+    return NULL;
+  }
   
   lastlen = 0;
 #ifdef SIOCGIFNUM
@@ -110,21 +115,34 @@ get_ifi_info(int family, int doaliases)
   }
   len *= sizeof(struct ifreq);
   buf = malloc(len);
+  if (buf == NULL) {
+    close(sockfd);
+    return NULL;
+  }
   ifc.ifc_len = len;
   ifc.ifc_buf = buf;
   if (ioctl(sockfd, SIOCGIFCONF, &ifc) < 0) {
-    perror("getifi_info ioctl error");
+    free(buf);
+    close(sockfd);
+    return NULL;
   }
 #else
   len = 100 * sizeof(struct ifreq); /* initial buffer size guess */
   for ( ; ; ) {
     buf = malloc(len);
+    if (buf == NULL) {
+      close(sockfd);
+      return NULL;
+    }
     memset(buf, 0, len);
     ifc.ifc_len = len;
     ifc.ifc_buf = buf;
     if (ioctl(sockfd, SIOCGIFCONF, &ifc) < 0) {
-      if (errno != EINVAL || lastlen != 0)
-	perror("getifi_info ioctl error");
+      if (errno != EINVAL || lastlen != 0) {
+        free(buf);
+        close(sockfd);
+        return NULL;
+      }
     } else {
       if (ifc.ifc_len == lastlen)
 	break; /* success, len has not changed */
@@ -176,12 +194,20 @@ get_ifi_info(int family, int doaliases)
     }
     memcpy(lastname, ifr->ifr_name, IFNAMSIZ);
     ifrcopy = *ifr;
-    ioctl(sockfd, SIOCGIFFLAGS, &ifrcopy);
+    if (ioctl(sockfd, SIOCGIFFLAGS, &ifrcopy) < 0) {
+      continue;
+    }
     flags = ifrcopy.ifr_flags;
     if ((flags & IFF_UP) == 0) {
       continue; /* ignore if interface not up */
     }
     ifi = malloc(sizeof(struct ifi_info));
+    if (ifi == NULL) {
+      free(buf);
+      close(sockfd);
+      free_ifi_info(ifihead);
+      return NULL;
+    }
     memset(ifi, 0, sizeof(struct ifi_info));
     *ifipnext = ifi; /* prev points to this new one */
     ifipnext = &ifi->ifi_next; /* pointer to next one goes here */
@@ -193,22 +219,33 @@ get_ifi_info(int family, int doaliases)
       sinptr = (struct sockaddr_in *)&ifr->ifr_addr;
       if(ifi->ifi_addr == NULL) {
 	ifi->ifi_addr = calloc(1, sizeof(struct sockaddr_in));
+	if (ifi->ifi_addr == NULL) {
+	  free(buf);
+	  close(sockfd);
+	  free_ifi_info(ifihead);
+	  return NULL;
+	}
 	memcpy(ifi->ifi_addr, sinptr, sizeof(struct sockaddr_in));
 #ifdef SIOCGIFBRDADDR
 	if (flags & IFF_BROADCAST) {
-	  ioctl(sockfd, SIOCGIFBRDADDR, &ifrcopy);
-	  ifi->ifi_brdaddr = malloc(sizeof(struct sockaddr_in));
-	  memset(ifi->ifi_brdaddr, 0, sizeof(struct sockaddr_in));
-	  memcpy(ifi->ifi_brdaddr, sinptr, sizeof(struct sockaddr_in));
-	  memcpy(ifi->ifi_brdaddr, sinptr, sizeof(struct sockaddr_in));
+	  if (ioctl(sockfd, SIOCGIFBRDADDR, &ifrcopy) == 0) {
+	    ifi->ifi_brdaddr = calloc(1, sizeof(struct sockaddr_in));
+	    if (ifi->ifi_brdaddr != NULL) {
+	      memcpy(ifi->ifi_brdaddr, &ifrcopy.ifr_broadaddr,
+	             sizeof(struct sockaddr_in));
+	    }
+	  }
 	}
 #endif
 #ifdef SIOCGIFDSTADDR
 	if(flags & IFF_POINTOPOINT) {
-	  ioctl(sockfd, SIOCGIFDSTADDR, &ifrcopy);
-	  sinptr = (struct sockaddr_in *)&ifrcopy.ifr_dstaddr;
-	  ifi->ifi_dstaddr = calloc(1, sizeof(struct sockaddr_in));
-	  memcpy(ifi->ifi_dstaddr, sinptr, sizeof(struct sockaddr_in));
+	  if (ioctl(sockfd, SIOCGIFDSTADDR, &ifrcopy) == 0) {
+	    sinptr = (struct sockaddr_in *)&ifrcopy.ifr_dstaddr;
+	    ifi->ifi_dstaddr = calloc(1, sizeof(struct sockaddr_in));
+	    if (ifi->ifi_dstaddr != NULL) {
+	      memcpy(ifi->ifi_dstaddr, sinptr, sizeof(struct sockaddr_in));
+	    }
+	  }
 	}
 #endif
       }
@@ -218,6 +255,7 @@ get_ifi_info(int family, int doaliases)
     }
   }
   free (buf);
+  close(sockfd);
   return(ifihead);    
 }
 

src/libs/msgapi/msgapi.c

@@ -733,19 +733,28 @@ MsgGetServerCredential(char *buffer)
 	char credential[4097];
 	char file[120];
 	FILE *credfile;
+	size_t count;
+	BOOL result = FALSE;
 
+	if (!buffer)
+		return FALSE;
 	memset(credential, 0, sizeof(credential));
+	memset(buffer, 0, NMAP_HASH_SIZE);
 
-	sprintf(file, "%s/credential.dat", XPL_DEFAULT_DBF_DIR);
+	if (snprintf(file, sizeof(file), "%s/credential.dat",
+	             XPL_DEFAULT_DBF_DIR) >= (int)sizeof(file))
+		return FALSE;
 	credfile = fopen(file, "rb");
 	if (credfile) {
-		fread(credential, sizeof(unsigned char), sizeof(credential), credfile);
-		fclose(credfile);
-		credfile = NULL;
-		HashCredential(credential, buffer);
-		return TRUE;
+		count = fread(credential, sizeof(unsigned char), sizeof(credential),
+		              credfile);
+		if (count == sizeof(credential) && !ferror(credfile))
+			result = HashCredential(credential, buffer);
+		if (fclose(credfile) != 0)
+			result = FALSE;
 	}
-	return FALSE;
+	memset(credential, 0, sizeof(credential));
+	return result;
 }
 
 EXPORT BOOL

src/libs/python/bongo/configuration/storage.py

@@ -176,7 +176,12 @@ class ConfigurationStore:
                 stdout=subprocess.PIPE,
                 stderr=subprocess.PIPE,
                 check=False,
+                timeout=30,
             )
+        except subprocess.TimeoutExpired as error:
+            raise ConfigurationError(
+                f"configuration helper timed out during {operation} for {name}") \
+                from error
         except OSError as error:
             raise ConfigurationError(
                 f"cannot execute {self.paths.admin}: {error}") from error