Subject: buffer overflow in c-client?

From: "Jaldhar H. Vyas" <[email-masked]>
Date: Thu, 7 Sep 2000 13:07:56 -0400 (EDT)

Hi Mark, 

some Debian users reported the following problem.  I was unable to
reproduce it myself.  However Leena has sent a bug report to the Pine
team which is pretty complete (and contains some excerpts from the Debian
discussion)

I think the problem is actually in c-client.  I wrote a patch to
src/osdep/unix/unix.c basically to use snprintf(3) instead of
sprintf(3) but I'm not sure that it was right.

I've run the c-client source through a scanner called its4 which looks for
potentially insecure programming practices.  It was most unsettling.  Why
in this day and age is there so much use of buffer overflow magnets like
sprintf(3) and gets(3) etc?

Your comments are greatly appreciated.

-- 
Jaldhar H. Vyas <[email-masked]>

---------- Forwarded message ----------
From: Juhapekka Tolvanen <[email-masked]>
To: [email-masked], [email-masked]
Date: Thu, 7 Sep 2000 18:15:28 +0300
Subject: [[email-masked]: bug in Pine 4.21 (Unix), possible mailbox corruption]

My friend iterated with Pine many times and in many ways and was able to
find situation, that makes pine crash always.

----- Forwarded message from Leena Heino <[email-masked]> -----

Date: Sun, 3 Sep 2000 00:10:24 +0300 (EET DST)
From: Leena Heino <[email-masked]>
To: [email-masked]
cc: [email-masked], [email-masked]
Subject: bug in Pine 4.21 (Unix), possible mailbox corruption
X-Sender: liinu@lipstikka

Short description:
Pine exits with Panic error message when email has a certain kind of
X-Keywords: header.

Long description:
Attached is a message to bugtraq that started this debugging. As to my
knowledge no solution has been presented to this problem. This "bug" is in
latest pine version 4.21.

Pine version: 4.21
System: Sun Solaris 2.6 with gcc c-compiler.

Presumptions:
- Mailbox does not have the hidden "Do not delete this mail" or this
hidden mail is corrupted and has to created.
- Mailbox has email with X-Keywords: header and this header is split into
two lines with second line beginning with "space" character
- Email with that special X-Keywords header is not deleted from mailbox

When above presumptions are present then pine will always exit with
this error message: Pine Panic: header size inconsistant

This bug is most likely in c-client library and therefore affects every
program that uses c-client library. This includes University of
Washington's IMAP and POP3 daemons.

Here's the relevant part from .pine-debug file:
    ---- QUIT SCREEN ----
- completely_done_with_adrbks -
expunge and close mail stream "/var/mail/mailbox"
about to end_tty_driver
Pine Panic: header size inconsistant

  -- Leena Heino ([email-masked])

It seems, that c-client libraries by University of Washington have
some bug(s), that makes some programs that depend upon those libraries
go crazy. AFAIK affected programs include at least Pine (read "pain"),
ipop3d and IMAPD. And those programs and libraries are commonly used in
Unixes. I don't know, if any patch, fix, work-around etc. exist.

 * * *

Problem was caused by my X-Keywords-header, that serves as so called spook line
(Hello, NSA! :-) ):

X-Keywords: kettutytöt, Sanna Sillanpää, IKL, Jammu Siltavuori, ryssä, somali,
lesbo, homo, lesbian, anarchism, nazi, communism, CIA, bomb, nuclear, Semtex,
satan, traitor, pedophile

I shortened it to this:

 X-Keywords: lesbo, homo, lesbian, anarchism, nazi, communism, CIA, bomb,
nuclear, Semtex, satan, traitor, pedophile

And then problems disappeared. I use a character set called ISO-LATIN-1. And my
original X-Keywords: -header had some scandinavic characters ("umlaut o"
aka "o with dots" and "umlaut a" aka "a with dots" ) in words
"kettutytöt" and "ryssä".

Here are some problem reports from mailing-lists of Debian:

 Date: Wed, 30 Aug 2000 23:52:12 +0200
 From: Cristian Ionescu-Idbohrn <[email-masked]>
 To: [email-masked]
 CC: [email-masked], [email-masked],
        [email-masked]
 Subject: imap mailbox killer

(Clip)

I don't know if it was your intension, but you managed to totally screw
up my inbox (no hard feelings)!

The IMAP daemon went crazy trying to make sense of that box and put it's
holy counts on the

  "Subject: DON'T DELETE THIS MESSAGE -- FOLDER INTERNAL DATA".

Is this a security hole?^X

 Date: Wed, 30 Aug 2000 15:31:12 -0700 (MST)
 To: Cristian Ionescu-Idbohrn <[email-masked]>
 cc: [email-masked]

(Clip)

I've been fighting this problem all day too.  Pine blows up when you try
to save the INBOX back out with any changes.  (I'm using fetchmail and
plain vanilla mail spool files.)  It was driving me nuts.  Thanks for
posting.  (I saved a copy of my mailbox and will pick through it with a
fine-tooth comb later.)

(Clip)

 Date: Thu, 31 Aug 2000 10:22:48 +0200 (CEST)
 From: Cristian Ionescu-Idbohrn <[email-masked]>
 To: Juhapekka Tolvanen <[email-masked]>
 cc: [email-masked]

(Clip)

Looks like all boxes get an extra message inserted. It looks something
like this:

,-----
| From MAILER-DAEMON  Wed Aug 30 09:54:25 2000
| Delivery-Date: Thu May 11 21:51:47 2000
| Date: Thu, 11 May 2000 21:51:47 +0200 (MET DST)
| From: Mail System Internal Data <[email-masked]>
| Subject: DON'T DELETE THIS MESSAGE -- FOLDER INTERNAL DATA
| X-IMAP: 0928135936 0000033614
| Status: RO
| X-Status:
| X-Keywords:
| X-UID: 2
|
| This text is part of the internal format of your mail folder, and is not
| a real message.  It is created automatically by the mail system software.
| If deleted, important folder data will be lost, and it will be re-created
| with the data reset to initial values.
`-----

I don't know if it's the IMAP daemon or the pine client who is responsible
for this.

One (or several) of Juhapekka message header entries, probably this:

,-----
| X-Keywords:
+=?iso-8859-1?Q?kettutyt=F6t=2C_Sanna_Sillanp=E4=E4=2C_IKL=2C_Jammu_Silta?=
|  =?iso-8859-1?Q?vuori=2C_ryss=E4=2C_somali=2C_lesbo=2C_homo=2C_lesbian=2C?=
|  =?iso-8859-1?Q?_anarchism=2C_nazi=2C_communism=2C_CIA=2C_bomb=2C_nuclear?=
|  =?iso-8859-1?Q?=2C_Semtex=2C_satan=2C_traitor=2C_pedophile?=
`-----

caused the daemon (or the client) screw up the "magic". I ended up with a
"magic" message looking like this:

,-----
| From MAILER-DAEMON Wed Aug 30 16:36:48 2000
| Date: 30 Aug 2000 16:36:48 +0200
| From: Mail System Internal Data <[email-masked]>
| Subject: DON'T DELETE THIS MESSAGE -- FOLDER INTERNAL DATA
| Message-ID: <[email-masked]>
| X-IMAP: 0967646162 0000000339
+=?iso-8859-1?Q?kettutyt=F6t=2C_Sanna_Sillanp=E4=E4=2C_IKL=2C_Jammu_Silta?=
| Status: RO
|
| This text is part of the internal format of your mail folder, and is not
| a real message.  It is created automatically by the mail system software.
| If deleted, important folder data will be lost, and it will be re-created
| with the data reset to initial values.
`-----

and a lot of NULL characters preceeding a few (5-6) of the messages in some
boxes.

Hope this helps to find the problem.
There's definitely a BUG lurking somewhere.

(Clip)

 Date: Thu, 31 Aug 2000 12:34:14 -0400 (EDT)
 From: "Jaldhar H. Vyas" <[email-masked]>
 Reply-To: "Jaldhar H. Vyas" <[email-masked]>
 To: Richard A Nelson <[email-masked]>
 cc: Juhapekka Tolvanen <[email-masked]>,
        Cristian Ionescu-Idbohrn <[email-masked]>, [email-masked],
        [email-masked]

(Clip)

> > There might be bug in either Pine or IMAP(D) or both.
>
> Both... I had to manually delete several messages in Pine 4.21 folders
> and I don't use IMAP
>

Pine also uses libc-client which is where the bug is.

(Clip)

 Date: Thu, 31 Aug 2000 12:31:03 -0400 (EDT)
 From: "Jaldhar H. Vyas" <[email-masked]>
 To: Buddha Buck <[email-masked]>
 cc: Richard A Nelson <[email-masked]>
        Juhapekka Tolvanen <[email-masked]>,
        Cristian Ionescu-Idbohrn <[email-masked]>, [email-masked],
        [email-masked]

(Clip)

> My school uses imap, but I didn't -directly- invoke it in this process.  It
> may have been invoked by their mailer behind the scenes, though.
>

Not necessarily.  However ipop3d and imapd both use the c-client library
for all the mail handling routines.  That's where the bug is so both would
have been affected.

(Clip)

--
Juhapekka "naula" Tolvanen * * * U of Jyväskylä * * [email-masked]
http://www.cc.jyu.fi/~juhtolv/index.html * "STRAIGHT BUT NOT NARROW!"
---------------------------------------------------------------------
"so impressed with all you do. tried so hard to be like you. flew too
high and burnt the wing. lost my faith in everything" nine inch nails

----- End forwarded message -----

-- 
Juhapekka "naula" Tolvanen * * * U of Jyväskylä * * [email-masked]
http://www.cc.jyu.fi/~juhtolv/index.html * "STRAIGHT BUT NOT NARROW!"
---------------------------------------------------------------------
"so impressed with all you do. tried so hard to be like you. flew too
high and burnt the wing. lost my faith in everything" nine inch nails

---

From: Mark Crispin <[email-masked]>
Date: Thu, 7 Sep 2000 11:27:45 -0700 (PDT)

The X-Keywords problem is not a buffer overflow.

Repeat.  The X-Keywords problem is *NOT* a buffer overflow.  I know that we
live in a world where every bug is called a buffer overflow security bug, but
that doesn't mean it so.

Did you notice that Pine crashed voluntarily with a message "Pine Panic:
header size inconsistant"?  This means that an assert-style bugtrap occurred,
not a buffer overflow.

The problem only affects imap-4.6 and imap-4.7.  It does not affect imap-2000.
The assert failure was that a preliminary calculation of the size of the
header of a message did not match the subsequent actual size.  The size
calculation is used to determine if the rewritten mailbox will fit within disk
quota.  It also validates that the overwrite and buffer management is going
correctly.  If it does not match the subsequent actual size, then there is a
serious question about the integrity of the data.  The program deliberately
crashed to avoid potentially causing worse damage.

In this particular case, the right thing would have happened if it had ignored
the problem and continued with the rewrite.  But the code didn't know that.

The specific cause of the problem was an internal header line (specifically,
Status, X-Status, X-UID, X-Keywords, X-IMAP, X-IMAPbase) which had a
continuation line.  Internal header lines are an unfortunate artifact of the
way that the standard UNIX mailbox format works.  They are supposedly only
written by the destination MUA, and never have continuation.  The code which
calculated the sizes didn't check for continuation.

The problem was discovered and fixed several months ago in the imap-2000
development cycle.  There have been several teething pains having to do with
the size calculations in the (much) faster mailbox rewrite algorithm that was
introduced in imap-4.6.  The old algorithm, which used a tmpfile(), didn't
need to check sizes; it just wrote the tmpfile and used the resulting size.

I'm sorry that these problems happened.  It was a large section of new code.
It got quite a bit of testing, however some obscure cases were missed.  That's
why there are beta tests.

> Why

Did you look to see what the gets() call was?  gets() is used in only one
program in the entire c-client distribution: a sample program called mtest,
which nobody actually uses in real life.

I hope that you don't believe that changing all occurances of sprintf() to
snprintf() in network protocol applications fixes buffer overflow security
bugs.  It creates new, much more obscure ones.  snprintf() just truncates the
text.  Really great idea to have a root level program validate a truncated
text, and let another routine act upon a less-truncated version of that text.

If you had a version of snprintf() that would abort() instead of silently
truncating the text, you might have an argument.  But that isn't how
snprintf() works.

There's also the small detail that snprintf() doesn't exist on all platforms,
including current offerings from major vendors.

The solution to buffer overflows is to fix the buffer overflow, not sweep it
under the rug with non-solutions like snprintf().

---

From: "Jaldhar H. Vyas" <[email-masked]>
Date: Thu, 7 Sep 2000 20:28:49 -0400 (EDT)

> header size inconsistant"?  This means that an assert-style bugtrap occurred,

I'm glad it's  not a buffer overflow.  But it is still a cause for
concern.  It seems to be corrupting peoples mailboxes.  Or causing pine 
to stop which is highly annoying to a user.  And what of
third-party products that use c-client (such as roxen web server and
PHP?)  If they aren't doing similar asserts they may be at risk.  And I
think it is reasonable for them to assume c-client is not munging the data
they feed it.

> the problem and continued with the rewrite.  But the code didn't know that.

As a distribution vendor, I have two choices

1.  Not bother about the problem.

2.  Immediately move to imap2000

3.  Backport  the fix to imap 4.7c

1 is out of the question for our users.  2, could be done.  I have been
packaging the release candidates but you seem to be so close to a full
release I really wanted to wait for that.  What do I need to do for 3?

> why there are beta tests.

No problem.  Let he who is without bugs cast the first stone. :-) 

> which nobody actually uses in real life.

Ok.

> text, and let another routine act upon a less-truncated version of that text.

snprintf returns -1 if truncation occured.  A defensive programmer would
check the return value and not let the program proceed at that stage.

> including current offerings from major vendors.

There are a number of free (not just GPL but less restrictive licenses
too.) implementations of snprintf available.  You could include one in
ansilib like you do for strtok etc.

> under the rug with non-solutions like snprintf().

I'm not saying that snprintf is the magic bullet that will fix everything.  
But the bounds checking it provides is a prudent measure.  Most security
experts I've talked to agree. 

Anyway, I'm relieved there isn't a bug and will inform my users.

---

From: Mark Crispin <[email-masked]>
Date: Thu, 7 Sep 2000 16:42:17 -0700 (PDT)

On Thu, 7 Sep 2000 20:28:49 -0400 (EDT), Jaldhar H. Vyas wrote:
> concern.  It seems to be corrupting peoples mailboxes.

The "corruption" consists of junk (the endof some other message) appended to
the end of a message, and possible duplication of messages after that point.
No data is lost; it's "extra junk" which is usually readily identifiable

> Or causing pine
> to stop which is highly annoying to a user.

This is the real problem; there is known bad message header data that triggers
the assert[-equivalent].  There is other known bad message header data that
does the same thing.

All known problems of this nature are fixed in imap-2000.

> And what of
> PHP?)  If they aren't doing similar asserts they may be at risk.

The problem isn't that they aren't doing the asserts; the problem is c-client
doing the assert.  If c-client ignored the condition and proceeded, the right
thing probably would have happened.

> release I really wanted to wait for that.  What do I need to do for 3?

I don't consider a backport to be practical.  It's not a "fix", it is "fixes".
In some cases, new code had to be written.  Much of the work in imap-2000 was
to identify all of the occurrances of these calculation errors and fix them.

If you really want to, you could remove the assert from imap-4.7 unix.c:
          if (j != elt->private.data) fatal ("header size inconsistent");
However, I *won't* take responsibility for the result; in a different
circumstance that assert may protect the mailbox from damage.  In other words,
you're on your own if you do that.

I consider (2) to be the only viable option.

> check the return value and not let the program proceed at that stage.

Not so fast.

Microsoft does -1.

The ISO C standard says:
       [...] The snprintf function returns the number of  characters
       that  would have been written had n been sufficiently large,
       not counting the  terminating  null  character.   Thus,  the
       null-terminated  output  has  been completely written if and
       only if the returned value is less than n.

Some Linux systems do -1, others do the ISO behavior.

> ansilib like you do for strtok etc.

Are you volunteering to supply an snprintf() implementation for all supported
c-client platforms that require it (yes, we really do have people using those
wierd ones), including testing which platforms have it and which don't?

Trust me.  You don't want to go there.  The ansilib functions are tiny by
comparison with snprintf().  None of them use varargs either.  Oh dear, we
have to deal with platform-dependent stack usage.

I don't want to go there either... :-)

> experts I've talked to agree.

I'm going to have a major belly laugh at the first security bug caused by
snprintf().  If people will go to the extreme of figuring out what machine
code to shove into an overflowed stack, they shouldn't stop at a mere
truncation problem.  Hey, it'd be portable, no more being immune because
you're not running Linux on an Intel CPU!

Better to read the code carefully and make sure that it can't happen.

> Anyway, I'm relieved there isn't a bug and will inform my users.

There is legitimate reason to contend that it represents a denial-of-service
problem, which is a form of security problem.