From bb62ae7b29a20f7c2d4e26b9f5ad330f1bcc3d73 Mon Sep 17 00:00:00 2001 From: Denys Fedoryshchenko Date: Tue, 11 Aug 2026 21:00:46 +0300 Subject: log_pgsql: bound username and sessionid in log header set_hdr() copied the session username and sessionid into the message header chunk with strcpy(). That chunk comes from chunk_pool and holds only LOG_CHUNK_SIZE + 1 bytes, while the username is peer supplied and may be up to 255 bytes long (PAP), so a long username overflowed the chunk and corrupted the heap when log-pgsql was enabled. Use snprintf() to truncate both fields to the space left in the chunk. --- accel-pppd/logs/log_pgsql.c | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) (limited to 'accel-pppd/logs/log_pgsql.c') diff --git a/accel-pppd/logs/log_pgsql.c b/accel-pppd/logs/log_pgsql.c index e4b5a9e9..99be5e64 100644 --- a/accel-pppd/logs/log_pgsql.c +++ b/accel-pppd/logs/log_pgsql.c @@ -12,6 +12,8 @@ #include "memdebug.h" +#define min(x,y) ((x)<(y)?(x):(y)) + static char *conf_conninfo; static int conf_queue_max = 1000; static char *conf_query; @@ -57,20 +59,28 @@ static void unpack_msg(struct log_msg_t *msg) static void set_hdr(struct log_msg_t *msg, struct ap_session *ses) { + const char *username = ses && ses->username ? ses->username : ""; + const char *sessionid = ses && ses->username ? ses->sessionid : ""; struct tm tm; + int pos, len, avail; localtime_r(&msg->timestamp.tv_sec, &tm); strftime(msg->hdr->msg, LOG_CHUNK_SIZE, "%Y-%m-%d %H:%M:%S", &tm); - msg->hdr->len = strlen(msg->hdr->msg) + 1; - if (ses && ses->username) { - strcpy(msg->hdr->msg + msg->hdr->len, ses->username); - msg->hdr->len += strlen(ses->username) + 1; - strcpy(msg->hdr->msg + msg->hdr->len, ses->sessionid); - msg->hdr->len += strlen(ses->sessionid) + 1; - } else - memset(msg->hdr->msg + msg->hdr->len, 0, 2); + pos = strlen(msg->hdr->msg) + 1; + + /* username is peer supplied and may be up to 255 bytes long, + * truncate it to what is left of the chunk, keeping one byte + * for the terminator of the sessionid */ + avail = LOG_CHUNK_SIZE - pos - 1; + len = snprintf(msg->hdr->msg + pos, avail, "%s", username); + pos += min(len, avail - 1) + 1; + + avail = LOG_CHUNK_SIZE - pos; + len = snprintf(msg->hdr->msg + pos, avail, "%s", sessionid); + pos += min(len, avail - 1) + 1; + msg->hdr->len = pos; } static void write_next_msg(void) -- cgit v1.2.3 From 1c69485e1ebd17bf3cc6e8fc4728f9bdb431de94 Mon Sep 17 00:00:00 2001 From: Denys Fedoryshchenko Date: Tue, 11 Aug 2026 21:37:18 +0300 Subject: utils: centralize min macro Several userspace translation units carry identical local min() definitions. Move the guarded definition to utils.h and include it from each user so there is one implementation to maintain. The Linux min() macro lives in kernel-internal headers and is not part of the userspace UAPI. Clang/LLVM does not provide a compatible min macro either: C++ code uses std::min and Clang's similarly named operations use explicit builtin names. The userspace interface, where available, exposes uppercase MIN instead. Keep the #ifndef guard to preserve the behavior of the existing local definitions and avoid redefining a lowercase min macro supplied by an unrelated third-party header. --- accel-pppd/ctrl/pppoe/pppoe.c | 5 +---- accel-pppd/ctrl/sstp/sstp.c | 3 --- accel-pppd/log.c | 5 +---- accel-pppd/logs/log_pgsql.c | 3 +-- accel-pppd/ppp/ppp_lcp.c | 5 +---- accel-pppd/utils.h | 4 ++++ 6 files changed, 8 insertions(+), 17 deletions(-) (limited to 'accel-pppd/logs/log_pgsql.c') diff --git a/accel-pppd/ctrl/pppoe/pppoe.c b/accel-pppd/ctrl/pppoe/pppoe.c index 72faab4e..0e65168d 100644 --- a/accel-pppd/ctrl/pppoe/pppoe.c +++ b/accel-pppd/ctrl/pppoe/pppoe.c @@ -24,6 +24,7 @@ #endif #include "iputils.h" +#include "utils.h" #include "connlimit.h" #include "vlan_mon.h" @@ -33,10 +34,6 @@ #define SID_MAX 65536 -#ifndef min -#define min(x,y) ((x)<(y)?(x):(y)) -#endif - struct pppoe_conn_t { struct list_head entry; struct triton_context_t ctx; diff --git a/accel-pppd/ctrl/sstp/sstp.c b/accel-pppd/ctrl/sstp/sstp.c index 2fd8cb35..72004cf8 100644 --- a/accel-pppd/ctrl/sstp/sstp.c +++ b/accel-pppd/ctrl/sstp/sstp.c @@ -47,9 +47,6 @@ #include "sstp_prot.h" #include "if_ppposeq.h" -#ifndef min -#define min(x,y) ((x) < (y) ? (x) : (y)) -#endif #ifndef max #define max(x,y) ((x) > (y) ? (x) : (y)) #endif diff --git a/accel-pppd/log.c b/accel-pppd/log.c index e4a1e6ed..7fac66df 100644 --- a/accel-pppd/log.c +++ b/accel-pppd/log.c @@ -11,15 +11,12 @@ #include "triton/mempool.h" #include "events.h" #include "ppp.h" +#include "utils.h" #include "log.h" #include "memdebug.h" -#ifndef min -#define min(x,y) ((x)<(y)?(x):(y)) -#endif - #define LOG_MSG 0 #define LOG_ERROR 1 #define LOG_WARN 2 diff --git a/accel-pppd/logs/log_pgsql.c b/accel-pppd/logs/log_pgsql.c index 99be5e64..5b3fd6ae 100644 --- a/accel-pppd/logs/log_pgsql.c +++ b/accel-pppd/logs/log_pgsql.c @@ -9,11 +9,10 @@ #include "log.h" #include "list.h" #include "ap_session.h" +#include "utils.h" #include "memdebug.h" -#define min(x,y) ((x)<(y)?(x):(y)) - static char *conf_conninfo; static int conf_queue_max = 1000; static char *conf_query; diff --git a/accel-pppd/ppp/ppp_lcp.c b/accel-pppd/ppp/ppp_lcp.c index 05b6c4a6..ed085b3b 100644 --- a/accel-pppd/ppp/ppp_lcp.c +++ b/accel-pppd/ppp/ppp_lcp.c @@ -13,13 +13,10 @@ #include "ppp_lcp.h" #include "events.h" #include "iputils.h" +#include "utils.h" #include "memdebug.h" -#ifndef min -#define min(x,y) ((x)<(y)?(x):(y)) -#endif - struct recv_opt_t { struct list_head entry; diff --git a/accel-pppd/utils.h b/accel-pppd/utils.h index aad4025f..63c1db0d 100644 --- a/accel-pppd/utils.h +++ b/accel-pppd/utils.h @@ -4,6 +4,10 @@ #include #include +#ifndef min +#define min(x, y) ((x) < (y) ? (x) : (y)) +#endif + char *u_ip6str(const struct in6_addr *addr, char *buf); char *u_ip4str(const struct in_addr *addr, char *buf); -- cgit v1.2.3 From 428333c9bb283ceb998332481867a59e7cea33b4 Mon Sep 17 00:00:00 2001 From: Denys Fedoryshchenko Date: Wed, 12 Aug 2026 00:40:07 +0300 Subject: log_pgsql: deprecate the module and rename its build flag The PostgreSQL logging target sees little use and has been a source of memory safety bugs. Mark it as deprecated and scheduled for removal, but give anyone depending on it a chance to object first. To make sure this is not missed, the LOG_PGSQL build flag now fails the build with an explanation, building the module requires the renamed LOG_PGSQL_DEPRECATED flag, cmake prints a warning when it is used, and the module logs a deprecation warning on startup. --- CHANGELOG.md | 7 +++++++ README.md | 7 +++++-- accel-pppd/accel-ppp.conf | 2 +- accel-pppd/accel-ppp.conf.5 | 5 +++++ accel-pppd/logs/CMakeLists.txt | 10 +++++++++- accel-pppd/logs/log_pgsql.c | 13 +++++++++++++ contrib/gentoo/net-dialup/accel-ppp/accel-ppp-9999.ebuild | 2 +- 7 files changed, 41 insertions(+), 5 deletions(-) (limited to 'accel-pppd/logs/log_pgsql.c') diff --git a/CHANGELOG.md b/CHANGELOG.md index cbce9ec7..3f6bdeec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,13 @@ ## Unreleased +### Deprecations +- `log_pgsql` is deprecated and scheduled for removal. The `LOG_PGSQL` build + flag now fails the build; build with `LOG_PGSQL_DEPRECATED=TRUE` to keep it + for now, and the module logs a warning on startup. If you depend on it, + please object at https://github.com/accel-ppp/accel-ppp/issues, otherwise it + will be deleted. + ### Features - New `metrics` module: HTTP endpoint exposing the same numbers as `accel-cmd show stat` at `/metrics`, in either Prometheus exposition or JSON format. Configurable listen address and optional IPv4 CIDR allow-list. diff --git a/README.md b/README.md index a78cb145..dd210758 100644 --- a/README.md +++ b/README.md @@ -48,7 +48,7 @@ monitoring kernel modules. Optional features require their corresponding development libraries: * Net-SNMP for NETSNMP=TRUE -* PostgreSQL client libraries for LOG_PGSQL=TRUE +* PostgreSQL client libraries for LOG_PGSQL_DEPRECATED=TRUE * Lua for LUA=TRUE or a specific Lua version such as LUA=5.3 @@ -74,7 +74,10 @@ Useful build options: * RADIUS=FALSE omits RADIUS support. * SHAPER=FALSE omits the traffic-shaping module. * NETSNMP=TRUE builds SNMP support. -* LOG_PGSQL=TRUE builds PostgreSQL logging support. +* LOG_PGSQL_DEPRECATED=TRUE builds PostgreSQL logging support. The module is + deprecated and scheduled for removal, the old LOG_PGSQL flag now fails the + build. If you depend on it, please say so at + https://github.com/accel-ppp/accel-ppp/issues. For example, to build the IPoE and VLAN monitoring modules for the running kernel: diff --git a/accel-pppd/accel-ppp.conf b/accel-pppd/accel-ppp.conf index 56fe0a8c..3fefcf27 100644 --- a/accel-pppd/accel-ppp.conf +++ b/accel-pppd/accel-ppp.conf @@ -2,7 +2,7 @@ log_file #log_syslog #log_tcp -#log_pgsql +#log_pgsql (deprecated, scheduled for removal) connlimit diff --git a/accel-pppd/accel-ppp.conf.5 b/accel-pppd/accel-ppp.conf.5 index bcddfed4..eee33bf0 100644 --- a/accel-pppd/accel-ppp.conf.5 +++ b/accel-pppd/accel-ppp.conf.5 @@ -36,6 +36,7 @@ This is logging target which logs messages over TCP/IP. .TP .BI log_pgsql This is logging target which logs messages to PostgreSQL. +Deprecated and scheduled for removal, see the [log-pgsql] section. .TP .BI pptp .br @@ -1337,6 +1338,10 @@ log all messages including debug messages .SH [log-pgsql] .br Configuration of log_pgsql module. +.br +This module is deprecated and scheduled for removal. It is built only when +accel-ppp is configured with LOG_PGSQL_DEPRECATED=TRUE. If you depend on it, +please object at https://github.com/accel-ppp/accel-ppp/issues. .TP .BI "conninfo=" conninfo Conninfo to connect to PostgreSQL server. diff --git a/accel-pppd/logs/CMakeLists.txt b/accel-pppd/logs/CMakeLists.txt index 4d8a631a..0052e1ae 100644 --- a/accel-pppd/logs/CMakeLists.txt +++ b/accel-pppd/logs/CMakeLists.txt @@ -33,7 +33,15 @@ IF(LOG_SYSLOG) ) ENDIF(LOG_SYSLOG) +# log_pgsql is deprecated and scheduled for removal, the build flag was +# renamed so that nobody keeps building it without noticing IF(LOG_PGSQL) + message(FATAL_ERROR "${Esc}[31mLOG_PGSQL is deprecated.\n The log_pgsql module is scheduled for removal in a future release.\n Build with -DLOG_PGSQL_DEPRECATED=TRUE if you still need it, and please say so at https://github.com/accel-ppp/accel-ppp/issues so that it is kept${Esc}[m") +ENDIF(LOG_PGSQL) + +IF(LOG_PGSQL_DEPRECATED) + message(WARNING "${Esc}[33mBuilding deprecated module log_pgsql, it is scheduled for removal in a future release.\n Please say so at https://github.com/accel-ppp/accel-ppp/issues if you need it kept${Esc}[m") + find_library(PGSQL_LIBRARY pq HINTS ${CMAKE_SOURCE_DIR}/lib/linux/gcc/${BIT}/lib PATHS ${CMAKE_SOURCE_DIR}/lib/linux/gcc/${BIT}/lib @@ -47,5 +55,5 @@ IF(LOG_PGSQL) INSTALL(TARGETS log_pgsql LIBRARY DESTINATION lib${LIB_SUFFIX}/accel-ppp ) -ENDIF(LOG_PGSQL) +ENDIF(LOG_PGSQL_DEPRECATED) diff --git a/accel-pppd/logs/log_pgsql.c b/accel-pppd/logs/log_pgsql.c index 5b3fd6ae..6589da0e 100644 --- a/accel-pppd/logs/log_pgsql.c +++ b/accel-pppd/logs/log_pgsql.c @@ -1,3 +1,13 @@ +/* + * DEPRECATED + * + * This module is scheduled for removal in a future release. It is built + * only when the deprecated LOG_PGSQL_DEPRECATED build flag is given. + * If you depend on it, please object at + * https://github.com/accel-ppp/accel-ppp/issues, otherwise it will be + * deleted. + */ + #include #include #include @@ -293,6 +303,9 @@ static void init(void) { char *opt; + log_warn("log_pgsql: this module is deprecated and is scheduled for removal," + " please object at https://github.com/accel-ppp/accel-ppp/issues if you need it\n"); + spinlock_init(&queue_lock); opt = conf_get_opt("log-pgsql", "conninfo"); diff --git a/contrib/gentoo/net-dialup/accel-ppp/accel-ppp-9999.ebuild b/contrib/gentoo/net-dialup/accel-ppp/accel-ppp-9999.ebuild index 02bf282f..3edee26f 100644 --- a/contrib/gentoo/net-dialup/accel-ppp/accel-ppp-9999.ebuild +++ b/contrib/gentoo/net-dialup/accel-ppp/accel-ppp-9999.ebuild @@ -48,7 +48,7 @@ src_configure() { fi if use postgres; then - mycmakeargs+=( "-DLOG_PGSQL=TRUE" ) + mycmakeargs+=( "-DLOG_PGSQL_DEPRECATED=TRUE" ) fi if use shaper; then -- cgit v1.2.3