patches follow...

[Bear Giles]

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.


patch1: 
replace INT32_T, U_INT32_T, SIZE_T, UID_T, GID_T, PID_T, MMAP_PTR
with ANSI equivalence

patch2: 
replace STATIC with static

patch3:
replace FREEVAL with void, FUNCTYPE with void, FUNCPTR with (*)()

patch 4:
replace POINTER with 'void *
replace CPOINTER with 'const void *'
replace STRING with 'char *'
deleted CSTRING - it was unused

I generally did not replace STRING with 'char const *' because it
would create a false impression that the existing 'char *' are required
to be non-const.

I also eliminated all casts to (void *) in function calls and
assignments.  'void *' is a special type - assignments *to* a
'void *' pointer should never require a cast, assignments *from*
a 'void *' pointer should always require one.

While the cast technically does not harm, casts in function arguments
are extremely dangerous because they prevent the compiler from doing
any type checking -- and encourages sloppy typing.

patch 5:
eliminated some, but not all, -Wall warnings.  Printf format errors
(usually long vs. int) were fixed, unused variables were usually deleted,
unused static procedures were commented out in a distinctive manner. 
(This is because deleting unused procedures often creates more unused 
procedures and variables.  The distinctive comments make it easy to
identify use in dead code when verifying warning.)

I usually did not attempt to fix warnings about ambiguous statements,
e.g. if (A && B || C) means ((A && B) || CC) or (AA && (BB || CC)).

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.

E.g., in a number of places I saw code which looked something like

  sprintf (_HDR(foo), "some new value");

where _HDR(foo) resolves to a 'char *' allocated to fit a string
earlier.  How does the programmer know that "some new value" will
fit into the allocated space?  Or does he?

A bit more commonly, the idiom

   char *s;

   s = NEW (char, (compute the required length));
   sprintf (s, "the stuff used above");

is safe, but it triggers advanced automatic search tools such as
"vi `find . -type f -exec grep -l sprintf {} \;". ;-)  It's usually
better to use

   char *s;
   size_t len;

   len = (compute the required length);
   s = NEW (char, len);
   snprintf (s, len, "the stuff used above");

For the same reason, I replaced some "strcpy()/strcat()/..."
sequences with snprintf().

I don't know how widely available snprintf() is - it may be necessary
to implement it in lib.


Security audit

This list is nonexhaustive

1) sprintf() and strcpy()/strcat() are extensively used.  In many
places it's not clear that overflow will be avoided.

2) some functions write to a char * buffer passed to them, but there
is no "size_t bufsiz" parameter specifying the size of that buffer.

3) malloc is often called, but results are often not checked before use.
The code either predates NEW()/xmalloc(), or the programmer wasn't
aware of them.

4) fopen() is usually checked, but not always, see frontends/sys2nf.c.
It isn't clear that an error condition has been checked, because of
functions that only open a standard file and leave it to the application 
to detect the problem and die.

5) strchr() is usually checked, but not always.  See innd/site.c,
lib/defdist.c.  It's not clear if there's some reason why these 
instances will never fail, or if it was an oversight.


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.

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

Bear Giles
bgiles (at) coyotesong (dot) com