Skip to content

W/THERETURN XPURGE & STATUS fixes + cservice security hardening - #88

Open
MrIron-no wants to merge 4 commits into
UndernetIRC:masterfrom
MrIron-no:fix/thereturn-xpurge-status
Open

W/THERETURN XPURGE & STATUS fixes + cservice security hardening#88
MrIron-no wants to merge 4 commits into
UndernetIRC:masterfrom
MrIron-no:fix/thereturn-xpurge-status

Conversation

@MrIron-no

@MrIron-no MrIron-no commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes for the W/THERETURN XPURGE/STATUS integration, plus a set of
mod.cservice security hardening changes (SQL injection and crashes on
malformed input) found while working on this branch.

W / THERETURN integration

  • WCommand: rename PURGE subcommand to XPURGE. The dispatch check
    and the user-facing help text (/msg X W XPURGE <#chan> -f) were still
    using the old PURGE name after a merge; the command was effectively
    unreachable under its intended name.
  • WCommand: add missing channel-name argument to a response. The
    cant_find_on_chan notice format expects a channel name, but only the
    W nickname was passed, so the second %s had no argument.
  • STATUSCommand: gate the "also registered with W" notice. It is now
    shown only when THERETURN_INTEGRATION is enabled or the requesting
    user is an admin (level >= 750), instead of to everyone.

Security fixes (mod.cservice)

SQL injection

The DB layer uses PQexec, which permits stacked statements, so an
unescaped quote in a query field allows arbitrary SQL. Fixed by wrapping
the affected user-supplied fields in escapeSQLChars():

  • CERT ADD — the free-text note (and fingerprint /
    nick!user@host) went into the users_fingerprints INSERT unescaped.
    note was reachable by any authenticated user, e.g.
    CERT ADD <valid-fp> x','y'); DROP TABLE ...; --.
  • HELLO — the e-mail address went into the duplicate-address lookup
    unescaped; validation permitted a single quote, so the string could be
    broken out of. (Compiled out by default via #undef ALLOW_HELLO, but
    fixed regardless.)

Crashes on malformed input

Asserts are compiled in, and StringTokenizer::getToken asserts on an
out-of-range subscript, aborting the whole process. Two paths indexed a
tokenizer without a sufficient size check:

  • SASL SCRAM client-first — a n= attribute with an empty value
    produced a zero-token tokenizer, and st3[0] aborted before the
    username-empty check. Reachable pre-auth from a connecting user's
    AUTHENTICATE data. Now guarded; an empty value falls through to the
    existing "missing username" error path.
  • doXROplistst[1]/st[2] were read before any size check (a
    2-token OPLIST reply aborted), and the guard only required 6 tokens
    while the body reads up to st[8]. Now checks size >= 3 before the
    NO test and size >= 9 before field extraction.

Testing

  • make clean on mod.cservice.
  • SASL SCRAM fix verified against the real StringTokenizer: the
    previously-aborting inputs (n=, n= ) now yield an empty username
    (handled by the existing error path), while normal tokens still parse.

User-supplied input was concatenated into SQL statements without
escaping, allowing quote breakout. Since the DB layer uses PQexec
(which permits stacked statements), this enabled arbitrary SQL.

- CERTCommand: escape note, fingerprint and nick!user@host in the
  users_fingerprints INSERT. note is free text from the user and was
  the primary injectable field.
- HELLOCommand: escape the e-mail address in the duplicate-address
  lookup. Validation permitted a single quote, so the query could be
  broken out of.
Both paths indexed StringTokenizer without a size check; getToken()
asserts on an out-of-range subscript, aborting the whole process.

- SASL SCRAM client-first: a "n=" attribute with an empty value
  produced a zero-token tokenizer, and st3[0] aborted before the
  username-empty check. This is reachable pre-auth from a connecting
  user's AUTHENTICATE data. Guard st3 before indexing; an empty value
  now falls through to the existing "missing username" error path.

- doXROplist: st[1]/st[2] were read before any size check (a 2-token
  OPLIST reply aborted), and the guard only required 6 tokens while the
  body reads up to st[8]. Check size >= 3 before the "NO" test and
  size >= 9 before the field extraction.
@MrIron-no MrIron-no changed the title Fixed wrong command name after merge. W/THERETURN XPURGE & STATUS fixes + cservice security hardening Aug 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant