apparmor: fix and improve the service

[ju1m]

This is an attempt to improve the integration of AppArmor into NixOS.
It's still a code which needs testing, but it is ready for review.

Motivation for this change

The current security.apparmor service has several problems :

• ExecStop is missing the included paths used in ExecStart therefore most profiles using #include are not removed and when a profile changes this leaves the old version loaded into the kernel, causing potential troubles.
• /etc/apparmor.d is not populated as expected by aa-* tools.
• Profiles distributed by AppArmor's upstream (eg. <abstractions/base>) use the FHS (Filesystem Hierarchy Standard) but the FHS does not match paths in the Nix store.
• Any enabling, disabling or change to an AppArmor's profile causes security.apparmor to be restarted and thus also all other services which have Requires=apparmor.service.
• All profile are loaded in enforce mode, there is no way to select between enforce and complain confinement modes for each profile.
• Caching of compiled profiles cannot be enabled.

Things done

• Fix reloading of apparmor.service, and enable reloadIfChanged (see comment next to this setting for the why and how).
• Use alias when it makes sense to use the FHS in order to reuse AppArmor's upstream profiles.
• Remove option profiles in favor of option policies allowing to disable individual profiles, or disable their enforcing (aka. complain mode).
• Add option includes to be able to fix or modify included profiles (like <abstractions/base>).
• Fix profile of ping.
• Add option enableCache to enable caching of compiled profiles (note that it works with transmission's profile but not ping's profile, I don't know why).
• Add binary aa-teardown, and use it in ExecStartPre and ExecStop.
• Add binary aa-remove-unknown, and use it in ExecReload.
• Add option killUnconfinedConfinables to kill processes which are not confined but have a profile.
• Configure apparmor_parser through /etc/apparmor/parser.conf.
• Configure aa-logprof through /etc/apparmor/logprof.conf.
• Adapt virtualisation.lxc, virtualisation.lxd and services.torrent.transmission.
• Make aa-enforce and ˋaa-complainˋ work?

• 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.

[doronbehar]

Looks like wonderful work @ju1m :) I can't judge or review because I don't use apparmor. Don't forget to add release notes once all reviews are done.

[bqv]

This works, and is a substantial improvement. I suggest merge once below is solved

[ju1m]

I would say that this profile falls under the introductive comment saying that I generally don't know the rules which are relevant, and likely that those current rules are incorrect or useless. Hence I would not add a comment to say what to do except investigate on how /etc/pythonX.Y is generated on NixOS, if it ever is. Those present rules match a configuration where /etc/pythonX.Y is a symlink, like it is for instance the case with /etc/systemd/system -> /etc/static/systemd/system, but maybe NixOS/Python people would generate it otherwise, eg. environment.etc."pythonX.Y/foo".text = "bar". Maybe we should simply remove those rules and let correct ones be added by a later PR once the /etc/pythonX.Y setup has been elucidated.

[FRidh]

No we don't use /etc/pythonX.Y. What kind of system-wide Python configuration could one expect?

[FRidh]

I don't know why this is needed, but at least remove all old Python versions.

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/sandboxing-all-programs-by-default/7792/6

[8573]

@ju1m, I see it looks like the email address with which you're committing doesn't appear to be linked to your GitHub account.

[Mic92]

How does this work? It seems a bit scary to me to kill processes.

[bqv]

It seems scarier to me to have processes around actively circumventing the profiles, without that being obvious to the user

[8573]

I suppose killing the unconfined confinable processes by default poses a certain risk of causing people who casually enable AppArmor (without looking at all of the module's options) and get their processes killed unexpectedly to forsake AppArmor with an unnecessarily poor opinion of it. As a compromise, I propose that killUnconfinedConfinables be disabled by default but the main security.apparmor.enable option's documentation recommend enabling it.

[Mic92]

Also asking upstream: @stevebeattie @cboltz This procedure seems a bit unorthodox, what is your recommendation here?

[cboltz]

It's indeed a bit unorthodox ;-)

I'm not familiar with NixOS, which makes recommending something a bit difficult. Please read the following lines with a grain of salt.

My recommendation not to use aa-teardown (and therefore not unloading any profiles) in my other comment is probably the most important part, because it will avoid that processes become accidently unconfined if someone runs systemctl restart apparmor.

That will however still leave the case of newly added profiles for already running processes, where killing/restarting the process is the only way to get them confined. Killing them by default is a secure way, but I'm not sure if the users will like it ;-)

[Mic92]

I think the biggest difference from a apparmor perspective between NixOS and other Linux distributions here is that new versions executables will get a new path in the nix store, while common Linux distributions will update executables in place.

[ju1m]

@8573 I've set killUnconfinedConfinables to true by default because it's how to get the running system to match the expectations declared in the NixOS configuration, and somehow "a secure way" as @cboltz put it. I've added warnings in the relase notes and the enable description. Also, AFAIK, when enabling AppArmor the first time, the system must be rebooted anyway to enable AppArmor in the kernel, and AFAICS the other main case that could trigger kills is in the rare situation when an AppArmor profile will be introduced/enabled for the first time into Nixpkgs.

I've also mentionned that killUnconfinedConfinables will unfortunately won't even work if we give names to the profiles in addition to their attachment paths because then /sys/kernel/security/apparmor/profiles will not expose the attachment paths and thus aa-status will not report processes from these paths as unconfined, and AFAICS it the same problem for attachment paths using regex, but because aa-status lacks support for these.

@cboltz Thanks for mentionning aa-remove-unknown, I've reinvented the wheel not knowing it existed ^^. Concerning my use of ˋaa-teardownˋ in ExecStop= I thing it's more faithful to what stopping means and is not problematic here because I'm using NixOS' reloadIfChanged= feature so that reconfiguring AppArmor shall only run ExecReload=. I'm also using ˋaa-teardownˋ in ExecStart= as a precaution because I want to be sure that only what is declared in the NixOS configuration is actually loaded into the kernel, I've had bad surprises in the previous AppArmor service with profiles lingering in the kernel and still active in addition to the profiles I wanted (in that case it happenned because apparmor_parser --remove had failed because of a missing included file, now this precise case should no longer happen but since the ExecStart= is normally only run at boot and only ExecReload= after, I prefer to be sure and remove all profiles upfront). So as I see it, a NixOS user should never have to run systemd start/stop/restart apparmor, and nixos-rebuild shall only run systemd reload apparmor.

[bqv]

Gosh, this has blown up

[ju1m]

Updated to revert #96034 in favor of this PR. Also, having a parserConfig does not seem needed, since one can directly put extra config lines in environment.etc."apparmor/parser.conf".text.

[bqv]

Is there any reason this isn't being looked at or merged? It's fairly trivial again

[7c6f434c]

So apparently it works somehow for @bqv and @joachifm has approved some intermediate version… I guess we are not getting any better reports here…

[vcunat]

This was reverted in 420f89c due to very hard evaluation problems (see cross-linked info). I expect it's a task for someone of you to prepare a fixed version of this and file a new PR.

[ju1m]

@vcunat sorry for the troubles, I'm trying to understand what's going wrong.

#99236 (comment) reports the following error related to AppArmor, which I can't make sense of:

a 'aarch64-linux' with features {} is required to build '/nix/store/dhwnmrcdsz42d7sb9j2hb19z4j66wnql-apparmor-parser-2.13.4.drv', but I am a 'x86_64-linux' with features {benchmark, big-parallel, kvm, nixos-test}"

But #99236 (comment) reports an other error, which makes more sense to me:

attempted to realize '/nix/store/4shyj8ckj7f6lxy5jvk0d9spx99dhgsi-apparmor-utils-2.13.4.drv' during evaluation but 'allow-import-from-derivation' is false"

So the error appears to be located into the apparmor-parser derivation, which is changed by this PR only to fix paths in upstream's parser/rc.apparmor.functions:
https://github.com/ju1m/nixpkgs/blob/fb6d63f3fdd95a5468d43a0693c8ca7c1894363f/pkgs/os-specific/linux/apparmor/default.nix#L222-L224

I'm not aware of this PR requiring allow-import-from-derivation, maybe someone would see something obviously wrong in the way I patch apparmor-parser? Like using $out or ${./fix-rc.apparmor.functions.sh}?

I'm trying to learn to use hydra to reproduce the problem myself in order to bisect it further down using:

nix-shell -p hydra-unstable --run 'NIX_PATH= hydra-eval-jobs -I . nixos/release-combined.nix --option allow-import-from-derivation false --verbose --show-trace'

I've never done it, I don't have a clear view of what it's trying to do, and I'm expecting many hours of buildtime considering the low power of my computer, or more if I have to restart it for whatever reason.

[vcunat]

At a quick glance, usage of apparmorRulesFromClosure looks like import-from-derivation.

[vcunat]

To clarify, import-from-derivation is disallowed on Hydra.nixos.org but normally that should just kill only the jobs requiring it and not the whole evaluation (I see at least one report for another job in successful eval). So, the biggest problem in this PR might be something else than IFD.

[bqv]

@vcunat is that to say you're not actually sure what part of this breaks hydra so comprehensively, but you're sure some part of it does? Because I can't see anything overly complicated here

[samueldr]

strace doesn't seem to point to anything particular here. Sorry for the false hope in my now hidden previous comment.

[vcunat]

Correct, I do not know (for sure at least). Still, import-from-derivation is better avoided in the official nixpkgs repo (not sure if there's some rule on that) and it's intentionally disabled on Hydra.nixos.org.

[ju1m]

When trying to reproduce on fb6d63f, I get:

$ nix-shell -p hydra-unstable --run 'NIX_PATH= time hydra-eval-jobs -I . nixos/release-combined.nix --option allow-import-from-derivation false --verbose'
[…]
[json.exception.type_error.302] type must be string, but is null

And when removing the call to closureInfo I get:

$ nix-shell -p hydra-unstable --run 'NIX_PATH= time hydra-eval-jobs -I . nixos/release-combined.nix --option allow-import-from-derivation false --verbose'
[…]
error: --- InvalidPath ------------------------------- hydra-eval-jobs
path '/nix/store/vycirqyzd6vf8v4g1bhp4jbx99wn0y6k-nixos-channel-20.09pre56789.gfedcba.drv' is not a valid store path
Command exited with non-zero status 1
19.19user 36.82system 4:13:52elapsed 0%CPU (0avgtext+0avgdata 91472maxresident)k
1186inputs+0outputs (251major+53123minor)pagefaults 0swaps

No clue of what I can do to get more feedback on what's going wrong.

[ju1m]

Correct, I do not know (for sure at least). Still, import-from-derivation is better avoided in the official nixpkgs repo (not sure if there's some rule on that) and it's intentionally disabled on Hydra.nixos.org.

@vcunat, using closureInfo is an important helper to write and maintain AppArmor profiles, I don't understand why it should not be used in the official Nixpkgs?

AFAICS closureInfo is already used, but only in NixOS:

nixpkgs/nixos/modules/security/systemd-confinement.nix

Line 176 in 19ac436

closureInfo = pkgs.closureInfo { inherit rootPaths; };

But in the AppArmor case it makes more sense to call closureInfo when building the packages (hence in Nixpkgs) because:

1. this is where dependencies are selected (using build parameters and such) and easily filterable at the program level instead of the package level. For instance, dependencies of ping are not necessarily the same than tftpd's, tough they share the same iputils package. So, if one would do like systemd-confinement.nix and derive rules restraining ping based upon its iputils package, one would get a profile more permissive than necessary. In that case it may not be an important security concern, but in other packages it may.
2. AppArmor is path-based, hence applies to program that may me called manually (eg. ping) not just to systemd services.

[vcunat]

I don't think closureInfo should be a problem by itself. It seems likely that on some place it's (unintentionally? transitively) required during evaluation already, but I haven't really looked.

EDIT: I suspect you can test with --option allow-import-from-derivation false even without cannons like hydra-eval-jobs.