[Exim] dnslist bugs/segfaults [incl. patch]

Top Page
Delete this message
Reply to this message
Author: Andrew - Supernews
Date:  
To: exim-users
Subject: [Exim] dnslist bugs/segfaults [incl. patch]
Hi,

searching failed to turn up an address specifically for bug reports
so I'm posting this here.

exim-4.20 on FreeBSD-stable can be made to segfault on certain types
of DNSBL lookups due to bugs in the code. Specifically, there seems to
be a (false) assumption made that a successful call to
dns_basic_lookup (and hence res_search) will return at least one record
of the desired type. This does not happen in the case where a CNAME is
present for the desired record, but the target of the CNAME does not have
a record of the requested type; in this case the lookup is successful,
but only the CNAME is returned in the response.

The segfault occurs on a subsequent expansion of uninitialised pointer
values in the cache block; setting MALLOC_OPTIONS=J on FreeBSD makes
the problem easily reproducible (it tends not to be easily visible
otherwise).

Additionally, the behaviour in respect of lists that return multiple A
records (such as relays.osirusoft.com) is faulty; the original code
was extracting only the first (effectively random) A record and
ignoring the others, so "dnslist relays.osirusoft.com=127.0.0.2" might
fail to match on a listed host, simply because a different A record was
returned first. There was also the same CNAME-related bug as for TXT
records, though I did not find a real case that would trigger it. It
might be worth auditing all the DNS lookups to see if there are any
more bugs of this kind.

The appended patch addresses both of these issues, though the text that
ends up in $dnslist_value is still indeterminate in the multiple-A-record
case.

--
Andrew, Supernews
http://www.supernews.com -- individual and corporate NNTP services


*** work/exim-4.20/src/verify.c.orig    Mon Jun 23 04:00:07 2003
--- work/exim-4.20/src/verify.c    Mon Jun 23 04:34:41 2003
***************
*** 2165,2170 ****
--- 2165,2171 ----
      HDEBUG(D_dnsbl) debug_printf("new DNS lookup for %s\n", query);
      cb->rc = dns_basic_lookup(&dnsa, query, T_A);
      cb->text_set = FALSE;
+     cb->rhs = NULL;


      /* If the lookup succeeded, cache the RHS address. The code allows for
      more than one address - this was for complete generality and the possible
***************
*** 2172,2190 ****
      status (August 2001) and may die out. So they may never get used at all,
      let alone in dnsbl records. However, leave the code here, just in case. */


      if (cb->rc == DNS_SUCCEED)
        {
        dns_record *rr;
        for (rr = dns_next_rr(&dnsa, &dnss, RESET_ANSWERS);
         rr != NULL;
         rr = dns_next_rr(&dnsa, &dnss, RESET_NEXT))
      {
          if (rr->type == T_A)
            {
!           cb->rhs = dns_address_from_rr(&dnsa, rr);
!           break;
            }
      }
        }


      store_pool = old_pool;
--- 2173,2202 ----
      status (August 2001) and may die out. So they may never get used at all,
      let alone in dnsbl records. However, leave the code here, just in case. */


+     /* andrew@supernews - in fact, returning multiple A records from a DNSBL is
+     both common and useful, and exim was mishandling that case and behaving
+     nondeterministically. */
+
      if (cb->rc == DNS_SUCCEED)
        {
        dns_record *rr;
+       dns_address **addrp = &(cb->rhs);
        for (rr = dns_next_rr(&dnsa, &dnss, RESET_ANSWERS);
         rr != NULL;
         rr = dns_next_rr(&dnsa, &dnss, RESET_NEXT))
      {
          if (rr->type == T_A)
            {
!           dns_address *da = dns_address_from_rr(&dnsa, rr);
!           if (da)
!             {
!             *addrp = da;
!             while (da->next) da = da->next;
!             addrp = &(da->next);
!             }
            }
      }
+       if (cb->rhs == NULL) cb->rc = DNS_NODATA;
        }


      store_pool = old_pool;
***************
*** 2237,2245 ****
--- 2249,2262 ----
      /* Either there was no IP list, or the record matched. Look up a TXT record
      if it hasn't previously been done. */


+     /* andrew@supernews - important to leave cb->text valid even if no TXT record
+     is returned in a successful response to the DNS query, since with many lists
+     there will be a CNAME present. */
+
      if (!cb->text_set)
        {
        cb->text_set = TRUE;
+       cb->text = NULL;
        if (dns_basic_lookup(&dnsa, query, T_TXT) == DNS_SUCCEED)
          {
          dns_record *rr;
***************
*** 2256,2262 ****
            store_pool = old_pool;
            }
          }
-       else cb->text = NULL;
        }


      HDEBUG(D_dnsbl)
--- 2273,2278 ----