From 9a4cbf4bad4ba3d2e8a50f0b9e05384fc4e206ce Mon Sep 17 00:00:00 2001 From: Mario Fetka Date: Sat, 18 Jul 2026 12:37:23 +0200 Subject: [PATCH] Harden Store administration and credential handling --- src/agents/store/stored.c | 6 +- src/apps/admin/admin.c | 39 +++++++++-- src/apps/config/tests/test_storage.py | 10 +++ src/libs/connio/CMakeLists.txt | 13 ++++ src/libs/connio/nmap.c | 3 +- src/libs/connio/tests/interface-list-test.c | 64 +++++++++++++++++++ src/libs/connio/unix-ip.c | 64 +++++++++++++++---- src/libs/msgapi/msgapi.c | 23 +++++-- .../python/bongo/configuration/storage.py | 5 ++ 9 files changed, 197 insertions(+), 30 deletions(-) create mode 100644 src/libs/connio/tests/interface-list-test.c diff --git a/src/agents/store/stored.c b/src/agents/store/stored.c index 54d66c3..cf8926a 100644 --- a/src/agents/store/stored.c +++ b/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()) { diff --git a/src/apps/admin/admin.c b/src/apps/admin/admin.c index cb2fb6a..ee64d9d 100644 --- a/src/apps/admin/admin.c +++ b/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")) diff --git a/src/apps/config/tests/test_storage.py b/src/apps/config/tests/test_storage.py index 3eacb5f..69e25e2 100644 --- a/src/apps/config/tests/test_storage.py +++ b/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") diff --git a/src/libs/connio/CMakeLists.txt b/src/libs/connio/CMakeLists.txt index 9703fc8..e66e8b8 100644 --- a/src/libs/connio/CMakeLists.txt +++ b/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") diff --git a/src/libs/connio/nmap.c b/src/libs/connio/nmap.c index 7ebfcb2..067ac14 100644 --- a/src/libs/connio/nmap.c +++ b/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); } diff --git a/src/libs/connio/tests/interface-list-test.c b/src/libs/connio/tests/interface-list-test.c new file mode 100644 index 0000000..923b13c --- /dev/null +++ b/src/libs/connio/tests/interface-list-test.c @@ -0,0 +1,64 @@ +/**************************************************************************** + * + * 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. + * + ****************************************************************************/ + +#include +#include +#include + +#include +#include + +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; +} diff --git a/src/libs/connio/unix-ip.c b/src/libs/connio/unix-ip.c index 845126d..d343ac5 100644 --- a/src/libs/connio/unix-ip.c +++ b/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); } diff --git a/src/libs/msgapi/msgapi.c b/src/libs/msgapi/msgapi.c index 56aaecc..479c2ed 100644 --- a/src/libs/msgapi/msgapi.c +++ b/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 diff --git a/src/libs/python/bongo/configuration/storage.py b/src/libs/python/bongo/configuration/storage.py index 1f9d8ce..f22d0ca 100644 --- a/src/libs/python/bongo/configuration/storage.py +++ b/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