freebsd security - Jan 2004 - short analysys of qmail integer overflow bug - let there be light

Hey folks.

There are rumors out there that setting /var/qmail/control/databytes to 
a reasonable value (for example 16384 = 16MB) will prevent the 
possibility of exploitation regarding the integer overflow in function 
blast().

That is not true.

This is how blast() is called:

void smtp_data() {
  int hops;
  unsigned long qp;
  char *qqx;
 
  if (!seenmail) { err_wantmail(); return; }
  if (!rcptto.len) { err_wantrcpt(); return; }
  seenmail = 0;
  if (databytes) bytestooverflow = databytes + 1;
  if (qmail_open(&qqt) == -1) { err_qqt(); return; }
  qp = qmail_qp(&qqt);
  out("354 go ahead\r\n");
 
 
received(&qqt,"SMTP",local,remoteip,remotehost,remoteinfo,fakehelo);
  blast(&hops);
  hops = (hops >= MAXHOPS);
  if (hops) qmail_fail(&qqt);
  qmail_from(&qqt,mailfrom.s);
  qmail_put(&qqt,rcptto.s,rcptto.len);
 
  qqx = qmail_close(&qqt);
  if (!*qqx) { acceptmessage(qp); return; }
  if (hops) { out("554 too many hops, this message is looping 
(#5.4.6)\r\n"); return; }
  if (databytes) if (!bytestooverflow) { out("552 sorry, that message 
size exceeds my databytes limit (#5.3.4)\r\n"); return; }
  if (*qqx == 'D') out("554 "); else out("451 ");
  out(qqx + 1);
  out("\r\n");
}

So you see, the input value is only checked against the databytes limit 
*after* the function blast() is called. The overflow resides in function 
blast(), thus even setting databytes to 1 (lowest possible value) will 
not prevent the overflow to happen.  People should not comment a bug 
without reading the code. (not from this mailing list).

The simplest way to fix is to define pos variable as unsigned int 
instead of int, in the blast() function.

What happens in blast()? Here what happens:

Input is read byte by byte, and if input byte != '\n', then pos is 
incremented. This is a neverending loop, without any logical tests. 
Thus, pos gets an "upper-bounds overflow" and becomes negative
(because
it is a signed integer). If it is defined as unsigned, it simply becomes 
0, and keeps being incremented in a neverending circle loop, untill the 
first '\n' is met. This renders any hack attack useless. DOS is 
possible, but DOS is possible in many other more effective ways. It's 
the way of the Internet protocols.

This is the original blast() function:

void blast(hops)
int *hops;
{
  char ch;
  int state;
  int flaginheader;
/* my comment here: unsigned int pos */
  int pos; /* number of bytes since most recent \n, if fih */
  int flagmaybex; /* 1 if this line might match RECEIVED, if fih */
  int flagmaybey; /* 1 if this line might match \r\n, if fih */
  int flagmaybez; /* 1 if this line might match DELIVERED, if fih */
 
  state = 1;
  *hops = 0;
  flaginheader = 1;
  pos = 0; flagmaybex = flagmaybey = flagmaybez = 1;
  for (;;) {
    substdio_get(&ssin,&ch,1);
    if (flaginheader) {
      if (pos < 9) {
        if (ch != "delivered"[pos]) if (ch !=
"DELIVERED"[pos])
flagmaybez = 0;
        if (flagmaybez) if (pos == 8) ++*hops;
        if (pos < 8)
          if (ch != "received"[pos]) if (ch !=
"RECEIVED"[pos])
flagmaybex = 0;
        if (flagmaybex) if (pos == 7) ++*hops;
        if (pos < 2) if (ch != "\r\n"[pos]) flagmaybey = 0;
        if (flagmaybey) if (pos == 1) flaginheader = 0;
      }
      ++pos;
      if (ch == '\n') { pos = 0; flagmaybex = flagmaybey = flagmaybez =
1; }
    }
    switch(state) {
      case 0:
        if (ch == '\n') straynewline();
        if (ch == '\r') { state = 4; continue; }
        break;
      case 1: /* \r\n */
        if (ch == '\n') straynewline();
        if (ch == '.') { state = 2; continue; }
        if (ch == '\r') { state = 4; continue; }
        state = 0;
        break;
      case 2: /* \r\n + . */
        if (ch == '\n') straynewline();
        if (ch == '\r') { state = 3; continue; }
        state = 0;
        break;
      case 3: /* \r\n + .\r */
        if (ch == '\n') return;
        put(".");
        put("\r");
        if (ch == '\r') { state = 4; continue; }
        state = 0;
        break;
      case 4: /* + \r */
        if (ch == '\n') { state = 1; break; }
        if (ch != '\r') { put("\r"); state = 0; }
    }
    put(&ch);
  }
}


One can see that pos is later used as an index for memory location. And 
that's all folks. :) I say it is exploitable. Just an opinion.


Cheers to all,
Alin-Adrian Anton.

Below there is a small rfc821 line too long implementing patch:

--- qmail-smtpd.c.orig    Mon Jun 15 13:53:16 1998
+++ qmail-smtpd.c    Mon Jan 19 23:29:35 2004
@@ -1,3 +1,15 @@
+/*
+*    This is a patched version of qmail, implementing RFC 821 regarding 
text line limitations.
+*    Developed by Alin-Adrian Anton 
(aanton@reversedhell.net,burebista@lasting.ro)
+*
+*    You may remove this banner if it annoys you. This patch is public 
domain, for the
+*    benefit of the community.
+*
+*    It also fixes an integer overflow in the blast() function.
+    NOTE: it implements the most relaxed RFC821, as it is specified there.
+*/
+
+
 #include "sig.h"
 #include "readwrite.h"
 #include "stralloc.h"
@@ -48,7 +60,6 @@
 void die_control() { out("421 unable to read controls (#4.3.0)\r\n");
flush(); _exit(1); }
 void die_ipme() { out("421 unable to figure out my IP addresses 
(#4.3.0)\r\n"); flush(); _exit(1); }
 void straynewline() { out("451 See 
http://pobox.com/~djb/docs/smtplf.html.\r\n"); flush(); _exit(1); }
-
 void err_bmf() { out("553 sorry, your envelope sender is in my 
badmailfrom list (#5.7.1)\r\n"); }
 void err_nogateway() { out("553 sorry, that domain isn't in my list of
allowed rcpthosts (#5.7.1)\r\n"); }
 void err_unimpl() { out("502 unimplemented (#5.5.1)\r\n"); }
@@ -58,7 +69,7 @@
 void err_noop() { out("250 ok\r\n"); }
 void err_vrfy() { out("252 send some mail, i'll try my
best\r\n"); }
 void err_qqt() { out("451 qqt failure (#4.3.0)\r\n"); }
-
+void err_longline() { out("500 Line too long, please read RFC 
821.\r\n"); flush(); _exit(1); }
 
 stralloc greeting = {0};
 
@@ -293,10 +304,46 @@
 void blast(hops)
 int *hops;
 {
+
+/*
+*RFC 821                                                      August 1982
+*                                           Simple Mail Transfer Protocol
+*
+*            text line
+*
+*               The maximum total length of a text line including the
+*               <CRLF> is 1000 characters (but not counting the leading
+*               dot duplicated for transparency).
+*
+*            recipients buffer
+*
+*               The maximum total number of recipients that must be
+*               buffered is 100 recipients.
+*
+*                                   
+*          ****************************************************
+*          *                                                  *
+*          *  TO THE MAXIMUM EXTENT POSSIBLE, IMPLEMENTATION  *
+*          *  TECHNIQUES WHICH IMPOSE NO LIMITS ON THE LENGTH *
+*          *  OF THESE OBJECTS SHOULD BE USED.                *
+*          *                                                  *
+*          ****************************************************
+*
+*         Errors due to exceeding these limits may be reported by using
+*         the reply codes, for example:
+*
+*            500 Line too long.
+*
+*            501 Path too long
+*
+*            552 Too many recipients.
+*
+*            552 Too much mail data.
+*/
   char ch;
   int state;
   int flaginheader;
-  int pos; /* number of bytes since most recent \n, if fih */
+  unsigned int pos; /* number of bytes since most recent \n, if fih */
   int flagmaybex; /* 1 if this line might match RECEIVED, if fih */
   int flagmaybey; /* 1 if this line might match \r\n, if fih */
   int flagmaybez; /* 1 if this line might match DELIVERED, if fih */
@@ -317,7 +364,8 @@
         if (pos < 2) if (ch != "\r\n"[pos]) flagmaybey = 0;
         if (flagmaybey) if (pos == 1) flaginheader = 0;
       }
-      ++pos;
+        if (++pos>65535-1) err_longline(); /* will bail out nicely with 
err 500 */
+                   
       if (ch == '\n') { pos = 0; flagmaybex = flagmaybey = flagmaybez =
1; }
     }
     switch(state) {