View Issue Details

IDProjectCategoryView StatusLast Update
0004961unrealircdpublic2017-08-10 08:35
ReporterGottemAssigned Tosyzop 
PrioritynormalSeverityminorReproducibilityalways
Status resolvedResolutionfixed 
Product Version 
Target VersionFixed in Version4.0.13 
Summary0004961: m_tkl ban_too_broad() logic errors
DescriptionI was messing around with a module and noticed sometimes checks inside ban_too_broad() never trigger because the conditions around it contain some errors. Or the function returns an incorrect value.

====
/zline *@8.0.0.0/8
This should be considered too broad, but Unreal happily accepts it. This is due to:

for (p = hostmask; *p; p++)
    if (*p != '*' && *p != '.' && *p != '?')
        cnt++;
if (cnt >= 4)
    return 0;

It doesn't take into account the CIDR notation at the end, and "8000" satisfies the if() so it returns 0 too soon.

====
/zline *@/24

Should be considered incorrect to begin with, but Unreal ends up saying it's "too broad" (earlier if() is false since cnt = 3 now). Except the /24 would actually be not-too-broad. This is due to:
if (strchr(hostmask, ':')) {
    if (cidrlen < 48)
        return 0; /* too broad IPv6 CIDR mask */
} else {
    if (cidrlen < 16)
        return 0; /* too broad IPv4 CIDR mask */

A cidrlen of /8 is indeed broader than /24, so it should return 1 here (same goes for IPv6). Alternatively, reverse only the conditions and do cidrlen >= 48 and cidrlen >= 16 instead.

====
/zline *@8.0.0.0/58963

Happily accepted as well. I noticed _match_user() does check for the validity of the cidrlen, but it doesn't throw a visible error. It just returns "nomatch" silently whenever a user is checked against tklines.

====
Bonus?
/gline *@8.8.8.0/24
Hmm, a G:Line with a CIDR mask, seems like that should be rejected. :>
Additional InformationQuite a wall of text, hope "you can make chocolate out of it" once I press submit. =]
TagsNo tags attached.
3rd party modulesN/A

Activities

syzop

2017-08-10 08:32

administrator   ~0019810

Thanks, fixed! :)
https://github.com/unrealircd/unrealircd/commit/18202a0f73ef8f32cc8cbf9c86a761740f2bbfb1

( As for the setting of invalid-never-matching-bans... I'm OK with leaving that in for now, just like with +b.. it simply won't match. )

syzop

2017-08-10 08:35

administrator   ~0019811

Last edited: 2017-08-10 08:35

View 3 revisions

By the way, just FYI: a GLINE with a CIDR mask is OK. You can use a hostname or IP (or CIDR) in GLINE.

The difference between a GLINE/KLINE and GZLINE/ZLINE is that the latter is enforced immediately after connect() before accepting any data. While the former only after USER and NICK have been received.. and a possible SSL/TLS handshake has been finished.
It can still be useful to use IP's in GZLINE/ZLINE because otherwise SSL/TLS users may not see the ban reason, although this may depend on the client.

^^ should probably document that better somewhere... but where.

Issue History

Date Modified Username Field Change
2017-06-03 21:49 Gottem New Issue
2017-08-10 08:32 syzop Assigned To => syzop
2017-08-10 08:32 syzop Status new => resolved
2017-08-10 08:32 syzop Resolution open => fixed
2017-08-10 08:32 syzop Fixed in Version => 4.0.13
2017-08-10 08:32 syzop Note Added: 0019810
2017-08-10 08:35 syzop Note Added: 0019811
2017-08-10 08:35 syzop Note Edited: 0019811 View Revisions
2017-08-10 08:35 syzop Note Edited: 0019811 View Revisions