Date: Wed, 17 Jan 2001 11:41:04 -0800
From: Aaron Lehmann <aaronl@vitelus.com>
To: gopher@complete.org
Cc: 82602@bugs.debian.org
Subject: [gopher] Re: Fwd: Bug#82602: gopherd: [SECURITY] gopherd is dangerous
Message-ID: <20010117114104.A26007@vitelus.com>
References: <20010116231004.A19307@vitelus.com> <87g0ii16o2.fsf@complete.org>
Mime-Version: 1.0
Content-type: text/plain; charset=us-ascii
Content-Disposition: inline
User-Agent: Mutt/1.3.12i
In-Reply-To: <87g0ii16o2.fsf@complete.org>;
 from jgoerzen@complete.org on Wed, Jan 17, 2001 at 11:31:57AM -0500
Content-Transfer-Encoding: 8bit
Sender: gopher-bounce@complete.org
Errors-to: gopher-bounce@complete.org
Precedence: bulk
Reply-to: gopher@complete.org


On Wed, Jan 17, 2001 at 11:31:57AM -0500, John Goerzen wrote:
> 
> severity 82602 normal
Execpt for the fact that gopher is not in wide use, I think the
severity "critical" would not be far off-track. See my comments below.
> The mere existance of sprintf, strcpy, and strcat does not mean that
> there is a bug.
Agreed. These functions are useful and encouraged. However, the ones
that I checked in the bug report seem to be serrious bugs. Since there
is an obvious presence of such bugs, the whole code base needs to be
audited, and with the sheer number of dangerous calls that might or
might not be bugs, it is very difficult to do.
> If the data being used is already of a known size,
> and that size is less than or equal to the location it is going, there
> is no problem.  Therefore, the grep is meaningless.
The grep is only meaningful as information on the diffuculty of
auditing the codebase. It is meaningless in terms of whether gopher
has bugs and/or security holes or not.
> For the rest, please provide specific file/line number references so
> that they can be checked to see if there is really a bug there or not.

What I provided in the bug report are merely examples of instances
that I think are currently serrious bugs. Some of them I'm not sure
about, but others (such as the <TITLE> code) are obvious
segfault-inducing bugs to any well-versed C programmer.

As far as providing line numbers goes, I considered it but decided
against it since I was looking in the CVS branch which tends to
change. If you want to look at the context of any of the examples I
provided, it is quite trivial to grep for a line I have quoted.

I think auditing and fixing this code-base would not be easy. If you
want to do it, the first thing you should decide on is a strategy for
handling strings. The current strategy is to allocate just about all
strings including pathnames, titles, menu entries, etc. as fixed-size
256-byte strings, and ignoring any possible buffer overflows. If
you're going to revamp the code base in order to remove the buffer
overflows, you might as well remove the silly and wasteful 256-char
limit. However, the alternative scheme is not obvious. Should all
variable-length strings be malloc'd? Who should have the
responsibility of freeing them? etc.

> Aaron Lehmann <aaronl@vitelus.com> writes:
> 
> > From: aaronl@vitelus.com
> > Subject: Bug#82602: gopherd: [SECURITY] gopherd is dangerous
> > To: submit@bugs.debian.org
> > Date: Tue, 16 Jan 2001 22:57:23 -0800
> > 
> > Package: gopherd
> > Version: 2.3.1-8
> > Severity: grave
> > 
> > 
> > First off:
> > 
> > $ egrep -r '(sprintf|strcpy|strcat)' * | wc -l
> >     539
> > 
> > *shudder*
> > 
> > 
> > Here are a few particular cases of fixed-size buffers that I think may
> > currently be security risks:
> > 
> >      char buf[256];
> > ...
> >       if (dochroot)
> >            sprintf(buf, "%s '%s'", decoder, pathname);
> >       else
> >            sprintf(buf, "%s '%s/%s'", decoder, Data_Dir, pathname);
> > 
> > As far as I can tell, neither decoder nor pathname is regulated in
> > size at all.
> > 
> > Here's another favorite:
> >      char         longname[256];
> > ...
> >            sprintf( longname, "%s  [%s%s%s, %ukb]", stitle,
> >               cdate+8,cdate+4,cdate+22, (statbuf.st_size+1023) / 1024);
> > 
> > Even if the length of stitle was regulated (which I doubt), it would
> > most likely be regulated to 256 bytes, which would be just as
> > disasterous.
> > 
> > Oh, and you had better hope that the path to your Data_Dir is < 256 chars:
> >      char tmpstr[256];
> > ...
> >             strcpy(tmpstr, Data_Dir);
> > 
> > Data_Dir is _not_ regulated in size:
> >       Data_Dir = strdup(argv[optind]);
> > ...
> >       Data_Dir = strdup(DATA_DIRECTORY);
> > 
> > How about this:
> > 
> >      if ((titlep = strcasestr(buf, "<TITLE>")) != NULL) {
> >       char *endtitle;
> >       char titletemp[256];
> > 
> >       titlep += 7;
> >       if ((endtitle = strcasestr(titlep, "</TITLE>")) != NULL) {
> >            strncpy(titletemp, titlep, (endtitle-titlep));
> >            titletemp[endtitle-titlep] = '\0';
> > 
> > So, list a directory containing a .html document with a title > 256
> > chars and you're likely to smash the stack.
> > 
> > I could go on and on. My reccomendation to the gopherd maintainer is
> > to throw out all of this code and write a more modern, secure
> > implentation from scratch. This is the worst C code I have ever read.
> > 
> > 
> > --  
> > To UNSUBSCRIBE, email to debian-bugs-dist-request@lists.debian.org
> > with a subject of "unsubscribe". Trouble? Contact listmaster@lists.debian.org
> > 
> > 
> > 
> > ----------
> > 
> > 
> > -- Attached file included as plaintext by Listar --
> > 
> > -----BEGIN PGP SIGNATURE-----
> > Version: GnuPG v1.0.4 (GNU/Linux)
> > Comment: For info see http://www.gnupg.org
> > 
> > iD8DBQE6ZUVMdtqQf66JWJkRAkfcAKC+DYo7IlV/uMhb9TiNFMehmoqDhQCfWdSG
> > D5NRK+qja4sbChxnEeh4m10=
> > =+VYC
> > -----END PGP SIGNATURE-----
> > 
> > 
> > 
> > 
> 
> -- 
> John Goerzen <jgoerzen@complete.org>                       www.complete.org
> Sr. Software Developer, Progeny Linux Systems, Inc.    www.progenylinux.com
> #include <std_disclaimer.h>                     <jgoerzen@progenylinux.com>
> 
> 
>