| Age | Commit message (Collapse) | Author |
|---|
|
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.
|
|
drivers/pptp is version 0.8.5 of the PPTP driver, the direct ancestor of mainline drivers/net/ppp/pptp.c. Mainline merged that code in 2.6.37 (2011) from the same author and has maintained it since; this copy received none of the subsequent fixes and is no longer worth carrying:
- It cannot be built. struct flowi's nl_u union, the 3-argument ip_route_output_key(), sock_no_poll, the old ip_select_ident() signature and nf_reset() all disappeared long ago, so nothing past roughly 2.6.36 compiles and -DBUILD_PPTP_DRIVER=TRUE is a build failure everywhere.
- It is not needed. The PPTP bits of accel-pppd/include/if_pppox.h are identical to the mainline UAPI header, so ctrl/pptp's socket(AF_PPPOX, SOCK_STREAM, PX_PROTO_PPTP) reaches the in-kernel module (alias net-pf-24-proto-2) unchanged. The daemon uses no interface the bundled driver added. The deleted drivers/pptp/if_pppox.h was included only by drivers/pptp/pptp.c; all other if_pppox.h includes use either the kernel UAPI header or accel-pppd's userspace copy.
- It is unsafe. Among others: the pskb_may_pull() in pptp_rcv() is commented out, so the GRE header is parsed with no length validation at all; the skb->len - headersize comparison in pptp_rcv_core() underflows and leads to an out-of-bounds read and a negative skb_pull(); pptp_getname() copies uninitialised stack to userspace; and pptp_bind()/pptp_connect() never check sockaddr_len (CVE-2015-8569). Mainline fixed each of these years ago.
Remove the directory along with the BUILD_PPTP_DRIVER option, the accel-pptp-kmod package and the ip_gre conflict warning, which only existed because this module claimed IPPROTO_GRE. accel-pppd/ctrl/pptp and accel-pppd/include/if_pppox.h are unaffected. PPTP now requires the kernel's own pptp module; kernels older than 2.6.37 are no longer supported.
|
|
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.
|
|
|
|
|
|
|
|
|
|
The coverage job fails while gcovr reads the data for triton.c:
Unrecognized GCOV output ... branch 2 taken -1
NegativeHits: Got negative hit value in gcov line 'branch 2 taken -1'
gcov emits those now and then, it is gcc bug 68080 and not something the
source can avoid. gcovr 6 and later treat it as a fatal parse error
unless --gcov-ignore-parse-errors names the case to tolerate.
Passing that option unconditionally is not enough, since the job runs on
both ubuntu-24.04 and ubuntu-22.04 and the gcovr in the latter predates
it and takes no value. Ask gcovr whether it knows the option before
adding it, and print what was decided so the log says which one ran.
|
|
rtnl_link_register() and struct rtnl_link_ops were reaching the driver
through some other header rather than through net/rtnetlink.h, which is
the kind of thing that only shows up when building against a different
kernel. Include it directly.
The workflows insert the kernel modules between two pytest runs, and the
runs that follow are marked 'if: always()' while the insmod steps are
not. A failure in an earlier, unrelated test therefore skips the insmod
but still runs the tests that need the module, which then report a
missing driver instead of the original problem. Mark the insmod steps
'if: always()' as well, so that the later runs test what they are
supposed to.
|
|
Three tests around the interfaces the ipoe module creates per session:
- kill accel-pppd with SIGKILL while a dhcp session is up, start it
again and check that the interfaces left behind by the killed
instance are gone,
- remove a session interface with 'ip link del',
- check that 'ip link add ... type ipoe' is refused, since a device
made through rtnetlink would have none of the private state that
IPOE_CMD_CREATE sets up.
They carry the ipoe_driver marker and sit next to the existing ipoe
tests, so the workflows run them in the steps that follow the insmod of
the module. No workflow change is needed.
The [modules] section has to list connlimit and radius before ipoe:
libipoe.so refers to symbols of both, and with a strict dynamic linker
loading it on its own fails with a relocation error instead of a missing
feature. The tests assert that accel-pppd came up, so that this kind of
misconfiguration is not reported as an unrelated cli connection failure.
Restarting accel-pppd needs no explicit synchronisation:
triton_load_modules() runs every DEFINE_INIT() before triton_run()
starts the threads that serve the cli, so by the time accel-cmd 'show
version' is answered the flush registered at DEFINE_INIT(19) has already
run.
Interfaces are compared by ifindex and not by name, because dhclient may
get a new session in the meantime and the fresh interface would reuse
the ipoe0 name. The check that nothing removes the interfaces while no
accel-pppd is running is only printed, not asserted, so that the module
is free to start doing it on its own later on.
|
|
Sessions left in the kernel by a dead daemon could only be got rid of by
restarting accel-pppd, which flushes all of them and so drops everyone
still online, or by unloading the module. Neither is of much use when a
single stale interface is holding an address and every subscriber that
is later handed it is refused with EEXIST.
Register rtnl_link_ops, so that RTM_DELLINK reaches the driver:
ip link show type ipoe enumerate them
ip -d link show ipoe42 reports kind "ipoe"
ip link del ipoe42 remove a single one
newlink returns EOPNOTSUPP rather than being left out. A session carries
private state that IPOE_CMD_CREATE sets up, and a kernel that falls back
to register_netdevice() when newlink is NULL would hand out a device
with zeroed private state and no percpu counters, which oopses on the
first packet or on ip -s link show.
dellink runs under rtnl and outside genl_mutex, and is therefore the
first writer in this driver that does not serialise against the genl
handlers: it can unlink a session that IPOE_CMD_DELETE is about to
unlink as well, since the device stays registered until both are done.
Add ipoe_session.dying, set under ipoe_wlock by whichever path starts
the teardown, and let the other one bail out.
That leaves rtnl nested inside ipoe_wlock, which is safe because no path
does it the other way round: ipoe_create() releases rtnl before taking
ipoe_wlock, ipoe_nl_cmd_delete() releases ipoe_wlock before calling
unregister_netdev(), and the interface commands use rtnl alone. dellink
does have to sleep in synchronize_rcu() and while draining the session
refcount with rtnl held, which the genl path avoids by deferring both
until after it has dropped everything.
A session left behind by a dead daemon is indistinguishable from one
that is still in use - same peer address, same lists, same flags - so
the removal cannot be refused on state alone. Warn instead, and only
when somebody is still subscribed to the packet multicast group, so that
clearing stale interfaces on a box where accel-pppd is not running stays
quiet.
|
|
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.
|
|
ipoe_nl_cmd_dump_sessions() increments idx before calling fill_info(),
so once fill_info() fails because the skb is full, idx already points
past the session that did not fit. cb->args[0] is set to that value and
the next round resumes one entry too far, dropping the session from the
dump entirely - one lost session per message boundary, roughly one in
every 90 at the current record size.
Step idx back before leaving the loop.
|
|
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.
|
|
ipoe_nl_cmd_delete() drops ipoe_wlock before sleeping in
synchronize_rcu() and taking rtnl via unregister_netdev(), then falls
through into the out_unlock label and releases it a second time. Every
successful session delete therefore increments the semaphore count by
one.
Since the count only ever grows, ipoe_wlock stops providing mutual
exclusion after a handful of teardowns: with the count at N, up to N+1
writers may hold it simultaneously. This is currently masked because
ipoe_nl_family does not set parallel_ops, so genetlink serialises every
.doit/.dumpit under genl_mutex and no two writers can overlap in
practice. It turns into a real race on the session hash lists as soon as
that changes, or as soon as a writer is introduced outside the genl
handlers.
It also silently defeats the barrier in ipoe_fini(): the down()/up()
pair meant to wait for an in-flight writer is satisfied immediately by
the leaked count and waits for nothing.
Return directly after unregister_netdev() so the success path releases
the lock exactly once. The early up() is deliberate and stays where it
is - holding a sleeping semaphore across unregister_netdev() would nest
ipoe_wlock inside rtnl, while ipoe_create() takes rtnl first and
ipoe_wlock afterwards. Error paths, return values and lock ordering are
unchanged.
|
|
sstp: add ppposeq transport to avoid userspace HDLC framing
|
|
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.
|
|
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.
Also fix 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
|
|
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
|
|
sstp: express escape buffer bound as one invariant
|
|
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
|
|
Fix ipv6 ifid and UB shift
|
|
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)
|
