Skip to content

Burst ban setter and timestamp in P11 — ban metadata is currently discarded on every relink #116

Description

@MrIron-no

Summary

struct Ban already carries the setter and the set-time, and RPL_BANLIST
already reports both to clients. They are simply not on the wire: the BURST
ban section carries the mask alone, so on receipt both fields are invented.
After any netsplit and relink, every ban that arrives over the burst reads
back as set by * at the moment of the relink.

This proposes extending the % ban section of BURST to carry
<mask> <ts> <who> triples for P11 peers, keeping the existing mask-only
form for P10 peers.

Current behaviour

The data exists locally. mode_parse_ban() populates it when a ban is set
(ircd/channel.c:3023-3024):

ircd_strncpy(newban->who, IsUser(state->sptr) ? cli_name(state->sptr) : "*", NICKLEN);
newban->when = TStime();

and send_ban_list() reports it:

send_reply(cptr, RPL_BANLIST, chptr->chname, lp->banstr, lp->who, lp->when);

But the burst emits only the mask (ircd/channel.c:1191-1205):

/* Attach all bans, space separated " :%ban ban ..." */
for (first = 2; lp2; lp2 = lp2->next)
{
  len = strlen(lp2->banstr);
  if (msgq_bufleft(mb) < len + 1 + first) { full = 1; break; }
  msgq_append(&me, mb, " %s%s", first ? ":%" : "", lp2->banstr);
  first = 0;
}

and the receiver fabricates the rest (ircd/m_burst.c:420-423):

newban = make_ban(ban);
strcpy(newban->who, "*");
newban->when = TStime();
newban->flags |= BAN_BURSTED;

So the two sides of a split disagree about their own ban lists as soon as they
rejoin, and users see it directly in /mode #chan b. Ban age is also the
natural basis for any future ban-expiry or ban-audit work, and it does not
currently survive a relink.

Proposed wire format

Keep the % marker. The field layout is selected by the protocol version of
the link, not by inspecting the content.

To a P10 peer, unchanged:

... :%<mask> <mask> <mask>

To a P11 peer:

... :%<mask> <ts> <who> <mask> <ts> <who> ...
  • <mask> — as today, after pretty_mask()/collapse().
  • <ts> — decimal seconds, the ban's when. 0 when unknown.
  • <who> — the setter's nickname, or * when unknown. Never empty.

Parse three tokens at a time. If the section's token count is not a multiple
of three, reject the whole section and raise protocol_violation() rather
than applying a partial parse.

Why space, and not a group separator

A space is the one delimiter guaranteed absent from a ban mask:
mode_parse_ban() truncates the argument at the first space
(ircd/channel.c:3010), and more fundamentally a space cannot survive inside
a single wire parameter. Using it needs no new character rules.

A comma was considered and rejected, because a comma is currently valid in
a ban mask. There is no charset validation on the path —
mode_parse_ban() rejects only empty strings and a leading :;
pretty_mask() (ircd/channel.c:1242) only locates ! and @ and truncates
the components to length; collapse() only squashes runs of *; and
set_ban_mask() (ircd/channel.c:164) is an ircd_strncpy(). There is no
IsHostChar/IsUserChar/IsNickChar call anywhere in channel.c.

Verified against a build of this tree:

-> MODE #test +b *!*@a,b.example.com
<- :tester!t@irc.test.net MODE #test +b *!*@a,b.example.com
<- :irc.test.net 367 tester #test *!*@a,b.example.com tester 1789422750

Under a comma-delimited encoding, a channel operator setting
+b *!*@x.com,*!*@victim.com 0 someoper would burst as two bans, the second
with an attacker-chosen setter and timestamp. Space-delimited triples have no
such surface.

(The who field is not affected either way: NTL_IRCNK is
digits + alpha + -_\`` per ircd/table_gen.c:116-117`, so a nickname can
contain neither a comma nor a space.)

Compatibility

Protocol(cptr) is already populated for adjacent and remote servers by
mr_server()/ms_server() and is available as cli_serv(x)->prot
(include/client.h:589). Emit triples only when Protocol(cptr) >= 11, and
parse them only when the link is P11 — never infer the layout from the
content.

One implementation obstacle is worth flagging up front. ms_burst() relays
what it accepted (ircd/m_burst.c:585):

sendcmdto_serv_butone(sptr, CMD_BURST, cptr, "%H %Tu%s%s%s", chptr,
                      chptr->creationtime, modestr, nickstr, banstr);

sendcmdto_serv_butone() builds one buffer and writes it to every downlink,
so a P11 hub with a mix of P10 and P11 downlinks cannot serve both layouts
from this call site. It needs either a protocol-aware sender — mirroring the
FLAG_IPV6 split already used for NICK in register_user()
(ircd/s_user.c:449,459) — or both banstr variants built during parsing and
emitted separately. This is the same structural limitation that any other
version-gated wire change runs into, so it is worth solving once.

The initial burst is simpler: send_channel_modes() already takes the
destination link and calls send_buffer(cptr, ...) directly
(ircd/channel.c:1209), so it can branch on Protocol(cptr) in place.

Receiver validation

All of this is a hostile-peer concern rather than tidiness:

  • who — cap at NICKLEN, reject if empty. A peer that smuggles a space
    into who would shift every subsequent field in the section, so the arity
    check above is load-bearing.
  • ts — parse with a validating 64-bit helper rather than atoi().
    Reject non-numeric input outright instead of silently yielding 0, and
    sanity-check against OLDEST_TS and a small future-skew allowance.
  • Arity — reject the whole section on a count that is not a multiple of
    three.

Merge semantics

Once bans carry metadata, two servers can hold the same mask with different
(when, who). ms_burst() currently merges on the mask alone
(ircd/m_burst.c:392-406). The tie-break must be deterministic and symmetric
or the two sides converge on different metadata and desync again on the next
burst, which is the failure this change exists to fix.

Proposed: lowest when wins; on a tie, the lexicographically smaller
who. A when of 0 sorts after any real timestamp, so a known setter
always beats an unknown one.

Size

Per-ban cost goes from strlen(mask) + 1 to
strlen(mask) + 1 + strlen(ts) + 1 + strlen(who). With a 10-digit timestamp
and a typical 12-character nickname that is +24 bytes per ban: a typical
25-character mask goes from 26 to 50 bytes, so bans per 512-byte line roughly
halves. Worst case is a 91-byte mask (NICKLEN 15 + USERLEN 10 + HOSTLEN 63 + 3)
plus 27, giving four bans per line.

Acceptable for a burst-time-only change, but the msgq_bufleft() arithmetic
at ircd/channel.c:1195 must be updated to account for the wider entries or
bans will be silently dropped at line boundaries.

If the size matters, delta-encoding ts against the channel's
creationtime — the way oplevels are already delta-encoded in the nick
section — would typically recover 4-6 bytes per ban. Probably not worth the
complexity in a first pass.

Ban exceptions

BAN_EXCEPTION exists in struct Ban (include/channel.h:257) and
mode_parse_ban() has a branch for it, but no channel mode maps to it —
chan_flags has no e, so *flag_p is always MODE_BAN and the flag is
vestigial for channels today. If +e is added later it should get its own
section marker rather than a flags field bolted onto the triple.

Test plan

  1. Round trip. P11 ↔ P11: set a ban, split, relink; who and when
    identical on both sides. Fails today.
  2. Mixed link. P11 ↔ P10: the P10 peer receives the mask-only form;
    the P11 side degrades to */0 for bans learned from the P10 peer.
  3. Comma in a mask. Set +b *!*@x.com,*!*@y.com, burst it, assert
    exactly one ban with the mask intact on the far side.
  4. Space injection in who. Inject a crafted section whose who
    contains a space; assert the section is rejected wholesale.
  5. Arity mismatch. A token count not divisible by three is rejected and
    logged.
  6. Merge determinism. Two servers, same mask, different (when, who);
    both converge on the same winner and a second burst produces no change.
  7. Line continuation. Enough bans to force several BURST lines; no ban
    lost or truncated at a boundary.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions