[ccp4bb]: gets() / fgets()

[Christopher Putnam <cdputnam@scripps.edu>]

***  For details on how to be removed from this list visit the  ***
***    CCP4 home page http://www.dl.ac.uk/CCP/CCP4/main.html    ***


On Wed, 31 Jan 2001, Harry Powell wrote:

> ***  For details on how to be removed from this list visit the  ***
> ***    CCP4 home page http://www.dl.ac.uk/CCP/CCP4/main.html    ***
>
> On Wed, 31 Jan 2001, James Henry Thorpe wrote:
>
> > The initial configure using --with-x unfortunately didn't result in the
> > x-win stuff being built with the rest complaining that "'get' function is
> > dangerous and should not be used", so I'm attempting to do each in turn.
>
> That's probably the 'gets' function is dangerous etc. The only compiler I
> know that complains about it is gcc - but anyway, it's a warning (not an
> error) and doesn't stop the compilation & link. I believe that someone
> came up with a safe fix for it a year or so ago, but I've forgotten what
> it was :-(
>

Fix is really simple:

This is the sort of code you've got:

{
    char buf[512];
    gets( buf );
    /* ... more code ... */
}

which will cause a buffer overrun if more than 512 characters are placed
in the buffer and randomly overwrite otehr memory.  This can cause a
number of hard-to-trace bugs and is just a dangerous idiom.

This is also the source of the the vulnerabilities in BIND and a huge
number of other programs--by feeding the buffer the right sequence, you
can actually get it execute code buried in the buffer---though I don't
think people will use CCP4 to break into UNIX boxes.

The code to replace it is really simple:

{
    char buf[512];
    fgets( buf, sizeof(buf), stdin );
    /*  ... more code ... */
}

The header file needed for the prototypes of both functions is still
stdio.h

The only slight complication is if you are feeding fgets a dynamically
allocated memory block.  In this case sizeof(stringptr) will be the number
of bytes for the pointer, not the block of memory.  In this case, you'll
have to keep track of the allocated memory size---but you _should_ be
doing that in those cases too.  For example:

{
    char *s;
    int  alloc_size = 512;

    s = (char *) malloc ( sizeof(char) * alloc_size );
    if ( s == NULL ) {
        return (SOME_FAILURE_CODE);
    }

    fgets( s, alloc_size, stdin );
    /*  ... more code ... */
}


Actually, it looks like most of the CCP4 program use fgets()!  Hurrah!


The offending source that I see on our version (4.0 Jan. 2000) of the
distribution is in:  procheck/nb.c (3 occurences)

   gets(tmpstr);

All of them appear to be in the function main and can all be replaced by:

    fgets(tmpstr,sizeof(tmpstr),stdin);

with no other additional code added.



However, I do see another dangerous idiom using fgets() in a number of
files:

solomon_/cmsk_io.c  (8 occurences)
solomon_/solomon.c  (3 occurences)

    sscanf(fgets(buffer, 1024, file), " %lf %lf %lf", ..... );

If fgets() fails for whatever reason, it returns a NULL pointer.  This
will cause sscanf to coredump.  I also haven't looked through all the code
to make sure that a similarly dangerous expression doesn't exist:

    fgets(buffer, 1024, file);
    /* no check for buffer==NULL */
    sscanf(buffer, " %lf %lf %lf", .... );

There's a similar flaw in pltdev.c using the getenv() function:

CHKPOS(fprintf (ps,"( User: %s  Date: %s ) show\n",getenv("USER"),my_time));
CHKPOS(fprintf (ps,"LBUser: %s  Date: %s \003; \n",getenv("USER"),my_time));
CHKPOS(fprintf (stdout,"User: %s  Date: %s",getenv("USER"),my_time));

Hope this helps.

Chris.


Chris Putnam, Ph.D.
------------------------------------------------------------------------------
                                               Department of Molecular Biology
                                                The Scripps Research Institute
                                               10550 N. Torrey Pines Road, MB4
                                                            La Jolla, CA 92037
                                                          cdputnam@scripps.edu