Re: [PATCH] Fix system() behaviour when parameter is NULL

["Steven M. Bellovin" <smb%cs.columbia.edu@localhost>]

On Thu, 28 Aug 2008 20:15:40 +0200
mouss <mouss%netoyen.net@localhost> wrote:

> Andy Shevchenko wrote:
> > The ISO/IEC 9899:1999 describes in 7.20.4.6 behaviour of the
> > system() when its parameter is NULL. So, we should check a presence
> > of a command interpreter instead of returning 1.
> > ---
> >  lib/libc/stdlib/system.c |   11 +++++++++--
> >  1 files changed, 9 insertions(+), 2 deletions(-)
> > 
> > diff --git a/lib/libc/stdlib/system.c b/lib/libc/stdlib/system.c
> > index 4cc3cbe..2b729b5 100644
> > --- a/lib/libc/stdlib/system.c
> > +++ b/lib/libc/stdlib/system.c
> > @@ -64,8 +64,15 @@ system(command)
> >     const char *argp[] = {"sh", "-c", NULL, NULL};
> >     argp[2] = command;
> >  
> > -   if (command == NULL)            /* just checking... */
> > -           return(1);
> > +   /*
> > +    * ISO/IEC 9899:1999 in 7.20.4.6 describes this special
> > case.
> > +    * We need to check availability of a command interpreter.
> > +    */ 
> > +   if (command == NULL) {
> > +           if (access(_PATH_BSHELL, R_OK | X_OK) == 0)
> > +                   return 1;
> > +           return 0;
> > +   }
> >  
> >     sa.sa_handler = SIG_IGN;
> >     sigemptyset(&sa.sa_mask);
> 
> 
> Is using access() a "clean" way? access(2) says:
> 
> "access() is a potential security hole and should never be used."
> 
> I understand "never" as never ;-p
> 
Actually, the statement is wrong...  Rather, it needs a few more
qualifiers.  One of the original purposes of access() was to let
setuid() programs check if the real uid had certain privileges on the
file.  That doesn't work properly because of race condition attacks.
However, if you're not looking at that issue, it's a perfectly fine
system call.

Better wording of the warning might be something like

        Although access() checks the real uid's permissions, it should
        never be used for access permission checks by setuid() programs.


                --Steve Bellovin, http://www.cs.columbia.edu/~smb