lynx-dev brief look at lynx-2.8-4 RPM (and 2.8.1 tarball) for security

[Antonomasia]

lynx-dev readers: programs commonly used under Linux are getting looked
at for their security properties.  Lynx is mainly of interest because of
remotaly-controlled input, but I've started on the more visible coding
matters such as race conditions, buffer overflows and use of the shell.


lynx is not set[ug]id by default.

LYUtils.c contains code to relax file permissions, seemingly used
as part of a download.  The S_ISREG follows a stat(), not an lstat().
This is also done during checks before a file move where aspects of
st_mode are checked.

PUBLIC void LYRelaxFilePermissions ARGS1(CONST char *, name)
    if (stat(name, &stat_buf) == 0 &&
        S_ISREG(stat_buf.st_mode) &&


Also I see a lot of system() and popen() where adverse shell behaviour is
tackled by quoting the command line arguments instead of avoiding the shell
altogether, which would be my preference.

    /*
     *  Quote the path to make it safe for shell command processing.
     *
     *  We use a simple technique which involves quoting the entire
     *  string using single quotes, escaping the real single quotes
     *  with double quotes. This may be gross but it seems to work.
     */
    PUBLIC char * quote_pathname ARGS1(
            char *,         pathname)
    {


File moves, removal and copying etc seem to be done via the shell when
there are straightforward ways to do this in C.  I'm not sure whether this
in intended to be a VMS, DOS portability aid.
When doing shell escapes $SHELL is used.


Variable 'fd' is used for the FILE * returned from popen().
Just a point of convention, 'fp' might be more suited.  principle
of least surprise and all that...


In LYDownload.c, line 279 $PWD is used in building the command
(prepending it to a filename)

        if (*buffer != '/')
            cp = getenv("PWD");
        else
            cp = NULL;
        if (cp) {
            sprintf(command, "%s/%s", cp, buffer);

command being a char [512] this looks like a buffer overflow, and
similar overflows occur in setting command with sprintf() elsewhere.
[This one I just checked in the very latest non-rpm lynx 2.8.1 at 
www.slcc.edu/lynx/release/ and it's still there.]

        if ((fp = fopen(buffer, "w")) != NULL) {
            fclose(fp);
            remove(buffer);

This looks like possible trouble if in /tmp.
It is preceded by a check on whether the file exists, but might be
beaten by a race to destroy the file of another user.


--
##############################################################
# Antonomasia   address@hidden                      #
# See http://www.notatla.demon.co.uk/                        #
##############################################################