[PATCH 1/2] report bad ELF interpreter if it causes exec to fail

[PATCH 1/2] report bad ELF interpreter if it causes exec to fail

[Kamil Dudka]

This is already implemented <https://bugzilla.redhat.com/60870> in some
distributions of bash and tcsh <https://bugzilla.redhat.com/711066>.

Steps to reproduce:

echo 'int main () { return 0; }' > u.c
gcc -o u u.c -Wl,-dynamic-linker,/foo/bar/baz
zsh -c ./u

Bug: https://bugzilla.redhat.com/711067
---
 Src/exec.c   | 133 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 configure.ac |   2 +-
 2 files changed, 131 insertions(+), 4 deletions(-)

diff --git a/Src/exec.c b/Src/exec.c
index 2a8185c..18408d7 100644
--- a/Src/exec.c
+++ b/Src/exec.c
@@ -30,6 +30,10 @@
 #include "zsh.mdh"
 #include "exec.pro"
 
+#ifdef HAVE_ELF_H
+# include <elf.h>
+#endif
+
 /* Flags for last argument of addvars */
 
 enum {
@@ -427,6 +431,112 @@ execcursh(Estate state, int do_exec)
     return lastval;
 }
 
+/* The following code is taken from <https://bugzilla.redhat.com/60870>. */
+#ifdef HAVE_ELF_H
+static int
+checkelfinterp(const char *pth, int fd, const char *sample, int sample_len)
+{
+    off_t offset = -1;
+    int eno = ENOENT;
+
+    /* Read the offset of the interpreter string. */
+    if (sample[EI_CLASS] == ELFCLASS32 && sample_len >= sizeof(Elf32_Ehdr)) {
+	Elf32_Ehdr ehdr;
+	Elf32_Phdr *phdr;
+	Elf32_Half nphdr;
+
+	/*
+	 * We have to copy the data since the sample buffer might not be
+	 * aligned correctly to be accessed as an Elf32_Ehdr struct.
+	 */
+	memcpy(&ehdr, sample, sizeof(Elf32_Ehdr));
+
+	nphdr = ehdr.e_phnum;
+	phdr = (Elf32_Phdr *) zalloc((size_t)nphdr * (size_t)ehdr.e_phentsize);
+	if (phdr != NULL) {
+	    if (lseek(fd, ehdr.e_phoff, SEEK_SET) != -1)
+		sample_len = read(fd, phdr, nphdr * ehdr.e_phentsize);
+	    else
+		sample_len = -1;
+
+	    if (sample_len == nphdr * ehdr.e_phentsize)
+		while (nphdr-- > 0)
+		    if (phdr[nphdr].p_type == PT_INTERP) {
+			offset = phdr[nphdr].p_offset;
+			break;
+		    }
+	    free(phdr);
+	}
+    } else if (sample[EI_CLASS] == ELFCLASS64
+	    && sample_len >= sizeof(Elf64_Ehdr)) {
+	Elf64_Ehdr ehdr;
+	Elf64_Phdr *phdr;
+	Elf32_Half nphdr;
+
+	/*
+	 * We have to copy the data since the sample buffer might not be
+	 * aligned correctly to be accessed as an Elf64_Ehdr struct.
+	 */
+	memcpy(&ehdr, sample, sizeof(Elf64_Ehdr));
+
+	nphdr = ehdr.e_phnum;
+	phdr = (Elf64_Phdr *) zalloc((size_t)nphdr * (size_t)ehdr.e_phentsize);
+	if (phdr != NULL) {
+	    if (lseek(fd, ehdr.e_phoff, SEEK_SET) != -1)
+		sample_len = read(fd, phdr,
+			nphdr * ehdr.e_phentsize);
+	    else
+		sample_len = -1;
+
+	    if (sample_len == nphdr * ehdr.e_phentsize)
+		while (nphdr-- > 0)
+		    if (phdr[nphdr].p_type == PT_INTERP) {
+			offset = phdr[nphdr].p_offset;
+			break;
+		    }
+	    free(phdr);
+	}
+    }
+
+    if (offset != -1) {
+	ssize_t maxlen = 0;
+	ssize_t actlen = 0;
+	ssize_t nread = 0;
+	off_t pos = 0;
+	char *interp = NULL;
+
+	for (;;) {
+	    if (actlen >= maxlen) {
+		char *newinterp = zrealloc(interp, maxlen += 200);
+		if (newinterp == NULL)
+		    /* out of memroy */
+		    break;
+		interp = newinterp;
+	    }
+
+	    if (lseek(fd, offset + pos, SEEK_SET) == -1)
+		break;
+
+	    if ((nread = read(fd, interp + pos, maxlen - pos)) == -1)
+		break;
+
+	    if (memchr(interp + pos, '\0', nread) != NULL) {
+		zerr("%s: %s: bad ELF interpreter", pth, interp);
+		eno = ENOEXEC;
+		break;
+	    }
+
+	    actlen += nread;
+	    pos += nread;
+	}
+
+	free(interp);
+    }
+
+    return eno;
+}
+#endif
+
 /* execve after handling $_ and #! */
 
 #define POUNDBANGLIMIT 64
@@ -468,12 +578,20 @@ zexecve(char *pth, char **argv, char **newenvp)
 	int fd, ct, t0;
 
 	if ((fd = open(pth, O_RDONLY|O_NOCTTY)) >= 0) {
+	    size_t hdrsize = POUNDBANGLIMIT;
+#ifdef HAVE_ELF_H
+	    /* Inspect 32 and 64 ELF */
+	    if (sizeof(Elf64_Ehdr) > hdrsize)
+		hdrsize = sizeof(Elf64_Ehdr);
+	    if (sizeof(Elf32_Ehdr) > hdrsize)
+		hdrsize = sizeof(Elf32_Ehdr);
+#endif
 	    argv0 = *argv;
 	    *argv = pth;
-	    ct = read(fd, execvebuf, POUNDBANGLIMIT);
-	    close(fd);
+	    ct = read(fd, execvebuf, hdrsize);
 	    if (ct > 0) {
 		if (execvebuf[0] == '#') {
+		    close(fd);
 		    if (execvebuf[1] == '!') {
 			for (t0 = 0; t0 != ct; t0++)
 			    if (execvebuf[t0] == '\n')
@@ -513,6 +631,7 @@ zexecve(char *pth, char **argv, char **newenvp)
 			execve("/bin/sh", argv - 1, newenvp);
 		    }
 		} else if (eno == ENOEXEC) {
+		    close(fd);
 		    for (t0 = 0; t0 != ct; t0++)
 			if (!execvebuf[t0])
 			    break;
@@ -521,7 +640,15 @@ zexecve(char *pth, char **argv, char **newenvp)
 			winch_unblock();
 			execve("/bin/sh", argv - 1, newenvp);
 		    }
-		}
+#ifdef HAVE_ELF_H
+		} else if (eno == ENOENT
+			&& ct > EI_NIDENT
+			&& memcmp(execvebuf, ELFMAG, SELFMAG) == 0) {
+		    eno = checkelfinterp(pth, fd, execvebuf, ct);
+		    close(fd);
+#endif
+		} else
+		    close(fd);
 	    } else
 		eno = errno;
 	    *argv = argv0;
diff --git a/configure.ac b/configure.ac
index e4de193..645385d 100644
--- a/configure.ac
+++ b/configure.ac
@@ -676,7 +676,7 @@ AC_CHECK_HEADERS(sys/time.h sys/times.h sys/select.h termcap.h termio.h \
 		 utmp.h utmpx.h sys/types.h pwd.h grp.h poll.h sys/mman.h \
 		 netinet/in_systm.h pcre.h langinfo.h wchar.h stddef.h \
 		 sys/stropts.h iconv.h ncurses.h ncursesw/ncurses.h \
-		 ncurses/ncurses.h)
+		 ncurses/ncurses.h elf.h)
 if test x$dynamic = xyes; then
   AC_CHECK_HEADERS(dlfcn.h)
   AC_CHECK_HEADERS(dl.h)
-- 
2.1.0

[PATCH 2/2] clear the heredoc list in case par_event() fails

[Kamil Dudka]

... in order to prevent SIGSEGV on the following input when running
in the interactive mode:

<<X;'
^C

^C

The console output looks like this:
kdudka% <<X;'
quote>
kdudka%
heredoc>
zsh: segmentation fault (core dumped)  zsh -f

Bug: https://bugzilla.redhat.com/972624
---
 Src/parse.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Src/parse.c b/Src/parse.c
index 91a81e1..374d18b 100644
--- a/Src/parse.c
+++ b/Src/parse.c
@@ -605,6 +605,7 @@ par_event(int endtok)
 	if (!par_event(endtok)) {
 	    ecused = oec;
 	    ecbuf[p] |= wc_bdata(Z_END);
+	    clear_hdocs();
 	}
     }
     return 1;
-- 
2.1.0

Re: [PATCH 1/2] report bad ELF interpreter if it causes exec to fail

[Daniel Shahaf]

Kamil Dudka wrote on Fri, Apr 17, 2015 at 15:25:54 +0200:
> +/* The following code is taken from <https://bugzilla.redhat.com/60870>. */
> +#ifdef HAVE_ELF_H
> +static int
> +checkelfinterp(const char *pth, int fd, const char *sample, int sample_len)

Is this code available under a license compatible with zsh's license?

The bug report doesn't state a license explicitly, so I presume any code
on the bug report is GPLed unless stated otherwise.

Sorry to be license nanny.

Cheers,

Daniel

Re: [PATCH 1/2] report bad ELF interpreter if it causes exec to fail

[Kamil Dudka]

On Friday 17 April 2015 13:49:57 Daniel Shahaf wrote:
> Kamil Dudka wrote on Fri, Apr 17, 2015 at 15:25:54 +0200:
> > +/* The following code is taken from <https://bugzilla.redhat.com/60870>.
> > */ +#ifdef HAVE_ELF_H
> > +static int
> > +checkelfinterp(const char *pth, int fd, const char *sample, int
> > sample_len)
> Is this code available under a license compatible with zsh's license?
> 
> The bug report doesn't state a license explicitly, so I presume any code
> on the bug report is GPLed unless stated otherwise.
> 
> Sorry to be license nanny.
> 
> Cheers,
> 
> Daniel

Good point.  I have asked the original author about this privately...

Kamil

Re: [PATCH 1/2] report bad ELF interpreter if it causes exec to fail

[Oliver Kiddle]

This behaviour got mentioned on one of the blogs that I tend to read
here:

http://utcc.utoronto.ca/~cks/space/blog/unix/BashSuperintelligentExec

I would agree with the conclusion of that blog writer that this is going
a little bit overboard. 

Also, I'm fairly sure that this would require linking with libelf on
some systems which is an added dependency.

Oliver

Re: [PATCH 1/2] report bad ELF interpreter if it causes exec to fail

[Philippe Troin]

On Fri, 2015-04-17 at 15:25 +0200, Kamil Dudka wrote:
> This is already implemented <https://bugzilla.redhat.com/60870> in some
> distributions of bash and tcsh <https://bugzilla.redhat.com/711066>.
> 
> Steps to reproduce:
> 
> echo 'int main () { return 0; }' > u.c
> gcc -o u u.c -Wl,-dynamic-linker,/foo/bar/baz
> zsh -c ./u
> 
> Bug: https://bugzilla.redhat.com/711067

This is such a corner case.
It shouldn't be the shell's job to reinterpret the errno codes returned
by the underlying OS.
This patch does not belong in the shell code IMHO, but at a lower-level.

I've noticed that this patch has not been accepted upstream in bash yet.

Phil.

Re: [PATCH 1/2] report bad ELF interpreter if it causes exec to fail

[Bart Schaefer]

On Fri, Apr 17, 2015 at 8:41 AM, Philippe Troin <phil@fifi.org> wrote:
>> Bug: https://bugzilla.redhat.com/711067
>
> This is such a corner case.
> It shouldn't be the shell's job to reinterpret the errno codes returned
> by the underlying OS.
> This patch does not belong in the shell code IMHO, but at a lower-level.

My thoughts exactly.  This is NOT the shell's responsibility and I
recommend rejecting this patch.

Re: [PATCH 1/2] report bad ELF interpreter if it causes exec to fail

[Kamil Dudka]

On Friday 17 April 2015 17:21:40 Oliver Kiddle wrote:
> This behaviour got mentioned on one of the blogs that I tend to read
> here:
> 
> http://utcc.utoronto.ca/~cks/space/blog/unix/BashSuperintelligentExec
> 
> I would agree with the conclusion of that blog writer that this is going
> a little bit overboard.
> 
> Also, I'm fairly sure that this would require linking with libelf on
> some systems which is an added dependency.
> 
> Oliver

Why would the patch require linking with libelf?

The patch does not rely on any symbols provided by the libelf library.  It 
just optionally uses <elf.h> if the header file is available during build.

Kamil

Re: [PATCH 2/2] clear the heredoc list in case par_event() fails

[Peter Stephenson]

On Fri, 17 Apr 2015 15:25:55 +0200
Kamil Dudka <kdudka@redhat.com> wrote:
> ... in order to prevent SIGSEGV on the following input when running
> in the interactive mode:
> 
> <<X;'
> ^C
> 
> ^C

I think that still leaves cruft around of other kinds.  I think the fix
is something like the following.

diff --git a/Src/parse.c b/Src/parse.c
index 91a81e1..985eb8e 100644
--- a/Src/parse.c
+++ b/Src/parse.c
@@ -605,6 +605,7 @@ par_event(int endtok)
 	if (!par_event(endtok)) {
 	    ecused = oec;
 	    ecbuf[p] |= wc_bdata(Z_END);
+	    return errflag ? 0 : 1;
 	}
     }
     return 1;

pws

Re: [PATCH 1/2] report bad ELF interpreter if it causes exec to fail

[Oliver Kiddle]

Kamil Dudka wrote:
> Why would the patch require linking with libelf?

My mistake sorry, I see the patch is using read() direct to the structures from
elf.h rather than calling routines. I guess it gets away without
handling byte order because that would result in an errno other than
ENOENT from exec.

Still I'm in agreement with Philippe and Bart on this one. The shell is
not the right place for this.

Oliver

Re: [PATCH 2/2] clear the heredoc list in case par_event() fails

[Kamil Dudka]

On Friday 17 April 2015 20:17:53 Peter Stephenson wrote:
> On Fri, 17 Apr 2015 15:25:55 +0200
> 
> Kamil Dudka <kdudka@redhat.com> wrote:
> > ... in order to prevent SIGSEGV on the following input when running
> > in the interactive mode:
> > 
> > <<X;'
> > ^C
> > 
> > ^C
> 
> I think that still leaves cruft around of other kinds.  I think the fix
> is something like the following.
> 
> diff --git a/Src/parse.c b/Src/parse.c
> index 91a81e1..985eb8e 100644
> --- a/Src/parse.c
> +++ b/Src/parse.c
> @@ -605,6 +605,7 @@ par_event(int endtok)
>  	if (!par_event(endtok)) {
>  	    ecused = oec;
>  	    ecbuf[p] |= wc_bdata(Z_END);
> +	    return errflag ? 0 : 1;
>  	}
>      }
>      return 1;
> 
> pws

Thanks for the patch!  This works for me.

Kamil

Re: [PATCH 2/2] clear the heredoc list in case par_event() fails

[Kamil Dudka]

[-- Attachment #1: Type: text/plain, Size: 921 bytes --]

On Friday, April 17, 2015 9:17:53 PM CET Peter Stephenson wrote:
> On Fri, 17 Apr 2015 15:25:55 +0200
> 
> Kamil Dudka <kdudka@redhat.com> wrote:
> > ... in order to prevent SIGSEGV on the following input when running
> > in the interactive mode:
> > 
> > <<X;'
> > ^C
> > 
> > ^C
> 
> I think that still leaves cruft around of other kinds.  I think the fix
> is something like the following.
> 
> diff --git a/Src/parse.c b/Src/parse.c
> index 91a81e1..985eb8e 100644
> --- a/Src/parse.c
> +++ b/Src/parse.c
> @@ -605,6 +605,7 @@ par_event(int endtok)
>  	if (!par_event(endtok)) {
>  	    ecused = oec;
>  	    ecbuf[p] |= wc_bdata(Z_END);
> +	    return errflag ? 0 : 1;
>  	}
>      }
>      return 1;
> 
> pws

Since the above patch was applied, zsh exits successfully (despite it reports 
parse error) on the attached example:

% zsh parse-error.sh; echo $?
parse-error.sh:4: parse error
0

Is this expected?

Kamil

[-- Attachment #2: parse-error.sh --]
[-- Type: application/x-shellscript, Size: 28 bytes --]

Re: [PATCH 2/2] clear the heredoc list in case par_event() fails

[Peter Stephenson]

On Thu, 2018-11-29 at 17:24 +0100, Kamil Dudka wrote:
> Since the above patch was applied, zsh exits successfully (despite it reports 
> parse error) on the attached example:
> 
> % zsh parse-error.sh; echo $?
> parse-error.sh:4: parse error
> 0
>
>...
>
> cat <<EOF
> $(print <<XXX
> EOF

I don't think the problem is with the changed code, which is signalling the
error as it should.  I think it's up above.

There's some slightly icky linkage between lex errors and the top level
requiring tok to be LEXERR.  The simple fix using the existing
signalling looks like the following.  I definitely don't think the tok =
LEXERR has a moral right to percolate through in the way it must
previously have been doing to avoid this, and the lexer does certainly
have the right to update the token when signalling a parse error, so
(famous last words) it's hard to see what could be wrong with this...

pws

diff --git a/Src/lex.c b/Src/lex.c
index fa29da3..f43bcc7 100644
--- a/Src/lex.c
+++ b/Src/lex.c
@@ -1613,6 +1613,7 @@ parsestr(char **s)
 		zerr("parse error near `%c'", err);
 	    else
 		zerr("parse error");
+	    tok = LEXERR;
 	}
     }
     return err;

Re: [PATCH 2/2] clear the heredoc list in case par_event() fails

[Bart Schaefer]

On Thu, Nov 29, 2018 at 9:42 AM Peter Stephenson
<p.stephenson@samsung.com> wrote:
>
> There's some slightly icky linkage between lex errors and the top level
> requiring tok to be LEXERR.  The simple fix using the existing
> signalling looks like the following.  I definitely don't think the tok =
> LEXERR has a moral right to percolate through in the way it must
> previously have been doing to avoid this, and the lexer does certainly
> have the right to update the token when signalling a parse error, so
> (famous last words) it's hard to see what could be wrong with this...

Might this need to be conditional upon ... something ... so that e.g.
${(z)...} on a malformed here-document doesn't throw an error?  I
don't have the code handy to poke around.

Re: [PATCH 2/2] clear the heredoc list in case par_event() fails

[Peter Stephenson]

On Thu, 29 Nov 2018 10:39:15 -0800
Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Thu, Nov 29, 2018 at 9:42 AM Peter Stephenson
> <p.stephenson@samsung.com> wrote:
> >
> > There's some slightly icky linkage between lex errors and the top level
> > requiring tok to be LEXERR.  The simple fix using the existing
> > signalling looks like the following.  I definitely don't think the tok =
> > LEXERR has a moral right to percolate through in the way it must
> > previously have been doing to avoid this, and the lexer does certainly
> > have the right to update the token when signalling a parse error, so
> > (famous last words) it's hard to see what could be wrong with this...  
> 
> Might this need to be conditional upon ... something ... so that e.g.
> ${(z)...} on a malformed here-document doesn't throw an error?  I
> don't have the code handy to poke around.

All this change is doing is setting the token to LEXERR in a case which
already loudly complains about the error and sets the error flag (this
is in a function parsestr() which calls parsetrnoerr() just above).

In most cases you'd expect the token in such a case already to be
LEXERR: Kamil's test includes an incomplete $( inside a here document so
there was some interesting nested parsing involved.

So anything reacting badly to the change in the token needs fixing.

pws

Re: [PATCH 2/2] clear the heredoc list in case par_event() fails

[Kamil Dudka]

On Thursday, November 29, 2018 6:34:04 PM CET Peter Stephenson wrote:
> On Thu, 2018-11-29 at 17:24 +0100, Kamil Dudka wrote:
> > Since the above patch was applied, zsh exits successfully (despite it
> > reports >
> > parse error) on the attached example:
> > 
> >
> > % zsh parse-error.sh; echo $?
> > parse-error.sh:4: parse error
> > 0
> >
> >...
> >
> > cat <<EOF
> > $(print <<XXX
> > EOF
> 
> I don't think the problem is with the changed code, which is signalling the
> error as it should.  I think it's up above.
> 
> There's some slightly icky linkage between lex errors and the top level
> requiring tok to be LEXERR.  The simple fix using the existing
> signalling looks like the following.  I definitely don't think the tok =
> LEXERR has a moral right to percolate through in the way it must
> previously have been doing to avoid this, and the lexer does certainly
> have the right to update the token when signalling a parse error, so
> (famous last words) it's hard to see what could be wrong with this...
> 
> pws
> 
> diff --git a/Src/lex.c b/Src/lex.c
> index fa29da3..f43bcc7 100644
> --- a/Src/lex.c
> +++ b/Src/lex.c
> @@ -1613,6 +1613,7 @@ parsestr(char **s)
>  		zerr("parse error near `%c'", err);
>  	    else
>  		zerr("parse error");
> +	    tok = LEXERR;
>  	}
>      }
>      return err;

Thanks for the quick fix!  It seems to work perfectly.

Kamil