Bug 243164 - blacklistd not handling masks correctly
Summary: blacklistd not handling masks correctly
Status: New
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: 12.1-STABLE
Hardware: Any Any
: --- Affects Many People
Assignee: freebsd-bugs mailing list
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-01-07 17:52 UTC by Helge Oldach
Modified: 2020-01-08 08:40 UTC (History)
3 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Helge Oldach 2020-01-07 17:52:41 UTC
I am afraid the blacklist.conf syntax and description might be a bit misleading. According to the manpage, an IP address may be specified without mentioning an explicit mask. The usual assumption obvioulsy being that lack of a mask spec represents a host address: 192.168.134.99 would be identical to 192.168.134.99/32.

It appears that blacklistd behaves differntly. For the sake of the exercise consider the following trivial (and complete) /etc/blacklistd.conf:

     # adr/mask:port         type    proto   owner   name    nfail   disable
     [local]
     ssh                     stream  *       *       *       3       3m
     
     # adr/mask:port         type    proto   owner   name    nfail   disable
     [remote]
     192.168.134.99:ssh      *       *       *       =       *       *

Basically meaning, ssh would be blocked for 3 minutes after 3 unsuccessful attempts - except when ssh originates at 192.168.134.99, which will always succeed even if invalid.

Now, I'm not connecting from that host but from 192.168.134.1. When connecting with an invalid user, access is *NOT* blocked after 3 unsuccessful attempts. Indeed the debug log shows a successful match:

Jan  7 18:12:08 latitude blacklistd[1565]: processing type=2 fd=6 remote=192.168.134.1:61329 msg=ssh uid=0 gid=0
Jan  7 18:12:08 latitude blacklistd[1565]: listening socket: 192.168.134.3:22
Jan  7 18:12:08 latitude blacklistd[1565]: look:        target:192.168.134.3:22, proto:6, family:2, uid:0, name:=, nfail:*, duration:*
Jan  7 18:12:08 latitude blacklistd[1565]: check:       target:22, proto:6, family:*, uid:*, name:*, nfail:3, duration:180
Jan  7 18:12:08 latitude blacklistd[1565]: found:       target:22, proto:6, family:*, uid:*, name:*, nfail:3, duration:180
Jan  7 18:12:08 latitude blacklistd[1565]: conf_apply: merge:   target:22, proto:6, family:*, uid:*, name:*, nfail:3, duration:180
Jan  7 18:12:08 latitude blacklistd[1565]: conf_apply: to:      target:192.168.134.3:22, proto:6, family:2, uid:0, name:=, nfail:*, duration:*
Jan  7 18:12:08 latitude blacklistd[1565]: conf_apply: result:  target:192.168.134.3:22, proto:6, family:2, uid:*, name:*, nfail:3, duration:180
Jan  7 18:12:08 latitude blacklistd[1565]: Applied address 192.168.134.1:22
Jan  7 18:12:08 latitude blacklistd[1565]: check:       target:192.168.134.99:22, proto:*, family:*, uid:*, name:=, nfail:*, duration:*
Jan  7 18:12:08 latitude blacklistd[1565]: found:       target:192.168.134.99:22, proto:*, family:*, uid:*, name:=, nfail:*, duration:*
Jan  7 18:12:08 latitude blacklistd[1565]: conf_merge: merge:   target:192.168.134.99:22, proto:*, family:*, uid:*, name:=, nfail:*, duration:*
Jan  7 18:12:08 latitude blacklistd[1565]: conf_merge: to:      target:192.168.134.1:22, proto:6, family:2, uid:*, name:*, nfail:3, duration:180
Jan  7 18:12:08 latitude blacklistd[1565]: conf_merge: result:  target:192.168.134.1:22, proto:6, family:2, uid:*, name:*, nfail:*, duration:*
Jan  7 18:12:08 latitude blacklistd[1565]: Applied address 192.168.134.1:22

Note there are *two* "found:" tokens in the log (one for the [local], and one for the [remote] part) which states that our source address (192.168.134.1) does match against 192.168.134.99. This is obviously incorrect.

Now, let's add a proper (host) network mask to /etc/blacklistd.conf

     ...
     # adr/mask:port         type    proto   owner   name    nfail   disable
     [remote]
     192.168.134.99/32:ssh      *       *       *       =       *       *

In this case, access *IS* blocked after three unsucessful attempts. Indeed the debug log reflects this (first attempt):

Jan  7 18:07:13 latitude blacklistd[1506]: processing type=2 fd=6 remote=192.168.134.1:61301 msg=ssh uid=0 gid=0
Jan  7 18:07:13 latitude blacklistd[1506]: listening socket: 192.168.134.3:22
Jan  7 18:07:13 latitude blacklistd[1506]: look:        target:192.168.134.3:22, proto:6, family:2, uid:0, name:=, nfail:*, duration:*
Jan  7 18:07:13 latitude blacklistd[1506]: check:       target:22, proto:6, family:*, uid:*, name:*, nfail:3, duration:180
Jan  7 18:07:13 latitude blacklistd[1506]: found:       target:22, proto:6, family:*, uid:*, name:*, nfail:3, duration:180
Jan  7 18:07:13 latitude blacklistd[1506]: conf_apply: merge:   target:22, proto:6, family:*, uid:*, name:*, nfail:3, duration:180
Jan  7 18:07:13 latitude blacklistd[1506]: conf_apply: to:      target:192.168.134.3:22, proto:6, family:2, uid:0, name:=, nfail:*, duration:*
Jan  7 18:07:13 latitude blacklistd[1506]: conf_apply: result:  target:192.168.134.3:22, proto:6, family:2, uid:*, name:*, nfail:3, duration:180
Jan  7 18:07:13 latitude blacklistd[1506]: Applied address 192.168.134.1:22
Jan  7 18:07:13 latitude blacklistd[1506]: check:       target:192.168.134.99/32:22, proto:*, family:*, uid:*, name:=, nfail:*, duration:*
Jan  7 18:07:13 latitude blacklistd[1506]: conf_amask_eq: a1: c0a88601 != a2: c0a88663 [0x20]
Jan  7 18:07:13 latitude blacklistd[1506]: Applied address 192.168.134.1:22

Note there is only a "found:" token against the [local] part of the configuration file, but not against the [remote] part. Further the "check:" line clearly states that the (hex) IP addresses 192.168.134.1 and 192.168.134.99 are not identical.

Documentation error or bug?

Note the blacklist.conf manpage contains an example without netmask, however the description seemingly does not match behaviour:

     # Never block 1.2.3.4
     1.2.3.4:ssh     *       *       *       *       *       *

Also please not I have manually applied the review D22259 updates to my 12-STABLE machine. That does not change behaviour.
Comment 1 Ed Maste freebsd_committer 2020-01-07 18:32:03 UTC
(In reply to Helge Oldach from comment #0)
Indeed, the changes in D22259 are quite minor and I wouldn't expect to see a difference here. Will try to take a look.
Comment 2 Conrad Meyer freebsd_committer 2020-01-08 01:45:55 UTC
I think the problem is this in contrib/blacklist/bin/conf.c:

   213 static int
   214 getmask(const char *f, size_t l, bool local, const char **p, int *mask)
   215 {
   216         char *d;
   217         const char *s = *p;
   218
   219         if ((d = strchr(s, ':')) != NULL) {
   220                 *d++ = '\0';
   221                 *p = d;
   222         }
   223         if ((d = strchr(s, '/')) == NULL) {
   224                 *mask = FSTAR;
   225                 return 0;


If there is no /mask, *mask is treated as "FSTAR".  In conf_addr_set:

   585         if (c->c_lmask == FSTAR)
   586                 c->c_lmask = (int)(alen * 8);

That, uh, suggests no mask should indicate /32 or /128 for v6.  I wonder what goes wrong.
Comment 3 Conrad Meyer freebsd_committer 2020-01-08 01:56:06 UTC
Ah.  I wonder if this is it?

   459 #define MASK(m)  ((uint32_t)~((1 << (32 - (m))) - 1))
   460
   461 static int
   462 conf_amask_eq(const void *v1, const void *v2, size_t len, int mask)
   463 {
   464         const uint32_t *a1 = v1;
   465         const uint32_t *a2 = v2;
   466         uint32_t m;
   467         int omask = mask;
   468
   469         len >>= 2;
   470         switch (mask) {
   471         case FSTAR:
   472                 if (memcmp(v1, v2, len) == 0)
   473                         return 1;
   474                 goto out;

// Above is definitely wrong, only compares the first 1/4 of the address
// bytes.  Maybe that's what we're seeing?

...
   484         for (size_t i = 0; i < len; i++) {
   485                 if (mask > 32) {
   486                         m = htonl((uint32_t)~0);
   487                         mask -= 32;
   488                 } else if (mask) {
   489                         m = htonl(MASK(mask));
   490                         mask = 0;
   493                 if ((a1[i] & m) != (a2[i] & m))
   494                         goto out;

// This part seems fine for v4, but I'm not sure if it's correct for v6.

   495         }
   496         return 1;
   497 out:
...
   506         return 0;
Comment 4 Conrad Meyer freebsd_committer 2020-01-08 01:58:18 UTC
Helge, does this fix the issue?

--- a/contrib/blacklist/bin/conf.c
+++ b/contrib/blacklist/bin/conf.c
@@ -466,7 +466,6 @@ conf_amask_eq(const void *v1, const void *v2, size_t len, int mask)
        uint32_t m;
        int omask = mask;

-       len >>= 2;
        switch (mask) {
        case FSTAR:
                if (memcmp(v1, v2, len) == 0)
@@ -481,7 +480,7 @@ conf_amask_eq(const void *v1, const void *v2, size_t len, int mask)
                break;
        }

-       for (size_t i = 0; i < len; i++) {
+       for (size_t i = 0; i < (len >> 2); i++) {
                if (mask > 32) {
                        m = htonl((uint32_t)~0);
                        mask -= 32;
@@ -497,7 +496,6 @@ conf_amask_eq(const void *v1, const void *v2, size_t len, int mask)
 out:
        if (debug > 1) {
                char b1[256], b2[256];
-               len <<= 2;
                blhexdump(b1, sizeof(b1), "a1", v1, len);
                blhexdump(b2, sizeof(b2), "a2", v2, len);
                (*lfun)(LOG_DEBUG, "%s: %s != %s [0x%x]", __func__,
Comment 5 Helge Oldach 2020-01-08 05:04:10 UTC
(In reply to Conrad Meyer from comment #3)
> // Above is definitely wrong, only compares the first 1/4 of the address
> // bytes.  Maybe that's what we're seeing?

Indeed. That matches with a real-life situation that consistently a (correct) mismatch is observed in case the first octet of an IPv4 address specified in [remote] is not identical to the first octet of the incoming address. In the exercise case, not being 192.
Comment 6 Helge Oldach 2020-01-08 05:26:13 UTC
(In reply to Conrad Meyer from comment #4)
> Helge, does this fix the issue?
Yes it does; we are now seeing a mismatch in case the source and [remote] addresses are different:

Jan  8 06:06:57 latitude blacklistd[14144]: processing type=1 fd=6 remote=192.168.134.1:61909 msg=ssh uid=22 gid=22
Jan  8 06:06:57 latitude blacklistd[14144]: listening socket: 192.168.134.3:22
Jan  8 06:06:57 latitude blacklistd[14144]: look:       target:192.168.134.3:22, proto:6, family:2, uid:22, name:=, nfail:*, duration:*
Jan  8 06:06:57 latitude blacklistd[14144]: check:      target:22, proto:6, family:*, uid:*, name:*, nfail:3, duration:180
Jan  8 06:06:57 latitude blacklistd[14144]: found:      target:22, proto:6, family:*, uid:*, name:*, nfail:3, duration:180
Jan  8 06:06:57 latitude blacklistd[14144]: conf_apply: merge:  target:22, proto:6, family:*, uid:*, name:*, nfail:3, duration:180
Jan  8 06:06:57 latitude blacklistd[14144]: conf_apply: to:     target:192.168.134.3:22, proto:6, family:2, uid:22, name:=, nfail:*, duration:*
Jan  8 06:06:57 latitude blacklistd[14144]: conf_apply: result: target:192.168.134.3:22, proto:6, family:2, uid:*, name:*, nfail:3, duration:180
Jan  8 06:06:57 latitude blacklistd[14144]: Applied address 192.168.134.1:22
Jan  8 06:06:57 latitude blacklistd[14144]: check:      target:192.168.134.99:22, proto:*, family:*, uid:*, name:=, nfail:*, duration:*
Jan  8 06:06:57 latitude blacklistd[14144]: conf_amask_eq: a1: c0a88601 != a2: c0a88663 [0xffffffff]
Jan  8 06:06:57 latitude blacklistd[14144]: Applied address 192.168.134.1:22

I followed the logic of your patch, it appears OK to me.

So it's not a documentation error as I was thinking but indeed a bug. While not being a security issue as such, it's kind of scary, as it might leave ports open unnecessary. Also this should probably be backported upstream to NetBSD.

What I still don't understand however is why the netmask can be FSTAR at all? What is the point? I can't follow the semantics. Why would we want to compare an incoming IP address (with implied /32 mask) to a template with an "unknown" netmask? I suspect a proper fix might involve setting it to 32 (or 128 in the IPv6 case) right away if no mask is specified?
Comment 7 Conrad Meyer freebsd_committer 2020-01-08 05:34:04 UTC
(In reply to Helge Oldach from comment #6)
Thanks for testing it out.

> So it's not a documentation error as I was thinking but indeed a bug.

Yep.  Nice find!

> What I still don't understand however is why the netmask can be FSTAR at all?
> What is the point? I can't follow the semantics. Why would we want to compare an
> incoming IP address (with implied /32 mask) to a template with an "unknown"
> netmask? I suspect a proper fix might involve setting it to 32 (or 128 in the
> IPv6 case) right away if no mask is specified?

I completely agree.

I'm also not exactly in love with the custom file format (with ad-hoc C parser and no formal grammar) and suggested just using UCL or JSON to Kurt a few years ago, but he was opposed at the time.  (IIRC he had indicated plans to use a formal grammar for the existing format, at least, but never got to it.)
Comment 8 Helge Oldach 2020-01-08 07:48:50 UTC
(In reply to Conrad Meyer from comment #7)
> I'm also not exactly in love with the custom file format (with ad-hoc
> C parser and no formal grammar)

Yes, and there are also potential bugs in that hand-rolled parser. Some time ago I have stumbled over IPv6 address/netmask:

    [2001:679:2807:60::]/64:ssh     stream  *       *               *       *      *

versus

    [2001:679:2807:60::/64]:ssh     stream  *       *               *       *      *

Only the first case works well, the second case however is silently accepted without throwing an error, but doesn't work. This should probably be documented.
Comment 9 Conrad Meyer freebsd_committer 2020-01-08 08:40:58 UTC
Should be fixed, not just documented.  :-(

If you have any other examples you know of, we can do small one-off fixes for these and then later think about a rearchitected config layer.