Bug 243164

Comment 1 Ed Maste 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 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 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 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__,

(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.

(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 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.)

(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 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.

(In reply to Conrad Meyer from comment #4)

Do you think it's worth trying to upstream this?

I decided to propose this patch upstream:

https://github.com/zoulasc/blocklist/pull/8

More than committing the patch itself, I'm interested to hear the comments from the author.

Thank you!

The patch has been committed upstream.