[rbldnsd] PATCH: clearing sqi in rbldnsd_combined.c

[Michael Tokarev]

12.10.2015 03:14, Alex Lasoriti write:
> Hello rbldnsders,

Hello!

> we have found what appears to be a bug in 0.997a.  It may show up
> exclusively when using combined zones using both IPv4 and IPv6 datasets,
> resulting in "listed" responses to queries relative to unlisted objects.
>
> The problem is caused by lack of initialization of the sqi structure
> in rbldnsd_combined.c.  Therefore, that structure may contain
> information left from the previous query.  Under some circumstances,
> that information is returned as an answer, causing unlisted objects
> to appear listed.  We observed this in testing and tracked it down.
>
> This patch fixes it:
>
> --- rbldnsd-0.997a/rbldnsd_combined.c 2013-03-14 05:28:02.000000000 +0000
> +++ rbldnsd-0.997a-patched/rbldnsd_combined.c 2015-08-18 14:05:38.000000000 +0000
> @@ -190,10 +190,12 @@
>    struct dnsqinfo sqi;
>    const struct dslist *dsl;
>    int found = 0;
> -  const struct zone *zone =
> -    findqzone(ds->ds_dsd->zlist,
> -              qi->qi_dnlen0 + 1, qi->qi_dnlab, qi->qi_dnlptr,
> -              &sqi);
> +  const struct zone *zone;
> +
> +  memset(&sqi, 0, sizeof(sqi));
> +  zone = findqzone(ds->ds_dsd->zlist,
> +           qi->qi_dnlen0 + 1, qi->qi_dnlab, qi->qi_dnlptr,
> +           &sqi);
>    if (!zone) return 0;
>    sqi.qi_tflag = qi->qi_tflag;
>    for (dsl = zone->z_dsl; dsl; dsl = dsl->dsl_next)

If the code needs zeroing the structure it means there's a deeper
bug somewhere.  By adding such an init we're hiding the bug more.

Let's see.

struct dnsqinfo {       /* qi */
  unsigned char *const *qi_dnlptr;
  const unsigned char *qi_dn;           /* cached query DN */
  unsigned qi_tflag;                    /* query RR type flag (NSQUERY_XX) */
  unsigned qi_dnlen0;                   /* length of qi_dn - 1 */
  unsigned qi_dnlab;                    /* number of labels in q_dn */
  int qi_ip4valid;                      /* true if qi_ip4 is valid */
  int qi_ip6valid;                      /* true if qi_ip6 is valid */
  ip4addr_t qi_ip4;                     /* parsed IP4 address */
  ip6oct_t qi_ip6[IP6ADDR_FULL];        /* parsed IP6 address */
};

There are 2 functions which declares and fills this structure,
replypacket() and ds_combined_query().

qi_tflag is set in replypacket() based on the incoming query type.
In ds_combined_query() it is copied from the "base" query.

Next, findqzone() is called to fill in most other parts of the
structure (if matching zone is not found we don't do anything
with this query anymore).

findqzone() always initializes qi_dn, qi_dnlptr, qi_dnlab and qi_dnlen0.

This, together with qi_tflag set prior to calling findqzone(), gives
us whole beginning of this structure, excluding IP address information
(qi_ip[46]valu, qi_ip4 and qi_ip6 fields).

Now, the most interesting part.  The same findqzone(), at the end, fills
in all this information about IP address(es) being queried, but only if
zone->z_dstflags has one of DSTF_IP4REV|DSTF_IP6REV -- which means that
at least one of the dataset types attached to this zone is interested in
IPv4 or IPv6 address from the query. If there's no interest in this info,
this info isn't computed.

And sure thing, a dataset should only use qi_ip* info only if it declared
that it is interested in IPv4 or IPv6.

If you see that by using ip4 and ip6 datasets together inside a combined
dataset, you observe the "wronly listed" behavour due to some things
being initialized, this means that some zubzone of this combined dataset
uses qi_ip* info but ths subzone flags does not indicate such interest.

Which, in turn, means that the info about ip addresses is NOT EVEN COMPUTED,
so one of the functions actually does not work.  By filling this struct with
zeros you making sure this subzone will never work at all.

The actual problem is that we don't compute (sub)zone->z_dstflags value
inside the combined dataset.  And this is where it should be fixed,
instead of adding useless initializers which just hides the problem
(but makes it consistent).

(Count me proud of myself here, I still understand what's going on :))

> For good measure, we have also added a line to reset the qi structure
> in rbldnsd_packet.c :
>
> --- rbldnsd-0.997a/rbldnsd_packet.c 2013-04-06 16:28:53.000000000 +0000
> +++ rbldnsd-0.997a-patched/rbldnsd_packet.c 2015-10-10 17:25:54.000000000 +0000
> @@ -283,6 +283,7 @@
>    int found;
>    extern int lazy; /*XXX hack*/
>
> +  memset(&qi, 0, sizeof(qi));
>    pkt->p_substrr = 0;
>    /* check global ACL */
>    if (g_dsacl && g_dsacl->ds_stamp) {

No, this is also wrong, or, rather, unnecessary.

Note: only qi_ip* info can be left uninitialized, the
rest of this struct, as shown above, is always initialized.
So adding something "for good measure" does not help here.
It is enough to zero-init qi_ip[46]valid, this saves us
extra memory zeroing.

(Please excuse me for this probably-nonsense.  When I wrote
this code, I tried to optimize it as much as I can while keeping
it readable still. Extra memset() or especially a system call
made significant difference on the hardware we had at that time.
Both public nameservers I controlled at that time sometimes
maxed at CPU utilization, and each new optimization bought it
back to acceptable levels.

At this place, I never had any initializers like this, but it
is also because of the reason I already described, initializer
doesn't actually help.  And sure, we can always fill the qi_ip*
info no matter if it will be used or not --- it is yet another
optimization for things like dnset.

And I still think in these categories, even today when CPUs
become much, much faster.)

> These modifications have been already tested in a production environment.

I can imagine :)

But the problem is elsewhere.  Lemme try to address the real one,
hopefully I still can remember how the code works :)

BTW, Alex, you still haven't replied to my last question in the
previous thread, about second form of IPv4-in-IPv6 addresses.
Is it okay to cut off the remaining /48 from it, is it not a
useful info?

Thanks,

/mjt