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.
