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

Too many features #5190

Closed
3 of 10 tasks
netblue30 opened this issue Jun 10, 2022 Discussed in #5183 · 18 comments
Closed
3 of 10 tasks

Too many features #5190

netblue30 opened this issue Jun 10, 2022 Discussed in #5183 · 18 comments
Labels
enhancement New feature request removal Removal of a feature/option

Comments

@netblue30
Copy link
Owner

netblue30 commented Jun 10, 2022

Done so far:

  • removed cgroups code
  • changed --disable-firetunnel into --enable-firetunnel in configure.ac
  • disabled chroot by default in /etc/firejail/firejail.config
  • shell none set as default
  • removed --shell=
  • private-lib disabled by defautl in /etc/firejail/firejail.config

Discussed in #5183

Originally posted by SkewedZeppelin June 8, 2022
The past few versions have been adding more and more arguable unnecessary features further increasing attack surface of firejail.

The best part of firejail is the collection of ready to use profiles for programs, nothing else offers this.
What ever happened to the minimal LTS version?

Can these be #ifdef'ed out?

  • ids is --disable-ids
  • --appimage
  • --bandwidth, --netstats, --nettrace are --disable-network
  • --build?
  • --cpu?
  • --tunnel is --disable-firetunnel

any others?

Originaly posted by rusty-snake June 8, 2022

Can these be #ifdef'ed out?

While for us #ifdef works good, the most users will get their firejail binary from their package-manager. Therefore we should make sure that there are options in firejail.config to disable those features too (--appimage has one).

any others?

  • --cgroup
  • --shutdown
  • --trace (subset of --build)
  • --tracelog?
@netblue30
Copy link
Owner Author

And that's exactly what we are going to do! I would say for most features we'll have compile time and run time configuration. Then we need to talk what we keep in the default configuration most people are using.

@netblue30 netblue30 added the enhancement New feature request label Jun 10, 2022
@amano-kenji
Copy link
Contributor

You can split privileged parts into a much smaller binary. Then, you can add more features in another binary.

@rusty-snake
Copy link
Collaborator

rusty-snake commented Jun 11, 2022

#510 and a few other places

Also #5157.

@netblue30
Copy link
Owner Author

You can split privileged parts into a much smaller binary. Then, you can add more features in another binary.

We've been doing it for a few years now. The files starting in "f" in /usr/lib/firejail directory (fseccomp, fcopy etc.) have been split out from SUID binary and are run in a temporary sandbox (see src/firejail/sbox.c). We even run external some programs this way - the largest one is ISC DHCP client (dhclient). I would say adding new fancy features in this moment is quite easy.

@netblue30
Copy link
Owner Author

Is anybody using --tracelog or --cgroup? I would take them out, I mean completely gone!

@rusty-snake
Copy link
Collaborator

--cgroup

Does it even work with an unified hierarchy (aka. v2)?

No, I don't think there is any relevant use.

--tracelog

It's still useful I think. Maybe we can keep libtracelog.so and make --tracelog just an alias for env LD_PRELOAD=....

@glitsj16
Copy link
Collaborator

Is anybody using --tracelog or --cgroup? I would take them out, I mean completely gone!

I do use tracelog quite extensively for auditing sandbox violations. IMO it's a nice tool to get a finer-grained view on what's happening in a Firejail sandbox. As such it provides valuable information in the context of creating tighter sandbox profiles.

Although I've used the cgroup option on occasion, I don't have particularly strong feelings about it personally.

BUT, and this is a general comment on this topic, I don't (fully) agree with the view that Firejail has too many features, hence a larger attack surface . Sure, we should implement a finer-grained compile/run time configuration model and have a 'reasonable' set of defaults. Cutting out pieces left and right haphazardly - in the spirit of tightening Firejail's attack surface - feels a bit awkward though. Attack surfaces will always exist and IMHO it's best to leave that up to users and their (more or less) educated guesses regarding personal threat-model(s).

Personally I always liked Firejail's more universal approach (for the lack of a better word): offering a very broad toolset and motivating users to customize the hell out of their profiles. It's very much possible I misunderstand the rationale behind this specific issue. Yet, it feels like a shift towards a more set-and-forget mindset, which I for one would deplore.

Just my two cents. I do understand this is a very complex and important issue and I do appreciate all the efforts and hard work that is continuously being done by the community to keep Firejail in the best of shapes. A big thank you to all involved. Let's take the time to think this through in the spirit of open-mindedness and collaboration worthy of its importance and future development.

@rusty-snake
Copy link
Collaborator

My two cents to your two cents: Firstly I fully agree with "implement[ing] a finer-grained compile/run time configuration model and have a 'reasonable' set of defaults." Secondly I think we should cut of code/feature. More precisely features that are less used, have high complexity and haven't seen any love since they were implemented.

@glitsj16
Copy link
Collaborator

@rusty-snake Thanks for your reply. I don't see much - if any - difference in our stance here. Underdeveloped features are indeed a sore point and I can understand if those would get dropped. Currently we have zero profiles that carry the cgroup option. Yet tracelog is part of quite a lot of profiles. Not that I would personally mind doing the work on removing it from those if it is decided to take that option out. I just happen to think it is valuable :-)

@netblue30
Copy link
Owner Author

OK, let's keep trackelog in, but we disable it by default in /etc/firejail/firejail.config

I have removed cgroups code, maybe we will bring it back if we ever implement the "unified" cgroups interface in the kernel.

More to come, I'll keep a list with what was done on the top item.

@WhyNotHugo
Copy link
Contributor

Has there been discussion on eventually dropping blacklist in favour of a plain whitelist approach? The existence of both makes firejail quite harder to grasp, and profiles a bit tricker to read/write.

whitelist has a more hardened approach too, since it starts with a "deny all" approach, which is usually the safe way to approach things.

@kmk3
Copy link
Collaborator

kmk3 commented Jun 14, 2022

@netblue30 commented on Jun 13:

OK, let's keep trackelog in, but we disable it by default in
/etc/firejail/firejail.config

I have removed cgroups code, maybe we will bring it back if we ever implement
the "unified" cgroups interface in the kernel.

More to come, I'll keep a list with what was done on the top item.

I don't have strong feelings on either change related to the issues below, but
please finish the ongoing removals before starting a new one:

@glitsj16
Copy link
Collaborator

Has there been discussion on eventually dropping blacklist in favour of a plain whitelist approach?

@WhyNotHugo I don't think that's part of the discussion here. It can be awkward to get used to the difference between those options, very true. I prefer whitelisting profiles (too), but not every application can (easily) be sandboxed like that. So I guess a blacklist option (or whatever one would prefer to call it) is here to stay for the foreseeable future. Good points though. Nothing stops you from opening a separate discussion thread for it :-)

@rusty-snake
Copy link
Collaborator

Agreeing with @glitsj16 making blacklist/whitelist is outside of the scope of this issue an far more complex as you need to redesign both because

  1. you need a way for blacklisting profile because not all programs work with whitelisting profiles.
  2. there are use cases like whitelist ${DOCUMENTS} but blacklist ${DOCUMENTS}/Secret

@netblue30
Copy link
Owner Author

I disabled private-lib in /etc/firejail/firejail.config. Out of 1191 profiles only 70 were using it.

@rusty-snake
Copy link
Collaborator

private-lib is still initialized as 1, isn't it?

int i;
for (i = 0; i < CFG_MAX; i++)
cfg_val[i] = 1; // most of them are enabled by default
cfg_val[CFG_RESTRICTED_NETWORK] = 0; // disabled by default
cfg_val[CFG_FORCE_NONEWPRIVS] = 0;
cfg_val[CFG_PRIVATE_BIN_NO_LOCAL] = 0;
cfg_val[CFG_FIREJAIL_PROMPT] = 0;
cfg_val[CFG_DISABLE_MNT] = 0;
cfg_val[CFG_ARP_PROBES] = DEFAULT_ARP_PROBES;
cfg_val[CFG_XPRA_ATTACH] = 0;
cfg_val[CFG_SECCOMP_ERROR_ACTION] = -1;
cfg_val[CFG_BROWSER_ALLOW_DRM] = 0;
cfg_val[CFG_ALLOW_TRAY] = 0;
cfg_val[CFG_CHROOT] = 0;
cfg_val[CFG_SECCOMP_LOG] = 0;

@netblue30
Copy link
Owner Author

Fixed, thanks!

kmk3 added a commit to kmk3/firejail that referenced this issue Jun 24, 2022
Before running test/fs/private-lib.exp.

Inspired by the configuration changes that are done on
test/root/checkcfg.exp.

Reason: Since commit 9741d0b ("fix disabled private-lib in
/etc/firejail/firejail.config", 2022-06-23), the "build_and_test" job
fails with the following error[1]:

    TESTING: private-lib (test/fs/private-lib.exp)
    spawn /bin/bash
    firejail --private-lib --private-bin=sh,bash,dash,ps,grep,ls,find,echo,stty
    runner@fv-az489-993:~/work/firejail/firejail/test/fs$
    <private-bin=sh,bash,dash,ps,grep,ls,find,echo,stty
    Error: private-lib feature is disabled in Firejail configuration file
    runner@fv-az489-993:~/work/firejail/firejail/test/fs$ TESTING ERROR 1

This fixes CI.

Fixes netblue30#5214.

Relates to netblue30#5190.

[1] https://github.com/netblue30/firejail/runs/7030862406
kmk3 added a commit to kmk3/firejail that referenced this issue Jun 24, 2022
Before running test/fs/private-lib.exp.

Inspired by the configuration changes that are done on
test/root/checkcfg.exp.

Reason: Since commit 9741d0b ("fix disabled private-lib in
/etc/firejail/firejail.config", 2022-06-23), the "build_and_test" job
fails with the following error[1]:

    TESTING: private-lib (test/fs/private-lib.exp)
    spawn /bin/bash
    firejail --private-lib --private-bin=sh,bash,dash,ps,grep,ls,find,echo,stty
    runner@fv-az489-993:~/work/firejail/firejail/test/fs$
    <private-bin=sh,bash,dash,ps,grep,ls,find,echo,stty
    Error: private-lib feature is disabled in Firejail configuration file
    runner@fv-az489-993:~/work/firejail/firejail/test/fs$ TESTING ERROR 1

This fixes CI.

Fixes netblue30#5214.

Relates to netblue30#5190.

[1] https://github.com/netblue30/firejail/runs/7030862406
kmk3 added a commit to kmk3/firejail that referenced this issue Jun 24, 2022
Before running test/fs/private-lib.exp.

Inspired by the configuration changes that are done on
test/root/checkcfg.exp.

Reason: Since commit 9741d0b ("fix disabled private-lib in
/etc/firejail/firejail.config", 2022-06-23), the "build_and_test" job
fails with the following error[1]:

    TESTING: private-lib (test/fs/private-lib.exp)
    spawn /bin/bash
    firejail --private-lib --private-bin=sh,bash,dash,ps,grep,ls,find,echo,stty
    runner@fv-az489-993:~/work/firejail/firejail/test/fs$
    <private-bin=sh,bash,dash,ps,grep,ls,find,echo,stty
    Error: private-lib feature is disabled in Firejail configuration file
    runner@fv-az489-993:~/work/firejail/firejail/test/fs$ TESTING ERROR 1

This fixes CI.

Fixes netblue30#5214.

Relates to netblue30#5190.

[1] https://github.com/netblue30/firejail/runs/7030862406
kmk3 added a commit that referenced this issue Jun 25, 2022
Before running test/fs/private-lib.exp.

Inspired by the configuration changes that are done on
test/root/checkcfg.exp.

Reason: Since commit 9741d0b ("fix disabled private-lib in
/etc/firejail/firejail.config", 2022-06-23), the "build_and_test" job
fails with the following error[1]:

    TESTING: private-lib (test/fs/private-lib.exp)
    spawn /bin/bash
    firejail --private-lib --private-bin=sh,bash,dash,ps,grep,ls,find,echo,stty
    runner@fv-az489-993:~/work/firejail/firejail/test/fs$
    <private-bin=sh,bash,dash,ps,grep,ls,find,echo,stty
    Error: private-lib feature is disabled in Firejail configuration file
    runner@fv-az489-993:~/work/firejail/firejail/test/fs$ TESTING ERROR 1

This fixes CI.

Fixes #5214.

Relates to #5190.

[1] https://github.com/netblue30/firejail/runs/7030862406
@kmk3 kmk3 added the removal Removal of a feature/option label Jul 12, 2022
@netblue30
Copy link
Owner Author

Closing it for now. Any other ideas we'll reopen it.

kmk3 added a commit to kmk3/firejail that referenced this issue Sep 19, 2022
And add the missing issue/PR references.

Misc: The items in question were added on commit 6d740d7 ("RELNOTES
and README.md - existing functionality modified for the next version",
2022-08-29).

Relates to netblue30#5190 netblue30#5196 netblue30#5200 netblue30#5209 netblue30#5216.
kmk3 added a commit to kmk3/firejail that referenced this issue Oct 4, 2022
This reverts commit 393c5be.

Which broke mpv:

    $ mpv --version
    Cannot start application: No such file or directory

Probably because mpv itself uses many libraries and it has plugins that
may depend on files in /usr/lib as well:

    $ pacman -Qlq mpv | grep /lib/ | grep -v '/$'
    /usr/lib/libmpv.so
    /usr/lib/libmpv.so.1
    /usr/lib/libmpv.so.1.109.0
    /usr/lib/pkgconfig/mpv.pc
    $ strings /usr/bin/mpv | grep '^lib.*\.so' | sort -u | wc -l
    53
    $ pacman -Qlq yt-dlp | grep /lib/ | grep -v '/$' |
      cut -f -4 -d / | sort -u
    /usr/lib/python3.10
    $ pacman -Q mpv yt-dlp
    mpv 1:0.34.1-5
    yt-dlp 2022.09.01-1

Environment: Artix Linux.

Also, private-lib is disabled by default in firejail.config (see netblue30#5190)
and mpv.profile does not use private-lib, so there should be no need to
whitelist anything in /usr/lib in the default profile.
@kmk3 kmk3 moved this to Done (on RELNOTES) in Release 0.9.72 Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature request removal Removal of a feature/option
Projects
Status: Done (on RELNOTES)
Development

No branches or pull requests

6 participants