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

os:cmd() invokes access() syscall as part of detecting which shell to use #8944

Open
yarisx opened this issue Oct 16, 2024 · 3 comments
Open
Assignees
Labels
bug Issue is reported as a bug team:VM Assigned to OTP team VM

Comments

@yarisx
Copy link
Contributor

yarisx commented Oct 16, 2024

Describe the bug
Since OTP25 the os:cmd() function via its helper os:mk_cmd() tries to determine which shell to use (to accommodate to Android's non-standard shell placement) by calling file:read_file_info(), which in turn invokes access() syscall with R_OK and W_OK flags. This causes issues in some secured environments where only read access to the shell is allowed. Moreover reading the whole inode of data only to check that the file exists looks unnecessary.

To Reproduce
In Linux: from Erlang shell call os:cmd("ls") while using strace -f -e trace=access on the Erlang VM process.

Expected behavior
os:cmd() should check the location of the system shell without reading a lot of info from the filesystem and triggering security modules like SELinux and AppArmor.

Affected versions
OTP25 and further

Additional context
It seems to be possible to use the access() syscall with F_OK flag to just check whether the file exists or not.
Probably it would be beneficial to have a separate file:check_access/2 function to check access rights on the file without reading all other data about it or opening (and immediately closing) it.

@yarisx yarisx added the bug Issue is reported as a bug label Oct 16, 2024
@IngelaAndin IngelaAndin added the team:VM Assigned to OTP team VM label Oct 17, 2024
@garazdawi
Copy link
Contributor

Another approach would be to add a configuration parameter to kernel, that lets the user decide which shell to use. Doing a config lookup should not be very expensive in the big scheme of things.

What do you think?

@yarisx
Copy link
Contributor Author

yarisx commented Oct 21, 2024

Yes, the config option would work to select the shell.
But our wish for file:check_access/2 still stays :-) We often check read/write access to files (logs, credentials etc) but we are not interested in other metadata for those files. Probably it should be a matter of another PR though.

@garazdawi
Copy link
Contributor

Yes, the config option would work to select the shell.

Will you send a PR?

But our wish for file:check_access/2 still stays :-) We often check read/write access to files (logs, credentials etc) but we are not interested in other metadata for those files. Probably it should be a matter of another PR though.

The problem with file:check_access/2 is that on Windows there is no equivilant check that makes sense (that we know of).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue is reported as a bug team:VM Assigned to OTP team VM
Projects
None yet
Development

No branches or pull requests

3 participants