[rbldnsd] Announce: rbldnsd-0.83, urgent security fix

[Michael Tokarev]

When auditing code, I realized that all versions of rbldnsd
since 0.2 contains a stupidiest bug in query parsing code -
there is just _no_ buffer bounds checks here _at all_.  Initial
code in 0.1 was correct, but it was "optimized" in 0.2, and
the check was removed altogether.

So I'm rolling new version now, 0.83, which corrects this idiotic
problem, and urgue to upgrade all production machines running
rbldnsd prior to 0.83 immediately.

http://www.corpit.ru/mjt/rbldnsd/rbldns_0.83.tar.gz -
http://www.corpit.ru/mjt/rbldnsd/rbldns_0.83.dsc -
   source code
http://www.corpit.ru/mjt/rbldnsd/rbldns_0.83.i386.deb -
   pre-built debian i386 binary package (glibc-2.3.1)
http://www.corpit.ru/mjt/rbldnsd/rbldnsd-0.83.i386.freebsd4.4.tar.gz
   pre-built FreeBSD 4.4 i386 binary (incl. docs) - not a package

News since 0.82:

0.83 (released 2003-04-19)

  - critical security fix in query parsing code - that check was
    here initially, in version 0.1, but was removed when I optimized
    that code.  Ugh!..

  - portability: 4.4 FreeBSD does not have mallinfo() and stdint.h
    (use appropriate -Ddefines, Makefile)

  - access control and filtering logging by IP

  - inlined qsort routine, speed up loading significantly.

  - removed some cruft from the code



The problem was in the query packed handling routine, which
is currently in rbldnsd_packet.c:replypacket():

WRONG:

int replypacket(struct dnspacket *p, int len, const struct zone *zone) {
   unsigned char *q, *x;
   unsigned char query[DNS_MAXDN+1];

   q = p->p + 12;
   x = p->p + len;
   ...
   x -= 4;
   while(*q)
     if (q + *q >= x) return 0;
     else q += *q + 1;
   qlen = ++q - (p->p + 12);
   dns_dntol(p->p + 12, query);
   ...

As is: there is NO bounds checking for query length (dns_dntol()
is like strcpy(), it expects that caller allocated enouth space).
`len' maximum is 512 bytes (DNS UDP packet size), and DNS_MAXDN
is 255.  DNS packet header size is 12 bytes long, and another 4
bytes should be after a domain name.  So maximum length of the
dn that this WRONG code will accept is 512 - 12 - 4 = 496... to
the 256-byte buffer...  Anything may be written to the stack
this way.

In 0.83, this code reads:

int replypacket(struct dnspacket *p, int len, const struct zone *zone) {
   unsigned char *q, *x;
   unsigned char query[DNS_MAXDN+1];

   q = p->p + 12;
   x = p->p + len;
   ...
   x -= 4;
   if (q + DNS_MAXDN < x) x = q + DNS_MAXDN; /* constrain query to MAXDN */
   while(*q)
     if (*q > DNS_MAXLABEL || q + *q >= x) return 0;
     else q += *q + 1;
   qlen = ++q - (p->p + 12);
   dns_dntol(p->p + 12, query);


Note I also added a check to ensure that every individual label
(component of a name, words between dots) fits within DNS protocol
standards (63 bytes).  Again, this check was here initially, but
was removed since.

I completed the code rewiev, but since such an idiotic bug has
been found, I'll do another round in a few days.

I apologize for this stupidiest bug I've ever did.

/mjt