mirror of
git://git.code.sf.net/p/cdesktopenv/code
synced 2025-02-13 11:42:21 +00:00
Mitigate PWD race condition in non-forking subshells
Virtual/non-forking subshells that change the present working
directory (PWD) with 'cd' suffer from a serious race condition. The
PWD is changed within the same process. This means it may not be
possible to change back to the original PWD when exiting the
subshell, as some other process may destroy the PWD or modify its
permissions in the meantime. ksh did not handle this error
condition at all, so, after exiting a subshell that invoked 'cd',
it could silently end up running the script's following command(s)
in the wrong directory. Which might be 'rm -rf *'. So, ouch.
The proper and obvious fix is never to allow a virtual subshell to
change the PWD, as it can never be guaranteed you can return to a
previous directory. If the PWD is changed in a child process, there
is no need to restore it in the parent process, and this whole
problem is avoided. So subshells really should always fork on
encountering a 'cd' command.
But forking is slow. It is not uncommon for scripts to 'cd' in a
subshell that is run repeatedly in a loop.
There is also the issue of custom builtins that can be added to ksh
via shared libraries. In the standard shell language, 'cd' is the
only command that changes the PWD, so we could just make that
command fork the subshell it is run from. But there's no telling
what a custom builtin might do.
So this commit implements a compromise that will not affect
performance unless there is the pathological condition of a PWD
that has been rendered inaccessible in some way:
1. When entering a virtual subshell, if the parent shell's PWD
proves inaccessible upon saving it, the subshell will now fork into
a separate process, avoiding the unrestorable PWD problem.
2. If some attack renders the parent shell's PWD unrestorable
*after* ksh enters a virtual subshell, ksh will now error out when
exiting it. There is nothing else left to do then. Continuing would
mean running arbitrary commands in the wrong PWD.
src/cmd/ksh93/sh/subshell.c:
- Put all the code/variables only needed for fchdir() behind '#if
_lib_fchdir'. This makes it clearer what's what.
(I don't know if there is still any system out there without
fchdir(3); I haven't found any. The chdir(3) fallback version may
be removed later as there is no way to make it remotely secure.)
- Fix the attempt to use the O_PATH mode for open(2) as a fallback
for nonexistent O_SEARCH on Linux. Define _GNU_SOURCE on Linux,
or <fcntl.h> (which is included indirectly) won't define O_PATH.
- Fix use of O_SEARCH. The code was simply wrong, repeating an
open(".",O_RDONLY) instead. Since a nonexistent O_SEARCH is now
redefined as either O_PATH or O_RDONLY, we can simply
open(".",O_SEARCH) and be done with it.
- Fix fatal error handling. Introduce fatal error condition for
failure to fchdir(3) back to the parent's PWD; rename 'duped' to
'fatalerror' and use it for error numbers; save and restore errno
on fatal error so the message will report the cause. (We must
call errormsg() near the end of sh_subshell() to avoid crashes.)
- If open(".",O_SEARCH) was not able get a file descriptor to our
PWD on entry, then call sh_subfork() immediately before running
the subshell commands. (Forking earlier causes a crash.)
- When restoring the PWD, if fchdir(3) fails, do *not* fall back to
chdir(3). We already know the PWD is inaccessible, so if chdir(3)
"succeeds" then, it's very likely to be a substitute injected by
an attacker.
src/cmd/ksh93/bltins/cd_pwd.c:
- If we don't have fchdir(3), then sh_subshell() must fall back to
chdir(2) to restore the PWD. That is highly vulnerable, as a
well-timed rename would allow an attacker to usurp the PWD. We
can't do anything about that if some custom builtin changes the
PWD, but we can at least make 'cd' always fork a subshell, which
slows down ksh but removes the need for the parent shell ever to
restore the PWD. (There is certainly no popular system where this
is relevant and there might not be any such current system.)
This commit adds no regression test because a portable regression
test is not really doable. Different kernels, external /bin/pwd
utilities, etc. all have quite different behaviour under the
pathological condition of an inaccessible PWD, so both the
before-fix and the after-fix behaviour differs. See link below.
Resolves: https://github.com/ksh93/ksh/issues/141
Thanks to Stéphane Chazelas for the bug report.
This commit is contained in:
Martijn Dekker
parent
4ae962aba6
commit
dd9bc22928
4 changed files with 70 additions and 18 deletions
14
NEWS
14
NEWS
|
|
@ -3,6 +3,20 @@ For full details, see the git log at: https://github.com/ksh93/ksh
|
|||
|
||||
Any uppercase BUG_* names are modernish shell bug IDs.
|
||||
|
||||
2020-10-06:
|
||||
|
||||
- The security of virtual/non-forking subshells that locally change the present
|
||||
working directory (PWD) using 'cd' has been improved in two ways.
|
||||
1. On entering a subshell, if the parent shell's PWD proves inaccessible upon
|
||||
saving it, the subshell will now fork into a separate process so the
|
||||
parent process never changes its PWD, avoiding the need to restore it.
|
||||
2. If some attack renders the parent shell's PWD unrestorable *after* ksh
|
||||
enters a virtual subshell, ksh will now error out on exiting it, as
|
||||
continuing would mean running arbitrary commands in the wrong PWD.
|
||||
Hopefully this is an acceptable compromise between performance and security.
|
||||
The proper fix would be to always fork a subshell when changing the working
|
||||
directory within it, but the resulting slowdown would likely be unpopular.
|
||||
|
||||
2020-09-30:
|
||||
|
||||
- Fixed: 'typeset -xu' and 'typeset -xl' (export + change case) failed to
|
||||
|
|
|
|||
|
|
@ -92,6 +92,14 @@ int b_cd(int argc, char *argv[],Shbltin_t *context)
|
|||
dir = nv_getval(opwdnod);
|
||||
if(!dir || *dir==0)
|
||||
errormsg(SH_DICT,ERROR_exit(1),argc==2?e_subst+4:e_direct);
|
||||
#if !_lib_fchdir
|
||||
/*
|
||||
* If sh_subshell() in subshell.c cannot use fchdir(2) to restore the PWD using a saved file descriptor,
|
||||
* we must fork any virtual subshell now to avoid the possibility of ending up in the wrong PWD on exit.
|
||||
*/
|
||||
if(shp->subshell && !shp->subshare)
|
||||
sh_subfork();
|
||||
#endif /* !lib_fchdir */
|
||||
#if _WINIX
|
||||
if(*dir != '/' && (dir[1]!=':'))
|
||||
#else
|
||||
|
|
|
|||
|
|
@ -17,4 +17,4 @@
|
|||
* David Korn <dgk@research.att.com> *
|
||||
* *
|
||||
***********************************************************************/
|
||||
#define SH_RELEASE "93u+m 2020-09-30"
|
||||
#define SH_RELEASE "93u+m 2020-10-06"
|
||||
|
|
|
|||
|
|
@ -26,6 +26,10 @@
|
|||
*
|
||||
*/
|
||||
|
||||
#ifdef __linux__
|
||||
#define _GNU_SOURCE /* needed for O_PATH */
|
||||
#endif
|
||||
|
||||
#include "defs.h"
|
||||
#include <ls.h>
|
||||
#include "io.h"
|
||||
|
|
@ -44,7 +48,7 @@
|
|||
# ifdef O_PATH
|
||||
# define O_SEARCH O_PATH
|
||||
# else
|
||||
# define O_SEARCH 0
|
||||
# define O_SEARCH O_RDONLY
|
||||
# endif
|
||||
#endif
|
||||
|
||||
|
|
@ -79,7 +83,6 @@ static struct subshell
|
|||
char *pwd; /* present working directory */
|
||||
const char *shpwd; /* saved pointer to sh.pwd */
|
||||
void *jobs; /* save job info */
|
||||
int pwdfd; /* file descriptor for PWD */
|
||||
mode_t mask; /* saved umask */
|
||||
short tmpfd; /* saved tmp file descriptor */
|
||||
short pipefd; /* read fd if pipe is created */
|
||||
|
|
@ -96,7 +99,10 @@ static struct subshell
|
|||
int subdup;
|
||||
char subshare;
|
||||
char comsub;
|
||||
#if _lib_fchdir
|
||||
int pwdfd; /* file descriptor for PWD */
|
||||
char pwdclose;
|
||||
#endif /* _lib_fchdir */
|
||||
} *subshell_data;
|
||||
|
||||
static unsigned int subenv;
|
||||
|
|
@ -470,7 +476,7 @@ Sfio_t *sh_subshell(Shell_t *shp,Shnode_t *t, volatile int flags, int comsub)
|
|||
{
|
||||
struct subshell sub_data;
|
||||
register struct subshell *sp = &sub_data;
|
||||
int jmpval,isig,nsig=0,duped=0;
|
||||
int jmpval,isig,nsig=0,fatalerror=0,saveerrno=0;
|
||||
unsigned int savecurenv = shp->curenv;
|
||||
int savejobpgid = job.curpgid;
|
||||
int *saveexitval = job.exitval;
|
||||
|
|
@ -513,7 +519,9 @@ Sfio_t *sh_subshell(Shell_t *shp,Shnode_t *t, volatile int flags, int comsub)
|
|||
shp->pathinit = 0;
|
||||
}
|
||||
sp->pathlist = path_dup((Pathcomp_t*)shp->pathlist);
|
||||
#if _lib_fchdir
|
||||
sp->pwdfd = -1;
|
||||
#endif /* _lib_fchdir */
|
||||
if(!shp->pwd)
|
||||
path_pwd(shp,0);
|
||||
sp->bckpid = shp->bckpid;
|
||||
|
|
@ -533,7 +541,7 @@ Sfio_t *sh_subshell(Shell_t *shp,Shnode_t *t, volatile int flags, int comsub)
|
|||
{
|
||||
struct subshell *xp;
|
||||
sp->shpwd = shp->pwd;
|
||||
#ifdef _lib_fchdir
|
||||
#if _lib_fchdir
|
||||
for(xp=sp->prev; xp; xp=xp->prev)
|
||||
{
|
||||
if(xp->pwdfd>0 && strcmp(xp->pwd,shp->pwd)==0)
|
||||
|
|
@ -544,9 +552,7 @@ Sfio_t *sh_subshell(Shell_t *shp,Shnode_t *t, volatile int flags, int comsub)
|
|||
}
|
||||
if(sp->pwdfd<0)
|
||||
{
|
||||
int n = open(".",O_RDONLY);
|
||||
if(O_SEARCH && errno==EACCES)
|
||||
n = open(".",O_RDONLY);
|
||||
int n = open(".",O_SEARCH);
|
||||
if(n>=0)
|
||||
{
|
||||
sp->pwdfd = n;
|
||||
|
|
@ -562,7 +568,7 @@ Sfio_t *sh_subshell(Shell_t *shp,Shnode_t *t, volatile int flags, int comsub)
|
|||
}
|
||||
}
|
||||
}
|
||||
#endif
|
||||
#endif /* _lib_fchdir */
|
||||
sp->pwd = (shp->pwd?strdup(shp->pwd):0);
|
||||
sp->mask = shp->mask;
|
||||
sh_stats(STAT_SUBSHELL);
|
||||
|
|
@ -633,6 +639,10 @@ Sfio_t *sh_subshell(Shell_t *shp,Shnode_t *t, volatile int flags, int comsub)
|
|||
if(shp->savesig < 0)
|
||||
{
|
||||
shp->savesig = 0;
|
||||
#if _lib_fchdir
|
||||
if(sp->pwdfd < 0 && !shp->subshare) /* if we couldn't get a file descriptor to our PWD ... */
|
||||
sh_subfork(); /* ...we have to fork, as we cannot fchdir back to it. */
|
||||
#endif /* _lib_fchdir */
|
||||
sh_offstate(SH_INTERACTIVE);
|
||||
sh_exec(t,flags);
|
||||
}
|
||||
|
|
@ -715,7 +725,10 @@ Sfio_t *sh_subshell(Shell_t *shp,Shnode_t *t, volatile int flags, int comsub)
|
|||
{
|
||||
close(1);
|
||||
if (fcntl(sp->tmpfd,F_DUPFD,1) != 1)
|
||||
duped++;
|
||||
{
|
||||
saveerrno = errno;
|
||||
fatalerror = 1;
|
||||
}
|
||||
sh_close(sp->tmpfd);
|
||||
}
|
||||
shp->fdstatus[1] = sp->fdstatus;
|
||||
|
|
@ -759,13 +772,15 @@ Sfio_t *sh_subshell(Shell_t *shp,Shnode_t *t, volatile int flags, int comsub)
|
|||
Namval_t *pwdnod = sh_scoped(shp,PWDNOD);
|
||||
if(shp->pwd)
|
||||
{
|
||||
if(sp->pwdfd >=0)
|
||||
#if _lib_fchdir
|
||||
if(fchdir(sp->pwdfd) < 0)
|
||||
#else
|
||||
if(chdir(sp->pwd) < 0)
|
||||
#endif /* _lib_fchdir */
|
||||
{
|
||||
if(fchdir(sp->pwdfd)<0)
|
||||
chdir(sp->pwd);
|
||||
saveerrno = errno;
|
||||
fatalerror = 2;
|
||||
}
|
||||
else
|
||||
chdir(sp->pwd);
|
||||
shp->pwd=sp->pwd;
|
||||
path_newdir(shp,shp->pathlist);
|
||||
}
|
||||
|
|
@ -780,8 +795,10 @@ Sfio_t *sh_subshell(Shell_t *shp,Shnode_t *t, volatile int flags, int comsub)
|
|||
}
|
||||
else
|
||||
free((void*)sp->pwd);
|
||||
#if _lib_fchdir
|
||||
if(sp->pwdclose)
|
||||
close(sp->pwdfd);
|
||||
#endif /* _lib_fchdir */
|
||||
if(sp->mask!=shp->mask)
|
||||
umask(shp->mask=sp->mask);
|
||||
if(shp->coutpipe!=sp->coutpipe)
|
||||
|
|
@ -838,11 +855,24 @@ Sfio_t *sh_subshell(Shell_t *shp,Shnode_t *t, volatile int flags, int comsub)
|
|||
if(sig==SIGINT || sig== SIGQUIT)
|
||||
kill(shgd->current_pid,sig);
|
||||
}
|
||||
if(duped)
|
||||
if(fatalerror)
|
||||
{
|
||||
((struct checkpt*)shp->jmplist)->mode = SH_JMPERREXIT;
|
||||
shp->toomany = 1;
|
||||
errormsg(SH_DICT,ERROR_system(1),e_redirect);
|
||||
switch(fatalerror)
|
||||
{
|
||||
case 1:
|
||||
shp->toomany = 1;
|
||||
errno = saveerrno;
|
||||
errormsg(SH_DICT,ERROR_system(1),e_redirect);
|
||||
case 2:
|
||||
/* reinit PWD as it will be wrong */
|
||||
shp->pwd = NULL;
|
||||
path_pwd(shp,0);
|
||||
errno = saveerrno;
|
||||
errormsg(SH_DICT,ERROR_system(1),"Failed to restore PWD upon exiting subshell");
|
||||
default:
|
||||
errormsg(SH_DICT,ERROR_system(1),"Subshell error %d",fatalerror);
|
||||
}
|
||||
}
|
||||
if(shp->ignsig)
|
||||
kill(shgd->current_pid,shp->ignsig);
|
||||
|
|
|
|||
Loading…
Reference in a new issue