PR #1776 range-checked the int64 from mux_atoi64 before narrowing to int,
closing the 32-bit truncation (4294967296 -> 0). But mux_atoi64 wraps
mod 2^64 with no overflow report (#1455), so a 20+-digit prefix wraps
back into [0,128] and passes the range check one width up:
10.0.0.0/18446744073709551617 (2^64 + 1) -> 1
10.0.0.0/18446744073709551616 (2^64) -> 0
2001:db8::/18446744073709551744 (2^64 + 128) -> 128
Same #1774 attack (conf string claims a narrow prefix, subnet built far
wider), now needing a 20-digit string. A valid prefix is 0..128 -- at
most three significant digits -- so reject anything longer before the
wrap can happen, skipping leading zeros so 10.0.0.0/024 still parses.
tests/netaddr grows the three wrap cases plus leading-zero acceptances;
74/74 pass (revert the guard and the three wraps parse as /1, /0, /128).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Range-check the prefix as int64 before narrowing to int so strings like
/4294967296 no longer wrap to /0 (whole IPv4). Free base/mask on the
out-of-range path. Expand tests/netaddr; stamp A7 Pass 14 on the audit map.
make test drives eight suites; on Windows a developer could run one of
them, and only after building it by hand. Every tests/* harness is a
Makefile invoking g++, and a box that can build TinyMUX has Visual Studio
but no make, gcc or cc.
tests/build-msvc.sh is the tests/ counterpart of the script #1414 added
for testcases/tools: locate the toolset with vswhere, compile with cl,
stage libmux.dll beside the exe (no rpath equivalent), and run. It
handles the three harnesses that link against libmux -- format, netaddr,
alarm. All three now build and pass:
format 31738 passed, 0 failed
netaddr 61 passed, 0 failed
alarm 8 passed, 0 failed
tests/dbt is deliberately not included. Five binaries, each needing a
different link, and all three DBT backends compiled into one image via -D
symbol renames: a real port rather than a compiler swap, and the
highest-value one still outstanding.
Two source changes were needed, and the second is the point of the whole
exercise.
test_netaddr.cpp included <arpa/inet.h> unconditionally for inet_pton;
on Windows that lives in <ws2tcpip.h>, which config.h already includes in
the required order, so the include moved below config.h and behind a
!WIN32 guard. It also needed WSAStartup: netmux does that during startup
so netaddr.cpp has no reason to, but a standalone harness does. Without
it every IPv6 parse_subnet fails while IPv4 passes, which reads
convincingly like an IPv6 bug rather than an uninitialised library.
With those two fixed, one genuine failure remained:
FAIL: v4-mapped ::ffff:1.2.3.4 vs 1.2.3.0/24 = kLessThan (outside) want inside
netaddr.cpp guarded the #800 v4-mapped canonicalization with
defined(IN6_IS_ADDR_V4MAPPED). That is a macro in glibc, but the Windows
SDK declares it as an INLINE FUNCTION in ws2ipdef.h, and defined() is
false for a function -- so the block has been preprocessed out of every
Windows build since #800, taking the ban-bypass protection with it. A
dual-stack listener delivers inbound IPv4 as ::ffff:a.b.c.d, and without
canonicalization it sorts past every v4 subnet, so an IPv4 site rule does
not match the connection it was written for. Filed as #1483.
Accepting WIN32 alongside the macro test is the minimal correct fix and
needs no configure regeneration. Checked for siblings, since the SDK
declares all the IN6_IS_ADDR_* helpers the same way: exactly one
occurrence in the tree.
Server smoke unchanged: 315 dispatched, 1479 succeeded, 17 failed -- the
known build-configuration failures on this box.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Pre-auth cap now keys on same_source_key (IPv4 host / IPv6 /64) so a
single /64 cannot fan out past max_preauth_sitecons. connect_rate_charge
runs only after pre-auth acceptance, matching the "charge accepted only"
invariant. Equal-subnet site inserts adopt ulThreshold so graduated
thresholds can be reconfigured without reset_site. Defense knobs use
cf_live_driver_int + a libmux driver-config sync callback so @admin
updates g_dc without a restart.
Ported from RhostMUSH, flagged in the prior-art survey as the best idea in
their design. Binary site policy is what makes shared addresses painful: a
dorm, a NAT gateway or a household is ONE address carrying many unrelated
players, so register_site on the campus punishes everyone for one abuser while
doing nothing leaves the site undefended. A threshold makes the middle
expressible -- "the dorm is fine until it isn't":
forbid_site 192.0.2.0/24 8 # CIDR + threshold
forbid_site 192.0.2.0 255.255.255.0 8 # address + mask + threshold
permit_site 127.0.0.1/32 3 # exemption WITH a ceiling
Every site directive takes the optional trailing count. Omitted or 0 means
"always", the historical behaviour. reset_site rejects one (it removes rules
rather than applying one) instead of ignoring it silently.
Direction depends on what the rule does. Rules that SET an HI_ flag (forbid,
register, noguest, suspect, nositemon) are restrictions and engage at or above
the threshold. Rules that CLEAR one (permit, guest, trust, sitemon) are
exemptions and engage only BELOW it -- an exemption that survived past its own
ceiling would not be a threshold at all. Same inversion Rhost applies to its
permit-side entries, and it is what makes "permitted up to N connections"
sayable.
Counting is per connecting ADDRESS, not per subnet: the subnet selects which
rule you land under, the address is what is counted. That is the right unit --
a NATted dorm is one address, so its own population is what the threshold
measures. The count excludes the incoming connection, so threshold N means
"engages once N connections from that address already exist", matching Rhost.
Cost is zero for existing configurations: the count is computed lazily, at
most once per lookup, and only when a thresholded rule is actually matched. An
ACL with no thresholds never walks the descriptor list.
A parsing bug the exemption case caught: the first implementation stripped the
trailing threshold with strstr(buf, token), which finds the FIRST copy of the
token's text. "permit_site 127.0.0.1/32 3" therefore truncated at the '3' of
"/32", leaving "127.0.0.1/" -- parse_subnet failed and the rule was SILENTLY
DROPPED. "forbid_site 127.0.0.0/8 4" worked only by luck (no earlier '4'),
which is exactly why testing the exemption direction mattered. Fixed by
cutting the last token via a backward scan, and factored into
parse_site_threshold() in netaddr.cpp so the bug class is unit-testable.
Thresholds also render in @list site_information ("Forbid (at 4+ conns)") so
an admin can tell "forbidden" from "forbidden once 8 connections are up".
Verified live, six sequential connections (. accepted, X refused):
forbid_site 127.0.0.0/8 4 . . . . X X
forbid_site 127.0.0.0/8 3 (digit collision) . . . X X X
forbid_site 127.0.0.0/8 + permit_site 127.0.0.1/32 3 . . . X X X
forbid_site 127.0.0.0/8 (regression) X X X X X X
no site rules (regression) . . . . . .
Dropping below the threshold re-admits immediately. Regression: smoke
1319/1319, stress 8/8, netaddr 57/57 (+11 parse_site_threshold cases,
including three digit collisions), ganl 14/14.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
retry_limit (3) is per-SOCKET: after three bad passwords the connection
closes and the attacker reconnects for three more. Nothing remembered
anything across connections, so brute-force-by-reconnect was unbounded.
Add that memory as a token bucket per source address, consumed only by
failed connect attempts:
login_fail_limit default 10, 0 = off -- burst of failed logins per source
login_fail_period default 60 seconds -- interval over which it refills
Sustained rate is limit/period, so the default is 10/minute against a
previously unlimited rate. Checked in check_connect BEFORE ConnectPlayer, so
a throttled source also stops costing a password hash per guess. Guests are
exempt (fixed password, separately bounded by the guest pool). On refusal the
socket is left open and retries_left untouched -- the attempt never reached a
password check, so it is not a failed login -- and conn_timeout still reaps an
idle one.
Two shapes of this defense are actively harmful in a MUSH and are not used:
* Per-ACCOUNT lockout. Player names are public (WHO, in-game, the
directory), so anyone could lock any player -- including a wizard -- out
of their own game by spamming failures at their name. That trades a
brute-force risk for a guaranteed griefing tool.
* A delay before answering a failed login. This server is single-threaded;
sleeping to slow one attacker stops the world for every other player. The
throttle must be non-blocking, so it refuses rather than stalls.
Keying: IPv6 by /64, not by address. One IPv6 customer normally holds a whole
/64, so a single host can source 2**64 addresses -- keying on the full address
would let one attacker both evade the throttle and flood the table with
single-use entries. mux_sockaddr::source_key() returns the 4-byte v4 address
or the 8-byte v6 /64 prefix; differing lengths keep the families from
colliding.
The table must not become the resource it protects: a fixed 512-slot array
scanned linearly, no allocation and no growth, consulted only on login
attempts (already bounded by max_preauth_per_site). When full, eviction takes
the LEAST suspicious entry (fullest bucket, oldest as tie-break) so table
pressure never costs us the record of an active attacker.
The dorm/NAT cost is real and deliberate: an exhausted bucket briefly refuses
legitimate players from a shared address, including ones typing the correct
password. It is bounded (continuous refill; seconds, not a lockout), the
default is generous relative to how often real players mistype, and admins can
widen or disable it. There is deliberately no "this source already has an
authenticated session" exemption -- it would read as dorm-friendly while
handing a full bypass to an existing player going after someone else's account.
Verified on a live netmux at limit 3 / period 600s: guesses 1-3 rejected
normally, 4-6 refused with the wait message, each from a FRESH connection --
reconnecting no longer buys a fresh batch. At limit 5 / period 20s the budget
demonstrably refills. Both the new CON/THR line and the earlier NET/SITE
pre-auth line confirmed to emit. Regression: smoke 1319/1319, stress 8/8,
netaddr 46/46 (+7 source_key tests), ganl 14/14.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Multi-connection is normal in MUSH -- a dorm or NAT puts many unrelated
players behind one address, households share one, and a single player
commonly sits on five or more alts at once -- so a per-IP cap on *total*
connections would break real play. That is why 36 years of TinyMUX shipped
only allow/deny site ACLs and never a per-IP count.
What is not normal is many connections from one address sitting at the login
prompt. Legitimate multi-play authenticates promptly; a slowloris holding
half-open sockets to exhaust the descriptor table never authenticates at all.
So count only connections without DS_CONNECTED.
max_preauth_per_site (default 2, 0 = unlimited, CA_GOD / CA_WIZARD)
Checked in the GANL accept path just before the new DESC joins
g_descriptors_list, so a refused connection never occupies a descriptor slot.
Refusal writes a short "try again in a moment" line raw via SOCKET_WRITE --
the route fcache_rawdump uses -- because the DESC is torn down immediately
and the normal output queue would never flush; it logs under
LOG_NET|LOG_SECURITY.
mux_sockaddr::operator== includes the source port, which differs for every
connection from one peer, so add mux_sockaddr::same_address() for family +
address-bytes equality. Cross-family (v4 vs v4-mapped v6) deliberately does
not unify: it can at most double a hostile client's allowance, and both forms
of one peer cannot arrive on the same listener.
Self-healing by construction -- a slot frees the instant a peer authenticates
or conn_timeout (120s) reaps it -- so a legitimate collision costs one retry,
never a lockout.
Verified with the default of 2: a third pre-auth socket from 127.0.0.1 is
refused with the explanatory message; authenticating a pending connection
immediately frees a slot; and 12 authenticated sessions from that same single
address were all accepted, leaving the dorm / household / five-alts case
untouched. Regression: smoke 1319/1319, stress 8/8 (its 16 simultaneous
logins from one address still pass), netaddr 39/39 (+6 new same_address
tests), ganl 14/14.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The netaddr harness tested only mux_subnet::compare_to happy paths;
parse_subnet's rejection branches (which gate the access-control site-ban
rule set) had zero coverage — make_subnet() treated any nullptr as a hard
FAIL. Add expect_reject()/expect_accept() and 13 cases:
- Rejections (nullptr): non-numeric mask (/abc), empty mask (/), prefix
out of range (/33, /-1, v6 /129), missing mask (no '/'), empty string,
malformed host (bad octet, non-address).
- Accept + normalize: 10.0.0.1/24 has a host bit set; parse_subnet clears
it rather than rejecting, so it parses and compares kEqual to
10.0.0.0/24.
- IPv6 shared-END nesting (::/1 ⊃ 7fff::/16) — the v6 analogue of the v4
0.0.0.0/1 case, exercising the #799 shared-bound containment logic on
the v6 path.
The error paths run the netmux-side cf_log_syntax, which is nullptr-safe
under the existing stubs because driver_log.h's STARTLOG guards on
g_pILog. 20 -> 33 assertions, all passing.
Also wire the harness into `make test` via a new test-netaddr target
(mirroring test-ganl) so this coverage actually runs in CI, and note it
in CLAUDE.md. macOS-verified; platform-independent (netaddr + libmux).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Defense-in-depth at the access *decision* point, complementing the
adapter-side ingress canonicalization (d841f7a36). compare_to(MUX_SOCKADDR*)
built a mux_in6_addr straight from an inbound sockaddr, so an IPv4-mapped
IPv6 source (::ffff:a.b.c.d, what a dual-stack listener delivers for an
inbound IPv4 connection) was compared cross-family and sorted past every
IPv4 subnet -> a v4 `forbid` rule never matched it and the ban was bypassed.
Now, when the sockaddr is IN6_IS_ADDR_V4MAPPED, compare it as native AF_INET
(embedded v4 = trailing 4 bytes). This makes the access decision correct
for any caller regardless of how the sockaddr was populated, not just the
onConnectionOpen path the adapter fix covers. No live behavior change (the
adapter already canonicalizes d->address before this is reached); genuine
IPv6 addresses are untouched.
Extends tests/netaddr to 20 cases: 6 address-vs-subnet, including the
IPv4-mapped #800 case. Bug-catch verified -- removing the v4-mapped branch
makes exactly that case fail (kLessThan/outside instead of kContains/inside)
while the other 19 pass. This is the mechanical lock #800 was missing; the
adapter-side fix remains for the canonical stored/displayed d->address.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was no test for mux_subnet::compare_to(mux_subnet*) -- the
subnet-vs-subnet comparison that drives the access-control (site-ban)
tree. fun_subnetmatch() exercises only the address-vs-subnet overload,
and the tree itself lives in net.cpp behind whole-driver dependencies, so
the shared-base/shared-end containment inversion (#799) shipped untested
(as did the historically-buggy operator==/< and tree remove/reset logic
that feeds the same comparator).
This harness links the netmux-side netmux-netaddr.o against libmux with
three driver-global stubs (g_bStandAlone, g_pILog, g_pINotify) and tests
the comparator directly -- 14 cases covering shared-base nesting, shared-
end nesting, equality, strict nesting, disjoint, and IPv6. Mirrors the
tests/libmux pattern (standalone Makefile, `make test`).
Bug-catch verified: reverting compare_to to the pre-#799 strict-'<' logic
makes exactly the four shared-bound cases fail (kContainedBy instead of
kContains -- the precise inversion) while the other ten pass; the fix
makes all 14 pass. This is the mechanical lock #799 was missing.
Future extensions (noted): address-vs-subnet via compare_to(MUX_SOCKADDR*),
and an IPv4-mapped-IPv6 case to cover #800 (whose fix is adapter-side).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
