Skip to content

modif: Stop forwarding own double-dash to the shell #5600

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

Merged
merged 1 commit into from
Jan 19, 2023

Conversation

kmk3
Copy link
Collaborator

@kmk3 kmk3 commented Jan 17, 2023

Currently, if double-dash ("--") is passed to firejail, it is forwarded
to the user shell:

$ firejail --debug --noprofile -- echo test 2>&1 |
  grep -e execvp -e test
Building quoted command line: 'echo' 'test'
Building quoted command line: 'echo' 'test'
Running 'echo' 'test'  command through /bin/bash
execvp argument 0: /bin/bash
execvp argument 1: -c
execvp argument 2: --
execvp argument 3: 'echo' 'test'
test

This causes issues when the user shell does not accept "--" / is not
POSIX-compatible:

$ /bin/bash -c -- 'echo test'
test
$ /bin/fish -c -- 'echo test'
fish: Unknown command: --
fish:
--
^

Fixes #5599.

Relates to #3434.

Reported-by: @iltep64
Reported-by: @ferreum

Currently, if double-dash ("--") is passed to firejail, it is forwarded
to the user shell:

    $ firejail --debug --noprofile -- echo test 2>&1 |
      grep -e execvp -e test
    Building quoted command line: 'echo' 'test'
    Building quoted command line: 'echo' 'test'
    Running 'echo' 'test'  command through /bin/bash
    execvp argument 0: /bin/bash
    execvp argument 1: -c
    execvp argument 2: --
    execvp argument 3: 'echo' 'test'
    test

This causes issues when the user shell does not accept "--" / is not
POSIX-compatible:

    $ /bin/bash -c -- 'echo test'
    test
    $ /bin/fish -c -- 'echo test'
    fish: Unknown command: --
    fish:
    --
    ^

Fixes netblue30#5599.

Relates to netblue30#3434.

Reported-by: @iltep64
Reported-by: @ferreum
@kmk3 kmk3 changed the title Stop forwarding own double-dash to the shell modif: Stop forwarding own double-dash to the shell Jan 17, 2023
Copy link
Collaborator

@glitsj16 glitsj16 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kmk3
Copy link
Collaborator Author

kmk3 commented Jan 17, 2023

To expand on the rationale:

I think that firejail should either always or never pass -- to the shell,
regardless of whether -- was passed to firejail itself.

Always passing -- seems to be a bit "safer"; from sh(1p) of POSIX.1-2017:

SYNOPSIS
       sh [-abCefhimnuvx] [-o option]... [+abCefhimnuvx] [+o option]...
           [command_file [argument...]]

       sh -c [-abCefhimnuvx] [-o option]... [+abCefhimnuvx] [+o option]...
           command_string [command_name [argument...]]

       sh -s [-abCefhimnuvx] [-o option]... [+abCefhimnuvx] [+o option]...
           [argument...]

[...]

APPLICATION USAGE
       [...]

       A conforming application must protect its first operand, if it starts
       with a <plus-sign>, by preceding it with the "--" argument that denotes
       the end of the options.

Though that would break running shell commands directly (such as with firejail echo test) for every user that has fish as the default shell.

Also, it seems that the first operand is always -c when firejail calls the
shell in this manner, in which case the scenario from the man page would not
apply. Otherwise, for that to be an issue, it appears that all of the
following would have to happen:

  • There exists a command that starts with + in some shell
  • This shell is set as the default user shell
  • The user tries to run something with this command as the first argument
    directly through firejail

Which seems rather unlikely.

So this PR does the opposite and changes firejail to never call the shell with
--.

@netblue30 netblue30 merged commit 62e6919 into netblue30:master Jan 19, 2023
@netblue30
Copy link
Owner

merged!

@kmk3 kmk3 deleted the fix-stop-ddash-sh branch January 19, 2023 13:43
kmk3 added a commit that referenced this pull request Jan 19, 2023
@kmk3 kmk3 added the modif This modifies existing behavior label Jan 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
modif This modifies existing behavior
Projects
Status: Done (on RELNOTES)
Development

Successfully merging this pull request may close these issues.

End-of-options indicator "--" leads to invalid shell invocation (fish shell)
3 participants