| Age | Commit message (Collapse) | Author |
|---|
|
An unparsable bind= value went through inet_addr() unchecked and became
255.255.255.255, so the only symptom was bind() failing with "Cannot
assign requested address", which does not point at the configuration.
Parse with inet_aton() and name the offending value instead.
Also zero the address before filling it in, so the padding passed to
bind() is not stack garbage.
|
|
The option value was the address of the listening descriptor rather than
a boolean, so the effect depended on the descriptor number: it enabled
SO_REUSEADDR only because that number happened to be non-zero, and would
disable it if the daemon were ever started with the lower descriptors
closed. Use a dedicated flag, as cli/telnet.c does.
|
|
load_config() accepts echo-failure=0, but the test for it was
"++echo_sent == conf_echo_failure", which can never match once the
counter has been incremented, so a zero left dead peers undetected
without saying so anywhere. Test the option first and compare with >=,
which keeps the behaviour for every configured value and makes the
disabled case readable, and document it in accel-ppp.conf.5.
|
|
conn->peer_call_id is assigned msg->call_id straight from the wire, so it
holds a network order value, but send_pptp_call_disconnect_notify() then
applies htons() to it. On little-endian hosts the field is swapped twice
and a peer call id of 0x1234 is sent as 0x3412, so the peer cannot match
the notify to its call. Big-endian hosts are unaffected, as both swaps
are no-ops there.
Store the call id in host order, which is what the htons() at the point
of use expects. Nothing else reads the field.
|
|
Both calls were issued on an uninitialised struct sockaddr_in and their
results ignored, so a failure would build the tunnel endpoints, and the
call socket's local call id, out of stack garbage. Fail the call instead.
In pptp_connect() the local address was fetched into the same variable
that held the peer address obtained from accept(), so a failure there
would silently set called-station-id to the calling station. Read it
into its own variable before the connection is set up, and re-arm the
address length before each accept() rather than leaving it at whatever
the previous iteration wrote.
|
|
The PPPoX socket created for the call is only handed to conn->ppp.fd
after the reply has been posted, so returning early on a post_msg()
failure leaks the descriptor: disconnect() knows nothing about it and
establish_ppp() has not run yet. The establish_ppp() failure path just
below already closes it.
|
|
PPTP_CTRL_SIZE() evaluates to 0 for unrecognised control types, so a
message declaring length 0 with such a type passed the length check,
reached process_packet() and was logged as unknown, after which
in_size -= 0 consumed nothing. The stale header stayed at the head of
the buffer and every later byte queued behind it, so the connection
could never make progress: it stalled until in_size reached
PPTP_CTRL_SIZE_MAX, at which point read() was called with a zero-length
buffer, returned 0 and was misreported as "disconnect by peer".
Require the declared length to cover the header, alongside the existing
upper bound.
|
|
post_msg() copies the unsent tail of a message into conn->out_buf and
enables the write handler, but never sets conn->out_size. pptp_write()
then computes out_size - out_pos as 0, writes nothing, sees out_pos ==
out_size, disables itself and returns, so the buffered remainder is
silently dropped. post_msg() still returns 0, so the caller believes the
message was sent.
The usual trigger is a peer that stops reading: once the send buffer
fills, write() returns EAGAIN, n is set to 0 and the whole message is
buffered and then discarded, losing replies such as
Start-Ctrl-Conn-Reply, Outgoing-Call-Reply and Call-Disconnect-Notify.
Record the remaining length so pptp_write() can flush it. out_pos is
already 0 here: post_msg() returns early unless out_size is 0, which
holds only before the first send or after pptp_write() has drained the
buffer and reset both fields.
|
|
|
|
|
|
|
|
|
|
On startup accel-pppd is expected to drop everything a previous instance
left behind in the kernel. For sessions it did so by dumping them with
IPOE_CMD_GET and sending one IPOE_CMD_DELETE per ifindex. That dump was
written for the session backup code removed in 1972a7e5c and was never
adapted to its new, destructive role:
- it was issued on the socket subscribed to the packet multicast group,
so it competed with the notifications that the still attached stale
rx handlers keep generating; on a loaded box the receive buffer
overruns and rtnl_dump_filter() gives up with ENOBUFS,
- its return value was discarded, so such a failure was silent,
- it ran before the interfaces were detached, which is what produces
those notifications in the first place,
- it was skipped entirely when the multicast group could not be
resolved, again with nothing but a warning about packet handling,
- and every session cost a socket, a round trip, a grace period and a
full unregister_netdev().
Whatever it missed stays in the kernel forever: the daemon has no record
of those sessions, so nothing ever deletes them, and every subscriber
later assigned one of their addresses is refused with EEXIST by
IPOE_CMD_MODIFY.
Add IPOE_CMD_FLUSH, which unlinks all sessions in one go, waits for a
single grace period and unregisters the devices with
unregister_netdevice_many(), and use it instead. The old path is kept as
a fallback for a module predating the command, which is reported as
EOPNOTSUPP, and now checks its return value and uses a private socket.
Reorder init() so the interfaces are detached before the multicast group
is joined, and so the flush also runs when only the group lookup failed.
|
|
genl_resolve_mcg() stored the resolved family id only after it had
established that the family advertises multicast groups, so a caller
that also needs the family id was left with nothing whenever the group
lookup failed.
Fill in fam_id as soon as it has been parsed. The return value is
unchanged, so callers interested only in the group are unaffected.
|
|
The request buffers are plain stack variables and only the nlmsghdr
fields and genlmsghdr.cmd were ever assigned, so genlmsghdr.version and
genlmsghdr.reserved reached the kernel holding whatever happened to be
on the stack.
Since 6.1 genetlink validates the reserved header fields of every
command whose id is >= genl_family.resv_start_op, and ipoe sets that
field to CTRL_CMD_GETPOLICY + 1, i.e. 11. IPOE_CMD_DEL_NET is 11, so
ipoe_nl_del_net(), which runs on startup and on every config reload, is
already rejected with EINVAL whenever that garbage is nonzero, and any
command added after it is affected as well.
|
|
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
|
|
|
|
|
