lynx-dev
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: lynx-dev brief look at lynx-2.8-4 RPM (and 2.8.1 tarball) for securi


From: dickey
Subject: Re: lynx-dev brief look at lynx-2.8-4 RPM (and 2.8.1 tarball) for security
Date: Fri, 18 Dec 1998 05:27:54 -0500 (EST)

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

it's not designed to be run setuid, and indeed will not work properly if
it is made setuid.
  
> 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) && 

yes - much of these things could be improved.  However some cannot be
improved much more without redesigning the whole program (or lessening its
portability).  If you have specific ideas for improving things, a patch
(if you're addressing one of the "improved" areas) would help.  For the
unrenovated areas - a reminder helps.
  
> 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. 

not a "lot" - perhaps a couple of dozen.  about 1/4 can be done with
non-shell code (such as removing files).  Copying files on VMS is non-trivial,
for example, since a copy that does not preserve the record structures is
worse than useless.

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

probably the latter - remember that Fote was primarily interested in VMS.

> When doing shell escapes $SHELL is used. 

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

might be - depends on what it was interfacing to inside the code.
  
> 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); 

"PWD" looks like a misfeature.
  
> 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.] 

I'm working on the fixed-buffer stuff, and am only about 1/3 through that
process.  You should look at the development version in
        http://www.slcc.edu/lynx/current/
to get an idea of what I'm doing.  When we're into pre-release, that would
be a good time to review those and see if I missed any.  (It's not simply
sprintf's, but strcpy's and strcat's into a fixed-buffer which are the
problem, of course).  Some of the sprintf/strcpy/strcat calls are fine as-is,
so bulk replacement won't help.
  
>         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. 

probably - on my list...
> # Antonomasia   address@hidden                      # 


-- 
Thomas E. Dickey
address@hidden
http://www.clark.net/pub/dickey

reply via email to

[Prev in Thread] Current Thread [Next in Thread]