----- Original Message -----
From: "Philip Hazel" <ph10@???>
To: <exim-users@???>
Sent: Thursday, February 14, 2002 2:16 PM
Subject: Re: [Exim] Fw: (bugtraq) Exim 3.34 and lower
> I have now analyzed the patch that was posted on bugtraq, and here are my
> comments for information and for the archive. Like Gaul, This long message
is
> divided into three parts:
>
> (1) An executive summary and overview of the posted patches.
> (2) My own patch for the 3 bugs that are recognized, and one that was
> nearby a bogus patch, but had been overlooked.
> (3) More detailed comments on the posted patches.
>
> Executive summary:
>
> Serious bugs found: 1
> Minor bugs found: 2
> Bugs introduced: 4
> Bugs overlooked: 1
> Bogus changes: 8
>
> The serious bug is the ability to crash Exim by calling it with -C
followed by
> an excessively long string. Note that if a non-privileged user did this,
Exim
> had already abdicated its root privilege, so this could not be exploited
by a
> non-admin user.
>
> The minor bugs are places where the lack of string length checking might
have
> caused problems. (I haven't checked to see if it could in practice.)
>
> The other patches seem to be the result of someone looking at calls to
strcpy()
> in the source, and more or less blindly changing them to strncpy().
> Unfortunately, this doesn't always work, because strncpy() is a rather
weird
> function, and people use it in different ways. For a discussion of
strncpy(),
> see David Wheeler's book on security at
>
> http://www.dwheeler.com/secure-programs/
>
> The author of the patch uses strncpy(a,b,n) to mean "copy string b to
buffer a,
> which is of length n". This is actually not sensible if n is a lot larger
than
> the length of string b. The reason it is not sensible is that strncpy()
insists
> on padding out the destination buffer with binary zeros to the length
given.
> So, if you call strncpy(a, "abc", 4096) for example, you waste a lot of
> resources writing 4092 unnecessary zeros, possibly touching memory pages
> which might otherwise have been left alone.
>
> I myself do not use strncpy() in that manner. I use strncpy(a,b,n) to mean
> "copy the first n characters of string b to buffer a". That is, I use it
to
> extract portions of strings. Of course, that doesn't provide any overflow
> protection. My code has to check that the buffer is big enough before
calling
> strncpy(). In a couple of places in the patch, the author has modified my
own
> used of strncpy() by changing the value of n. The effects of these changes
is
> to introduce bugs, because the wrong number of characters will be copied.
>
> It seems to me that a better way to achieve "copy string b to buffer a
with a
> check on not overflowing the buffer size n" is to use the sprintf()
function
> like this: sprintf(a, "%.*s", n-1, b). This copies no more than n-1
characters,
> and adds ONE zero on the end. In fact, there were two instances of this
usage a
> few lines up the code from the point where the serious bug was found. I'd
just
> forgotten to do it right in one case.
>
> The bogus changes were almost entirely strcpy() to strncpy() changes that
were
> unnecessary because the buffer length had been checked in advance. In the
Exim
> 4 source I have added comments at the bogus change points (those that
still
> exist in Exim 4 - some have disappeared) pointing out why those calls to
> strcpy() are OK. This is in the hope that the next auditor will read the
> comments. I haven't bothered to do this for Exim 3.
>
> Despite some of the published patches being bogus and incorrect, I am
grateful
> to those who took the time to run tests on Exim and to examine the code.
It
> isn't easy understanding somebody else's code, and my paradigms and
formatting
> conventions are different to many people's because of my personal
computing
> history. It's not surprising that some false alarms are raised. As long as
they
> don't come in their hundreds, this is fine. It causes me to look and
double
> check, which is no bad thing.
>
> I am about to go away for the weekend to the FOSDEM meeting in Brussels.
When I
> get back next week I'll put out a 3.35 release. There are a very small
number
> of other minor tweaks that have accumulated.
>
> Here follows my patch for Exim 3.34:
>
> --------------------------------------------------------------------------
-----
> *** exim-3.34/src/deliver.c Wed Dec 19 11:50:27 2001
> --- deliver.c Thu Feb 14 10:32:02 2002
> ***************
> *** 4068,4074 ****
> if (slen + len + 4 >= size)
> {
> int oldsize = size;
> ! size *= 2;
> if (!store_extend(h->text, oldsize, size))
> {
> char *newtext = store_get(size);
> --- 4068,4074 ----
> if (slen + len + 4 >= size)
> {
> int oldsize = size;
> ! while (slen + len + 4 >= size) size *= 2;
> if (!store_extend(h->text, oldsize, size))
> {
> char *newtext = store_get(size);
> *** exim-3.34/src/readconf.c Wed Dec 19 11:50:30 2001
> --- readconf.c Thu Feb 14 09:58:23 2002
> ***************
> *** 2240,2246 ****
>
> /* Finally, try the unadorned name */
>
> ! strcpy(big_buffer, config_filename);
> if (config_file == NULL) config_file = fopen(big_buffer, "r");
>
> /* Failure to open the configuration file is a serious disaster. */
> --- 2240,2246 ----
>
> /* Finally, try the unadorned name */
>
> ! sprintf(big_buffer, "%.256s", config_filename);
> if (config_file == NULL) config_file = fopen(big_buffer, "r");
>
> /* Failure to open the configuration file is a serious disaster. */
> *** exim-3.34/src/match.c Wed Dec 19 11:50:29 2001
> --- match.c Thu Feb 14 11:13:49 2002
> ***************
> *** 876,882 ****
> "+caseful" in the list, it restores a caseful copy from the original
address.
> */
>
> ! strcpy(address, origaddress);
> for (p = address + ((caseless || llen < 0)? 0 : llen); *p != 0; p++)
> *p = tolower(*p);
>
> --- 876,882 ----
> "+caseful" in the list, it restores a caseful copy from the original
address.
> */
>
> ! sprintf(address, "%.*s", big_buffer_size-1, origaddress);
> for (p = address + ((caseless || llen < 0)? 0 : llen); *p != 0; p++)
> *p = tolower(*p);
>
> *** exim-3.34/src/tree.c Wed Dec 19 11:50:31 2001
> --- tree.c Thu Feb 14 11:30:35 2002
> ***************
> *** 32,38 ****
> {
> char *p = s + (int)strlen(s);
> while (p > s && p[-1] != '@') p--;
> ! if (p <= s) strcpy(prepared_address, s); else
> {
> char *t = prepared_address;
> char *pp = p - 2;
> --- 32,38 ----
> {
> char *p = s + (int)strlen(s);
> while (p > s && p[-1] != '@') p--;
> ! if (p <= s) sprintf(prepared_address, "%.*s", sizeof(prepared_address) -
1, s);
> {
> char *t = prepared_address;
> char *pp = p - 2;
> --------------------------------------------------------------------------
-----
> --------------------------------------------------------------------------
-----
>
>
>
> Here follows my analysis of the patch that was posted on bugtraq:
>
>
> > diff -Nru exim-3.34/src.old/accept.c exim-3.34/src/accept.c
> > --- exim-3.34/src.old/accept.c Tue Feb 12 13:40:44 2002
> > +++ exim-3.34/src/accept.c Tue Feb 12 13:47:33 2002
> > @@ -1506,7 +1506,7 @@
> >
> > /* Save for comparing with next one */
> >
> > -strcpy(last_message_id, message_id);
> > +strncpy(last_message_id, message_id, MESSAGE_ID_LENGTH); /* Fixed a
one-byte overflow -- Mixter */
> >
> > /* Add the current message id onto the current process info string if
> > it will fit. */
>
> This is not only bogus, but incorrect. The code has just constructed
message_id
> to be of precisely the correct length, and the buffer is also set up to be
the
> correct size. Copying just the message length would fail to copy the
> terminating zero, which is a bug.
> --------------------------------------------------------------------------
-----
>
> > diff -Nru exim-3.34/src.old/deliver.c exim-3.34/src/deliver.c
> > --- exim-3.34/src.old/deliver.c Tue Feb 12 13:40:44 2002
> > +++ exim-3.34/src/deliver.c Tue Feb 12 14:15:53 2002
> > @@ -3704,7 +3704,7 @@
> > the message size. */
> >
> > deliver_force = forced;
> > -strcpy(message_id, id);
> > +strncpy(message_id, id, MESSAGE_ID_LENGTH);
> > return_count = 0;
> > message_size = 0;
>
> Same comment as above. Not copying the terminating zero might cause
problems.
> One worry is that the id here could come from the command line as part of
a -M
> or -q item. However, the arguments for those items are checked to ensure
that
> they are valid message ids. So we know that the length of id is OK.
> --------------------------------------------------------------------------
-----
>
> > @@ -4083,7 +4083,8 @@
> > slen += 3;
> > }
> >
> > - strcpy(h->text + slen, s);
> > + /* Fixed potential remote vulnerability -- Mixter */
> > + strncpy(h->text + slen, s, size-slen-1);
> > slen += len;
> > }
>
> This is bogus. The destination has been extended, if necessary, to be long
> enough, in the code a few lines above. However, there is a bug in that
code
> which wasn't spotted.
> --------------------------------------------------------------------------
-----
>
> > diff -Nru exim-3.34/src.old/host.c exim-3.34/src/host.c
> > --- exim-3.34/src.old/host.c Tue Feb 12 13:40:44 2002
> > +++ exim-3.34/src/host.c Tue Feb 12 19:19:52 2002
> > @@ -281,7 +281,7 @@
> > }
> >
> > sender_fullhost =
> > - store_malloc((int)strlen(fullhost) + (int)strlen(rcvhost) + 2);
> > + store_malloc((int)strlen(fullhost) + (int)strlen(rcvhost) + 3);
> > sender_rcvhost = sender_fullhost + (int)strlen(fullhost) + 1;
> > strcpy(sender_fullhost, fullhost);
> > strcpy(sender_rcvhost, rcvhost);
>
> Bogus. I don't understand the patch at all.
> --------------------------------------------------------------------------
-----
>
> > @@ -471,7 +471,7 @@
> >
> > next = store_malloc(sizeof(ip_address_item));
> > next->next = NULL;
> > - strcpy(next->address, s);
> > + strncpy(next->address, s, 46);
> >
> > if (yield == NULL) yield = last = next; else
> > {
>
> Bogus. The ip_address_item structure contains a vector that is large
enough for
> an IPv6 address. We know that "s" is a string representation of an IP
address.
> Therefore, it will fit.
> --------------------------------------------------------------------------
-----
>
> > @@ -571,7 +571,7 @@
> > /* If there is no buffer, put the string into some new store. */
> >
> > if (buffer == NULL) return string_copy(yield);
> > -strcpy(buffer, yield);
> > +strncpy(buffer, yield, 46);
> > return buffer;
> > }
>
> Bogus again. When this function is called with a non-NULL value of buffer,
it
> is known to be long enough to contain an IPv6 address.
> --------------------------------------------------------------------------
-----
>
> > diff -Nru exim-3.34/src.old/log.c exim-3.34/src/log.c
> > --- exim-3.34/src.old/log.c Tue Feb 12 13:40:44 2002
> > +++ exim-3.34/src/log.c Tue Feb 12 14:37:56 2002
> > @@ -61,6 +61,14 @@
> > if (!syslog_timestamp) s += 20;
> > len = (int)strlen(s);
> >
> > +/* Added safeguard against syslog overflows -- Mixter */
> > +if(len > 4096)
> > +{
> > + len = 4026;
> > + memset(s+4000,0,strlen(s)-4000);
> > + strcat(s, " WARNING: Message cut off!");
> > +}
> > +
> > #ifndef NO_OPENLOG
> > if (!syslog_open)
> > {
>
> This is a weird one. It appears to be truncating messages to syslog when
they
> are more than 4096 characters long. But if the person who generated the
patch
> had read the comments in the source just a few lines earlier, they would
have
> read this:
>
> /* The given string is split into sections according to length, or at
> embedded newlines, and syslogged as a numbered sequence if there is
more
> than one line. */
>
> The macro MAX_SYSLOG_LEN (defined as 1000) controls the length of string
which
> is actually passed to any one syslog call. Longer strings get sent as
several
> syslog calls, and the Exim manual explains all this in detail. So I don't
> understand this 4096 truncation.
>
> Incidentally, if you do want to truncate a string, there's no need to
memset()
> a huge amount of memory to zero. One zero is enough. Also, strcat() is a
> timewaster if you already know where the end of the string is.
> --------------------------------------------------------------------------
-----
>
> > @@ -185,7 +193,7 @@
> > has been cycled, then open the file. The static slot for saving it is
the same
> > size as buffer, and the text has been checked above to fit. */
> >
> > -if (strcmp(name, "main") == 0) strcpy(mainlog_name, buffer);
> > +if (strcmp(name, "main") == 0) strncpy(mainlog_name, buffer,
LOG_NAME_SIZE);
> >
> > /* After a successful open, arrange for automatic closure on exec(). */
>
> Bogus. The size of mainlog_name has been checked.
> --------------------------------------------------------------------------
-----
>
> > @@ -585,7 +593,7 @@
> > {
> > spaceleft = seplen + 1;
> > ptr = log_buffer + LOG_BUFFER_SIZE - spaceleft;
> > - strcpy(ptr - (int)strlen(tmsg), tmsg);
> > + strncpy(ptr - (int)strlen(tmsg), tmsg, spaceleft);
> > }
> > (void)string_format(ptr, spaceleft, separator);
> > while(*ptr) ptr++;
>
> Bogus. There is known to be enough space in the buffer.
> --------------------------------------------------------------------------
-----
>
> > diff -Nru exim-3.34/src.old/match.c exim-3.34/src/match.c
> > --- exim-3.34/src.old/match.c Tue Feb 12 13:40:45 2002
> > +++ exim-3.34/src/match.c Tue Feb 12 14:39:45 2002
> > @@ -876,7 +876,7 @@
> > "+caseful" in the list, it restores a caseful copy from the original
address.
> > */
> >
> > -strcpy(address, origaddress);
> > +strncpy(address, origaddress, big_buffer_size);
> > for (p = address + ((caseless || llen < 0)? 0 : llen); *p != 0; p++)
> > *p = tolower(*p);
>
> Minor bug. This is indeed safer if the copy is checked. I've rewritten it
> to use sprintf() because it is copying into what might be quite a large
buffer.
> --------------------------------------------------------------------------
-----
>
> > diff -Nru exim-3.34/src.old/readconf.c exim-3.34/src/readconf.c
> > --- exim-3.34/src.old/readconf.c Tue Feb 12 13:40:45 2002
> > +++ exim-3.34/src/readconf.c Tue Feb 12 14:25:01 2002
> > @@ -356,7 +356,7 @@
> > char *newbuffer;
> > big_buffer_size += BIG_BUFFER_SIZE;
> > newbuffer = store_malloc(big_buffer_size);
> > - strcpy(newbuffer, big_buffer);
> > + strncpy(newbuffer, big_buffer, big_buffer_size-1);
> > store_free(big_buffer);
> > big_buffer = newbuffer;
> > if (fgets(big_buffer+newlen, big_buffer_size-newlen, config_file)
== NULL)
>
> Bogus. We know the data will fit in the new buffer, because we've just got
a
> new buffer that is bigger than the old one, precisely because the old one
is
> too small.
> --------------------------------------------------------------------------
-----
>
> > @@ -440,7 +440,7 @@
> > {
> > int newsize = big_buffer_size + BIG_BUFFER_SIZE;
> > char *newbuffer = store_malloc(newsize);
> > - strcpy(newbuffer, big_buffer);
> > + strncpy(newbuffer, big_buffer, big_buffer_size-1);
> > s = newbuffer + (s - big_buffer);
> > ss = newbuffer + (ss - big_buffer);
> > t = newbuffer + (t - big_buffer);
>
> Bogus again. Same remark.
> --------------------------------------------------------------------------
-----
>
> > @@ -461,7 +461,7 @@
> > memmove(p + replen, pp, ss - pp + 1);
> > ss += moveby;
> > }
> > - strncpy(p, m->replacement, replen);
> > + strncpy(p, m->replacement, replen-2);
> > t = p + replen;
> > }
> > }
>
> This would introduce a bug. The value of replen is the number of
characters to
> be copied. I don't understand the patch.
> --------------------------------------------------------------------------
-----
>
> > @@ -2240,7 +2240,8 @@
> >
> > /* Finally, try the unadorned name */
> >
> > -strcpy(big_buffer, config_filename);
> > +/* Fixed overflow. 256 chars are maximally needed here. -- Mixter */
> > +strncpy(big_buffer, config_filename,
big_buffer_size>256?256:big_buffer_size);
> > if (config_file == NULL) config_file = fopen(big_buffer, "r");
> >
> > /* Failure to open the configuration file is a serious disaster. */
>
> This is the -C bug. My fix uses sprintf() because big_buffer is quite big,
and
> zeroing most of it is pointless.
> --------------------------------------------------------------------------
-----
>
> > @@ -2326,7 +2327,7 @@
> > m->next = NULL;
> > m->command_line = FALSE;
> > if (mlast == NULL) macros = m; else mlast->next = m;
> > - strcpy(m->name, name);
> > + strncpy(m->name, name, namelen-1); /* fixed potential overflow --
Mixter */
> > m->replacement = string_copy(s);
> > }
>
> This is another patch I don't understand. The length of the string that is
> being copied is "namelen". Reducing it by 1 will break the code. The line
> before the ones quoted gets an amount of store which is large enough for
the
> data.
> --------------------------------------------------------------------------
-----
>
> > diff -Nru exim-3.34/src.old/tree.c exim-3.34/src/tree.c
> > --- exim-3.34/src.old/tree.c Tue Feb 12 13:40:46 2002
> > +++ exim-3.34/src/tree.c Tue Feb 12 14:30:45 2002
> > @@ -32,7 +32,7 @@
> > {
> > char *p = s + (int)strlen(s);
> > while (p > s && p[-1] != '@') p--;
> > -if (p <= s) strcpy(prepared_address, s); else
> > +if (p <= s) strncpy(prepared_address, s, 512); else /* fixed potential
remote overflow -- Mixter */
> > {
> > char *t = prepared_address;
> > char *pp = p - 2;
>
> This is another place that could benefit from a doublecheck. I have
changed it
> to use sprintf().
> --------------------------------------------------------------------------
-----
>
> --
> Philip Hazel University of Cambridge Computing Service,
> ph10@??? Cambridge, England. Phone: +44 1223 334714.
>
>
>
>
>
