In 3.2.x we didn't fix these bugs since servers are trusted and
should send correct commands. In 4.0.x we changed this so we would
fix them when we come across such issues at normal priority (not
consider them security issues). I now took it a step further and
actively checked/looked for these issues and a bunch of them were
found. Almost all are NULL pointer dereferences, with some exceptions.
* S2S: MODE: check conv_param return value (NULL ptr crash)
* S2S: MODE: floodprot: More checks (NULL ptr crash)
* S2S: MODE: OOB write of NULL (write NULL past last element in an array)
* S2S: NICK: old compat fixes (NULL ptr crash)
* S2S: PROTOCTL: Check for double SID=
* S2S: SERVER: require at least 3 parameters (NULL ptr crash)
* S2S: SJOIN: require at least 3 parameters (NULL ptr crash)
* S2S: SJOIN: Fix OOB read (read 1 byte past buffer)
* S2S: TKL: validate set_at and expire_at (NULL ptr crash)
* S2S: TKL: require at least 9 parameters for spamf, not 8 (NULL ptr crash)
* S2S: TKL: ignore invalid spamfilter matching type (remove abort() call)
* S2S: TOPIC: querying for topic is not permitted (NULL ptr crash)
* S2S: UID: require 12 parameters (NULL ptr crash)
* S2S: WATCH: this is not a server command (NULL ptr crash)
* Fix OOB read (1 byte beyond string) for timevals. This was reachable
from config code, TKL (S2S) and /*LINE (Oper). In practice no crash.
* MODE: make code less confusing (effectively no change)
* TRACE: remove strange output in case of 0 lines of output
* Fix unimportant memory leak on boot (#4713, reported by dg)
* Fix small memory leak upon 'DNS i' (oper only command)
* Always work on a copy in clean_ban_mask(). This fixes a bug that could
result in a strlcpy(buf, buf, sizeof(buf)). So, overlapping strings,
which is undefined behavior.
* API change for HOOKTYPE_PRE_INVITE:
(aClient *sptr, aClient *target, aChannel *chptr, int *override)
Modules must now send the error message instead of only returning
HOOK_DENY. Also check for operoverride and set *override=1.
This so modules can send their own error messages instead of the
default message being sent ("channel is +V" - which is not true).
Reported by Gottem (#5023).
For example: '+f [5j#i1,5m#m1,3n]:3' and then '+f [5j#i1,5m]:3'
In that case the '3n' was not removed and still effective, as
could be seen by a '/MODE #chan'. Reported by The_Myth (#4883).
This resulted in 5-10 changes in the existing code where parameters were off.
Hopefully I didn't make too many mistakes when writing the hook prototypes as it was a tedious job.
An (unintentional) benefit of this new system is that you can see the hook prototypes in include/modules.h like:
/* Hook prototypes */
int hooktype_local_quit(aClient *sptr, char *comment);
....
Though, the wiki is likely a better place: https://www.unrealircd.org/docs/Dev:Hook_API
with a default of 3:90 (3 joins per 90 seconds). There's rarely any need
to configure this on a per-channel basis and this way it's enabled by
default for everyone (unless you decide not to load load the module)
Not totally tested - I validated it built, I validated ACL validation worked, I validated that most of the ripped out functionality seemed to be absent, eg: we still set the modes (backwards compat w/ services?) but we don't actually check them anywhere, or add them to your whois.
Bram Matthys