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

nixos/firejail: wrappedPackages #108204

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

ryneeverett
Copy link
Contributor

Motivation for this change

This is a rebase and continuation of the work at #45469. See commit messages.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@symphorien
Copy link
Member

Would you be interested in adding support for custom options (at least, a custom profile)?

@ryneeverett
Copy link
Contributor Author

Would you be interested in adding support for custom options (at least, a custom profile)?

I'm not particularly motivated to do that but I can see where a similar backwards compatible strategy as you implemented in #103813 would work here. Maybe I don't fully understand the use case -- wouldn't profiles in ~/.config/firejail or /etc/firejail work here? (I'll admit I haven't tested it.) Isn't that sufficient for all practical desktop use cases?

@symphorien
Copy link
Member

If I'm not mistaken, firejail picks up a generic (permissive) profile by default. By running a program with a dedicated profile (and upstream provides many) you get a tighter sandbox.
Adding profiles in ~/config/firejail does not really help as it only registers new profiles which are not used by default.
My use case for additional options is to use --private=/some/dir so that some proprietary programs I must run cannot see my home directory.

@ryneeverett
Copy link
Contributor Author

ryneeverett commented Jan 25, 2021

@symphorien

If I'm not mistaken, firejail picks up a generic (permissive) profile by default.

Fortunately you seem to be mistaken. Testing this out with libreoffice, firejail does the right thing by reading the upstream profiles:

$ libreoffice
Reading profile /nix/store/1ry4v2vnyli2gpi5h0cx2sm8bmjji4yv-firejail-0.9.62/etc/firejail/soffice.profile
Reading profile /nix/store/1ry4v2vnyli2gpi5h0cx2sm8bmjji4yv-firejail-0.9.62/etc/firejail/libreoffice.profile
Reading profile /nix/store/1ry4v2vnyli2gpi5h0cx2sm8bmjji4yv-firejail-0.9.62/etc/firejail/libreoffice.local
...

Adding profiles in ~/config/firejail does not really help as it only registers new profiles which are not used by default.
My use case for additional options is to use --private=/some/dir so that some proprietary programs I must run cannot see my home directory.

I'm not completely familiar with how firejail's profile precedence works, but local overrides are supposed to override the default profiles. If they do not that is a bug in the nix implementation which it would be better to fix if possible rather than create configuration options to work around it. My best understanding is that local overrides currently do work: #83515.

@symphorien
Copy link
Member

Fortunately you seem to be mistaken.

Oh I had only tested with zoom, for which it does not work. It looks like it is due to the fact that the executable is called zoom-us and the profile zoom.

Copy link
Member

@symphorien symphorien left a comment

Choose a reason for hiding this comment

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

I tried this with gimp and the wrapping is circular

image

@ryneeverett
Copy link
Contributor Author

I tried this with gimp and the wrapping is circular

I can't reproduce this on stable:

$ head $(which gimp)
#! /nix/store/a3fc4zqaiak11jks9zd579mz5v0li8bg-bash-4.4-p23/bin/bash -e
exec /run/wrappers/bin/firejail "/nix/store/4a0d7inzfq8vc09lqbmp5px9n6czvs7y-gimp-2.10.20/bin/gimp-2.10" "$@"

Any chance you have gimp firejailed elsewhere such as in wrappedBinaries or an override?

@symphorien
Copy link
Member

Nope that was in a nixos vm with full config:

{ config, pkgs, lib, ...}: {
  config = {
    users.users.root.initialPassword = "root";
    users.users.alice = {
      isNormalUser = true;
      initialPassword = "alice";
    };
    console = {
      font = "Lat2-Terminus16";
      keyMap = "fr";
    };
    programs.firejail = {
      enable = true;
      wrappedPackages = [
        pkgs.gimp
      ];
    };
    services.xserver = {
      enable = true;
      layout = "fr";
      xkbVariant = "oss";
      libinput = {
        enable = true;
        tapping = false;
      };
      xkbOptions = "caps:swapescape, compose:prsc";
      displayManager = {
        lightdm.enable = true;
        defaultSession = "xfce";
      };
      desktopManager = {
        xterm.enable = false;
        xfce = {
          enable = true;
        };
      };
    };
  };
}

@ryneeverett
Copy link
Contributor Author

First off thanks for introducing me to nixos vm's! Somehow I'd never noticed the build-vm subcommand before.

I still can't reproduce the circular wrapping though and made only the necessary changes so that I could use the keyboard:

{ config, pkgs, lib, ...}: {
  config = {
    users.users.root.initialPassword = "root";
    users.users.alice = {
      isNormalUser = true;
      initialPassword = "alice";
    };
    console = {
      font = "Lat2-Terminus16";
      # keyMap = "fr";
    };
    programs.firejail = {
      enable = true;
      wrappedPackages = [
        pkgs.gimp
      ];
    };
    services.xserver = {
      enable = true;
      # layout = "fr";
      # xkbVariant = "oss";
      libinput = {
        enable = true;
        tapping = false;
      };
      xkbOptions = "caps:swapescape, compose:prsc";
      displayManager = {
        lightdm.enable = true;
        defaultSession = "xfce";
      };
      desktopManager = {
        xterm.enable = false;
        xfce = {
          enable = true;
        };
      };
    };
  };
}

image

I don't know which environment variable is bothering firejail in this system but it doesn't seem to be caused by this patch.

@symphorien
Copy link
Member

For full reproducibility:
on top of 03c4f7d7dff27a93a47d8b013ccb71e0b1223228 (your PR), and with this english keymap configuration:

{ config, pkgs, lib, ...}: {
  config = {
    users.users.root.initialPassword = "root";
    users.users.alice = {
      isNormalUser = true;
      initialPassword = "alice";
    };
    console = {
      font = "Lat2-Terminus16";
      # keyMap = "fr";
    };
    programs.firejail = {
      enable = true;
      wrappedPackages = [
        pkgs.gimp
      ];
    };
    services.xserver = {
      enable = true;
      # layout = "fr";
      # xkbVariant = "oss";
      libinput = {
        enable = true;
        tapping = false;
      };
      # xkbOptions = "caps:swapescape, compose:prsc";
      displayManager = {
        lightdm.enable = true;
        defaultSession = "xfce";
      };
      desktopManager = {
        xterm.enable = false;
        xfce = {
          enable = true;
        };
      };
    };
  };
}

then when in run nixos-rebuild build-vm -I nixpkgs=$(pwd) -I nixos-config=$(readlink -f vm.nix) I get
/nix/store/him8q86pfk4yf5if00hjk1dqpkycs9nl-nixos-vm/bin/run-nixos-vm
After removing nixos.qcow2 i can still reproduce the issue.

image

Can you check that you get the same store paths ? This should help you reproducing.

about the error message about environment variable size: I suppose firejail appends things to environment variable, so if it invokes itself, the environment variable size grows infinitely.

@ryneeverett
Copy link
Contributor Author

then when in run nixos-rebuild build-vm -I nixpkgs=$(pwd) -I nixos-config=$(readlink -f vm.nix) I get
/nix/store/him8q86pfk4yf5if00hjk1dqpkycs9nl-nixos-vm/bin/run-nixos-vm

Hmm, with the updated config and the exact same commit and invocation I get /nix/store/l3hn02rdsn4fj4mvwnpxiv5sp3v1fmfd-nixos-vm/bin/run-nixos-vm.

about the error message about environment variable size: I suppose firejail appends things to environment variable, so if it invokes itself, the environment variable size grows infinitely.

I don't see how the error message could be related to the circular wrapping issue because as you can see from my vm session above, I got this error even when the circular wrapping wasn't occurring.

@ryneeverett
Copy link
Contributor Author

Hmm, with the updated config and the exact same commit and invocation I get /nix/store/l3hn02rdsn4fj4mvwnpxiv5sp3v1fmfd-nixos-vm/bin/run-nixos-vm

Just reproduced this output on a freshly provisioned machine, though I'll grant it's configuration is fairly similar to the first.

@ryneeverett
Copy link
Contributor Author

ryneeverett commented Jul 7, 2021

I pushed an additional commit using makeWrapper to simplify the implementation. I would think this would reduce the likelihood of a circular wrapping issue. edit: I reverted this commit because it was based on an error of makeWrapper usage. I no longer think it's possible to use makeWrapper for this application.

@ryneeverett ryneeverett force-pushed the firejail-wrapped-packages-2 branch 2 times, most recently from 5ddaab0 to d33e911 Compare September 21, 2021 17:39
@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Aug 12, 2022
Robert Schütz and others added 2 commits February 15, 2023 17:07
Using this, not only the wrapped binaries but also all other
directories from the derivations are put into /run/current-system/sw.
This is useful if you need desktop entries, icons, etc.
- Remove unnecessary buildInputs and add indentation per review of the
original PR NixOS#45469.
- Warn that desktop files won't be firejailed if they have an absolute
path per discussion in NixOS#45469.
- Update wrappedPackages to use runtimeShell and exec per NixOS#70477.
- Test that wrappedPackages works.
Per @symphorien's review, it's better to error out when a desktop file
specifies an absolute path to the unwrapped binary than to echo a
warning which will likely be missed.

In cases where the desktop file cannot be wrapped, the same behavior can
be achieved by adding the binaries to wrappedBinaries and doing a
conventional installation of the given package.
Many packages have symlinked aliases which don't work when just passing
the binary name to firejail, presumably because the binary name points
to the firejailed wrapper which results in the firejailed wrapper
calling another firejailed wrapper.

For example, before this change the libreoffice wrapper called "firejail
soffice" where the soffice wrapper called "firejail /nix/store...soffice".
The soffice wrapper worked but the libreoffice wrapper yielded:

    firejail: /nix/store/v6l2sacryfr88yqq0pq7sia8wfgm9q31-wrapper.c:203: main: Assertion `!(st.st_mode & S_ISUID) || (st.st_uid == geteuid())' failed.
Per SuperSandro2000's review.
Given that no consensus was reached on NixOS#68035, it's probably best to try
to patch these than to fix upstream at this point.
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Feb 15, 2023
@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants