Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix an infinite loop related to $_ when ksh is /bin/sh #90

Merged
merged 2 commits into from
Jul 24, 2020

Conversation

JohnoKing
Copy link

An infinite loop can occur if ksh is the system's /bin/sh. The following explanation is mostly taken from Tomas Klacko's bug report on the old mailing list:

  1. When ksh starts a binary, it sets its environment variable $_ to "number/path/to/binary". Where "number" is the pid of the ksh process.

  2. The binary forks and the child executes a suid root shell script which begins with #!/bin/sh. For this bug to occur, ksh must be /bin/sh.

  3. The ksh process interpreting the suid shell script leaves the $_ variable as not set (nv_getval(L_ARGNOD) returns NULL) because the "number" from step 1 is not the pid of its parent process.

  4. Because $_ is not set and the script is suid root, the following if condition is true in src/cmd/ksh93/sh/main.c, which results in the following code being executed:

    ksh/src/cmd/ksh93/sh/main.c

    Lines 272 to 284 in f207cd5

    /*
    * try to undo effect of solaris 2.5+
    * change for argv for setuid scripts
    */
    if(((type = sh_type(cp = av[0])) & SH_TYPE_SH) && (!(name = nv_getval(L_ARGNOD)) || !((type = sh_type(cp = name)) & SH_TYPE_SH)))
    {
    av[0] = (type & SH_TYPE_LOGIN) ? cp : path_basename(cp);
    /* exec to change $0 for ps */
    execv(pathshell(),av);
    /* exec fails */
    shp->st.dolv[0] = av[0];
    fixargs(shp->st.dolv,1);
    }

  5. When the $SHELL environment variable contains "/bin/sh" pathshell() returns "/bin/sh". This becomes an infinite
    loop of /bin/sh /dev/fd/3 executing /bin/sh /dev/fd/3.

/dev/fd/3 is the suid root script passed in via file descriptor 3.

The first /bin/sh in the loop is the #!/bin/sh from the suid script. The second and subsequent /bin/sh in the loop is from the $SHELL environment variable.

The bugfix does the following:
- In get_lastarg(), the check for if the "number" refers to the process id of the parent process is removed.
- In sh_main(), prevent an infinite loop when $_ is not passed in from the environment.

The reproducer is a C program that sets all the necessary conditions and triggers the infinite loop when ksh is /bin/sh:

#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>
#include <sys/stat.h>
#include <sys/types.h>
#include <unistd.h>

#define ERROR \
do{ \
    fprintf(stderr, "[%s, %d] error\n", __FILE__, __LINE__); \
    exit(EXIT_FAILURE); \
}while(0)

int main(int argc, char *argv[])
{
    char devfd[]="/dev/fd/xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx";
    char *arg[5];
    int fd;

    if(argc!=2)
        ERROR;

    fd=open(argv[1], O_RDONLY);
    if(fd<0)
        ERROR;

    sprintf(devfd, "/dev/fd/%d", fd);

    arg[0]="sh";
    arg[1]=devfd;
    arg[2]="AAA";
    arg[3]="BBB";
    arg[4]=NULL;

    if(argc>1)
        unsetenv("_");
    else
        setenv("_", "*1234*/garbage", 1);

    setenv("SHELL", "/bin/sh", 1);

    execvp("/bin/sh", arg);

    ERROR;
}

Oracle applies this bugfix to their version of ksh:
https://github.com/oracle/solaris-userland/blob/master/components/ksh93/patches/190-17432413.patch

The following explanation is mostly taken from Tomas Klacko's report on
the old mailing list (which also contains a C program reproducer) [*]:

1. When ksh starts a binary, it sets its environment variable "_"
   to "*number*/path/to/binary". Where "number" is the pid of the
   ksh process.

2. The binary forks and the child executes a suid root shell script
   which begins with #!/bin/sh. For this bug to occur, ksh must be /bin/sh.

3. The ksh process interpreting the suid shell script leaves the "_"
   variable as not set (nv_getval(L_ARGNOD) returns NULL) because
   the "number" from step 1 is not the pid of its parent process.

4-5. Because "_" is not set and the script is suid root, an infinite
   loop occurs because when the SHELL environment variable contains
   "/bin/sh" pathshell() returns "/bin/sh". This becomes an infinite
   loop of /bin/sh /dev/fd/3 executing /bin/sh /dev/fd/3.

src/cmd/ksh93/sh/init.c: get_lastarg():
- Disable the check for if the "number" refers to the process id of
  the parent process.

src/cmd/ksh93/sh/main.c: sh_main():
- Prevent an infinite loop when '$_' is not passed in from the environment.

Solaris applies this bugfix to their version of ksh:
https://github.com/oracle/solaris-userland/blob/master/components/ksh93/patches/190-17432413.patch

[*]: https://www.mail-archive.com/[email protected]/msg01680.html
@JohnoKing JohnoKing changed the title Fix an infinite loop related to $_ if ksh is /bin/sh Fix an infinite loop related to $_ when ksh is /bin/sh Jul 23, 2020
@McDutchie
Copy link

I can't get the reproducer to work on Linux (Debian 10):

$ ./issue90 arch/*/bin/ksh
/dev/fd/3: 2: /dev/fd/3: Syntax error: ")" unexpected

…or on my Mac:

$ ./issue90 arch/*/bin/ksh
/dev/fd/3: /dev/fd/3: cannot execute binary file

…or on FreeBSD:

$ ./issue90 arch/*/bin/ksh
/dev/fd/3: ELF: not found
/dev/fd/3: @��@8: not found
/dev/fd/3: cannot open 5�����@�,����pw��z�
                                            ��ڮ͗�{q��l��M�y��v�: No such file or directory
/dev/fd/3: �O�4: not found
�]: not found3���G�
/dev/fd/3: �����=9�Z: not found
/dev/fd/3: 16: Syntax error: word unexpected (expecting ")")

…or even on Solaris 11.3 (where I can get ksh to compile and work now, though not on 11.4):

$ ./issue90 arch/*/bin/ksh
sh: sh: cannot execute [Exec format error]

What am I missing?

@JohnoKing
Copy link
Author

JohnoKing commented Jul 23, 2020

The reproducer should be given /dev/null as an argument (./issue90 /dev/null) and /bin/sh needs to be a symlink to ksh.

@McDutchie
Copy link

Confirmed, thanks.

@McDutchie McDutchie merged commit 8c16f38 into ksh93:master Jul 24, 2020
@JohnoKing JohnoKing deleted the fix-larg-infinite branch July 24, 2020 02:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants