patches follow...

Russ Allbery rra at stanford.edu
Sun Jun 10 05:31:00 UTC 2001


Bear Giles <bear at coyotesong.com> writes:

> Following are a series of patches against inn2_2.3.1, as distributed in
> the Debian source package.  Ironically, because I'm running a 2.2 server
> and haven't updated yet, I haven't been able to test the changes on a
> live server.  That shouldn't be a problem since the changes are so
> mechanical.  I have verified that the package builds, of course.

Thank you very much for all of this!

Unfortunately, much of your work duplicates work that was already done in
the current INN CVS; in particular, patches 1 through 4 and some of your
patch 5 has already gone into INN CURRENT.

CURRENT has snprintf; I'd be very interested to see your patch 6 redone
against CURRENT.  Unfortunately, porting from 2.3.1 to CURRENT will be a
reasonable amount of work given how much the code base has already
digressed.

Could you grab a current CVS snapshot from

    <ftp://ftp.isc.org/isc/inn/snapshots/>

from the CURRENT series and look at porting the parts of this that still
apply?  I'd really like to apply your patches but I'm not going to have
time personally to port them to CURRENT in the near future.

I've been holding off on making some of these changes myself because I'm
hoping to get both IPv6 and a new history API integrated before making
lots of mechanical code changes to make the merges easier.

> patch 6:
> cursory security audit.
> replaced sprintf with snprintf, where possible
> replaced strcpy with strncpy, where possible
> cleared buffers containing passwords after use, where possible.

> The need to avoid buffer overflows is obvious.  While it is possible
> to use sprintf(), strcpy() and strcat() safely, it is difficult and
> more importantly does not communicate important information to the
> programmer.

This is definitely a good idea.  The vast majority of sprintf and strcpy
invocations in INN are actually safe (several people in the past have
checked this, and the ones that are left from the original code base,
which is many of them, are actually pretty well-written).  But I
wholeheartedly agree that safe or not, they're dangerous and aren't the
direction that we should be going; snprintf should be used practically
everywhere instead now that CURRENT has its own version in case the system
one doesn't work or isn't present.

> 4) fopen() is usually checked, but not always, see frontends/sys2nf.c.

frontends/sys2nf.c is dead code; we should probably either move it to
contrib or just remove it.  It's not compiled by default, so it hasn't
been looked at in previous cleanups.  The problem that it's trying to
solve (conversion of C News sys files to newfeeds files) isn't one that
people need to solve much any more.

> Structural issues

> 1) the code still uses K&R function definitions, even though the README
> says that an ANSI C compiler is required and large chunks of the code
> will not compile without one.  Weirdest of all are the K&R definitions
> that use ANSI constructs, e.g., "const" or "void".  If an ANSI C
> compiler is required, the code should match.

This has been fixed in CURRENT in part; I've been holding off on cleaning
up the rest of it because of the other code we've been wanting to land.

> 2) function parameters should have 'const' added wherever possible.

This could probably use more work.

-- 
Russ Allbery (rra at stanford.edu)             <http://www.eyrie.org/~eagle/>


More information about the inn-workers mailing list