| Age | Commit message (Collapse) | Author |
|---|
|
metrics: expose session details in JSON output
|
|
A peer can send an SSTP packet with a zero encoded length and an unknown packet type. The receive dispatcher accepts the unknown type, after which buf_pull() consumes no data and the handler loops forever on the same packet, monopolizing a Triton worker.
Reject all packet lengths smaller than the SSTP header before dispatch so malformed packets close the connection without entering the non-progressing loop.
|
|
A pty is a byte stream, so the tty flip buffer merges frames written
back to back and sstp has to re-delimit them with async HDLC escaping
and a CRC-16 FCS. On a 1452-byte payload that is ~3600 ns per frame,
most of it spent on the FCS.
PPPOSEQ is a pppox protocol whose socket is the ppp endpoint itself,
so one datagram is one frame and no framing is needed at all. The
same payload takes ~380 ns per frame, about 9 times less. Requires
kernel 2.6.37, the first with PX_MAX_PROTO 3, whose remaining slot
it claims.
Supported kernels are from 2.6.37 to 7.2.
The new ppp-mode option selects the transport; auto, the default,
falls back to async when the module is unavailable, so hosts with
prebuilt kernels are unaffected.
PPP_SYNC is removed, being disabled and unfixable over a pty: frame
boundaries cannot be recovered from the stream, and coalescing cannot
be prevented since frames arrive from the network stack.
|
|
fixes http client warnings (curl):
< HTTP/1.1 404 Not Found
< Date: Sun, 02 Aug 2026 14:05:21 GMT
* no chunk, no close, no size. Assume close to signal end
|
|
Drain out_queue to the stream in sstp_disconnect before closing, so a
queued response is sent before the connection is torn down. Best-effort,
non-blocking, via a sstp_flush() helper that mirrors sstp_write.
Fixes: 635ab1b7
|
|
Reverts 635ab1b7, e7a03684, 382b02b6, 4fbba471 on
accel-pppd/ctrl/sstp/sstp.c:
- http_send_response: sstp_send(buf) || sstp_write(&hnd) -> sstp_send(buf)
- http_handler: drop the r/return 1 path
- sstp_read: drop else if (n > 0) return 1
- remove the sstp_write forward decl
|
|
Add an opt-in sessions setting for the JSON metrics renderer. Include session identity, addressing, protocol state, interface context, uptime, and traffic counters while keeping Prometheus output aggregate-only.
The session list is walked with ses_lock held, so report the accounting counters the session last sampled rather than calling ap_session_read_stats(): that issues a synchronous netlink round trip per session, which would stall session setup and teardown for the duration of a scrape, it writes back into the session while only the read lock is held, and it needs the thread local net of the session's namespace, which the metrics context does not have. Counter freshness therefore follows accounting, which the documentation spells out.
Escape malformed UTF-8 in peer supplied strings so a single bad username cannot make the whole document undecodable, and reserve room for the response header in front of the rendered body so a body that can be megabytes is not copied a second time.
Document the privacy-sensitive option in both accel-ppp.conf and the man page, and cover the empty session list, the aggregate-only Prometheus output and the response framing in the metrics integration test.
|
|
(size + PPP_FCSLEN) * 2 + 2 equals 8b781b94's size*2 + 2 + PPP_FCSLEN*2
but can't collapse back to the 1801847a under-allocating form.
|
|
Docs/readme markdown, configs updates, man updates
|
|
Radius leak fixes
|
|
ipoe: fix two memory leaks (relay reply packet, username string)
|
|
ppp: don't terminate session on IPV6CP TermReq unless IPv6 is required
|
|
ppp: close unit fd only after session cleanup finishes
|
|
ipv6: fix NULL deref and OOB read in dnssl/AFTR-Name config parsing
|
|
triton: reject concurrent config reload requests
|
|
cli: proper ipv6 support for cli interface
|
|
For prefix lengths 65..127 the mask for the host bits was built with
(1 << (128 - prefix_len)) - 1 using a plain int literal, which is
undefined behavior for shift counts of 31 and above, i.e. for any
prefix length from 65 to 97. Use a 64-bit constant for the shift.
|
|
The default fixed interface-ids conf_intf_id_val=1 and
conf_peer_intf_id_val=2 were plain host-order integers, while every
consumer (build_ip6_addr(), ifcfg.c, nd.c, dhcpv6.c) treats intf_id as
an opaque 8-byte value in network byte order and copies it verbatim
into the low 64 bits of the IPv6 address. parse_intfid() also produces
network byte order, so only the built-in defaults were affected.
On little-endian hosts this produced fe80::100:0:0:0 (and ::200:0:0:0
for the peer) instead of the intended fe80::1 / ::2 whenever
ipv6-intf-id / ipv6-peer-intf-id were not set in the config.
Store the defaults with htobe64() so the resulting addresses are ::1
and ::2 regardless of host endianness. The assignment is done in
init() because htobe64() is not a constant expression on all libcs.
Note: on little-endian deployments this changes the server link-local
address from fe80::100:0:0:0 to fe80::1 when ipv6-intf-id is unset.
|
|
|
|
|
|
|
|
destablish_ppp() closed the ppp unit fd (via
triton_md_unregister_handler(..., 1)) before calling
ap_session_finished(). Closing the fd releases the ppp unit index, so
the kernel can assign the same unit (and thus the same pppX ifname) to
a new session while the old session's cleanup is still running.
pppd_compat performs its cleanup from the EV_SES_FINISHED handler: it
runs the ip-down script (blocking the context until the script exits)
and then deletes radattr.pppX. If the unit index is reused in that
window, the ip-down script is executed with an IFNAME that now belongs
to a different, active session, and remove_radattr() deletes the
radattr file of that new session. External accounting/shaper scripts
that read radattr.pppX then fail for a live session.
Fix this by unregistering the unit fd handler without closing the fd
and closing it only after ap_session_finished() returns, unless the fd
was handed to the unit cache. This keeps the unit index reserved until
session cleanup has finished, so the ifname cannot be reused early.
The unit-cache path is not affected (the cached fd already keeps the
unit reserved); the race only hits configurations with unit-cache
disabled. Only PPP sessions (PPPoE/PPTP/L2TP/SSTP) go through this
path; IPoE is unaffected. Note the unit index is now held slightly
longer during teardown (while ip-down runs) - this is intentional.
Signed-off-by: Denys Fedoryshchenko <denys.f@collabora.com>
|
|
triton_conf_reload() kept the notify callback in a single global slot,
and the CLI reload command likewise stored its wakeup context in a
global. A second reload issued while the first was still pending (from
another CLI connection or SIGUSR1) overwrote both, so only the last
requester was notified when the reload completed; the earlier CLI
context slept forever in triton_context_schedule() and that connection
hung while the rest of the daemon kept running.
Make triton_conf_reload() return -1 when a reload is already pending,
checked atomically under threads_lock, and pass a caller-provided arg
through to the notify callback so each requester keeps its own state
instead of sharing globals. The CLI reload's request struct is
heap-allocated rather than kept on reload_exec's stack, since
triton_context_schedule() can migrate a suspended context onto a
different worker thread's stack before the notify callback runs,
which would otherwise leave conf_reload_notify() writing through a
stale stack pointer.
Also mark the reload as running (need_config_reload = 2) before
dropping threads_lock to call __config_reload(). Previously a worker
woken during the reload (e.g. by the notify callback waking the CLI
context, or any stray context wakeup) could loop through the idle
path, decrement the active count back to zero and re-enter
__config_reload() while need_config_reload was still set, running a
second concurrent conf_reload() and invoking the notify callback
twice - corrupting the wakeup list and, with the CLI's heap request,
writing through freed memory.
The SIGUSR1 handler used to call triton_conf_reload() directly from
signal context, taking spinlocks and potentially running the whole
config parse inside the handler. It now only sets a flag; the main
thread waits with sigtimedwait() and performs the reload (and logs a
warning when one is already in progress) from normal thread context.
The CLI now replies "reload is already in progress" instead of losing
the first requester's wakeup.
|
|
add_dnssl() in nd.c and dhcpv6.c, and its copy add_aftr_gw() in
dhcpv6.c, call strlen(val) before the "if (!val)" guard, so a dnssl
option without a value crashes on config load before the check is
ever reached (also reported by cppcheck: "Either the condition '!val'
is redundant or there is possible null pointer dereference").
Moving strlen() after the guard is not enough: an empty value such as
a bare "dnssl=" or "aftr-gw=" passes the NULL check with n == 0 and
the following "val[n - 1]" reads one byte before the string.
Reject both NULL and empty values before taking the length.
Note these functions are only reached from the config parser (the
[ipv6-dns] section and the ipv6-dhcp "aftr-gw" option) at startup or
on config reload; nothing from received packets flows into them. So
this is a robustness fix for invalid/malformed configuration files
(local DoS at worst), not a remotely triggerable issue.
The NULL-check ordering in add_dnssl() was originally fixed by
[anp/hsw] in PR #13; this extends it to empty values and to the same
pattern in add_aftr_gw().
Co-authored-by: [anp/hsw] <sysop@880.ru>
|
|
The ipoe-level ses->username always holds an allocated string
(_strdup of ifname/calling-station-id, u_inet_ntoa buffer or lua
result), but ipoe_session_free() never released it. Ownership is
normally transferred in auth_result() via ap_session_set_username(),
so the string was leaked whenever a session died before auth_result()
ran: termination while starting, PWDB_WAIT never completing, or
ipoe_create_interface() failure.
The create-interface failure path also leaked the freshly allocated
local copy outright, since it returned before the string was stored
anywhere.
Store the string in ses->username as soon as it is obtained and free
it in ipoe_session_free(). auth_result() clears ses->username before
handing ownership to ap_session_set_username(), so no double free is
possible.
Reported-by: Louis Scalbert (#101)
|
|
dhcpv4_relay_read() takes a reference on the reply packet for every
registered listener context and hands it over via triton_context_call().
The receiving ipoe_ses_recv_dhcpv4_relay() consumes that reference by
storing the packet in ses->dhcpv4_relay_reply, but the early-return
branch taken when the original request is already gone dropped the
reference without freeing the packet.
This leaks one packet every time a relay reply races with the request
being released, which happens regularly on busy relay-mode deployments.
Reported-by: Louis Scalbert (#101)
|
|
Some broken CPE routers (e.g. Phicomm KE2M, older Linksys) negotiate
IPV6CP successfully but then fail to configure IPv6 locally and send
IPV6CP TermReq while intending to keep using the session for IPv4.
accel-ppp currently terminates the whole session on any IPV6CP TermReq,
which violates the RFC 1661 section 3.7 implementation note:
"the fact that one NCP has Closed is not sufficient reason to cause
the termination of the PPP link, even if that NCP was the only NCP
currently in the Opened state."
Terminate the session only when ipv6=require. Otherwise mark the layer
passive so session bring-up can proceed if IPV6CP never started.
The TermReq handler change alone is not enough for these routers: they
send TermReq *after* IPV6CP has opened, so ipv6cp->started is already
set and ipv6cp_layer_finished() (reached via TermAck or the restart
timer once the FSM winds down through Stopping->Stopped) would still
kill the session through its started branch. Apply the same policy
there: only terminate if ipv6=require, otherwise log and keep the
session running IPv4-only.
In require mode the TermReq path still records TERM_USER_REQUEST as
before (and sets ses->terminating, so layer_finished doesn't override
the cause); TERM_USER_ERROR in layer_finished now only covers genuine
negotiation failures.
Based on the fix proposed by Marek Michalkiewicz and reworked by
Alarig Le Lay.
Closes: https://github.com/accel-ppp/accel-ppp/issues/57
Supersedes: https://github.com/accel-ppp/accel-ppp/pull/298
Signed-off-by: Denys Fedoryshchenko <denys.f@collabora.com>
|
|
req_wakeup() ignored the return value of req->send(req, 1). When a
request dequeued from the req-limit queue failed at socket setup
(__rad_req_send returns -2, e.g. under ephemeral port exhaustion or a
routing error), the server's req_cnt slot taken in
rad_server_req_exit() was never released and the request was orphaned
with no callback and no timer.
Leaked slots accumulate until req_cnt permanently saturates req-limit,
after which every request queues forever and the server is effectively
dead until restart.
Handle -2 the same way rad_server_req_enter() does: release the slot,
mark the server failed and drive the request through the regular
failover path so it either retries on another server or reports the
failure to its owner.
Signed-off-by: Denys Fedoryshchenko <denys.f@collabora.com>
|
|
When an accounting Stop request cannot be retransmitted because no
server is available (single server inside its fail-timeout window,
server removed on config reload, or a transient socket/connect error
that marks the server failed), rad_acct_stop_timeout() reset req->try
and returned. The retransmit timer is one-shot (no period), so it
never fired again: the request leaked forever together with its open
UDP socket, epoll registration and timerfd.
The same dead end existed in rad_acct_stop_sent(): a deferred Stop
request (req->rpd == NULL) whose queued send was cancelled by
rad_server_fail() fell through the failure branch without freeing the
request or scheduling a retry.
With the RADIUS client bound to a source address (bind=/nas-ip-address)
every leaked socket pins one ephemeral port. On a busy NAS each session
terminating during a short RADIUS outage leaks one socket; after months
of uptime the ephemeral port range is exhausted and every new request
fails with "radius:bind: Address already in use" followed by
"no available servers", requiring a restart.
Fix by re-arming the one-shot timer on send failure instead of
resetting the try counter, so retries are bounded by max-try and the
request is freed cleanly once attempts are exhausted.
Both defects date back to the accounting rewrite (62e89248, 2014).
Fixes: https://github.com/accel-ppp/accel-ppp/issues/324
Signed-off-by: Denys Fedoryshchenko <denys.f@collabora.com>
|
|
When a mac-filter file line contained an octet > 255, the error path freed
the entry but kept writing to it and linked it into mac_list. Validate all
octets before allocating so invalid lines are skipped entirely.
This is not considered a security vulnerability: the mac-filter file can
only be configured by an administrator with access to the daemon config,
or the CLI, both of which require privileged access.
Fixes #307
Signed-off-by: Denys Fedoryshchenko <denys.f@collabora.com>
|
|
We had several flaws on server side that was breaking ipv6 cli support.
accel-pppd/cli/cli_p.h — three shared helpers used by both listeners: cli_parse_hostport() (accepts host:port, [ipv6]:port, bare ::1:port via last-colon split, and :port for wildcard), cli_bind_addr()
(builds an IPv4 or IPv6 sockaddr, modeled on the existing SSTP pattern), and cli_addr_str() (formats peer addresses for logging, including v4-mapped).
accel-pppd/cli/tcp.c and telnet.c — listeners now create a socket of whichever family the configured address is; client address storage switched from sockaddr_in to sockaddr_storage so connection logging prints IPv6 peers correctly.
An invalid address now logs an error instead of silently binding garbage (previously inet_addr() failure went unchecked). Also fixed a pre-existing quirk where the fd itself was passed as the SO_REUSEADDR option value.
accel-pppd/accel-ppp.conf.5 — documented the new [::1]:2000 syntax and [::]:port for the IPv6 wildcard.
Might fix: https://github.com/accel-ppp/accel-ppp/issues/317
Signed-off-by: Denys Fedoryshchenko <denys.f@collabora.com>
|
|
Stability fixes
|
|
radius: update server secret on config reload
|
|
log_file: don't register general target when log-file is unset
|
|
Several bugfixes for problems reported by Khodor Tahech
|
|
|
|
|
|
|
|
|
|
|
|
|
|
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
|
On EV_CONFIG_RELOAD, __add_server() re-parses each server line into a
fresh rad_server_t and, when it matches an existing server by
addr/auth_port/acct_port, copied over only the timeout/limit/bind
fields before freeing the new struct. The freshly parsed secret was
discarded, so editing a shared secret and reloading had no effect until
a full restart. The strdup'd secret on the freed struct was also leaked
on every matched reload.
Adopt the new secret into the existing server (freeing the old one) so
secret changes take effect on reload. New requests read
req->serv->secret directly, so they pick up the update immediately.
This covers both the modern "server=" path and the legacy
auth-server/acct-server path, which both funnel through __add_server().
|
|
The general log target was registered unconditionally in init(), even
when no log-file= option was configured. The backing log_file pointer is
only allocated when the option is present, so the first general-routed
log message dereferenced NULL in queue_log() (spin_lock(&NULL->lock)),
crashing the daemon. This made commenting out log-file= a foot-gun.
Guard the registration on log_file being allocated, mirroring how the
fail/per-user/per-session targets are already conditional. Also bail out
of general_reopen() early when log-file is unset, so a SIGHUP after a
config reload that dropped log-file can't call open(NULL, ...).
|
|
Replace the pre-generated free-list (one heap node per allocatable
address/prefix) with a per-pool bitmap. Each pool holds a list of
contiguous ranges, each range owning one bitmap (1 bit per unit);
a lease is a small per-session malloc wrapper around the ipdb item,
so pool memory is no longer shared or mutated by sessions.
Memory now scales with capacity at ~1 bit/unit instead of ~96B (v4) /
~128B (v6) per unit; startup, `show ippool`, and backup-restore are
O(1) instead of O(N)/O(N*M). Oversized IPv6 ranges (prefix_len-mask
> 24) are rejected at parse time instead of OOMing in the malloc loop.
The ipdb_t vtable, the owner-based put dispatch, ipdb.h structs, and
struct ap_session are unchanged; RADIUS and chap-secrets backends are
untouched (reconcile filters by owner).
Preserved behavior: p2p/net30 allocators (via a step/gw_offset/
peer_offset geometry triple), shuffle (randomized scan start), named
pools, next-chains, gw-ip-address, RADIUS pool-name attrs, and the
USE_BACKUP save/restore path. `gw=` is now accepted-and-ignored (its
per-address local gateway was already overridden at allocation time).
New: online reconfiguration. An EV_CONFIG_RELOAD handler rebuilds the
pool set and reconciles live sessions (sessions are the source of
truth, the bitmap is rebuilt from them) under
pool_set_rwlock(write) -> ses_lock(read) -> per-pool spinlock.
Sessions whose address left the pools are handled per a new
`reload-orphan = keep|disconnect` knob (default keep); foreign
in-range addresses are reserved to avoid duplicate assignment.
Adds extra/bitpool.h (shared bit-array helpers) and a standalone
extra/bitpool_test.c (not wired into cmake) covering the bitmap and
the v4/v6 address<->bit math, cross-checked against an __int128
reference.
Signed-off-by: Denys Fedoryshchenko <denys.f@collabora.com>
|
|
Signed-off-by: Denys Fedoryshchenko <denys.f@collabora.com>
|
|
The unsupported-PPPoE-type branch in disc_read() logs a warning but
falls through to forward() instead of dropping the packet. Compare
with the unsupported-version branch immediately above, which has the
same shape but continues:
if (hdr->ver != 1) {
if (conf_verbose)
log_warn(...unsupported version...);
continue;
}
if (hdr->type != 1) {
if (conf_verbose)
log_warn(...unsupported type...);
/* falls through into forward() */
}
Add the missing continue so malformed packets are dropped instead of
processed.
Signed-off-by: khedor <khedor@gmail.com>
|
|
free_net() compacts the nets[] array by sliding entries left after
removing one:
memcpy(nets + i, nets + i + 1, net_cnt - i - 1);
Two bugs:
1. The count is a raw element count, not a byte count. nets[] holds
'struct disc_net *' pointers, so only (net_cnt - i - 1) bytes are
moved instead of (net_cnt - i - 1) * sizeof(nets[0]). On the usual
8-byte-pointer build, 7 of every 8 surviving pointers are lost,
leaving uninitialised holes in the array.
2. Source and destination overlap (nets + i and nets + i + 1), so
memcpy is undefined behaviour. The correct primitive is memmove.
Switch to memmove and multiply the count by sizeof(nets[0]).
Signed-off-by: khedor <khedor@gmail.com>
|
|
In init_net(), the buffer for struct disc_net was sized as
n = _malloc(sizeof(*net) + (HASH_BITS + 1) * sizeof(struct tree));
but 'net' is the const struct ap_net * argument, not the disc_net being
allocated. sizeof(*net) is therefore sizeof(struct ap_net), which is
substantially smaller than sizeof(struct disc_net). Every field of *n
written past the ap_net-sized prefix (ctx, hnd, net, refs, etc.) lands
in unallocated heap memory.
Use sizeof(*n) so the allocation matches the actual destination type.
Signed-off-by: khedor <khedor@gmail.com>
|
|
If a RADIUS dictionary contains a line consisting of exactly one
word with no trailing spaces (for example, standard keywords like
"END-VENDOR\n" or "END-TLV\n" ), this bug getting triggered.
Triggering crash is compiler dependent, it might not happen now,
but a bit different compiler, flags, and it might crash on load.
Reported-by: Khedor <khedor@gmail.com>
Signed-off-by: Denys Fedoryshchenko <denys.f@collabora.com>
|
