isaacs / isexe

Minimal module to check if a file is executable.
ISC License
48 stars 17 forks source link

isexe fails to recognize an executable that is not owned by current user #18

Closed bndnico closed 1 year ago

bndnico commented 6 years ago

On Linux, the isexe module does not recognize an executable file as being executable when

  1. the file's owner is different from the current user, and
  2. the file's group is different from the current user's main group, and
  3. the file is not readable by "others".

So a file with the permissions rwxrwx--- with user=nobody and group=adm is not recognized as executable, even though my user is a member of the group "adm" and the group "adm" does have the "x" permission on that particular file. On the shell however, the file executes just fine!

In mode.js, process.getgid() is used the get the current main groups. However, all secondary groups of the current user are not checked. I think mode.js should use process.getgroups() instead and check every group. (Or maybe just use process.access() like https://www.npmjs.com/package/is-executable does?)

Demonstration:

Set up a test project and install "isexe" module

/tmp$ mkdir isexetest
/tmp$ cd isexetest/
/tmp/isexetest$ npm init -y
(...)
/tmp/isexetest$ npm install isexe
(...)
+ isexe@2.0.0
added 1 package from 1 contributor in 1.661s

Create a simple executable file and test it

/tmp/isexetest$ cat << EOF > hello.sh
> #!/bin/bash
> echo hello
> EOF
/tmp/isexetest$ chmod +x hello.sh 
/tmp/isexetest$ ./hello.sh 
hello
/tmp/isexetest$ ls -la hello.sh 
-rwxrwxr-x 1 nico nico 23 Mai  6 14:24 hello.sh
/tmp/isexetest$ node -e "require('isexe')('hello.sh', console.log)"
null 1

So I can run this program on the terminal and "isexe" returns 1 which is truthy, so everything seems to be in order correct.

Change the file ownership and test it again

First, I restrict the file permissions to 770, so only the owner and members of the group can access the file. Then I set the owner to "nobody" and the group to "adm". Note that those values are rather arbitrary. I chose them because they are present on most systems and my user is already a member of the "adm" group.

/tmp/isexetest$ id
uid=1000(nico) gid=1000(nico) groups=1000(nico),4(adm),20(dialout),24(cdrom),27(sudo),30(dip),46(plugdev),113(lpadmin),128(sambashare),999(vboxsf)
/tmp/isexetest$ chmod 770 hello.sh 
/tmp/isexetest$ sudo chown nobody:adm hello.sh 
/tmp/isexetest$ ls -la hello.sh 
-rwxrwx--- 1 nobody adm 23 Mai  6 14:24 hello.sh
/tmp/isexetest$ ./hello.sh 
hello
/tmp/isexetest$ node -e "require('isexe')('./hello.sh', console.log)"
null false

As you can see I can still execute the file (as expected), but "isexe" now returns false, which is incorrect.

Some version information

/tmp/isexetest$ cat /etc/issue
Ubuntu 16.04.4 LTS \n \l

/tmp/isexetest$ node -v
v8.11.1
/tmp/isexetest$ npm list
isexetest@1.0.0 /tmp/isexetest
+-- isexe@2.0.0
isaacs commented 1 year ago

I realize this is 5 years late, but this module has just quietly been working without issue and hasn't gotten much attention for that reason :)

Using the full list of groups would be a good improvement, since if any of them match the gid on the file, they should be considered part of the group.

However, using fs.access is not ideal, since on linux (but not bsd systems like macOS), it uses the actual uid/gid/groups, rather than the effective uid/gid/groups. This is a security feature in some cases, because it allows setuid executables to determine what files the user actually has access to, but it also means that if you run the initial process as the root user, and then downgrade permissions by using process.setgid() and process.setuid(), you'll get the wrong answer. On bsd systems, this behavior is reversed, making it even more confusing.

Also, this module lets you specify an effective uid/gid, which there's of course no way to provide to access(2).