Using end-of-options indicator "--" and blacklisting $SHELL causes Cannot start application

[rusty-snake]

Description

Blacklisting $SHELL and using -- causes Cannot start application.

Steps to Reproduce

1. firejail --quiet --noprofile --blacklist=$SHELL -- echo "*NULL"

Expected behavior

See *NULL

Actual behavior

Cannot start application: Permission denied

Behavior without a profile

N/A

Additional context

This does not happen with firejail --quiet --noprofile --blacklist=$SHELL echo "*NULL".

Relates to #5599. Relates to #5598.

Environment

• Fedora 37
• 52898f4

Checklist

• [x] The issues is caused by firejail (i.e. running the program by path (e.g. /usr/bin/vlc) "fixes" it).
• [ ] I can reproduce the issue without custom modifications (e.g. globals.local).
• [ ] The program has a profile. (If not, request one in https://github.com/netblue30/firejail/issues/1139)
• [ ] The profile (and redirect profile if exists) hasn't already been fixed upstream.
• [ ] I have performed a short search for similar issues (to avoid opening a duplicate).
  • [ ] I'm aware of browser-allow-drm yes/browser-disable-u2f no in firejail.config to allow DRM/U2F in browsers.
• [ ] I used --profile=PROFILENAME to set the right profile. (Only relevant for AppImages)

Log

logs

```console $ firejail --quiet --noprofile --blacklist=$SHELL --debug -- echo "*NULL" Building quoted command line: 'echo' '*NULL' Command name #echo# ... Starting application LD_PRELOAD=(null) Running 'echo' '*NULL' command through /bin/bash execvp argument 0: /bin/bash execvp argument 1: -c execvp argument 2: 'echo' '*NULL' Child process initialized in 10.02 ms Cannot start application: Permission denied ... ``` ```console $ firejail --quiet --noprofile --blacklist=$SHELL --debug echo "*NULL" Building quoted command line: 'echo' '*NULL' Command name #echo# ... Starting application LD_PRELOAD=(null) execvp argument 0: echo execvp argument 1: *NULL Child process initialized in 7.60 ms Searching $PATH for echo trying #/home/rusty-snake/.config/firecfg.py/overrides/bin/echo# trying #/etc/firecfg.py/overrides/bin/echo# trying #/usr/local/bin/echo# trying #/usr/local/sbin/echo# trying #/usr/bin/echo# *NULL ```

[rusty-snake]

Looks like this is intentional. https://github.com/netblue30/firejail/commit/7ad735deafa80114a17b20790de63f7e973b1bb4#diff-767ac3de885e9c994fed6eb2a0f8234fd8d8611a5f817d2b0a37b50f4101b321

[wentasah]

Hmm. Blacklisting a shell is one of the primary use cases for firejail. Not using -- is not an option here, because then, there is no way to pass the jailed program an option with the same name as some firejail option. This can be clearly seen from the the code of check_arg function.

The commit mentioned by @rusty-snake refers to #5196. I don't see how deprecating --shell relates to the use of -- changed in the referenced commit.

[rusty-snake]

Not using -- is not an option here, because then, there is no way to pass the jailed program an option with the same name as some firejail option.

Correction. Passing an option with the same name as an firejail option is possible. What you can not do is to start a program whose name starts with -- or an untrusted command line.

This can be clearly seen from the the code of check_arg function.

https://github.com/netblue30/firejail/blob/31d0c32be58413897780606b55b2e60026f9a47c/src/firejail/main.c#L986-L990

if argv[i] == "--" or not argv[i].startswith("--"):
    break

[rusty-snake]

I don't see how deprecating --shell relates to the use of -- changed in the referenced commit.

Read the comment in line 529: start the program without using a shell.

=> Starting a shell only for -- but not for normal start is intentional.

It's a random feature (rather than a bug) that nobody can explain for what it is good.

[wentasah]

Yes, you're right. I was too quick reading the code :-)

[rusty-snake]

FWIW: sailfishos/sailjail is going to revert this. https://github.com/sailfishos/firejail/pull/17/files#diff-579c2ba1109122d6e4ac5657a8127e06989706c4816601f367a853f4ebf5deab

[tstarling]

=> Starting a shell only for -- but not for normal start is intentional.

It's a random feature (rather than a bug) that nobody can explain for what it is good.

@netblue30 introduced this behaviour in 7ad735deafa80114a17b20790de63f7e973b1bb4 without explanation. Prior to 4b4d752158e2a7164765c6c7247ef8b4d6014689 (the previous day), the shell was always used unless --shell=none was given. My theory is that @netblue30 erroneously believed that there was a need to preserve the feature which forwarded the double dash to the shell.

The POSIX specification of sh requires that shell options prefixed with - and + be respected when they appear after -c and before the command string. It also requires that sh implement -- as an end-of-options marker. So to safely pass a command string to a POSIX-compliant shell, it is necessary to always add --.

Firejail was adding -- to the shell arguments only when -- appeared in its input arguments. This was incorrect. It should have always added --.

It was reported in #5599 that Fish is not POSIX compliant in that it does not correctly implement -c --. In my opinion, the bug in Firejail was that it was using the user's shell when it needed a POSIX-compliant shell. Firejail should have used /bin/sh, or indeed, used no shell.

In https://github.com/netblue30/firejail/pull/5600#issuecomment-1386044151 , @kmk3 posted a correct analysis of the situation, but came to the conclusion that it's more important to support fish than it is to support sh. I disagree.

Assuming that the rationale for 7ad735deafa80114a17b20790de63f7e973b1bb4 was to preserve double-dash forwarding, the merge of #5600 has rendered that rationale moot by removing the feature. 7ad735deafa80114a17b20790de63f7e973b1bb4 should be reverted.

I'm here because, per https://phabricator.wikimedia.org/T353194 , we are adding -- to firejail input arguments when the user's shell is /usr/sbin/nologin and $SHELL is unset. That worked prior to fc912c0821b02be7a556ddf71c65db3abaa6be9c (part of the same sequence of commits in June 2022), which removed guess_shell() in favour of using the shell from getpwuid(). I would like that commit to also be reverted. The user's shell in the password database is conventionally set to a non-functional shell for system accounts which do not allow login.