[gopher] Re: Fwd: Bug#82602: gopherd: [SECURITY] gopherd is dangerous

[John Goerzen <jgoerzen@xxxxxxxxxxxx>]

Aaron Lehmann <aaronl@xxxxxxxxxxx> writes:

> 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 reason I did that was because there was no specific problem
reported.  A bug of "you suck" may be a bug but it's not critical :-)

When I found the specific lines of code you were talking about, I
fixed it, hence the later setting of it to "fixed".  If I had not
fixed them, I would have set it back to critical.

> > 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.

Agreed.

> 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.

Which I did.  Fortunately all of them were obscure :-)  Had you pasted
a line that occurs multiple times, it could have been more difficult.
I was just replying to the report on the face of it, to get an initial
analysis out quickly.  As you can see, when things like this happen, I
keep people up-to-date with progress even as circumstances and
analyses change.

> 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.

The current strategy works and makes it pretty easy to add things like
snprintf's because sizeof() works in about 90% of the cases I've
seen.  I think we can continue along that route for now, adding 'n'
calls wherever possible.

-- John


> 
> > Aaron Lehmann <aaronl@xxxxxxxxxxx> writes:
> > 
> > > From: aaronl@xxxxxxxxxxx
> > > Subject: Bug#82602: gopherd: [SECURITY] gopherd is dangerous
> > > To: submit@xxxxxxxxxxxxxxx
> > > 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@xxxxxxxxxxxxxxxx
> > > with a subject of "unsubscribe". Trouble? Contact 
> > > listmaster@xxxxxxxxxxxxxxxx
> > > 
> > > 
> > > 
> > > ----------
> > > 
> > > 
> > > -- 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@xxxxxxxxxxxx>                       www.complete.org
> > Sr. Software Developer, Progeny Linux Systems, Inc.    www.progenylinux.com
> > #include <std_disclaimer.h>                     <jgoerzen@xxxxxxxxxxxxxxxx>
> > 
> > 
> > 
> 
> 
> 

-- 
John Goerzen <jgoerzen@xxxxxxxxxxxx>                       www.complete.org
Sr. Software Developer, Progeny Linux Systems, Inc.    www.progenylinux.com
#include <std_disclaimer.h>                     <jgoerzen@xxxxxxxxxxxxxxxx>