procmail
[Top] [All Lists]

Re: Procmail test suite? (was: Re: MIME bugs)

1998-08-15 21:10:42
Liviu Daia <daia(_at_)stoilow(_dot_)imar(_dot_)ro> writes:
On 11 August 1998, D.A. Harris <rodmur(_at_)ecst(_dot_)csuchico(_dot_)edu> 
wrote:

Maybe the point of the note I saw was that there are numerous strcpy,
strcmp, and strcat's that exist in procmail's source, which might need
conversion to strncpy, etc, etc., so as to minimize potential future
buffer overflows.

   Well, as I said before, I don't think anybody in his right minds
would volunteer to dig through the code looking for overflows.  But
there is something that could be done about it with relatively minimal
effort: run Procmail against a memory debugger.  I happen to have
access to Insure++ here (that's a commercial memory debugger, similar
to but much better than Purify); so if anybody out there would care to
put together a kind of test suite, I'll run it, and post the results
here.  If there is enough interest for that I'll also post the coverage
analysis report so that the suite can be improved.  Comments?

Well, I've dug through the procmail source quite a bit, to deduce
procmail's behaviour in various odd ball situations, to track down a
couple bugs, and modify it (I actually have pre-alpha hacks to teach
procmail LMTP, but they're too ugly for sharing).  I know of two buffer
overflows in the procmail suite.  One was brought up on this list, only
affects formail, and involves an off-by-one bug in formail.c.  I've
only seen it manifested under glibc on a linux box, but any glibc
system on a little-endian machine (and possibly big-endian machines as
well) would probably see it (it depends on how your malloc() works).
I've included the patch below.

The other known buffer overflow involves the expansion of variables
into condition lines or variable assignments when the total length is
greater than $LINEBUF.  Triggering this generally indicates incorrect
assumptions on the part of the recipe writer.  I've thought about
fixing it (i.e., protecting the recipe writer from themselves), but
a) I really don't have the time right now, and b) it allows me to make
fun of recipe writers who forget ("That's a joke, son!  You're
supposed to laugh!").

Okay, other than known overflows, are there any?  Possibly.  procmail
was written to be as fast as possible given the design decisions, and
it mostly meets that criteria by way of being totally bummed** code.
I'm relatively trusting in it, despite the obscurity of the code, for a
combination of two reasons.  Having read the code and banged my head on
it, I can say that its author was very conscientious about the whole
issue (for example, there isn't a single staticly sized string in
procmail itself) and worked hard to be paranoid.  The other factor that
tends to keep me from having nightmares is that fact that other than
the variable expansion problem, no one has reported one.  If you think
about how much email is processed by procmail across the globe, on how
many different platforms, this seems like relatively good evidence.
I'm not stupid enough to claim that procmail doesn't have any unknown
overflows, but I personally file it below most of libc on the ladder of
buffer overflow risks.

As for developing a test suite for procmail, good luck.  I would
suggest that you dig up a copy of the source to MH 6.x.x and read the
first two paragraphs of the comment written by Van Jacobson at the top
of sbr/m_getfld.c regarding the hacking of m_getfld().***  A sample of
email, incoming and outgoing, from a random machine on the Internet
would probably be a good start from the coverage angle.


Philip Guenther


** bum 1. vt To make highly efficient, either in time or space, often
at the expense of clarity (Jargon File, 1992)

*** Okay, okay.  For those who can't do so I'll quote here:
   (Written by Van Jacobson for the mh6 m_getfld, January, 1986):

   This routine was accounting for 60% of the cpu time used by most mh
   programs.  I spent a bit of time tuning and it now accounts for <10%
   of the time used.  Like any heavily tuned routine, it's a bit
   complex and you want to be sure you understand everything that it's
   doing before you start hacking on it.  Let me try to emphasize
   that:  every line in this atrocity depends on every other line,
   sometimes in subtle ways.  You should understand it all, in detail,
   before trying to change any part.  If you do change it, test the
   result thoroughly (I use a hand-constructed test file that exercises
   all the ways a header name, header body, header continuation,
   header-body separator, body line and body eom can align themselves
   with respect to a buffer boundary).  "Minor" bugs in this routine
   result in garbaged or lost mail.

   If you hack on this and slow it down, I, my children and my
   children's children will curse you.



*** src/formail.c       1997/04/11 22:46:52     1.1.1.3
--- src/formail.c       1998/06/12 18:55:08
***************
*** 589,595 ****
           if(chp[-1]==HEAD_DELIMITER)
              if(*chp!=' '&&fldp->Tot_len>j+1)
               { chp=j+(*afldp=fldp=
!                 realloc(fldp,FLD_HEADSIZ+(i= ++fldp->Tot_len)))->fld_text;
                 tmemmove(chp+1,chp,i-j);*chp=' ';
               }
              else if(fldp->Tot_len<=j+2)
--- 589,595 ----
           if(chp[-1]==HEAD_DELIMITER)
              if(*chp!=' '&&fldp->Tot_len>j+1)
               { chp=j+(*afldp=fldp=
!                 realloc(fldp,FLD_HEADSIZ+(i=fldp->Tot_len++)+1))->fld_text;
                 tmemmove(chp+1,chp,i-j);*chp=' ';
               }
              else if(fldp->Tot_len<=j+2)

<Prev in Thread] Current Thread [Next in Thread>