From: Herbert Xu <herbert@gondor.apana.org.au>
To: Nicola Lamacchia <nicola.lamacchia@gmail.com>
Cc: dash@vger.kernel.org,
Andrej Shadura <andrew.shadura@collabora.co.uk>,
874264@bugs.debian.org
Subject: [v2 PATCH] exec: Check executable bit when searching path
Date: Fri, 5 Apr 2024 17:55:46 +0800 [thread overview]
Message-ID: <Zg/Kom9EMBrBPqr5@gondor.apana.org.au> (raw)
In-Reply-To: <CA+z1gfX9ZexoQC8tm9V0hV_Vo3kMxxDw5GJdeNTy_DFGu_CMKg@mail.gmail.com>
On Mon, May 30, 2022 at 07:02:30PM +0200, Nicola Lamacchia wrote:
>
> the following patches `command -v` to make it abide by POSIX
> specifications[1] as previously described by orbea in this mailing
> list[2]. Behavior comparison at the bottom[5].
>
> Please note that POSIX does not specify the exit status in this
> case[1]: "Otherwise, no output shall be written and the exit status
> shall reflect that the name was not found." Although it requires the
> exit status to be 127 in case of command not found[3].
>
> Also note that in order for a command to be considered "found" when
> using the PATH variable, it has to be an executable file[4]: "The list
> shall be searched from beginning to end, applying the filename to each
> prefix, until an executable file with the specified name and
> appropriate execution permissions is found."
>
> Therefore an exit status of 127 seems to be suitable in this case.
> Which would leave the exit status 126 to be used when the user tries
> to execute a file while not having the right permissions to do so.
Thanks for the report. This is really the same issue as
https://lore.kernel.org/all/20211110065303.GA4283@gondor.apana.org.au/
However, that patch doesn't actually work. Here is an updated
version which should fix this properly:
---8<---
Andrej Shadura <andrew.shadura@collabora.co.uk> wrote:
>
> Here's an old bug from 2017, but it was brought to my attention in some
> recent discussion about which "which" is which. There's also a patch in
> one of the follow-ups, but I'm afraid I don't know enough about that
> part of code to judge the consequences of it being applied:
>
> https://bugs.debian.org/874264
>
> -------- Forwarded Message --------
> Subject: dash: 'command -v' mistakenly returns a shell script whose
> executable is not set
> Date: Mon, 04 Sep 2017 10:45:48 -0400
> From: Norman Ramsey <nr@cs.tufts.edu>
> To: Debian Bug Tracking System <submit@bugs.debian.org>
>
> Package: dash
> Version: 0.5.8-2.4
> Severity: normal
>
> Dear Maintainer,
>
>
> I tracked a build bug in s-nail to a problem with dash. Symptom:
> building s-nail tries to run /home/nr/bin/clang, a script whose
> executable bit is not set. We tracked the problem to the result of
> running `command -v clang` with /bin/sh:
>
> nr@homedog ~/n/s-nail> /bin/sh -c 'command -v clang'
> /home/nr/bin/clang
> nr@homedog ~/n/s-nail> ls -l /home/nr/bin/clang
> -rw-rw-r-- 1 nr nr 1009 Aug 29 2011 /home/nr/bin/clang
> nr@homedog ~/n/s-nail> ls -l /bin/sh
> lrwxrwxrwx 1 root root 4 Jan 24 2017 /bin/sh -> dash
> nr@homedog ~/n/s-nail> ksh -c 'command -v clang'
> /usr/bin/clang
> nr@homedog ~/n/s-nail> bash -c 'command -v clang'
> /usr/bin/clang
> nr@homedog ~/n/s-nail> sh -c 'command -v clang'
> /home/nr/bin/clang
> nr@homedog ~/n/s-nail> dash -c 'command -v clang'
> /home/nr/bin/clang
> nr@homedog ~/n/s-nail> fish -c 'command -v clang'
> /usr/bin/clang
>
> When I run `command -v clang` I expect it to answer /usr/bin/clang.
>
> -- System Information:
> Debian Release: 9.1
> APT prefers stable
> APT policy: (990, 'stable'), (500, 'stable'), (1, 'experimental')
> Architecture: i386 (x86_64)
> Foreign Architectures: amd64
>
> Kernel: Linux 4.9.0-3-amd64 (SMP w/4 CPU cores)
> Locale: LANG=C, LC_CTYPE=C (charmap=UTF-8) (ignored: LC_ALL set to
> en_US.utf8), LANGUAGE=C (charmap=UTF-8) (ignored: LC_ALL set to en_US.utf8)
> Shell: /bin/sh linked to /bin/dash
> Init: systemd (via /run/systemd/system)
>
> Versions of packages dash depends on:
> ii debianutils 4.8.1.1
> ii dpkg 1.18.24
> ii libc6 2.24-11+deb9u1
>
> dash recommends no packages.
>
> dash suggests no packages.
>
> -- debconf information:
> * dash/sh: true
This is inherited from NetBSD. There is even a commented-out
block of code that tried to fix this.
Anyway, we now have faccessat so we can simply use it.
Reported-by: Norman Ramsey <nr@cs.tufts.edu>
Reported-by: Nicola Lamacchia <nicola.lamacchia@gmail.com>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
diff --git a/src/bltin/test.c b/src/bltin/test.c
index c7fc479..fd8a43b 100644
--- a/src/bltin/test.c
+++ b/src/bltin/test.c
@@ -18,6 +18,7 @@
#include <unistd.h>
#include <stdarg.h>
#include "bltin.h"
+#include "../exec.h"
/* test(1) accepts the following grammar:
oexpr ::= aexpr | aexpr "-o" oexpr ;
@@ -148,11 +149,6 @@ static int isoperand(char **);
static int newerf(const char *, const char *);
static int olderf(const char *, const char *);
static int equalf(const char *, const char *);
-#ifdef HAVE_FACCESSAT
-static int test_file_access(const char *, int);
-#else
-static int test_access(const struct stat64 *, int);
-#endif
#ifdef HAVE_FACCESSAT
# ifdef HAVE_TRADITIONAL_FACCESSAT
@@ -527,7 +523,7 @@ static int has_exec_bit_set(const char *path)
return st.st_mode & (S_IXUSR | S_IXGRP | S_IXOTH);
}
-static int test_file_access(const char *path, int mode)
+int test_file_access(const char *path, int mode)
{
if (faccessat_confused_about_superuser() &&
mode == X_OK && geteuid() == 0 && !has_exec_bit_set(path))
@@ -657,7 +653,7 @@ static int test_file_access(const char *path, int mode)
* (euid==uid&&egid==gid), but uses st_mode for '-x' iff running as root.
* i.e. it does strictly conform to 1003.1-2001 (and presumably 1003.2b).
*/
-static int test_access(const struct stat64 *sp, int stmode)
+int test_access(const struct stat64 *sp, int stmode)
{
gid_t *groups;
register int n;
diff --git a/src/exec.c b/src/exec.c
index 83cba94..ca68462 100644
--- a/src/exec.c
+++ b/src/exec.c
@@ -325,7 +325,22 @@ printentry(struct tblentry *cmdp)
out1fmt(snlfmt, cmdp->rehash ? "*" : nullstr);
}
+static int test_exec(const char *fullname, struct stat64 *statb)
+{
+ if (!S_ISREG(statb->st_mode))
+ return 0;
+ if ((statb->st_mode & 0111) != 0111 &&
+#ifdef HAVE_FACCESSAT
+ !test_file_access(fullname, X_OK)
+#else
+ !test_access(statb, X_OK)
+#endif
+ )
+ return 0;
+
+ return 1;
+}
/*
* Resolve a command name. If you change this routine, you may have to
@@ -354,9 +369,12 @@ find_command(char *name, struct cmdentry *entry, int act, const char *path)
if (errno == EINTR)
continue;
#endif
+absfail:
entry->cmdtype = CMDUNKNOWN;
return;
}
+ if (!test_exec(name, &statb))
+ goto absfail;
}
entry->cmdtype = CMDNORMAL;
return;
@@ -451,9 +469,6 @@ loop:
e = errno;
goto loop;
}
- e = EACCES; /* if we fail, this will be the error */
- if (!S_ISREG(statb.st_mode))
- continue;
if (lpathopt) { /* this is a %func directory */
stalloc(len);
readcmdfile(fullname);
@@ -464,20 +479,9 @@ loop:
stunalloc(fullname);
goto success;
}
-#ifdef notdef
- /* XXX this code stops root executing stuff, and is buggy
- if you need a group from the group list. */
- if (statb.st_uid == geteuid()) {
- if ((statb.st_mode & 0100) == 0)
- goto loop;
- } else if (statb.st_gid == getegid()) {
- if ((statb.st_mode & 010) == 0)
- goto loop;
- } else {
- if ((statb.st_mode & 01) == 0)
- goto loop;
- }
-#endif
+ e = EACCES; /* if we fail, this will be the error */
+ if (!test_exec(fullname, &statb))
+ continue;
TRACE(("searchexec \"%s\" returns \"%s\"\n", name, fullname));
if (!updatetbl) {
entry->cmdtype = CMDNORMAL;
diff --git a/src/exec.h b/src/exec.h
index 423b07e..8707d36 100644
--- a/src/exec.h
+++ b/src/exec.h
@@ -62,6 +62,8 @@ union node;
extern const char *pathopt; /* set by padvance */
+struct stat64;
+
void shellexec(char **, const char *, int)
__attribute__((__noreturn__));
int padvance_magic(const char **path, const char *name, int magic);
@@ -78,6 +80,9 @@ void unsetfunc(const char *);
int typecmd(int, char **);
int commandcmd(int, char **);
+int test_file_access(const char *path, int mode);
+int test_access(const struct stat64 *sp, int stmode);
+
static inline int padvance(const char **path, const char *name)
{
return padvance_magic(path, name, 1);
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
prev parent reply other threads:[~2024-04-05 9:55 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-05-30 17:02 [PATCH] exec.c: Fix exit status for command -v on non-executable files Nicola Lamacchia
2024-04-05 9:55 ` Herbert Xu [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Zg/Kom9EMBrBPqr5@gondor.apana.org.au \
--to=herbert@gondor.apana.org.au \
--cc=874264@bugs.debian.org \
--cc=andrew.shadura@collabora.co.uk \
--cc=dash@vger.kernel.org \
--cc=nicola.lamacchia@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).