| Age | Commit message (Collapse) | Author |
|---|
|
Remove debian11 from CI (as EoL), add ubuntu 26:04 (build, test), remove qemu src-build for big-endian test
|
|
|
|
|
|
tests: give the veth pair fixed MAC addresses
|
|
utils: centralize max macro
|
|
Closes #354
|
|
The tests create the veth pair with 'ip link add ... type veth peer name
...' and let the kernel pick random hardware addresses, then start
accel-pppd right away. On distributions where systemd-udevd rewrites the
address of a freshly created veth (MACAddressPolicy in its .link rules,
seen on Ubuntu 22.04) this is a race: accel-pppd reads the address of the
interface it serves once, at startup, and when udev wins it advertises an
address the interface no longer has.
Discovery still completes - the client copies the advertised address into
its kernel PPPoE session as the peer address - but from then on the two
ends disagree about it, no PPP frame is exchanged in either direction, and
the session dies on LCP timeouts. Every PPPoE session test can fail this
way; test_pppoe_ccp_ipcp_race failed 8 times out of 12 runs in one qemu
Ubuntu-22.04 job.
Create the pair with fixed, locally administered addresses derived from
the name the pair already gets, so udev has nothing to change. The same
job passes 12 out of 12 with this.
This only removes the trigger in the tests. accel-pppd not following the
address of the interface it serves is tracked in #363 and needs a fix of
its own.
|
|
Ipcp ccp delay ack
|
|
Fix several unsafe or unaligned integer accesses found in protocol parsing paths, including option-gated MPPE, DHCP, PPPoE, RADIUS, IPCP and IPv6CP code.
|
|
The option walkers of lcp_recv_conf_req() and its IPCP, IPV6CP and CCP
counterparts bail out with *_OPT_FAIL as soon as an option header does not
fit in the remaining bytes or carries an impossible length, and the caller
turns that into ap_session_terminate(TERM_USER_ERROR).
The bail out happens before the "recv [LCP ConfReq id=..." line is emitted,
so the session simply disappeared with nothing in the log to point at the
peer. The ConfAck, ConfNak and ConfRej walkers break out of their loop
instead and at least close the line they had already started.
Log the offending length, option id and the number of bytes left before
returning, so a peer sending malformed options can be told apart from the
other reasons a session ends with TERM_USER_ERROR.
|
|
ipcp_recv() and ipv6cp_recv() only rejected packets whose header length
field was below PPP_HEADERLEN. The upper bound was missing, so the option
walker was handed a size derived purely from the peer chosen hdr->len.
ppp->buf is a fixed 8192 byte mempool block and ppp->buf_size holds the
number of bytes actually read, so a short frame declaring hdr->len 0xffff
made the option loop run off the end of the block: it kept fetching option
headers from whatever followed in the heap and stored pointers to them in
the ropt list. The read is harmless in itself but easily reaches unmapped
memory and kills the daemon.
Reject any packet whose declared length does not fit in what was received,
the way lcp_recv() and ccp_recv() already do. The two byte slack accounts
for the protocol field that precedes the header inside the buffer, and the
existing buf_size >= PPP_HEADERLEN + 2 test just above keeps the
subtraction from underflowing.
|
|
|
|
Allocation sizes are arbitrary, so store and load the trailing uint64_t canary with memcpy instead of dereferencing a potentially unaligned pointer.
|
|
Copy interface IDs and prefix words between aligned temporaries and byte arrays instead of casting IPv6 address storage to uint64_t pointers.
|
|
Use memcpy for scalar backup headers and restored session, pool, and RADIUS values because variable-length tags do not guarantee native integer alignment.
|
|
Serialize integer fields via memcpy, restrict attribute parsing to the RADIUS header's declared packet length, reject undersized packet lengths, and reject truncated attribute headers.
|
|
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.
|
|
Enforce fixed option prefixes centrally, bound nested relay messages to their declared payload, require one valid Relay-Message, and decode ORO and diagnostic fields without unaligned or oversized reads.
|
|
Serialize the MPPE option through an aligned temporary and exercise mppe=prefer in the existing unauthenticated PPPoE session test.
|
|
Bound LCP and CCP declared lengths by the received frame, reject truncated option headers across all control protocols, and decode LCP payload integers without alignment assumptions. IPCP and IPv6CP outer frame bounds remain owned by the delayed-ack change in PR #359.
|
|
Various fixes on compiler warnings
|
|
Covers the previous commit: with 'mppe=prefer' CCP is non-passive, so it is
still negotiating (and needs one more round trip because pppd rejects the
MPPE option) when pppd, which does not delay IPCP on CCP, sends its IPCP
ConfReq. The test asserts that the session becomes active and that no IPCP
TermAck was sent, reading the negotiation from the accel-pppd debug log.
The check is skipped if the peer happened to send its IPCP ConfReq only
after CCP was done, in that case the race is not exercised.
|
|
When the peer's IPCP (or IPV6CP) ConfReq arrives before CCP negotiation
has finished, delay_ack is set and send_conf_ack() answers with a TermAck
instead of the ConfAck. A TermAck is only a valid response to a TermReq,
and the trick relies on the peer retransmitting its ConfReq: conformant
peers recover only after their restart timer (3 seconds added to session
setup), while some clients (MikroTik RouterOS over L2TP, see issue #353)
treat it as a failure and drop the session.
Withhold the ConfAck instead and send it as soon as CCP settles. CCP now
notifies IPCP/IPV6CP when it comes up or gives up (passive); that clears
delay_ack, brings the layer up if the FSM is already Opened, and flushes
the withheld ConfAck. Nothing is sent to the peer while CCP is still in
progress, so IP data cannot flow before MPPE is set up.
Verified with pppoe + mschap-v2: with mppe=prefer and a client that does
not require MPPE, the peer's ConfReq used to be answered with a TermAck
and the session came up 3 seconds later; now the ConfAck is emitted right
after ccp_layer_started and the session comes up immediately. mppe=require
and mppe=deny sessions are unaffected.
|
|
Ipoe stale session flush fixes
|
|
PPTP fixes
|
|
pptp: drop the out-of-tree kernel driver
|
|
auth: fix challenge-name lifetime on reload
|
|
Stability fixes and function unification
|
|
l2tp hidden-AVP length fix, per-session IPv6 DNS, crypto.h cleanup
|
|
The ipv6_nd and ipv6_dhcp modules could only advertise the DNS servers
configured in [ipv6-dns], the same set for every subscriber. RFC 6911
defines DNS-Server-IPv6-Address (attribute 169) for exactly this, and
the attribute was already in the shipped dictionary and in attr_defs.h;
nothing read it.
Give struct ap_session an ipv6_dns list, filled by the radius module
from that attribute, and have both modules advertise it when the session
has one: in the RDNSS option of the router advertisements, and in the
DNS_SERVERS option of DHCPv6 replies. Sessions without a list of their
own keep getting the configured servers, so nothing changes for anyone
not sending the attribute.
The selection is a single ipv6_dns_get() shared by both modules rather
than a copy in each: they already duplicate the whole [ipv6-dns] parser,
and two copies of a precedence rule are two chances to drift. It caps
what it returns, so neither the RDNSS option length (one byte, in units
of 8) nor the router advertisement buffer can be pushed around by what a
RADIUS server sends. The radius module caps at the same 3 servers as
[ipv6-dns] accepts and warns once when a reply carries more.
An Access-Accept which carries the attribute replaces the whole
previously assigned list rather than appending to it, so a re-authorized
session ends up with the servers of the latest reply and not with a
concatenation. One which does not carry it leaves the current list
alone, which is how the IPv4 MS-Primary-DNS-Server attribute already
behaves.
ipv6_dns_test.c covers the selection: assigned wins over configured,
empty list means "nothing assigned" rather than "no DNS", the cap holds
for both sources, and a caller with no room gets nothing rather than a
stomped buffer. Wired into the ASAN/UBSAN workflow.
Inspired by the per-session IPv6 DNS support in accel-ppp-ng (commit
2df6eb99), reimplemented against mainline's ap_session and ipdb types.
|
|
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.
|
|
metrics: expose session details in JSON output
|
|
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.
|
|
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.
|
|
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.
|
|
list_entry() computed the member offset as &((type *)0)->member, which
is a member access on a null pointer and undefined. UBSan reports it on
every list traversal in the tree:
runtime error: member access within null pointer of type
'struct dhcpv6_option'
The generated code is the same either way, and the offset happens to be
zero for many of the structures involved, so nothing misbehaves today.
offsetof() is what the standard provides for this.
|
|
print_time() read a 32 bit word out of the Elapsed Time option, which
RFC 8415 21.9 defines as exactly two octets, and clients include it in
almost every message they send. With verbose enabled that is a two byte
read past the option on every DHCPv6 transaction:
ERROR: AddressSanitizer: heap-buffer-overflow
READ of size 4 in print_time dhcpv6_packet.c:499
The value was not byte swapped either, so what got logged was not the
elapsed time. Read two octets, convert them, and copy them out rather
than dereferencing a pointer into the packet that need not be aligned.
print_uint8() has the same shape, so check the length there too before
reading the Preference octet.
|
|
opth->data already points past the option header, so adding
sizeof(*opth) again counted it twice and left endptr four bytes beyond
the end of the Relay-Message option, and possibly beyond the received
packet.
The bounds check at the top of the option loop is written against that
endptr, so on the next pass it accepted an option header that lies
outside the buffer and read opth->len from it. ASan on a Relay-Forward
packet:
ERROR: AddressSanitizer: heap-buffer-overflow
READ of size 2 in dhcpv6_packet_parse dhcpv6_packet.c:158
The end of the relayed message is the option payload, nothing more.
|
|
The relay decapsulation loop only advances pkt->hdr when it finds a
Relay-Message option inside the current relay header. A Relay-Forward
packet that carries no Relay-Message option leaves pkt->hdr pointing at
the same header, so the outer loop runs again on the same input and
allocates another struct dhcpv6_relay on every pass.
A 34 byte packet, a Relay-Forward header with no options at all, is
enough to allocate without limit. Instrumented with ASan it reaches
11.8 million allocations and over a gigabyte in a few seconds:
ERROR: libFuzzer: out-of-memory (used: 1249Mb; limit: 512Mb)
Live Heap Allocations: 781874575 bytes in 11834644 chunks
#1 in dhcpv6_packet_parse dhcpv6_packet.c:142
The socket is per session, so this needs an established session, but the
packet is parsed before any DHCPv6 level validation and one client can
exhaust memory for the whole daemon.
Remember the header at the top of each pass and treat a pass that did
not move it as a malformed packet.
|
|
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.
|
|
print_status() reads the 2-byte status code at offset 4 of the option,
but parse_option() only guarantees that the option header plus its
declared payload length are within the packet. A Status Code option with
a payload length below 2 as the last option in a packet therefore made
print_status() read past the end of the received buffer.
Skip the option if its payload is too short to hold the code.
|
|
A missing comma made "UseMulticast" and "NoPrefixAvail" concatenate into
a single string literal, so the table held 6 entries instead of 7:
UseMulticast printed as "UseMulticastNoPrefixAvail" and NoPrefixAvail had
no entry at all.
The bounds check compared the status code against sizeof(status_name),
which is the size of the table in bytes (48), not its number of entries.
Codes 6..48 passed the check and indexed past the end of the table, so
print() was handed whatever pointer-sized garbage followed it. A client
can trigger this by sending a Status Code option with an out-of-range
code, which is printed verbatim when verbose is enabled.
Compare against the entry count instead, and drop the ntohs() < 0 test,
which can never be true for an unsigned value.
|
|
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.
|
