| Age | Commit message (Collapse) | Author |
|---|
|
Additional safeguards
|
|
Closes #354
|
|
Require exactly one PADR Service-Name, drop Echo-Requests exceeding the
negotiated MTU, and clear pooled payloads before reuse. Retain upstream's
silent malformed-PADR rejection and received-packet length checks.
Adapted from Ritika Chopra's accel-ppp-ng PR #40, T8464/T8830.
Co-authored-by: Ritika Chopra <r.chopra@vyos.io>
|
|
|
|
Read peer-controlled PPPoE tag and TR-101 integer fields through aligned temporaries, validate PPP-Max-Payload before formatting, and avoid unaligned cookie timestamp accesses.
|
|
Validate known options before extracting fields, bound Relay-Agent and classless-route subformats, and replace unaligned DHCP option accesses in notification, relay, and session paths.
|
|
Various fixes on compiler warnings
|
|
Ipoe stale session flush fixes
|
|
PPTP fixes
|
|
pptp: drop the out-of-tree kernel driver
|
|
Stability fixes and function unification
|
|
c912d090 ("crypto: Removed internal tomcat crypto.") deleted the
crypto/ tree but left accel-pppd/include/crypto.h behind, a tracked
symlink to ../../crypto/crypto.h which has pointed at nothing since.
backup_file.c still includes it, so it fails to compile with
fatal error: crypto.h: No such file or directory
That goes unnoticed because accel-pppd/CMakeLists.txt has
ADD_SUBDIRECTORY(backup) commented out, i.e. backup_file.c is not part
of any build; the breakage only shows up for whoever re-enables it.
Include <openssl/md5.h> instead, which is what the file actually needs
(MD5_CTX and friends) and what c912d090 did for every file it touched.
sstp.c and radius/packet.c defer to crypto.h for the rationale behind
their OPENSSL_API_COMPAT define. Spell it out locally instead, and point
at the project wide ADD_DEFINITIONS() in the top level CMakeLists.txt
added by c1689506 ("openssl: suppress deprecated API warnings"), noting
why the local define is kept despite being redundant with it: it has to
be visible before the first OpenSSL header.
Then remove the symlink, which nothing references anymore.
|
|
decode_avp() only checked the 2 bytes length prefix of the Hidden AVP
Subformat when the ciphered attribute spanned more than one MD5 block.
For an attribute of at most 16 bytes it deciphered the single block and
returned straight away, leaving l2tp_recv() to take the prefix at face
value:
orig_avp_len = ntohs(*(uint16_t *)avp->val) + sizeof(*avp);
...
attr->length = orig_avp_len - sizeof(*avp);
That prefix is an output of the cipher, so a peer which does not know
the secret (i.e. anyone able to reach the L2TP socket, no handshake
needed beyond a preceding Random-Vector AVP) turns it into 16 random
bits. Worse than an over-read: orig_avp_len is a uint16_t, so a prefix
of 0xffff wraps to 5, attr->length becomes -1, and the ATTR_TYPE_STRING
and ATTR_TYPE_OCTETS cases then run
attr->val.string = _malloc(attr->length + 1); /* _malloc(0) */
memcpy(attr->val.string, orig_avp_val, attr->length); /* SIZE_MAX */
that is an unbounded memcpy() into a zero sized allocation. Remotely
triggerable heap corruption on any tunnel with a secret configured. The
accel-ppp encoder always pads hidden AVPs by at least 16 bytes, so it
never produces an attribute short enough to reach this path; only a
crafted packet does.
Fix it at the source rather than at the call site: decode_avp() now
returns the deciphered length through an output parameter, and the
length is bounded against the room actually available in the received
AVP on every path out of the function, single block included. Callers
can no longer re-derive it from the AVP body and get it wrong, and the
existing multi-block check keeps guarding the deciphering loop itself.
Add packet_test.c, a standalone test which drives the real parser
through a real UDP socket: hand-crafted packets cover the length prefix
checks (including the boundaries of what fits and the single block path
the encoder cannot produce), and l2tp_packet_send()/l2tp_recv() round
trips cover the multi-block cipher and the unaligned AVP accessors. It
reproduces the corruption above under ASan on unpatched code. Wire it,
and the so far unused bitpool_test.c, into the ASAN/UBSAN workflow.
Inspired by the equivalent hardening in accel-ppp-ng (commit a8ca0f3f),
which bounds the length at the call site; the fix here is placed inside
decode_avp() and covered by a regression test.
|
|
AVPs are packed back to back in the receive buffer, so the offset of any
given AVP is the sum of the lengths of all the AVPs before it, i.e. a
value the peer picks. Casting avp->val to uint16_t/uint32_t/uint64_t
therefore dereferences a pointer with an arbitrary alignment, which is
undefined behaviour, is caught by -fsanitize=alignment, and is only
harmless on x86 by accident. The same applies to the send path, where
the AVPs being built are laid out the same way.
Introduce unaligned_{ntohs,ntohl,be64toh}() and their store
counterparts, all memcpy() based, and use them for every multi-byte
field read out of or written into an AVP body. Accesses to the members
of struct l2tp_avp_t itself are fine as it is declared packed.
memxor() had the same problem, in a worse form: it cast both of its
uint8_t pointers to uintmax_t and walked them word by word. Since it is
only ever called on MD5 sized chunks, replace it with a plain byte loop
which compilers vectorize just as well, and which no longer breaks
strict aliasing either.
No functional change intended, this is a portability and UB fix; it also
removes a source of noise for the s390x (big endian) CI job.
|
|
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 <sys/param.h> 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.
|
|
The PADI tag loop read tag_len before checking that the tag header itself
fits into the declared payload length, so a PADI ending with a partial tag
made it read up to 2 bytes past the receive buffer. print_packet() and the
PADR loop already have this check, add the missing one.
|
|
add_tag2() checked that the tag payload fits into the packet buffer, but
the memcpy() copied the 4 byte tag header as well, so the check was short
by sizeof(struct pppoe_tag) - 1 bytes.
All callers build the packet in a ETHER_MAX_LEN stack buffer and pass tags
taken from the received discovery packet. A PADI carrying a Host-Uniq tag
of 1454..1456 bytes therefore made pppoe_send_PADO() write up to 3 bytes
of peer supplied data past the end of the buffer. The PADS, PADT and error
paths are affected in the same way.
Include the tag header in the bound, and drop the tag_len < 0 test which
can never be true since ntohs() returns an unsigned value.
|
|
The netmask was built by shifting ~0, which is a signed int holding -1,
and left shifting a negative value is undefined in C. Every compiler we
build with produces the expected mask, so this is not a behaviour fix,
but it trips -fsanitize=shift and relies on latitude the standard does
not grant. Shift an unsigned operand instead.
The prefix length itself was not validated properly either. strtoul()
accepts a leading minus and negates, so 'local-net=10.0.0.0/-1' yields
ULONG_MAX, which truncates to -1 in the int and passes the 'mask > 32'
test. The shift count then becomes 33, which is out of range whether the
operand is signed or unsigned. endptr was set but never looked at, so
trailing garbage was silently ignored and '/abc' quietly became a /0.
Keep the parsed value unsigned, reject anything that is not a complete
number in 0..32, and only then narrow it.
|
|
The destination was always read as a 32 bit word regardless of how many
significant octets the prefix length implies, and the gateway was read
without checking that four bytes remain in the option. Neither read was
bounded by the end of the option, so a client could make the decoder run
past it. dhcpv4_check_options() only enforces a minimum length of 5 for
option 121, which is short of the 9 bytes a /32 route needs.
Reading the destination as a word was also wrong for any prefix shorter
than /32, as it pulled in the first octets of the gateway: 10.0.1.0/24
via 1.1.1.1 printed as 10.0.0.1/24.
Read only the significant octets, check the remaining length before both
reads, and copy the gateway rather than dereferencing a possibly
unaligned pointer.
|
|
The netmask was built with a loop shifting 1 into place, which was wrong
in three ways. At i == 0 it evaluated 1 << 32, undefined for a 32 bit
int, and at i == 1 it shifted into the sign bit of a signed value. The
shift amount was off by one, so a /24 produced 0xfffffe00 rather than
0xffffff00. And mask1 was initialized once before the loop over the
routes and only ever OR'ed into, so the mask of every route accumulated
into the routes that followed it.
A well formed option carrying 10.0.1.0/24 via 1.1.1.1 and 172.16.0.0/12
via 2.2.2.2 printed as 10.0.0.1/24 and 172.16.2.0/12.
Compute the mask directly instead, and reject a prefix length above 32
rather than shifting by a negative amount.
|
|
Linux 2.6.37 and later provide the PPTP socket address and protocol definitions in linux/if_pppox.h. Use that header directly instead of carrying an old private copy containing obsolete kernel-internal declarations.
|
|
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.
|
|
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.
|
|
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
|
|
(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.
|
|
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)
|
|
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>
|
|
Stability fixes
|
|
Several bugfixes for problems reported by Khodor Tahech
|
|
|
|
|
|
|
|
|
|
Signed-off-by: Denys Fedoryshchenko <denys.f@collabora.com>
|
